Migrate tests to JUnit 5 #900
Conversation
|
WIP looks great, plan looks good too. |
lutovich
force-pushed
the
junit5
branch
8 times, most recently
from
428d457 to
a93cf1a
Compare
| } | ||
| } | ||
| @EnabledForJreRange(min = JAVA_11) | ||
| abstract class EclipseWtpFormatterCommonTests extends EclipseCommonTests { |
There was a problem hiding this comment.
I did not find a good way to combine parametrization with test inheritance in JUnit 5. That's why I split this test class into:
EclipseWtpFormatterCssTestEclipseWtpFormatterHtmlTestEclipseWtpFormatterJsonTestEclipseWtpFormatterJsTestEclipseWtpFormatterXmlTest
separate test classes. Each class defines language-specific input and test expectation. Separate test classes replace the WTP enum.
I also effectively removed EclipseWtpFormatterStepTestOld because of the same reason. Method #getSupportedVersions() now returns both old and new versions in a single array.
Does this seem like a sensible approach? Suggestions are much appreciated
There was a problem hiding this comment.
I think the old stuff looks better, but the new stuff isn't bad. Adopting JUnit 5 is quite nice, and I think it's worth merging this quickly to avoid conflicts. Can always refactor back together in the future. This okay with you @fvgh?
There was a problem hiding this comment.
There was a problem hiding this comment.
LGTM. A little overhead having so many test classes, but it is readable.
@lutovich Thanks for putting in so much effort. I always missed JUnit5 functionality.
testlib/src/test/java/com/diffplug/spotless/NonSerializableListTest.java
Show resolved
Hide resolved
Main changes: * all tests use JUnit Jupiter APIs and assertions * remove public modifiers from test classes and test methods because they are unnecessary with JUnit Jupiter * replace JVM version assumptions with `@EnabledOnJre` annotations that support version ranges * replace OS assumptions with `@EnabledOnOs` and `@DisabledOnOs` * enable JUnit Jupiter in Gradle build scripts for the test task with `useJUnitPlatform()`
|
I vote for pushing new commits, rather than squashing every time. We can squash in final merge, but hard to follow/review progress if every new push is a squash. |
|
@nedtwigg sorry, I did not realize you were reviewing. I was re-reviewing myself and making some minor tweaks. Also, added a better commit message. PR is ready for review now. |
… previous behavior of EclipseWtpFormatterStepTest vs EclipseWtpFormatterStepTestOld.
nedtwigg
left a comment
nedtwigg
left a comment
There was a problem hiding this comment.
From the Circle CI test results:
- test_windows: 255 -> 245
- test_npm_8: broken before and after this PR, test results not getting parsed :(
- test_windows, test_nomaven_15, test_nomaven_11: 255 -> 245
- test_justmaven_11: 72 -> 72
So all tests are still there and passing except for 10, which comes from:
- lost EclipseWtpFormatterStepTestOld x10
- lost EclipseWtpFormatterStepTest x10
- added x10 from EclipseWtpFormatter[Blah]Test
The difference is that EclipseWtpFormatterStepTestOld ran on all JRE, whereas non-old required JRE11 or later. So all tests are accounted for, and I think this is ready to merge!
|
Thanks a ton @lutovich, that was a lot of work! |
3 participants
Main changes:
@EnabledOnJreannotations that support version ranges@EnabledOnOsand@DisabledOnOscom.diffplug.spotless.tagpackageuseJUnitPlatform()