-
Notifications
You must be signed in to change notification settings - Fork 79
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 mockito full verifications and Refactor tests #617
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions could not be made:
- src/main/java/org/openrewrite/java/testing/jmockit/JMockitBlockRewriter.java
- lines 112-113
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions could not be made:
- src/main/java/org/openrewrite/java/testing/jmockit/JMockitBlockRewriter.java
- lines 112-113
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions could not be made:
- src/main/java/org/openrewrite/java/testing/jmockit/JMockitBlockRewriter.java
- lines 112-113
- src/main/java/org/openrewrite/java/testing/junit5/EnclosedToNested.java
- lines 30-30
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions could not be made:
- src/main/java/org/openrewrite/java/testing/jmockit/JMockitBlockRewriter.java
- lines 112-113
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great!
class JMockitTestBase implements RewriteTest { | ||
|
||
@Override | ||
public void defaults(RecipeSpec spec) { | ||
setDefaultParserSettings(spec); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer not to create a base class just to share a single method invocation's worth of configuration. Single inheritance of JMockitTestBase
limits the ability of tests to extend other base classes for little code reuse. I would prefer to have setDefaultParserSettings()
called explicitly in the defaults
method of each individual test class.
What's changed?
Migrate JMockit FullVerifications to Mockito
https://www.javadoc.io/doc/org.jmockit/jmockit/latest/mockit/FullVerifications.html
https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/Mockito.html <- for verifyNoMoreInteractions
Replacing FullVerifications with standard mockito verify and then add one line of verifyNoMoreInteractions(mocks. ..)
Also do a refactor of tests to remove some unnecessary code - add a new JMockitTestBase which sets the default parser settings and other tests can inherit from that, hope it's ok to use inhertance here, if not please let me know. I think it makes a lot of sense here.
What's your motivation?
Just wanted to add this feature to enhance the current migration scope, InOrderVerifications feature was requested here jmockit/jmockit1#729 (comment) and this will also help pave the way for that
Anyone you would like to review specifically?
@timtebeek @tinder-dthomson as FYI, and if wants to review also (I know he is busy)
Checklist