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

Migrate JMockit Verifications to Mockito #540

Merged
merged 13 commits into from
Jul 3, 2024

Conversation

shivanisky
Copy link
Collaborator

@shivanisky shivanisky commented Jun 27, 2024

Tests are failing, need to fix and also add tests once they pass. The import for verify is not being added. I don't know why this is not working as it is working for the when statements and the same code is being used.

https://jmockit.github.io/tutorial/Mocking.html#verification
https://www.javadoc.io/doc/org.jmockit/jmockit/1.45/mockit/Verifications.html

What's changed?

What's your motivation?

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

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Any additional context

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

…ockito verify statements. Tests are failing, need to fix and also add tests once they pass.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@timtebeek timtebeek changed the title First draft of adding feature to migrate JMockit Verifications to mockito Migrate JMockit Verifications to Mockito Jun 27, 2024
@timtebeek timtebeek self-requested a review June 27, 2024 08:54
@timtebeek timtebeek added the enhancement New feature or request label Jun 27, 2024
shivanisky and others added 9 commits June 28, 2024 00:28
…w line between migrated mockit when statements as mockito statements are more compact than Expectations
… when we change the expected output to the actual generated output by including the static import, we get error that the type is missing or malformed. This looks like a bug in the framework. It seems to only occur when we add a second class in the before.
@timtebeek
Copy link
Contributor

@tinder-dthomson Any thoughts on this PR from your experience with the frameworks & recipes?

@tinder-dthomson
Copy link
Contributor

@tinder-dthomson Any thoughts on this PR from your experience with the frameworks & recipes?

I would not recommend removing the type validation, frustrating as it is. Usually this happens when you're trying to reference types that are not properly loaded, which shouldn't be a major problem in a unit test (just use standard java classes).

@timtebeek
Copy link
Contributor

The type validation is only disabled selectively for method invocations on this one test that indeed uses a custom object.

void whenClassArgumentMatcher() {
//language=java
rewriteRun(
spec -> spec.afterTypeValidationOptions(TypeValidation.builder().methodInvocations(false).build()),

We could modify this test to indeed use some class from the JDK itself to get around that, even if we would similarly lack type information in actual cases as well. That makes me inclined to keep this as it is, seeing how it's use is limited to a case we indeed can't seem to cover as easily. We've discussed this in some more detail on Discord; there's one option left to restore .imports(fqn) and add .contextSensitive(), but overall I'd say this is great work already, and need not be help up over this small imperfection that's common with other mocking frameworks as well.

Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Approved already from my side; let me know if either of you feels we should further explore the missing types for verify before we merged.

I'd say the missing types are acceptable here given the limited scope of these migration recipes, and hard to work around (in a performant manner) for limited added value, as there's no method or recipe chaining expected here.

Copy link
Contributor

@tinder-dthomson tinder-dthomson left a comment

Choose a reason for hiding this comment

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

Great contribution!

@timtebeek timtebeek merged commit 2096894 into openrewrite:main Jul 3, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants