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

Feature 2921 generic types #2923

Merged
merged 27 commits into from
Mar 9, 2023

Conversation

jfrantzius
Copy link
Contributor

Fixes #2921 by preserving and using the generic Java type information that was previously lost to type erasure.

Contains a JUnit test with usage example, demonstrating that fields with generic types do not need to have matching names anymore.

@codecov-commenter
Copy link

codecov-commenter commented Feb 27, 2023

Codecov Report

Patch coverage: 87.71% and project coverage change: +0.01 🎉

Comparison is base (8b96cc1) 85.65% compared to head (3ac47be) 85.66%.

❗ Current head 3ac47be differs from pull request most recent head 40a2fe6. Consider uploading reports for the commit 40a2fe6 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2923      +/-   ##
============================================
+ Coverage     85.65%   85.66%   +0.01%     
- Complexity     2847     2861      +14     
============================================
  Files           325      325              
  Lines          8623     8678      +55     
  Branches       1060     1070      +10     
============================================
+ Hits           7386     7434      +48     
- Misses          964      967       +3     
- Partials        273      277       +4     
Impacted Files Coverage Δ
...guration/injection/PropertyAndSetterInjection.java 97.67% <ø> (ø)
...ion/injection/filter/NameBasedCandidateFilter.java 100.00% <ø> (ø)
.../injection/filter/TerminalMockCandidateFilter.java 84.61% <ø> (ø)
...o/internal/creation/settings/CreationSettings.java 96.49% <50.00%> (-3.51%) ⬇️
...ion/injection/filter/TypeBasedCandidateFilter.java 90.56% <88.88%> (-9.44%) ⬇️
...nternal/configuration/MockAnnotationProcessor.java 97.36% <100.00%> (+0.07%) ⬆️
...to/internal/configuration/SpyAnnotationEngine.java 98.50% <100.00%> (+0.09%) ⬆️
...rg/mockito/internal/creation/MockSettingsImpl.java 88.57% <100.00%> (+0.22%) ⬆️
...nal/creation/instance/ConstructorInstantiator.java 96.20% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

WildcardType wildcardTypeToMock = (WildcardType) typeToMock;
Type[] upperBounds = wildcardTypeToMock.getUpperBounds();
return Arrays.stream(upperBounds).anyMatch(t -> isCompatibleTypes(t, mockType));
} else if (typeToMock instanceof GenericArrayType && mockType instanceof GenericArrayType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

CodeCov reports this branch is not covered. Can you please expand the coverage here or remove this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The WildcardType part is covered:
image
But you are right that the GenericTypeArrayType part from line 64 isn't. I'll remove it, as Mockito cannot mock Array types (as I found out in a test)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed not covered code

Copy link
Contributor

Choose a reason for hiding this comment

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

There is still some coverage missing, can you either remove the uncovered branches or add regression tests that cover the logic? https://app.codecov.io/gh/mockito/mockito/pull/2923/blob/src/main/java/org/mockito/internal/configuration/injection/filter/TypeBasedCandidateFilter.java Since this is intricate logic and a hot codepath, I would like to see full coverage here.

@jfrantzius
Copy link
Contributor Author

@TimvdLippe please review and resolve where appropriate

@jfrantzius
Copy link
Contributor Author

https://github.com/mockito/mockito/actions/runs/4296719412/jobs/7488794171

> Task :androidTest:connectedDebugAndroidTest
Starting 0 tests on test(AVD) - 13
Test run failed to complete. No test results. onError: commandError=true message=Attempt to invoke interface method 'boolean android.app.IActivityManager.startInstrumentation(android.content.ComponentName, java.lang.String, int, android.os.Bundle, android.app.IInstrumentationWatcher, android.app.IUiAutomationConnection, int, java.lang.String)' on a null object reference

> Task :androidTest:connectedDebugAndroidTest FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':androidTest:connectedDebugAndroidTest'.
> There were failing tests. See the report at: file:///Users/runner/work/mockito/mockito/subprojects/androidTest/build/reports/androidTests/connected/index.html

I don't know what to make of this, maybe just re-run?

@jfrantzius
Copy link
Contributor Author

@TimvdLippe thx for restarting the build. Did you have a chance to take a look at my comments on your review?

Sorry for maybe being pushy, but I have some time on my hands for this at the moment, which might change soon. Also I don't know under what circumstances you are able to devote time to Mockito :)

@TimvdLippe
Copy link
Contributor

I am on vacation this week, but should be back next week to review.

@jfrantzius
Copy link
Contributor Author

Alright, have a nice vacation!

@TimvdLippe
Copy link
Contributor

@jfrantzius Apart from the missing code coverage, this PR LGTM!

@jfrantzius
Copy link
Contributor Author

@TimvdLippe I added more test coverage

@TimvdLippe
Copy link
Contributor

@jfrantzius please look at the code coverage report and see which branches are missing coverage. There are still some missing, for example one where the generic types don't match.

@jfrantzius
Copy link
Contributor Author

I added support for generic injectee classes, e.g.

        public class UnderTestWithTypeParameters<T1, T2> {
            List<T1> t1List;
            Set<T2> t2Set;
        }

There is one line that cannot be covered in tests, please see the comment there: https://github.com/jfrantzius/mockito/blob/feature-2921-generic-type/src/main/java/org/mockito/internal/configuration/injection/filter/TypeBasedCandidateFilter.java#L70

@jfrantzius
Copy link
Contributor Author

Could you please approve the workflow @TimvdLippe ?

@jfrantzius
Copy link
Contributor Author

@TimvdLippe I'm off for today, if you trigger the build then the last line missing in coverage should now be covered as well.

@jfrantzius
Copy link
Contributor Author

I guess it's time to setup Spotless in my IDE ...

@jfrantzius
Copy link
Contributor Author

I would love to be able to start builds, but unfortunately I have to always bug you about it @TimvdLippe :)

@jfrantzius
Copy link
Contributor Author

@TimvdLippe build might need another nudge in https://github.com/mockito/mockito/actions/runs/4372977101/jobs/7658608894

/bin/sh -c exit $(cat gradle.exit)
Error: The process '/bin/sh' failed with exit code 1
Terminate Emulator
INFO    | Wait for emulator (pid 3622) 20 seconds to shutdown gracefully before kill;you can set environment variable ANDROID_EMULATOR_WAIT_TIME_BEFORE_KILL(in seconds) to change the default value (20 seconds)

@jfrantzius
Copy link
Contributor Author

WDYT @TimvdLippe , is it good to merge?

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

Thanks for all the work, this LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use generic type information in TypeBasedCandidateFilter to circumvent type erasure
4 participants