Fixes #3005 #3123
Fixes #3005 #3123
Conversation
fixes mockito#3005 and others
Signed-off-by: Harald Fassler <harald.fassler+9974@gmail.com>
|
My feedback from #3022 (comment) still stands. This PR is massive and the additional code is not worth the complications that |
|
Hmm... not sure, where the separate artefact should exactly hook into. The situation for the other extensions loaded through Maybe you have a concrete idea how to achieve what you proposed, then please let me know. So I propose of the following:
|
Signed-off-by: Harald Fassler <harald.fassler+9974@gmail.com>
|
@TimvdLippe I have now moved the changes into a separate submodule "inject-generics". To achieve that I made the following steps:
Is that an approach you can live with? The name of the new submodule is a matter of taste, if you think of a better name, pls. let me know. Deprecation of Please let me know, what you think. |
|
Hey, this approach looks a lot better indeed! Unfortunately I am quite busy atm with work, but will get back to you the weekend after next for a full review. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3123 +/- ##
============================================
- Coverage 85.48% 85.32% -0.17%
- Complexity 2892 2896 +4
============================================
Files 329 339 +10
Lines 8826 9131 +305
Branches 1095 1160 +65
============================================
+ Hits 7545 7791 +246
- Misses 994 1014 +20
- Partials 287 326 +39
☔ View full report in Codecov by Sentry. |
Signed-off-by: Harald Fassler <harald.fassler+9974@gmail.com>
Signed-off-by: Harald Fassler <harald.fassler+9974@gmail.com>
|
As you probably noticed, I haven't had the time to look at this PR. My current precious free time is spent on #3037 which requires our attention at the moment. Hopefully when that discussion settles, we can go back to this PR. |
|
@nineninesevenfour thanks for working on this! My team is also eagerly waiting for this set of fixes. There seems to be some conflicts already, I wonder if you have time to resolve them before @TimvdLippe hops in to review the changes? |
There was a problem hiding this comment.
There's a lot of code here which took me a while to get through. Overall, I like the regression tests and I understand why all of the implementation is required. Therefore, I think this is good to be optional for users to include, as the overhead of these filters can be large, but sometimes is required to get things right.
I left some comments on small improvements that we need to make, but overall this approach LGTM! Thanks again for the work and apologies for the long review time because of the large content of this PR.
| @@ -63,6 +65,8 @@ public class DefaultMockitoPlugins implements MockitoPlugins { | |||
| DEFAULT_PLUGINS.put( | |||
| DoNotMockEnforcer.class.getName(), | |||
| "org.mockito.internal.configuration.DefaultDoNotMockEnforcer"); | |||
| DEFAULT_PLUGINS.put( | |||
| MockCandidateFilter.class.getName(), TypeBasedCandidateFilter.class.getName()); | |||
There was a problem hiding this comment.
While type-safety is nice, we don't want to refer to the concrete implementations here so to avoid loading the classes if they are overridden. Therefore, let's use the string names here instead like the other plugin definitions.
| apply from: "$rootDir/gradle/java-library.gradle" | ||
| apply from: "$rootDir/gradle/root/coverage.gradle" | ||
|
|
||
| description = "Filter @InjectMocks according to Java's assignment rules for generics" |
There was a problem hiding this comment.
Should we make this artifact name inject-mocks instead? The longterm plan would be to move @InjectMocks into this package so that users can rely on that instead. While the current artifact would solve the problem with generics, I ultimately want to move @InjectMocks here and move all related machinery into one place.
| "integerList ,stringList ,false", | ||
| "stringList ,integerList ,false", | ||
|
|
||
| "stringList ,wildcardList ,true", |
There was a problem hiding this comment.
Nit: add a case for "integerList ,wildcardListInteger ,true",
| @CsvSource({ | ||
| "integerList ,integerCollectionBox ,CollectionBox ,collection ,true", | ||
| "integerList ,stringCollectionBox ,CollectionBox ,collection ,false", | ||
| "integerList ,wildcardCollectionBox ,CollectionBox ,collection ,false", |
There was a problem hiding this comment.
In the previous test stringList was assignable to wildcardList. I would have expected the same here. Why is it false in this case?
| * Mockito won't fail to inject (even if mock field name doesn't match under test's field name). | ||
| */ | ||
| @ExtendWith(MockitoExtension.class) | ||
| public class GenericTypeMockTest { |
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 relevantFixes several issues with @Injectmocks when generics are involved. Rewrite of matching logic based on resolving the concrete types down the type hierarchy.