Use JUnit5 conditions to disable some tests on CI server#3457
Conversation
lenhard
left a comment
There was a problem hiding this comment.
Why do you reduce the visibility of some of the test methods? I mean a tight scope for visibility is nice, but JUnit 4 tests have more or less been public by convention. Is there something in JUnit 5 that changes this? In any case, it should be consistent within the tests and in this PR some are changed to default visibility, whereas others are kept.
|
Good question! IntelliJ warned about it and this is why I changed it. If you look that the examples from the JUnit team, they also always use the default visibility (e.g. here). I have no strong opinion about this. |
|
In that case, I'd say we go for default visibility in all JUnit5 tests, regardless of whether they're new or migrated from a JUnit4 tests. |
|
Ok, I now used the default visibility everywhere. |
|
Default Visibility is normally considered an anti pattern and is marked by coday: Since: PMD 3.4 Use explicit scoping instead of the default package private level. |
Instead of assumptions, we can now disable tests on the CI server using a new
@DisabledOnCIServerannotation. I converted the fetcher tests affected to junit 5.Moreover, I extracted some common tests (null / empty) to a new parametrized fulltextfetcher class.
gradle localizationUpdate?