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

JMockit to Mockito Migration Recipe - Rewrite Expectations Block #415

Merged

Conversation

tinder-dthomson
Copy link
Contributor

@tinder-dthomson tinder-dthomson commented Oct 18, 2023

What's changed?

  • Added a new recipe ExpectationsToMockito to handle converting a JMockit Expectations block to a Mockito when statement. (WIP)
  • Added declarative recipe org.openrewrite.java.testing.jmockit.JMockitToMockito, to eventually list all the recipes necessary for the migration.
  • Added a unit test JMockitToMockitoTest for testing the declarative recipe.

What's your motivation?

Work towards a full JMockit to Mockito migration recipe. This migration is quite involved for large codebases, and JMockit presents challenges for migrating to Java 17.

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've added the license header to any new files through ./gradlew licenseFormat
  • I've used the IntelliJ IDEA auto-formatter on affected files

@tinder-dthomson tinder-dthomson changed the title Recipe to convert JMockit Expectations block to Mockito.when statement JMockit to Mockito Migration - Rewrite Expectations Block Oct 18, 2023
@tinder-dthomson tinder-dthomson changed the title JMockit to Mockito Migration - Rewrite Expectations Block JMockit to Mockito Migration Recipe - Rewrite Expectations Block Oct 18, 2023
@tinder-dthomson tinder-dthomson changed the title JMockit to Mockito Migration Recipe - Rewrite Expectations Block [WIP] JMockit to Mockito Migration Recipe - Rewrite Expectations Block Oct 18, 2023
@tinder-dthomson tinder-dthomson marked this pull request as draft October 18, 2023 03:16
@SimonVerhoeven
Copy link
Contributor

I noticed the recipes were located in the Mockito package, maybe it would be better to create a JMockit one since it's migration related to be in line with https://github.com/openrewrite/rewrite-testing-frameworks/tree/main/src/main/java/org/openrewrite/java/testing/hamcrest?

@kunli2 kunli2 marked this pull request as ready for review October 18, 2023 17:55
@kunli2 kunli2 changed the title [WIP] JMockit to Mockito Migration Recipe - Rewrite Expectations Block JMockit to Mockito Migration Recipe - Rewrite Expectations Block Oct 18, 2023
@kunli2
Copy link
Contributor

kunli2 commented Oct 18, 2023

Hi Devin, Thanks for your contribution!
I updated the code and it can pass the test now, the only thing I am not quite sure about is why the when() from the template does not have a method type.

@tinder-dthomson
Copy link
Contributor Author

Hi Devin, Thanks for your contribution! I updated the code and it can pass the test now, the only thing I am not quite sure about is why the when() from the template does not have a method type.

Thank you so much! Do you mind explaining your changes a bit?

From what I saw, your changes can be broken into three categories:

  • Classpath Updates
  • Setting typeValidationOptions to TypeValidation.none() on the test spec parser
  • Using newMethod.withPrefix(newClass.getPrefix()) (curious if nc.getPrefix() would have been okay too?)

I have the following specific questions:

  • Were classpath updates you made were functionally necessary or simply a best practice? I couldn't tell why you added mockito-junit-jupiter-3.12 to the test parser classpath, for example.
  • Would using nc.getPrefix() as the param to newMethod.withPrefix() be problematic? I would guess newClass and nc are the same pointer, unless calling super.visitNewClass clones it.

@kunli2
Copy link
Contributor

kunli2 commented Oct 18, 2023

Hi Devin, Thanks for your contribution! I updated the code and it can pass the test now, the only thing I am not quite sure about is why the when() from the template does not have a method type.

Thank you so much! Do you mind explaining your changes a bit?

From what I saw, your changes can be broken into three categories:

  • Classpath Updates
  • Setting typeValidationOptions to TypeValidation.none() on the test spec parser
  • Using newMethod.withPrefix(newClass.getPrefix()) (curious if nc.getPrefix() would have been okay too?)

I have the following specific questions:

  • Were classpath updates you made were functionally necessary or simply a best practice? I couldn't tell why you added mockito-junit-jupiter-3.12 to the test parser classpath, for example.
  • Would using nc.getPrefix() as the param to newMethod.withPrefix() be problematic? I would guess newClass and nc are the same pointer, unless calling super.visitNewClass clones it.

Adding those classpaths is to get type attributions, and we just need to add necessary minimum set of classpaths.
import org.mockito.junit.jupiter.MockitoExtension I think this import requires mockito-junit-jupiter-3.12.
Yeah, using nc.getPrefix() is better here, it's supposed to be supposed to be supported by autoFormat here, but replacing the prefix also works here.

Copy link
Contributor

@kunli2 kunli2 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your contribution again!

@kunli2 kunli2 merged commit 8e1dffb into openrewrite:main Oct 19, 2023
@tinder-dthomson tinder-dthomson deleted the jmockit-expectations-to-mockito branch October 19, 2023 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants