Issue182: Load recheck.ignore file by suite #193
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
beatngu13
suggested changes
May 8, 2019
| @@ -58,7 +62,7 @@ | |||
|
|
|||
| private final Map<String, DefaultValueFinder> usedFinders = new HashMap<>(); | |||
| private final TestReplayResultPrinter printer; | |||
| private final GlobalIgnoreApplier ignoreApplier; | |||
| private final Filter ignoreApplier; | |||
There was a problem hiding this comment.
I think changing the field name would also be a good idea:
Suggested change
| private final Filter ignoreApplier; | |
| private final Filter filter; |
| final GlobalIgnoreApplier globalIgnoreApplier = RecheckIgnoreUtil.loadRecheckIgnore(); | ||
| final GlobalIgnoreApplier suiteIgnoreApplier = RecheckIgnoreUtil.loadRecheckSuiteIgnore( getSuitePath() ); | ||
|
|
||
| ignoreApplier = new CompoundFilter( Lists.newArrayList( globalIgnoreApplier, suiteIgnoreApplier ) ); |
There was a problem hiding this comment.
IMHO it is good to not rely on Guava if there is similar JDK method:
Suggested change
| ignoreApplier = new CompoundFilter( Lists.newArrayList( globalIgnoreApplier, suiteIgnoreApplier ) ); | |
| ignoreApplier = new CompoundFilter( Arrays.asList( globalIgnoreApplier, suiteIgnoreApplier ) ); |
| @@ -18,13 +20,30 @@ | |||
| public class LoadFilterWorker { | |||
|
|
|||
| private final Counter counter; | |||
| private final Optional<Path> ignoreFile; | |||
There was a problem hiding this comment.
I know that this is a controversial topic, but I wouldn't use Optional in fields.
See Stuart Marks' (who works for the core libraries team in the JDK group at Oracle) talk "Optional – The Mother of All Bikesheds" from Devoxx 2016 at 54:04:
Why Not Use Optional in Fields?
- More a style issue than a correctness issue
- usually there's a better way to model absence of a value
- use of Optional in fields often arises from slavish desire to eliminate nullable fields
- remember, eliminating nulls isn't a goal of Optional
- Using Optional in fields…
- creates another object for every field
- introduces a dependent load from memory on every field read
- clutters up your code
- to what benefit? ability to chain methods?
| @@ -51,7 +41,8 @@ void using_strange_stepText_should_be_normalized() throws Exception { | |||
| } catch ( final Exception e ) { | |||
| // ignore exception, fear AssertionErrors... | |||
| } | |||
| assertThat( check.wasCalled ).isTrue(); | |||
| Mockito.verify( check ).createFileNamer( "de.retest.recheck.RecheckImplTest" ); | |||
There was a problem hiding this comment.
We almost always use static importants for this reduce noise.
| @Test | ||
| void loading_without_ignore_file_should_fail( @TempDir final Path root ) throws Exception { | ||
| final LoadFilterWorker worker = new LoadFilterWorker( NopCounter.getInstance(), root.toFile() ); | ||
| assertThrows( IllegalArgumentException.class, () -> worker.load() ); |
There was a problem hiding this comment.
We usually use AssertJ for assertions as they are—in our opinion—more flexible and readable.
* global project base is still the default * added minimum unit tests
* adds it to the global ignorer * adapted unit test
* both exception message and method name now reflect their current purpose
beatngu13
approved these changes
May 15, 2019
|
Sorry, only got around to this now - thanks for the adaptations and the link to the interesting talk! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request pertains to the first half of retest/recheck-web#182 :
The RetestImpl now loads a second, optional, suite-specific filter in addition to the already loaded global one.
The new suite filter is loaded from the suite's golden master directory. It is loaded with the same mechanism as the global filter (e.g., also allows for a rules file). One difference is that a missing suite filter is not logged as an error (but as a debug message).
The remaining part of issue 182 is adding the step-wise filters (called level 3 in issue 182)