Load ignore files/rules per step #267
Conversation
and locally loaded step ignore files. Includes some prerequisite refactorings: * Made currentStepDir a state field similar to currentTestResult for access in capTest. * The test printer has been changed from field to a local variable in capTest. (It is created on the fly with the step filter.) * Renamed the filter field to suiteFilter to distinguish it from the specific test filter. * Renamed filter loading methods in RecheckIgnoreUtil to reflect local vs. global filters. (The local one loads both suite and step filters.)
|
Thanks a lot for the PR! 🙏 We will go through this ASAP. |
| final Set<LeafDifference> uniqueDifferences = finishedTestResult.getDifferences( filter ); | ||
|
|
||
| final GlobalIgnoreApplier stepIgnoreApplier = RecheckIgnoreUtil.loadRecheckIgnore( currentStepDir ); | ||
| final Filter stepFilter = new CompoundFilter( Arrays.asList( suiteFilter, stepIgnoreApplier ) ); |
There was a problem hiding this comment.
Wouldn't this need 3 filters?
- Global
recheck.ignore - Suite
recheck.ignore - Step
recheck.ignore
There was a problem hiding this comment.
That's a good point - the three filters are already there, but in an obfuscated way: The global one is hiding in the misleadingly named suiteFilter, which is already constructed as a compound filter from global and suite filters in the constructor.
I guess we could either rename it to globalAndSuiteFilter (or similar) for clarification, or we can keep both filters in the RecheckImpl and then have the three item list you had expected. Any preference?
There was a problem hiding this comment.
Ah, indeed! I think globalAndSuiteFilter is a good idea. 👍
There was a problem hiding this comment.
Oh my! Good thing you asked about a test: It made me recheck my manual HTML test example to see if I could extract something unit-like. Instead, after a few trials my palm met my forehead: The code has an elementary conceptual problem (which is obvious in hindsight, don't know how I missed it then):
Currently, it only loads one step filter in RecheckImpl::capTest, i.e., only for the last step in a test. In general, RecheckImpl does step-wise stuff mostly via the ActionReplayResults, but capTest can currently only use one suite+global filter when finalizing the test results.
Unfortunately, I see no elegant way to fix this: Both TestReplayResult::getDifferences and the whole TestReplayResultPrinter currently also assume a single, constant filter when accumulating results from different ActionReplayResults. One alternative would be for them to use a Map<ActionReplayResult, Filter> to look up the (step-specific) filter per action; but this seems a little hackish to me, and it would change more public signatures and associated behaviour than this feature might be worth. I have checked these changes in so you can take a look yourself.
I have also verified the changed code with my little manual html test (which Github won't let me attach here, strangely), and I am very certain that it now works as desired, but I would understand completely if you think it might be better to close this pull request for the moment. (The suite filters, part 1 of this issue, are not affected.)
Let SLF4J do the formatting (thanks to beatngu13!) Co-Authored-By: Daniel Kraus <daniel.kraus@mailbox.org>
| final Set<LeafDifference> uniqueDifferences = finishedTestResult.getDifferences( filter ); | ||
|
|
||
| final GlobalIgnoreApplier stepIgnoreApplier = RecheckIgnoreUtil.loadRecheckIgnore( currentStepDir ); | ||
| final Filter stepFilter = new CompoundFilter( Arrays.asList( suiteFilter, stepIgnoreApplier ) ); |
There was a problem hiding this comment.
Ah, indeed! I think globalAndSuiteFilter is a good idea. 👍
* It is a composition of the two.
* The step filter was only loaded in capTest, so only the filter from the last step would be applied. * Now map action results to their respective filter instead. * This entails some signature changes down the call chain, where only a single, constant filter across steps was used before.
|
First of all: Sorry for the late reply! Glad you find that issue—it is indeed not straightforward to come up with a proper solution. And as you said before, I would suggest to keep this PR open, we will discuss possible alternatives in retest/recheck-web#182. But in any case, we need tests for this (although we should probably wait until we know which way we want to go). We will keep you updated, and thanks again for your contribution! |
|
We decided that we currently do not have the resources to properly handle this PR (as probably stated due to the long time this PR was opened). However, we will revise this once there is need. Again, thanks for the contribution and sorry for the long delay. |
This pull request proposes a solution for part 2 (level 3) of retest/recheck-web#182: Having an optional filter for each test step.
The two main changes are in
RecheckImpl:File currentStepDir(kept in a fashion similar to the already existingcurrentTestResult). This makes the information about the current step available incapTestto construct a step-specific filter from the suite filter.TestReplayResultPrinterfield is now locally constructed inRecheckImpl::getDifferencesErrorMessageusing the step-specific filter.The loading of the step-specific filter delegates to the same logic used by the global and suite filters in
RecheckIgnoreUtil. As with suite filters, a missing step filter will only log a debug message.On a related note, the
GlobalIgnoreAppliershould perhaps be renamed: It is now used for 1. the global filter, 2. the suite filter, and 3. the step filter. It is rather like a mutable version of theCompoundFilter; it also happens to be the type used for loading and saving. So it seems to be something like aMutablePersistableCompoundFilter. IMHO, its inner classPersistableGlobalIgnoreAppliercould also be replaced with a simpler list copy during (de)serialization.