Skip to content
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

Test to ParameterizedTest annotation #349

Closed
wants to merge 7 commits into from

Conversation

AlekSimpson
Copy link
Contributor

What's changed?

This draft PR adds a new recipe which adds missing ParameterizedTest annotations when ValueSource is used or replaces Test with ParameterizedTest.

What's your motivation?

#314

Anything in particular you'd like reviewers to focus on?

For some reason whenever the visitor gets to the ValueSource annotation the ValueSource AnnotationMatcher check does not return true. I was having a similar issue with the Test annotation but that was because I wasn't including junit and jupiter resources in the recipe spec. ValueSource is from jupiter so I don't think its the same issue now that I have included those resources in the spec.

Anyone you would like to review specifically?

@timtebeek

@AlekSimpson AlekSimpson self-assigned this Jun 7, 2023
@AlekSimpson AlekSimpson marked this pull request as draft June 7, 2023 20:57
@timtebeek timtebeek added the recipe Recipe request label Jun 7, 2023
@timtebeek timtebeek self-requested a review June 7, 2023 21:00
public void defaults(RecipeSpec spec) {
spec
.parser(JavaParser.fromJavaVersion()
.classpathFromResources(new InMemoryExecutionContext(), "junit-4.13", "junit-jupiter-api-5.9"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're only using JUnit Jupiter here, so there's no need for the JUnit 4 dependency.

Suggested change
.classpathFromResources(new InMemoryExecutionContext(), "junit-4.13", "junit-jupiter-api-5.9"))
.classpathFromResources(new InMemoryExecutionContext(), "junit-jupiter-api-5.9"))

rewriteRun(
java(
"""
@Test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is lacking imports, and thus likely to have missing types, and through that failing matchers.

Same goes for the tests below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might help to check that the code snippets that you use in a text block are actually valid stand alone classes, by briefly copy-pasting these into a new file. Your IDE will then immediately tell you if you're missing imports for instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya I was planning on updating those once I got the first test working, but I'll update them now.


public class AddParameterizedTestAnnotation extends Recipe {
private static final AnnotationMatcher TEST_ANNOTATION_MATCHER = new AnnotationMatcher("@org.junit.jupiter.api.Test");
private static final AnnotationMatcher VALUE_SOURCE_ANNOTATION_MATCHER = new AnnotationMatcher("@org.junit.jupiter.params.provider.ValueSource");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had you noticed that this matcher takes in a pattern that can include parameters as well? That might be why your matcher fails. Perhaps something to trace with the debugger.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How should I update the pattern? I kinda know what needs to be added but I'm not sure how to do it in a way thats generic enough? Would something like ValueSource("#{}") work?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe ValueSource(..) in case it's in line with our other matchers? I'd tried to look for examples, but I'm not finding a lot: https://public.moderne.io/results/1SllD

@timtebeek
Copy link
Contributor

Superseeded by #353

@timtebeek timtebeek closed this Jun 13, 2023
@AlekSimpson AlekSimpson deleted the TestToParameterizedTestAnnotation branch June 13, 2023 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recipe Recipe request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants