-
Notifications
You must be signed in to change notification settings - Fork 72
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
Alek/parameterized test #353
Conversation
…esn't return true when getting to ValueSource annotations
…hrough that instead of individually visiting Annotations
…array, ValueSource annotation matcher not returning true when it finds a ValueSource annotation
…AddImport() and .imports()
…AddImport() and .imports()
…AddImport() and .imports()
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.
@AlekSimpson I think the hints I provided should help you make progress. The problem is that the JUnit API is split across multiple artifacts and you need to add them all explicitly to the classpath.
src/main/java/org/openrewrite/java/testing/junit5/AddParameterizedTestAnnotation.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/junit5/AddParameterizedTestAnnotationTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/junit5/AddParameterizedTestAnnotationTest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/junit5/AddParameterizedTestAnnotation.java
Outdated
Show resolved
Hide resolved
…dded more test cases.
d38d38c
to
c2c7876
Compare
I updated the code with your suggested changes. Two tests are still failing, one fails because there is one extra space added at the start of the lines with the annotations and the method declaration. I'm not sure why that extra space is being included. My only guess is it has something to do with the The other test fails because when I try and reorder the annotations so that the @ParameterizedTest is on top it correctly does that but when it gets converted back into the code the annotations are on the same line (ex: |
src/test/java/org/openrewrite/java/testing/junit5/AddParameterizedTestAnnotationTest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/junit5/AddParameterizedTestAnnotation.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/testing/junit5/AddParameterizedTestAnnotationTest.java
Outdated
Show resolved
Hide resolved
With the newest changes I made today all the tests are passing. I'm not sure why the checks on the ci build are still failing. |
You were missing the license headers; I've gone ahead and fixed that in c5c11bb ; it's also part of the checklist above. |
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.
Some quick suggestions for improvement
src/main/java/org/openrewrite/java/testing/junit5/AddParameterizedTestAnnotation.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/junit5/AddParameterizedTestAnnotation.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/junit5/AddParameterizedTestAnnotation.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/junit5/AddParameterizedTestAnnotation.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/testing/junit5/AddParameterizedTestAnnotation.java
Outdated
Show resolved
Hide resolved
And removing @test with RemoveAnnotationVisitor
Thanks a lot for this effort! I've added a polish commit to simplify things a bit:
|
What's changed?
Adds a recipe to update
@Test
to@ParameterizedTest
under certain conditionsWhat's your motivation?
Closes #314
Anything in particular you'd like reviewers to focus on?
The import statement for @ParameterizedTest is not being added when I run the tests. I've included the classpath and used all the necessary import methods.
Anyone you would like to review specifically?
@timtebeek
Checklist
./gradlew licenseFormat