Switch the default mockmaker to the inline mockmaker on JDK 17+

[reta]

Signed-off-by: Andriy Redko drreta@gmail.com

Fixes #2589

Checklist

• Read the contributing guide
• PR should be motivated, i.e. what does it fix, why, and if relevant how
• If possible / relevant include an example in the description, that could help all readers
  including project members to get a better picture of the change
• Avoid other runtime dependencies
• Meaningful commit history ; intention is important please rebase your commit history so that each
  commit is meaningful and help the people that will explore a change in 2 years
• The pull request follows coding style
• Mention Fixes #<issue number> in the description if relevant
• At least one commit should mention Fixes #<issue number> if relevant

[TimvdLippe]

Seems like there are a couple of test failures :( Some of these look cryptic with the missing MethodDispatcher file. We had that before and I thought we fixed that? Do we need the fix that we did for mockito-inline maybe: https://github.com/mockito/mockito/blob/main/subprojects/inline/src/main/resources/mockito-extensions/org.mockito.plugins.MemberAccessor

[reta]

Seems like there are a couple of test failures :( Some of these look cryptic with the missing MethodDispatcher file. We had that before and I thought we fixed that?

Thanks @TimvdLippe , yeah, I am looking into that but it seems like we have to change not only MockMaker default but the MemberAccessor as well (as you pointed out) to work with the new defaults.

[TimvdLippe]

@reta Thanks for all the work thus far! This is the last PR for Mockito 5, is there something I can help you out with?

[reta]

@reta Thanks for all the work thus far! This is the last PR for Mockito 5, is there something I can help you out with?

Thanks @TimvdLippe , I am slowly crunching over test cases (it takes time for me to troubleshoot some of them ) so far there are just few left in Mockito core, I hope to fix them in the next couple of days:

    DefaultInternalRunnerTest. does_not_fail_second_test_when_first_test_fail
    ReturnsSmartNullsTest. should_return_a_new_mock_that_has_been_defined_with_method_generic_and_provided_in_argument
    ReturnsSmartNullsTest. should_return_a_new_mock_that_has_been_defined_with_method_generic_and_provided_in_var_args
    ReturnsSmartNullsTest. should_return_an_object_that_has_been_defined_with_class_generic
    ParameterizedConstructorInstantiatorTest. should_fail_if_an_argument_instance_type_do_not_match_wanted_type
    PartialMockingWithSpiesTest. shouldStackTraceGetFilteredOnUserExceptions

The real blocker it seems like is OSGi, where inline mock maker may not be used as a default: the issue is that InlineDelegateByteBuddyMockMaker creates mockitoboot.jar and modifies classpath at runtime, not very OSGi friendly way. If you have time to look at it (osgi-test module), that would be very helpful.

Thank you!

[TimvdLippe]

@raphw do you have some more context on the previous comment with regards to OSGi?

[raphw]

Hmm, the only possible solution I see would be to move MockMethodDispatcher to the java.lang package. This would then also require that we use unsafe to inject the class instead of the official API. This way, we would no longer be fully Java API compatible.

Alternatively, we could instrument all class loaders to always load the dispatcher class from the boot loader to possibly override any custom behavior. But this is rather intrusive. Maybe we could make this configurable?

What do you think would be the best way forward?

[reta]

Alternatively, we could instrument all class loaders to always load the dispatcher class from the boot loader to possibly override any custom behavior. But this is rather intrusive. Maybe we could make this configurable?

@raphw I think this is the best option to try so far and should work with most (if not all) OSGi containters, luckily most OSGi runtimes support customization of boot classloader delegation, I was able to fix our test cases which are based of Eclipse Equinox (and boot loader classpath is altered by instrumentation):

configuration.put(Constants.FRAMEWORK_BOOTDELEGATION, "org.mockito.internal.creation.bytebuddy.inject");

[codecov-commenter]

Codecov Report

Base: 84.88% // Head: 84.78% // Decreases project coverage by -0.10% ⚠️

Coverage data is based on head (4a943da) compared to base (e910bcc).
Patch coverage: 96.15% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2834      +/-   ##
============================================
- Coverage     84.88%   84.78%   -0.11%     
+ Complexity     2875     2868       -7     
============================================
  Files           327      327              
  Lines          8761     8780      +19     
  Branches       1060     1068       +8     
============================================
+ Hits           7437     7444       +7     
- Misses         1047     1063      +16     
+ Partials        277      273       -4     

Impacted Files	Coverage Δ
...nal/stubbing/defaultanswers/ReturnsSmartNulls.java	96.96% <90.00%> (-3.04%)	⬇️
...l/configuration/plugins/DefaultMockitoPlugins.java	91.89% <100.00%> (+0.22%)	⬆️
...al/creation/bytebuddy/InlineBytecodeGenerator.java	85.23% <100.00%> (+3.87%)	⬆️
...xceptions/stacktrace/DefaultStackTraceCleaner.java	100.00% <100.00%> (ø)
...util/reflection/InstrumentationMemberAccessor.java	89.32% <100.00%> (+5.65%)	⬆️
src/main/java/org/mockito/plugins/MockMaker.java	0.00% <0.00%> (-87.50%)	⬇️
...creation/instance/DefaultInstantiatorProvider.java	50.00% <0.00%> (-50.00%)	⬇️
...a/org/mockito/internal/debugging/LocationImpl.java	0.00% <0.00%> (-35.09%)	⬇️
.../mockito/internal/util/reflection/FieldReader.java	77.77% <0.00%> (-22.23%)	⬇️
...creation/bytebuddy/SubclassByteBuddyMockMaker.java	50.00% <0.00%> (-21.63%)	⬇️
... and 13 more

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.

[TimvdLippe]

I have reviewed all of these changes and they LGTM. Massive work @reta, thank you so much!

I am a bit confused as to why some of the tests weren't previously failing in our mockmaker-inline CI branch (the mock-maker-inline job in our matrix). However, that's not a problem for this PR and something later to figure out.

@raphw do you mind doing a full check as well, just to make sure everything looks okay from your side? Then we can merge this and publish Mockito 5 🎉

[reta]

Thanks @TimvdLippe !

I am a bit confused as to why some of the tests weren't previously failing in our mockmaker-inline CI branch (the mock-maker-inline job in our matrix). However, that's not a problem for this PR and something later to figure out.

I think there is a gap in tests: the tests get mock maker instance (from environment setting) but not the member accessor it supposed to be used with. As such, the tests used reflection, not module accessor, all the time. Does it make sense?

[raphw]

This looks very good to me, but I think the OSGi configuration deserves some explicit documentation.

[TimvdLippe]

@reta yes that makes sense! Let's fix that in a separate PR. This looks good to me, thank you so much again!