Fixes #3160 : Fix interference between spies when spying on records.

[LeMikaelF]

This fixes #3160. This is a bug where spied records end up having all their fields null. Here's a reproducer of the bug:

@Test
void myTest() {
    spy(List.of("something"));

    record MyRecord(String name) {}
    MyRecord spiedRecord = spy(new MyRecord("something"));
    assertEquals("something", spiedRecord.name()); // fails because spiedRecord.name() is null
}

The issue only occurs when spying records if AbstractCollection (or one of its children) is also spied. This is why this reproducer passes if the first line is commented out.

The problem happens because all superclasses and interfaces of java.util.ImmutableCollections.List12 (the implementation returned by List.of() are transformed by ByteBuddyAgent and InlineDelegateByteBuddyMockMaker (see InlineBytecodeGenerator::triggerRetransformation. However, one of the superclasses, AbstractCollection, happens to also be used by MethodHandle, and when it does, Mockito trips over itself and its fallback strategy also fails for records.

In other words, in the process of constructing a record for spying, InstrumentationMemberAccessor::newInstance calls MethodHandle::invokeWithArguments, which uses a collection. Since the AbstractCollection class was instrumented during the earlier spy creation, the construction is intercepted and calls isMockConstruction (in InlineDelegateByteBuddyMockMaker). Since the mockitoConstruction boolean is true, because there is indeed an ongoing mock construction, Mockito thinks that the current constructor AbstractCollection() is the correct constructor to implement, and isMockConstruction returns true. onConstruction then does one last check that the type of the constructor matches the object to spy, and when it doesn't, it throws an exception (InlineDelegateByteBuddyMockMaker line 287).

Mockito then considers that the spy creation has failed and falls back on creating a mock and then copying fields from the object to spy to the newly created mock (MockUtil::createMock). This strategy fails for records, because their fields cannot be set by MethodHandles::unreflectSetter (see the javadoc on the method), as opposed to classes, where even final fields can be set. This failure is then ignored, and fields are left uninitialized (see LenientCopyTool::copyValues). The interference betwen spies at the root of this issue also occurs for classes, but because the fallback can successfully copy the fields, the issue probably went unnoticed.

Testing: I was unable to add a test for this, because the language level is set to 11, before records existed. All existing tests are still passing, though, and the reproducer above fails on the master branch but passes on mine.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bfee15d) 85.50% compared to head (510a36a) 85.34%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #3173      +/-   ##
============================================
- Coverage     85.50%   85.34%   -0.16%     
+ Complexity     2914     2911       -3     
============================================
  Files           334      334              
  Lines          8864     8866       +2     
  Branches       1097     1099       +2     
============================================
- Hits           7579     7567      -12     
- Misses          995     1007      +12     
- Partials        290      292       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[TimvdLippe]

Please add a regression test for this. Otherwise good fix!

[LeMikaelF]

I did try adding a regression test, but the issue is, it needs a record to work, and Mockito is on java 11, before records existed. Is there somewhere I didn't look where I could add this test?

[TimvdLippe]

You can add a new subproject that uses a newer version of Java. See the module-test as inspiration when we were testing Java 9 stuff while still supporting Java 8. For this, we can name it record-test and not hardcode a specific version for it.

[TimvdLippe]

Similar to #3167 I think we should introduce subprojects/java21. Can you send a separate PR for that so that we can coordinate the fixes? Thanks!

[LeMikaelF]

I've added a test in the Java 21 module. It passes on this branch and fails on main.

[TimvdLippe]

Thanks for the fix!