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

junit-jupiter-params dependency is not added for @ParameterizedTest #574

Open
FieteO opened this issue Aug 14, 2024 · 8 comments
Open

junit-jupiter-params dependency is not added for @ParameterizedTest #574

FieteO opened this issue Aug 14, 2024 · 8 comments
Labels
assertj bug Something isn't working mockito

Comments

@FieteO
Copy link
Contributor

FieteO commented Aug 14, 2024

What version of OpenRewrite are you using?

2.16.0

How are you running OpenRewrite?

I am applying the Maven Command Line command on a single module project.

Can you share your configuration so that we can rule out any configuration issues?
I am not explicitly changing any configuration.

What is the smallest, simplest way to reproduce the problem?

package some.package;

import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.when;

import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;

@RunWith(value = Parameterized.class)
public class DataCopyTest {

    public enum QueryStyle {
        FOO, BAR
    }

    @Parameterized.Parameter
    public QueryStyle queryStyle;

    @Parameterized.Parameters(name = "QueryStyle.{0}")
    public static Collection<Object[]> data() {
        return Arrays.asList(new Object[][] {{QueryStyle.FOO}, {QueryStyle.BAR}});
    }

    @Test
    public void someTest() {
        assertTrue((queryStyle.equals(QueryStyle.FOO) || queryStyle.equals(QueryStyle.BAR)));
    }
}

pom.xml that doesn't have a dependency that (transitively) adds junit-jupiter-params (i.e spring-boot-starter-test):

<dependency>
	<groupId>org.springframework</groupId>
	<artifactId>spring-test</artifactId>
	<scope>test</scope>
</dependency>
<dependency>
	<groupId>org.springframework.boot</groupId>
	<artifactId>spring-boot-test</artifactId>
	<scope>test</scope>
</dependency>
<dependency>
	<groupId>junit</groupId>
	<artifactId>junit</artifactId>
	<scope>test</scope>
</dependency>
<dependency>
	<groupId>org.junit.vintage</groupId>
	<artifactId>junit-vintage-engine</artifactId>
	<scope>test</scope>
</dependency>

What did you expect to see?

<dependency>
	<groupId>org.springframework</groupId>
	<artifactId>spring-test</artifactId>
	<scope>test</scope>
</dependency>
<dependency>
	<groupId>org.springframework.boot</groupId>
	<artifactId>spring-boot-test</artifactId>
	<scope>test</scope>
</dependency>
<dependency>
    <groupId>org.junit.jupiter</groupId>
    <artifactId>junit-jupiter-params</artifactId>
    <version>5.7.2</version>
    <scope>test</scope>
</dependency>

or more specific to spring-boot:

<dependency>
	<groupId>org.springframework.boot</groupId>
	<artifactId>spring-boot-starter-test</artifactId>
	<scope>test</scope>
</dependency>

What did you see instead?

No change to those dependencies.

@FieteO FieteO added the bug Something isn't working label Aug 14, 2024
@timtebeek
Copy link
Contributor

Hi! We have a recipe step that should add this dependency:

- org.openrewrite.java.dependencies.AddDependency:
groupId: org.junit.jupiter
artifactId: junit-jupiter-params
version: 5.x
onlyIfUsing: org.junit.jupiter.params.ParameterizedTest
acceptTransitive: true
scope: test

That runs after the recipe that should convert you test class.

- org.openrewrite.java.testing.junit5.ParameterizedRunnerToParameterized

Are you seeing the corresponding change on your test class already? That's not immediately clear from the above.

@timtebeek
Copy link
Contributor

There were a few details missing above, but with some effort I was able to replicate your problem with a unit test:

@Test
@Issue("https://github.com/openrewrite/rewrite-testing-frameworks/issues/574")
void junitParamsDependencyAdded() {
    rewriteRun(
      mavenProject("project",
        srcTestJava(
          //language=java
          java(
            """
              import static org.junit.Assert.assertTrue;
              import static org.mockito.Mockito.spy;
              import static org.mockito.Mockito.when;

              import org.junit.Rule;
              import org.junit.Test;
              import org.junit.runner.RunWith;
              import org.junit.runners.Parameterized;
              import java.util.Arrays;
              import java.util.Collection;

              @RunWith(value = Parameterized.class)
              public class DataCopyTest {

                  public enum QueryStyle {
                      FOO, BAR
                  }

                  @Parameterized.Parameter
                  public QueryStyle queryStyle;

                  @Parameterized.Parameters(name = "QueryStyle.{0}")
                  public static Collection<Object[]> data() {
                      return Arrays.asList(new Object[][] {{QueryStyle.FOO}, {QueryStyle.BAR}});
                  }

                  @Test
                  public void someTest() {
                      assertTrue((queryStyle.equals(QueryStyle.FOO) || queryStyle.equals(QueryStyle.BAR)));
                  }
              }
              """,
            """
              import static org.junit.jupiter.api.Assertions.assertTrue;
              import static org.mockito.Mockito.spy;
              import static org.mockito.Mockito.when;

              import java.util.Arrays;

              import org.junit.jupiter.params.ParameterizedTest;
              import org.junit.jupiter.params.provider.MethodSource;

              import java.util.Collection;

              public class DataCopyTest {

                  public enum QueryStyle {
                      FOO, BAR
                  }
                  public QueryStyle queryStyle;

                  public static Collection<Object[]> data() {
                      return Arrays.asList(new Object[][] {{QueryStyle.FOO}, {QueryStyle.BAR}});
                  }

                  @MethodSource("data")
                  @ParameterizedTest(name = "QueryStyle.{0}")
                  public void someTest(QueryStyle queryStyle) {
                      initDataCopyTest(queryStyle);
                      assertTrue((queryStyle.equals(QueryStyle.FOO) || queryStyle.equals(QueryStyle.BAR)));
                  }

                  public void initDataCopyTest(QueryStyle queryStyle) {
                      this.queryStyle = queryStyle;
                  }
              }
              """
          )
        ),
        //language=xml
        pomXml(
          """
            <project>
                <modelVersion>4.0.0</modelVersion>
                <groupId>org.acme</groupId>
                <artifactId>project</artifactId>
                <version>0.0.1</version>
                <dependencies>
                    <dependency>
                        <groupId>org.springframework</groupId>
                        <artifactId>spring-test</artifactId>
                        <version>5.3.9</version>
                        <scope>test</scope>
                    </dependency>
                    <dependency>
                        <groupId>org.springframework.boot</groupId>
                        <artifactId>spring-boot-test</artifactId>
                        <version>2.7.18</version>
                        <scope>test</scope>
                    </dependency>
                    <dependency>
                        <groupId>junit</groupId>
                        <artifactId>junit</artifactId>
                        <version>4.13.2</version>
                        <scope>test</scope>
                    </dependency>
                    <dependency>
                        <groupId>org.junit.vintage</groupId>
                        <artifactId>junit-vintage-engine</artifactId>
                        <version>5.10.3</version>
                        <scope>test</scope>
                    </dependency>
                </dependencies>
            </project>
            """,
          """
            <project>
                <modelVersion>4.0.0</modelVersion>
                <groupId>org.acme</groupId>
                <artifactId>project</artifactId>
                <version>0.0.1</version>
                <dependencies>
                    <dependency>
                        <groupId>org.junit.jupiter</groupId>
                        <artifactId>junit-jupiter</artifactId>
                        <version>5.11.0</version>
                        <scope>test</scope>
                    </dependency>
                    <dependency>
                        <groupId>org.junit.jupiter</groupId>
                        <artifactId>junit-jupiter-params</artifactId>
                        <version>5.11.0</version>
                        <scope>test</scope>
                    </dependency>
                    <dependency>
                        <groupId>org.springframework</groupId>
                        <artifactId>spring-test</artifactId>
                        <version>5.3.9</version>
                        <scope>test</scope>
                    </dependency>
                    <dependency>
                        <groupId>org.springframework.boot</groupId>
                        <artifactId>spring-boot-test</artifactId>
                        <version>2.7.18</version>
                        <scope>test</scope>
                    </dependency>
                </dependencies>
            </project>
            """
        )
      )
    );
}

@FieteO
Copy link
Contributor Author

FieteO commented Aug 14, 2024

Hey @timtebeek thanks for replying already! Sorry for not being clearer above.
My bug description was really just about the pom, but I thought I should include a test case that would trigger the behaviour.

On a tangential note: I also observed that the hamcrest-junit dependency is left untouched if it is declared in the pom,
while it arguably can be removed. Shall I open another ticket for that?

@timtebeek
Copy link
Contributor

Hey @timtebeek thanks for replying already! Sorry for not being clearer above. My bug description was really just about the pom, but I thought I should include a test case that would trigger the behaviour.

No worries; it's just that we need the full context to write a unit test to replicate the issue, to make it runnable and debug it.

On a tangential note: I also observed that the hamcrest-junit dependency is left untouched if it is declared in the pom, while it arguably can be removed. Shall I open another ticket for that?

Yes please! A quick draft PR would be even more appreciated, as we have some examples already.

- org.openrewrite.java.dependencies.RemoveDependency:
groupId: org.junit.vintage
artifactId: junit-vintage-engine

@FieteO
Copy link
Contributor Author

FieteO commented Aug 14, 2024

Great, I'll create the ticket for now. I will see if I can create the PR in the evening, when I am back from work.

@timtebeek
Copy link
Contributor

On the topic of this issue I've found that if I remove the acceptTransitive: true on this line the test needs a second cycle to pass:

Not quite sure of the cause just yet. There does not already appear to be any such transitive dependency on the input pom.xml.

[INFO] --- dependency:3.7.0:tree (default-cli) @ project ---
[INFO] org.acme:project:jar:0.0.1
[INFO] +- org.springframework:spring-test:jar:5.3.9:test
[INFO] |  \- org.springframework:spring-core:jar:5.3.9:test
[INFO] |     \- org.springframework:spring-jcl:jar:5.3.9:test
[INFO] +- org.springframework.boot:spring-boot-test:jar:2.7.18:test
[INFO] |  \- org.springframework.boot:spring-boot:jar:2.7.18:test
[INFO] |     \- org.springframework:spring-context:jar:5.3.31:test
[INFO] |        +- org.springframework:spring-aop:jar:5.3.31:test
[INFO] |        +- org.springframework:spring-beans:jar:5.3.31:test
[INFO] |        \- org.springframework:spring-expression:jar:5.3.31:test
[INFO] +- junit:junit:jar:4.13.2:test
[INFO] |  \- org.hamcrest:hamcrest-core:jar:1.3:test
[INFO] \- org.junit.vintage:junit-vintage-engine:jar:5.10.3:test
[INFO]    +- org.junit.platform:junit-platform-engine:jar:1.10.3:test
[INFO]    |  +- org.opentest4j:opentest4j:jar:1.3.0:test
[INFO]    |  \- org.junit.platform:junit-platform-commons:jar:1.10.3:test
[INFO]    \- org.apiguardian:apiguardian-api:jar:1.1.2:test

@timtebeek timtebeek moved this to Backlog in OpenRewrite Aug 14, 2024
@FieteO
Copy link
Contributor Author

FieteO commented Aug 14, 2024

By the way, the repository that I am running this on is: https://gerrit.onap.org/r/admin/repos/aai/aai-common,general
Maybe that can help you reproducing the issue as well.

@crankydillo
Copy link

crankydillo commented Aug 20, 2024

I think we are suffering from the same thing (however, I was experiencing it in hamcrest->assertj migration). I created a reproducer here. If this is not the same thing (a problem with AddDependency + onlyIfUsing not seeing the modified LST), let me know and I'll create a new issue. Apologies if my case is unrelated:) As with OP, I tried to work up a rewriteTest, but not sure I have that bit right. I think it indicates 2 cycles are required to make the change (AddDependency + onlyIfUsing works on second cycle as it sees new LST that has assertj usage), but still not confident enough to rely solely on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assertj bug Something isn't working mockito
Projects
Status: Backlog
Development

No branches or pull requests

3 participants