Junit4 rule testutil migration #3031
Conversation
vogella
force-pushed
the
junit4-rule-testutil-migration
branch
from
137027c to
1b9aa7c
Compare
| import org.eclipse.core.runtime.jobs.Job; | ||
|
|
||
| /** | ||
| * JUnit 4 rule to clean up state that can otherwise leak through SWT between |
There was a problem hiding this comment.
I’m by far not an expert. Aren’t we trying to get all the tests to be junit5?
There was a problem hiding this comment.
Step by step yes. This is not this step :-)
There was a problem hiding this comment.
I’m by far not an expert. Aren’t we trying to get all the tests to be junit5?
In general yes, but as Lars said, probably in another step.
For JUnit-5 the strategy is a bit different, but not actually quite simple: One can provide custom org.junit.jupiter.api.extension.Extension implementations that are then use decoratively by adding
@org.junit.jupiter.api.extension.ExtendWith(MyExtension.class)
to the tests that should use it. The sub-interfaces of Extension define callbacks, that are then called at the corresponding time, if a custom extension implements it, for example the AfterAllCallback is called after all test cases of a class have run (similar to methods annotated with @AfterAll). Of course there are more options.
That being said, I wonder if TestUtilCleanupRule should be it's own top-level class that even duplicates methods from TestUtil. I think it would be better to define TestUtilCleanupRule as static nested classes in TestUtil, you could just name it CleanupRule and use it as TestUtil.CleanupRule (then we could have more rules too).
This would also allow to have a corresponding JUnit-5 extension in parallel as static nested class and would allow easy step-by-step migration of tests.
I'm pretty sure that technically it is possible for JUnit-4 Rules and JUnit-5 extensions to be defined as static nested class. Of course it, also works as separate class, but I think it's not worth the extra file.
There was a problem hiding this comment.
I moved the rule to TestUtil, please have a look @HannesWell if I did as suggested by you
|
Would all the direct calls to cleanUp be cleaned up and would this kick in after the tear down of each test? |
The after method is triggered after each test, if this rule is used. |
vogella
force-pushed
the
junit4-rule-testutil-migration
branch
2 times, most recently
from
a36325b to
0cd32ee
Compare
|
@HannesWell you suggested the rule. What do you think? |
vogella
force-pushed
the
junit4-rule-testutil-migration
branch
2 times, most recently
from
4bfa999 to
bb8824a
Compare
|
Any opinion here? If I do not hear anything I plan to merge. |
|
My experience with JUnit 4 to JUnit 5 migration is that JUnit 4 So likely this PR will generate more work in future. At least changes to my tests where I used |
Please rebase on master before merge, the tests were executed on a repo state that is over a month old now. |
Junit @ExtendWith are replacements for Rules. |
In eclipse-platform#3029 it was suggested to add a rule, this is the rule. It is applied to the tests. Some tearDown methods did also spin the event loop, that is not necessary as the rule does this already.
vogella
force-pushed
the
junit4-rule-testutil-migration
branch
from
bb8824a to
f4c5740
Compare
Yes, and unlike find-replace from command line ( |
|
Common cleanup logic is something to best place in JUnit 4 rules / JUnit 5 extensions. Otherwise, you will likely miss or misplace the custom addition of cleanups at one place or the other. In the platform resource tests there was some cleanup logic spread around as well, which was inconsistently used. We migrated it to a JUnit 4 rule first and then to a JUnit 5 extension: https://github.com/eclipse-platform/eclipse.platform/blob/master/resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/util/WorkspaceResetExtension.java Yes, that was some effort, but doing so also revealed several flaws in the tests. So in my opinion, taking the first step of moving the functionality to a rule makes sense. |
No description provided.