AddMissingTestBeforeAfterAnnotations recipe for overridden JUnit test methods

[magicwerk]

I created AddMissingTestBeforeAfterAnnotations with a test AddMissingTestBeforeAfterAnnotationsTest.

The recipe does not handle the JUnit4 annotations BeforeClass/AfterClass as they cannot only be used on static methods which cannot be overridden. I update the text in the issue accordingly

• Inherited lifecycle annotations are not migrated properly #443

I added org.openrewrite.java.testing.junit5.AddMissingTestBeforeAfterAnnotations to the JUnit4to5Migration recipe. I therefore also created a new test migrateInheritedTestBeforeAfterAnnotations() in JUnit5MigrationTest.

Fixes #443

[timtebeek]

Solid work, thanks! I've pushed some small changes mostly to limit where this recipe is run, as Preconditions help keep our recipes performant.

I've got a couple questions called out below that I think we should be able to work through quickly. Let me know your thoughts and we'll see this through.

[magicwerk]

I think I answered your questions, but somehow I cannot see your questions or my answers any more - did you get them?

[timtebeek]

I think I answered your questions, but somehow I cannot see your questions or my answers any more - did you get them?

They're part of the resolved conversations above, which can be unfolded if needed; thanks for sharing what you had tried and found already. I'll trust that this works as expected then, and thanks for the addition here!

[magicwerk]

I think I have to revise my statement regarding whether a check for the new annotations on the superclass should be done or not.
As said, I was originally also checking for the new annotations, but then thought it would be bad practice, but I think it is necessary so users get the expected behavior.

Currently I am now working on the migration of the large project and do this component per component. Now I run into the problem that my base classes are already migrated (so they have JUnit 5 annotations), so if I run the migration on another component the AddMissingTestBeforeAfterAnnotations is not applied.

Regarding your comment about intentionally overriding a method, I still think we're on the safe side:
If you have written your tests already in JUnit 5 and rely on the fact that you override a test method without annotating it as @test or @disabled, then you are already on JUnit 5, so there is no need to run the JUnit4to5Migration recipe.

On the other hand if you try to do the migration step by step, looking for the new annotations will support this use case.
So yes, I would add the check for new annotations as discussed.

I would create a new PR to change to the behavior if you agree.

[timtebeek]

Yes I agree with a PR that also checks for the new annotations, and thanks for taking that on!

The reason I saw some unlikely but potential issues with folks adding implicit overrides without test annotations on Junit Jupiter is that we have a suite of best practices recipes that we want them to be able to run repeatedly: https://docs.openrewrite.org/recipes/java/testing/junit5/junit5bestpractices

In the unlikely case that folks
are already on Junit Jupiter
and override methods
and intentionally not add @Test or @Disabled
and run JUnit Jupiter best practices
then and only then might we add those annotations until they explicitly add @Disabled.

Seems like a tolerable risk to me, but something to document perhaps on the next PR just in case, just above the line that looks for the new annotations too.

[magicwerk]

Ok, I will create the PR.

Regarding the suite of recipes you mentioned:

Currently JUnit4to5Migration contains
org.openrewrite.java.testing.junit5.LifecycleNonPrivate
which makes no sense for me, as lifecycle annotations must be public in JUnit 4.

Should this probably be moved to
JUnit5BestPractices
or be deleted altogether?

[timtebeek]

I was going to say I don't know how that came about, but turns out it's from back in my contributor days. Good to move to JUnit5BestPractices indeed, just to be sure folks (or recipes) don't make any mistakes in how the framework should be used. I added another change recently for cases that should not happen, but unfortunately do

• Also swap assertNull and assertNotNull argument order #440