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.

Sign up for GitHub

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

Jump to bottom

Further improvements to replace powermockito method calls by mockito #315

Merged

sambsnyd merged 71 commits into openrewrite:main from klauerm:feature/replace-power-mockito-recipe

Apr 7, 2023

Contributor

klauerm commented Feb 15, 2023

No description provided.

klauerm and others added 30 commits

January 27, 2023 15:07


          Unit Test for replacing mockStatic method.

246e416


          Modified recipe list

95abee1


          Tried to use org.powermock for unit test

8c576bd


          Update src/test/java/org/openrewrite/java/testing/mockito/ReplacePowe…

15f5f99

…rMockitoIntegrationTest.java

Co-authored-by: Tim te Beek <timtebeek@gmail.com>


          Update src/test/java/org/openrewrite/java/testing/mockito/ReplacePowe…

38061bc

…rMockitoIntegrationTest.java


          added unit test parser dependency to powermock

24e1209


          Add powermock jars for classpathFromResources

dea6eeb


          Removed additional Powermock annotations

e36f38d


          Exclude Visual Studio Code settings from git

69a84c0


          New Recipe PowerMockitoMockStaticToMockito

c660098

- First it removes the PowerMockTestCaseConfig extension


          Try to replace @PrepareForTest annotation with new MockedStatic fields

0df451e


          Give the test method a meaningful name

82c9c62


          Mostly get the test cases passing. There is still a problem with the …

b109223

…template which I think is a bug in OpenRewrite.


          Get tests passing


          Add missing license headers

9b1b3f3


          Updated mockito.yml and IntegrationTest to reflect changes from last …

0d1dfae

…commit of code review


          Added tearDown() methods to the refactored tests

a081505


          Refactoring and code clean up

7fb043a


          Fixed issue with missing statements in tearDown method.

f9c188f

- Unit tests are failing because of missing type


          Merge branch 'main' into feature/replace-power-mockito-recipe

cbb5fb4

# Conflicts:
#	build.gradle.kts
#	src/main/java/org/openrewrite/java/testing/mockito/PowerMockitoMockStaticToMockito.java
#	src/test/java/org/openrewrite/java/testing/mockito/PowerMockitoMockStaticToMockitoTest.java


          Tried to replace Mockito.when with MockedStatic.when

a94a114

- Unit tests are failing because of missing type


          Specified requirements in ReplacePowerMockitoIntegrationTest

c778cbc

- Unit tests are failing because of missing type


          Fixed unit tests and 1 integration test

3f00589

- The testNG case is still missing

- Unit tests are failing because of missing type


          First version of recipe to work with TestNG

f27868a


          Refactor test framework information holder and fix unit test issues

e556caa


          Merge pull request #1 from tpoeppke/feature/replace-power-mockito-recipe

3a3afec

Move static mock creation into before method


          Merge branch 'main' into feature/replace-power-mockito-recipe

947938f


          Enable ReplacePowerMockitoIntegrationTest and add testng-7.7.1 to cla…

319c2e0

…sspathFromResources


          Fix annotation pattern in yml file

a7434b6


          Merge branch 'main' into feature/replace-power-mockito-recipe

a1b0d44

Contributor Author

klauerm commented Feb 22, 2023

The rewrite on our code base is working now. Can someone please help me to fix the failing unit tests in ReplacePowerMockitoIntegrationTest. That would be great.
@sambsnyd @timtebeek @jkschneider

timtebeek reviewed

View reviewed changes

Contributor

timtebeek left a comment

We've discussed the option within the team to have the tests pass, for now, by ignoring the type errors within the recipe. The type issues are nested far down, so unlikely to trip up subsequent recipe executions. If they do, we can revisit the issue by adding the type attribution as required.

src/test/java/org/openrewrite/java/testing/mockito/ReplacePowerMockitoIntegrationTest.java Show resolved Hide resolved

src/test/java/org/openrewrite/java/testing/mockito/ReplacePowerMockitoIntegrationTest.java Show resolved Hide resolved

Contributor

timtebeek commented Feb 22, 2023

Just wanted to say, again, we really appreciate the amount of work you've put into this! I know it's been a tough one to see through in full, but we're glad to hear on your end you've already been able to migrate successfully. Thanks again!

timtebeek requested a review from sambsnyd

February 22, 2023 20:01

timtebeek marked this pull request as ready for review

February 23, 2023 08:18

timtebeek and others added 2 commits

February 23, 2023 09:21


          Apply suggestions from code review

5ac373b


          Only create mocked field, if mockStatic is acutally called for the class

b2ee49e

sambsnyd approved these changes

View reviewed changes

Member

sambsnyd left a comment •

edited

Sorry it took so long for me to review this. I left some suggestions which may help. Any time there is missing type attribution after running a recipe which uses JavaTemplate missing imports on JavaTemplate.Builder is the first thing I check for. In any situation where you're not completely sure that an import will always already be present for every type referenced within the template go ahead and add one.

src/main/java/org/openrewrite/java/testing/mockito/PowerMockitoMockStaticToMockito.java Outdated Show resolved Hide resolved

src/main/java/org/openrewrite/java/testing/mockito/PowerMockitoMockStaticToMockito.java Outdated

Comment on lines 369 to 382

+                          List<J.Block> methodBodies = classBody.getStatements().stream()
+                            .filter(statement -> statement instanceof J.MethodDeclaration)
+                            .map(J.MethodDeclaration.class::cast)
+                            .map(J.MethodDeclaration::getBody)
+                            .filter(Objects::nonNull)
+                            .collect(Collectors.toList());
+                          Set<J.MethodInvocation> mockStaticMethodInvocations = new HashSet<>();
+                          for (J.Block methodBody : methodBodies) {
+                              mockStaticMethodInvocations.addAll(
+                                methodBody.getStatements()
+                                  .stream().filter(statement -> statement instanceof J.MethodInvocation)
                                   .map(J.MethodInvocation.class::cast)
-                                  .filter(MOCKED_STATIC_CLOSE_MATCHER::matches)
-                                  .filter(methodInvocation -> methodInvocation.getSelect() instanceof J.Identifier)
-                                  .anyMatch(methodInvocation -> ((J.Identifier) methodInvocation.getSelect()).getSimpleName()
-                                          .equals(staticMock.getSimpleName()));
+                                  .filter(MOCKED_STATIC_MATCHER::matches)
+                                  .collect(Collectors.toSet()));

Member

sambsnyd Feb 24, 2023

Iterating over the statements in a method body will miss any statements nested inside of another statement, such as a loop or conditional.

void foo() {
  bar(); // will be found iterating over statements
  if(true) {
    bar(); // will not be found iterating over statements
  }
}

Dealing with the nested nature of these hierarchical structures is exactly what the visitor pattern is for. Consider newing up a JavaVisitor inline to collect method invocations.

src/main/java/org/openrewrite/java/testing/mockito/PowerMockitoMockStaticToMockito.java

Comment on lines +424 to +431

+                                  JavaTemplate.builder(() -> getCursor().getParentTreeCursor(),
+                                      "private MockedStatic<#{}> " + MOCK_PREFIX + "#{};")
+                                    .javaParser(() -> JavaParser.fromJavaVersion()
+                                      .classpathFromResources(ctx, "mockito-core-3.12.4")
+                                      .build())
+                                    .staticImports("org.mockito.Mockito.mockStatic")
+                                    .imports(MOCKED_STATIC)
+                                    .build(),

Member

sambsnyd Feb 24, 2023

This may be missing a calls JavaTemplate.Builder.import() for MockedStatic and fullyQualifiedType. Possible source of missing type attribution as surfaced in the test cases.

src/main/java/org/openrewrite/java/testing/mockito/PowerMockitoMockStaticToMockito.java Outdated Show resolved Hide resolved

src/main/java/org/openrewrite/java/testing/mockito/PowerMockitoMockStaticToMockito.java Outdated

Comment on lines 165 to 170

+                                            JavaTemplate.builder(() -> getCursor().getParentTreeCursor(),
+                                                "mocked#{any(org.mockito.MockedStatic)} = #{any(org.mockito.Mockito)};")
+                                              .javaParser(() -> JavaParser.fromJavaVersion()
+                                                .classpathFromResources(ctx, "mockito-core-3.*")
+                                                .build())
+                                              .build(),

Member

sambsnyd Feb 24, 2023

Possibly missing imports for MockedStatic and Mockito. I'm not 100% sure from code review alone if it makes a difference in this context, but since the tests are showing some missing type attribution, I'll point this out as a possible source.

Contributor Author

klauerm commented Feb 24, 2023

Thank you for the comments. I will look into this. Please do not merge this PR as it is still a draft and I need to implement at least 2 more features that popped up as issues during my testing.

klauerm marked this pull request as draft

February 24, 2023 09:40


          Replace PowerMockito do-method calls with Mockito ones

9094e62

timtebeek reviewed

View reviewed changes

src/main/resources/META-INF/rewrite/mockito.yml Outdated Show resolved Hide resolved

timtebeek reviewed

View reviewed changes

src/test/java/org/openrewrite/java/testing/mockito/ReplacePowerMockitoIntegrationTest.java Outdated Show resolved Hide resolved

timtebeek reviewed

View reviewed changes

src/test/java/org/openrewrite/java/testing/mockito/ReplacePowerMockitoIntegrationTest.java Outdated Show resolved Hide resolved


          Reformat code; fix typo in mockito.yml

c86d5b3

timtebeek reviewed

View reviewed changes

src/test/java/org/openrewrite/java/testing/mockito/ReplacePowerMockitoIntegrationTest.java Outdated Show resolved Hide resolved

klauerm added 6 commits

February 27, 2023 15:31


          Reformat code; Tests are now successful

fe2a9b9


          Merge branch 'main' into feature/replace-power-mockito-recipe

d405d4e

# Conflicts:
#	src/test/java/org/openrewrite/java/testing/mockito/PowerMockitoMockStaticToMockitoTest.java
#	src/test/java/org/openrewrite/java/testing/mockito/ReplacePowerMockitoIntegrationTest.java


          Fixed unit tests after merge of master

03f1b39


          Consider default implementations of interfaces

8624b09


          Replaced instanceof in filter lambdas with class::isInstance

8f58000


          Make use of visitors instead of parsing the class body

ae2c9f8

klauerm marked this pull request as ready for review

March 11, 2023 10:52

Contributor

timtebeek commented Mar 12, 2023

Thank you for your continued work on this @klauerm !

klauerm added 2 commits

March 12, 2023 17:45


          Setup and teardown method must have same group as the test method

505b68d


          Reduced cognitive complexity

c265a6b

Contributor Author

klauerm commented Mar 13, 2023

I'm done with the implementation. Please review and merge, if there are no issues.

timtebeek requested a review from sambsnyd

March 13, 2023 12:41


          Merge branch 'main' into feature/replace-power-mockito-recipe

8dd424b

sambsnyd merged commit d4e0b9d into openrewrite:main

1 check failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment