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
Only run injection once #2603
Only run injection once #2603
Conversation
See issue #2602 for context |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2603 +/- ##
============================================
- Coverage 85.70% 85.69% -0.01%
+ Complexity 2876 2874 -2
============================================
Files 326 326
Lines 8733 8727 -6
Branches 1078 1077 -1
============================================
- Hits 7485 7479 -6
Misses 971 971
Partials 277 277
☔ View full report in Codecov by Sentry. |
src/test/java/org/mockito/internal/configuration/InjectingAnnotationEngineTest.java
Show resolved
Hide resolved
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 legit
@ZNhatAnhZ sadly nobody ever looked at it :D The current state is a mess. My Pull Request cleans up the mess and aliens the code with what is promised in the docs. It therefore has to remove a strange integration test that tests a bug(or a feature that was never there). |
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.
Based on user feedback, this is the appropriate fix after all. Running the tests one more time and then will merge it. Sorry for the long wait, it was unclear to me at the time what the appropriate behaviour of Mockito is.
ToBeMockedInTestSuperClass toBeMockedInTestSuperClass; | ||
|
||
public TestClassToBeInitiatedViaConstructorInSuperClass(ToBeMockedInTestSuperClass toBeMockedInTestSuperClass) { | ||
assert toBeMockedInTestSuperClass != null; |
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.
These asserts need to be changed to AssertJ
, as it fails the build.
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.
@1-alex98 Did you see this comment? We need to change the code, not just a rebase.
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.
yes I did that locally. I am not sure why that did not make it here. I will check if I am home. I probably forgot to push or something. @TimvdLippe
I will update the PR tonight if I find the time |
1556f76
to
b4d89f9
Compare
Add regression test that makers sure constructor injection is used when possible Fixes mockito#2602
b4d89f9
to
27991f0
Compare
Add regression test that makers sure constructor injection is used when possible
Fixes #2602
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 relevant