-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Working on Issue #3156, Adding MockitoMockedStatic and MockitoMockedS… #3161
base: main
Are you sure you want to change the base?
Conversation
…MockedStaticTest
if (tree.getModifiers().getAnnotations().stream() | ||
.anyMatch(annotation -> | ||
state.getSourceForNode(annotation).equals("@org.mockito.Mock") | ||
&& state.getSourceForNode(tree.getType()).equals("org.mockito.MockedStatic"))) { |
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.
Should this check for MockedConstruction too? Both have the same issue I think in that mocking them results in a lifecycle-bound mock that has to be closed to prevent interfering with external components
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.
Would you reckon that should be a separate file or in the same one?
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.
Not entirely sure. Might be best to wait for confirmation from one of the maintainers! I'd assume it is almost identical logic minus the naming though so whether it is something that is reusable by just passing the class name and error message or not..?
They might not even want to cover the construction case though, so I'd definitely wait for confirmation :-)
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.
Let's check for both in this same check. In that case, let's name the checker MockitoIncompatibleMockTypes
...cts/errorprone/src/test/java/org/mockito/errorprone/bugpatterns/MockitoMockedStaticTest.java
Show resolved
Hide resolved
...cts/errorprone/src/test/java/org/mockito/errorprone/bugpatterns/MockitoMockedStaticTest.java
Show resolved
Hide resolved
Regarding the module errors you posted in those screenshots, I guess you will need to add the JVM flags at https://github.com/mockito/mockito/blob/main/subprojects/errorprone/errorprone.gradle#L20 to your test runner configuration in IntelliJ. Failing that, you should be able to just run When I've used errorprone before, I've used these JVM args:
In Maven, IntelliJ seems to be able to automatically pull these from the |
Think so. I believe you should get a reports directory in one of the |
Well it seems to fail when I remove the semi-colon again and when it is there runs it just concerns me when it says skipped. I am currently using JDK 17.0.2 which is just what I had installed at the time. Let me know if the above warning is something I should be concerned about "WARNING: package com.sun.tools.javac.type not in jdk.compiler" |
oh is that the retry test? Think that just reruns failed tests to deflake them |
public Description matchVariable(VariableTree tree, VisitorState state) { | ||
if (tree.getModifiers().getAnnotations().stream() | ||
.anyMatch(annotation -> | ||
state.getSourceForNode(annotation).equals("@org.mockito.Mock") |
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.
Do not use getSourceForNode
to compare annotations, as that is equivalent to comparing toString()
. Instead, use https://errorprone.info/api/latest/com/google/errorprone/util/ASTHelpers.html#isSameType(com.sun.tools.javac.code.Type,com.sun.tools.javac.code.Type,com.google.errorprone.VisitorState)
if (tree.getModifiers().getAnnotations().stream() | ||
.anyMatch(annotation -> | ||
state.getSourceForNode(annotation).equals("@org.mockito.Mock") | ||
&& state.getSourceForNode(tree.getType()).equals("org.mockito.MockedStatic"))) { |
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.
Let's check for both in this same check. In that case, let's name the checker MockitoIncompatibleMockTypes
import org.junit.runners.JUnit4; | ||
|
||
@RunWith(JUnit4.class) | ||
public class MockitoMockedStaticTest { |
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 think you missed the case where you have a @Mock private MockedStatic<Object>
in the original issue description. You can use // BUG: diagnostic contains:
to verify that the compilation would fail. Example: https://github.com/google/error-prone/blob/18d5cdf10ec1cc798a588d5c48a9e02a8888c6a1/core/src/test/java/com/google/errorprone/bugpatterns/ArrayAsKeyOfSetOrMapTest.java#L60
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3161 +/- ##
============================================
- Coverage 85.53% 85.49% -0.04%
- Complexity 2914 2917 +3
============================================
Files 334 335 +1
Lines 8861 8868 +7
Branches 1096 1098 +2
============================================
+ Hits 7579 7582 +3
- Misses 993 995 +2
- Partials 289 291 +2
☔ View full report in Codecov by Sentry. |
…taticTest
Checklist
including project members to get a better picture of the change
commit is meaningful and help the people that will explore a change in 2 years
Fixes #<issue number>
in the description if relevantFixes #<issue number>
if relevantThis pull request is for code commenting for TimvdLippe in regards to the Issue #3156 where there is an error with MockedStatic and Mock, so I am creating a catch for it with regards to previous methods. The main intention of this is to get some code feedback.
When running tests.