Sporadic mock verification failures related to hashCode/equals on 3.12.1

[chadlwilson]

Since updating from 3.11.2 to 3.12.0 (and getting similar errors on 3.12.1), I have seen some strange sometimes-reproducible verification failures on Mockito for tests which previously passed consistently.

I can't yet establish a pattern or simple reproduction; it feels like it is somehow dependent on the order of test evaluation/usage with respect to other tests within the same JVM instance, and I have been unable to replicate locally.

There are two main classes I am seeing

1. mocks with interactions being tracked (and thus failing verifyNoInteractions(mock) after being put into HashSets (these did not seem to previously count as an interaction, and usually do not)

   com.thoughtworks.go.server.materials.postcommit.git.GitPostCommitHookImplementerTest > shouldReturnEmptyListIfParamHasNoValueForRepoURL() FAILED
       org.mockito.exceptions.verification.NoInteractionsWanted: 
       No interactions wanted here:
       -> at com.thoughtworks.go.server.materials.postcommit.git.GitPostCommitHookImplementerTest.shouldReturnEmptyListIfParamHasNoValueForRepoURL(GitPostCommitHookImplementerTest.java:93)
       But found this interaction on mock 'gitMaterial':
       -> at java.base/java.util.HashMap.hash(HashMap.java:340)
       Actually, above is the only interaction with this mock.
           at com.thoughtworks.go.server.materials.postcommit.git.GitPostCommitHookImplementerTest.shouldReturnEmptyListIfParamHasNoValueForRepoURL(GitPostCommitHookImplementerTest.java:93)
   

   https://github.com/gocd/gocd/blob/b7bba59201c857efc010c3493664802f218d2bae/server/src/test-fast/java/com/thoughtworks/go/server/materials/postcommit/git/GitPostCommitHookImplementerTest.java#L81-L94

2. mocks that are not considered equal to themselves when put inside a Collection and then searched for

   com.thoughtworks.go.server.materials.postcommit.git.GitPostCommitHookImplementerTest > shouldReturnListOfMaterialMatchingThePayloadURL() FAILED
       java.lang.AssertionError: 
       Expected: a collection containing <Mock for GitMaterial, hashCode: 0>
            but: was <Mock for GitMaterial, hashCode: 0>, was <Mock for GitMaterial, hashCode: 0>
           at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
           at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:8)
           at com.thoughtworks.go.server.materials.postcommit.git.GitPostCommitHookImplementerTest.shouldReturnListOfMaterialMatchingThePayloadURL(GitPostCommitHookImplementerTest.java:58)
   

   https://github.com/gocd/gocd/blob/b7bba59201c857efc010c3493664802f218d2bae/server/src/test-fast/java/com/thoughtworks/go/server/materials/postcommit/git/GitPostCommitHookImplementerTest.java#L41-L65

I have a handful of other examples too in different classes.

This could be confirmation bias, however since I saw some stuff was done with respect to hashCodes in #2331 I wondered if it could be related. Logging it here to see if anyone else has seen any similar weirdness or might know what is going on.

Environment

• Currently I cannot replicate locally (MacOS Big Sur w/AdoptOpenJDK 15.0.2) even where it is replicable on build/CI automation environment.
• The failures seem to be (somewhat) reproducible on AL2 Linux w/ Oracle JDK 15.0.1) but I cannot distinguish a pattern, and sometimes a different combination/ordering of tests does not seem to yield the same failure on exactly the same test code.

[vyazelenko]

I have the same issue after upgrade from 3.11.2 to 3.12.1, i.e. the mock is not equal to itself.
Here is an example of the recent failure in CI https://github.com/real-logic/agrona/runs/3401033492?check_suite_focus=true:

BroadcastReceiverTest > shouldReceiveTwoMessagesFromBuffer() FAILED
    java.lang.AssertionError: 
    Expected: is <Mock for UnsafeBuffer, hashCode: 0>
         but: was <Mock for UnsafeBuffer, hashCode: 0>
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
        at org.agrona.concurrent.broadcast.BroadcastReceiverTest.shouldReceiveTwoMessagesFromBuffer(BroadcastReceiverTest.java:131)

The issue is only reproducible in CI and only on Linux but is green locally or on Windows in CI.

[TimvdLippe]

Hm, the fallout of #2331 is indeed unfortunate. Apologies for the inconvenience.

Looking at the problems listed in this issue, the HashSet problem seems to me like it is working as intended. E.g. Mockito now correctly identifies a call on the mock (namely .hashCode()) and the code wasn't expecting that call to be there. I think it is fair to argue that the previous behavior was fine, but I think there is a reasonable argument to make that the new behavior is better.

With the second problem (that @vyazelenko also appears to run into), seems to be an issue with Hamcrest and assertThat. The issue is puzzling, as Mockito now correctly delegates any calls to equals to the mock. E.g. the real implementation of equals is called, which should be both safe (per the Object interface) and semantically more correct. I wonder if this might be an issue in Hamcrest itself (or at least the integration of both Hamcrest and Mockito).

While I think the new behavior is the way to go, especially in the .hashCode() situation, I think it is fair to introduce some way to ignore such calls. I wonder if we can add a setting on a mock that would say "you can call hashCode/equals and we don't mind" or something.

Would that be a workable solution for you?

[TimvdLippe]

Hm, wait. I just realized that both linked tests are using mocks, not spies. Then this behavior is unexpected and is a regression indeed. We should probably update the code from #2369 to only apply to spies.

Would one of you be willing to contribute a potential fix for this?

[vyazelenko]

@TimvdLippe Here is a reproducer for this issue:

public class TestMockito
{
    private final long time = System.currentTimeMillis();

    public boolean equals(final Object o)
    {
        if (!(o instanceof TestMockito))
        {
            return false;
        }
        return time == ((TestMockito)o).time;
    }

    public int hashCode()
    {
        return Long.hashCode(time);
    }

    public static void main(final String[] args)
    {
        final TestMockito test = new TestMockito();
        final TestMockito spy = Mockito.spy(test);
        final TestMockito mock = Mockito.mock(TestMockito.class);
        if (!spy.equals(test) || spy.hashCode() != test.hashCode())
        {
            throw new Error("Spy is broken!");
        }
        if (!mock.equals(mock) || mock.hashCode() != mock.hashCode())
        {
            throw new Error("Mock is broken!");
        }
    }
}

If you run this code as is with 3.12.1 you'll get the following exception:

Exception in thread "main" java.lang.Error: Mock is broken!
	at org.agrona.TestMockito.main(TestMockito.java:34)

However if you swap mock and spy calls, i.e. call mock before calling spy then it fails with:

Exception in thread "main" java.lang.Error: Spy is broken!
	at org.agrona.TestMockito.main(TestMockito.java:30)

So depending on what is called first the result changes.

[TimvdLippe]

Aha. So the issue is that the second call breaks things. That's very unexpected. I presume that's happening because we are leaving some invalid state somewhere. However, from the code changes, it is not obvious as to how that could be the case. @raphw do you mind doing a post-commit review of 123beb8 and see if we accidentally did something wrong there?

[vyazelenko]

@TimvdLippe The problem is with the org.mockito.internal.creation.bytebuddy.TypeCachingBytecodeGenerator#mockClass which does not include defaultAnswer when creating MockitoMockKey. Therefore both spy and mock end up using the same cached type and depending what is called first either of them becomes broken.

I'll add a PR to fix it.

[chadlwilson]

Thanks for digging @vyazelenko - seems to explain the semantics I observe. After digging around, I believe the cases we had all had both spies and mocks created across different tests for the same class.

[vyazelenko]

PR that fixes this issue #2400.

[TimvdLippe]

3.12.2 is currently being pushed to Maven Central and should be available in a couple of hours.

[vyazelenko]

@TimvdLippe Thanks for a quick release!

[chadlwilson]

For anyone else who stumbles across this due to problems in their environments, it appears the changes in #2369 at the root of this breakage have been subsequently reverted in 3.12.3 based on PR #2402.

[vyazelenko]

@chadlwilson Correction. the changes in #2400 are not the root the breakage but the PR #2369 is. The #2402 reverted both, because #2400 is not needed if the #2369 is removed.

[chadlwilson]

Sorry my bad, I meant to link to #2369 as the root of the breakage rather than #2400 (will edit)