-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AddMissingTestBeforeAfterAnnotations
recipe for overridden JUnit test methods
#444
Conversation
AddMissingTestBeforeAfterAnnotations
recipe for overridden JUnit test methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/main/java/org/openrewrite/java/testing/junit5/AddMissingTestBeforeAfterAnnotations.java
Show resolved
Hide resolved
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! |
I think I have to revise my statement regarding whether a check for the new annotations on the superclass should be done or not. 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: On the other hand if you try to do the migration step by step, looking for the new annotations will support this use case. I would create a new PR to change to the behavior if you agree. |
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 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. |
Ok, I will create the PR. Regarding the suite of recipes you mentioned: Currently JUnit4to5Migration contains Should this probably be moved to |
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 |
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
I added org.openrewrite.java.testing.junit5.AddMissingTestBeforeAfterAnnotations to the JUnit4to5Migration recipe. I therefore also created a new test migrateInheritedTestBeforeAfterAnnotations() in JUnit5MigrationTest.
Fixes #443