Repository: mockito/mockito
Issue: Sporadic mock verification failures related to hashCode/equals on 3.12.1

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 `HashSet`s (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.

Issue comments:
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.
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?
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?
@TimvdLippe Here is a reproducer for this issue:
```java
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:
```java
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:
```java
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.
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 https://github.com/mockito/mockito/commit/123beb8311ed6303ba71d682cff8434fe36b7f0f and see if we accidentally did something wrong there?
@TimvdLippe The problem is with the [org.mockito.internal.creation.bytebuddy.TypeCachingBytecodeGenerator#mockClass](https://github.com/mockito/mockito/blob/b04c7039031c170dae5f90acb3ec74c16289886f/src/main/java/org/mockito/internal/creation/bytebuddy/TypeCachingBytecodeGenerator.java#L42-L46) 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.
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.
PR that fixes this issue https://github.com/mockito/mockito/pull/2400.
3.12.2 is currently being pushed to Maven Central and should be available in a couple of hours.
@TimvdLippe Thanks for a quick release!
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.
@chadlwilson Correction. the changes in https://github.com/mockito/mockito/pull/2400 are **not** the root the breakage but the PR https://github.com/mockito/mockito/pull/2369 is. The https://github.com/mockito/mockito/pull/2402 reverted both, because https://github.com/mockito/mockito/pull/2400 is not needed if the https://github.com/mockito/mockito/pull/2369 is removed.
Sorry my bad, I meant to link to #2369 as the root of the breakage rather than #2400 (will edit)

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/org/mockito/internal/creation/bytebuddy/TypeCachingBytecodeGenerator.java b/src/main/java/org/mockito/internal/creation/bytebuddy/TypeCachingBytecodeGenerator.java
index 30ed949bf5..5ce57b1115 100644
--- a/src/main/java/org/mockito/internal/creation/bytebuddy/TypeCachingBytecodeGenerator.java
+++ b/src/main/java/org/mockito/internal/creation/bytebuddy/TypeCachingBytecodeGenerator.java
@@ -5,12 +5,14 @@
 package org.mockito.internal.creation.bytebuddy;
 
 import java.lang.ref.ReferenceQueue;
+import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.locks.ReadWriteLock;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 import net.bytebuddy.TypeCache;
 import org.mockito.mock.SerializableMode;
+import org.mockito.stubbing.Answer;
 
 class TypeCachingBytecodeGenerator extends ReferenceQueue<ClassLoader>
         implements BytecodeGenerator {
@@ -43,7 +45,8 @@ public <T> Class<T> mockClass(final MockFeatures<T> params) {
                                     params.mockedType,
                                     params.interfaces,
                                     params.serializableMode,
-                                    params.stripAnnotations),
+                                    params.stripAnnotations,
+                                    params.defaultAnswer),
                             () -> bytecodeGenerator.mockClass(params),
                             BOOTSTRAP_LOCK);
         } catch (IllegalArgumentException exception) {
@@ -83,15 +86,18 @@ private static class MockitoMockKey extends TypeCache.SimpleKey {
 
         private final SerializableMode serializableMode;
         private final boolean stripAnnotations;
+        private final Answer defaultAnswer;
 
         private MockitoMockKey(
                 Class<?> type,
                 Set<Class<?>> additionalType,
                 SerializableMode serializableMode,
-                boolean stripAnnotations) {
+                boolean stripAnnotations,
+                Answer defaultAnswer) {
             super(type, additionalType);
             this.serializableMode = serializableMode;
             this.stripAnnotations = stripAnnotations;
+            this.defaultAnswer = defaultAnswer;
         }
 
         @Override
@@ -107,7 +113,8 @@ public boolean equals(Object object) {
             }
             MockitoMockKey that = (MockitoMockKey) object;
             return stripAnnotations == that.stripAnnotations
-                    && serializableMode.equals(that.serializableMode);
+                    && serializableMode.equals(that.serializableMode)
+                    && Objects.equals(defaultAnswer, that.defaultAnswer);
         }
 
         @Override
@@ -115,6 +122,7 @@ public int hashCode() {
             int result = super.hashCode();
             result = 31 * result + (stripAnnotations ? 1 : 0);
             result = 31 * result + serializableMode.hashCode();
+            result = 31 * result + Objects.hashCode(defaultAnswer);
             return result;
         }
     }


Test patch:
diff --git a/src/test/java/org/mockito/internal/creation/bytebuddy/TypeCachingMockBytecodeGeneratorTest.java b/src/test/java/org/mockito/internal/creation/bytebuddy/TypeCachingMockBytecodeGeneratorTest.java
index 6ffe72950f..e26b36d531 100644
--- a/src/test/java/org/mockito/internal/creation/bytebuddy/TypeCachingMockBytecodeGeneratorTest.java
+++ b/src/test/java/org/mockito/internal/creation/bytebuddy/TypeCachingMockBytecodeGeneratorTest.java
@@ -14,6 +14,8 @@
 import java.lang.ref.ReferenceQueue;
 import java.lang.ref.WeakReference;
 import java.util.Collections;
+import java.util.IdentityHashMap;
+import java.util.Set;
 import java.util.WeakHashMap;
 
 import org.junit.Before;
@@ -142,6 +144,42 @@ public void ensure_cache_returns_different_instance_serializableMode() throws Ex
         assertThat(other_mock_type).isNotSameAs(the_mock_type);
     }
 
+    @Test
+    public void ensure_cache_returns_different_instance_defaultAnswer() throws Exception {
+        // given
+        ClassLoader classloader_with_life_shorter_than_cache =
+                inMemoryClassLoader()
+                        .withClassDefinition("foo.Bar", makeMarkerInterface("foo.Bar"))
+                        .build();
+
+        TypeCachingBytecodeGenerator cachingMockBytecodeGenerator =
+                new TypeCachingBytecodeGenerator(new SubclassBytecodeGenerator(), true);
+
+        Answers[] answers = Answers.values();
+        Set<Class<?>> classes = Collections.newSetFromMap(new IdentityHashMap<>());
+        classes.add(
+                cachingMockBytecodeGenerator.mockClass(
+                        withMockFeatures(
+                                classloader_with_life_shorter_than_cache.loadClass("foo.Bar"),
+                                Collections.<Class<?>>emptySet(),
+                                SerializableMode.NONE,
+                                false,
+                                null)));
+        for (Answers answer : answers) {
+            Class<?> klass =
+                    cachingMockBytecodeGenerator.mockClass(
+                            withMockFeatures(
+                                    classloader_with_life_shorter_than_cache.loadClass("foo.Bar"),
+                                    Collections.<Class<?>>emptySet(),
+                                    SerializableMode.NONE,
+                                    false,
+                                    answer));
+            assertThat(classes.add(klass)).isTrue();
+        }
+
+        assertThat(classes).hasSize(answers.length + 1);
+    }
+
     @Test
     public void
             validate_simple_code_idea_where_weakhashmap_with_classloader_as_key_get_GCed_when_no_more_references()
diff --git a/src/test/java/org/mockitousage/spies/MockCreationShouldNotAffectSpyTest.java b/src/test/java/org/mockitousage/spies/MockCreationShouldNotAffectSpyTest.java
new file mode 100644
index 0000000000..c77e589594
--- /dev/null
+++ b/src/test/java/org/mockitousage/spies/MockCreationShouldNotAffectSpyTest.java
@@ -0,0 +1,48 @@
+/*
+ * Copyright (c) 2007 Mockito contributors
+ * This program is made available under the terms of the MIT License.
+ */
+package org.mockitousage.spies;
+
+import org.junit.Test;
+import org.mockitoutil.TestBase;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+
+public class MockCreationShouldNotAffectSpyTest extends TestBase {
+
+    @Test
+    public void test() {
+        TestClass instance = new TestClass(42);
+
+        TestClass mock = mock(TestClass.class);
+        assertEquals(mock.hashCode(), mock.hashCode());
+        assertEquals(mock, mock);
+
+        TestClass spy = spy(instance);
+        assertEquals(instance.hashCode(), spy.hashCode());
+        assertEquals(spy, instance);
+        assertEquals(instance, spy);
+    }
+
+    static class TestClass {
+        private final long value;
+
+        TestClass(final long value) {
+            this.value = value;
+        }
+
+        public boolean equals(final Object o) {
+            if (!(o instanceof TestClass)) {
+                return false;
+            }
+            return value == ((TestClass) o).value;
+        }
+
+        public int hashCode() {
+            return Long.hashCode(value);
+        }
+    }
+}
diff --git a/src/test/java/org/mockitousage/spies/SpyCreationShouldNotAffectMockTest.java b/src/test/java/org/mockitousage/spies/SpyCreationShouldNotAffectMockTest.java
new file mode 100644
index 0000000000..a57ec082c1
--- /dev/null
+++ b/src/test/java/org/mockitousage/spies/SpyCreationShouldNotAffectMockTest.java
@@ -0,0 +1,48 @@
+/*
+ * Copyright (c) 2007 Mockito contributors
+ * This program is made available under the terms of the MIT License.
+ */
+package org.mockitousage.spies;
+
+import org.junit.Test;
+import org.mockitoutil.TestBase;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
+
+public class SpyCreationShouldNotAffectMockTest extends TestBase {
+
+    @Test
+    public void test() {
+        TestClass instance = new TestClass(Long.MIN_VALUE);
+
+        TestClass spy = spy(instance);
+        assertEquals(instance.hashCode(), spy.hashCode());
+        assertEquals(spy, instance);
+        assertEquals(instance, spy);
+
+        TestClass mock = mock(TestClass.class);
+        assertEquals(mock.hashCode(), mock.hashCode());
+        assertEquals(mock, mock);
+    }
+
+    static class TestClass {
+        private final long value;
+
+        TestClass(final long value) {
+            this.value = value;
+        }
+
+        public boolean equals(final Object o) {
+            if (!(o instanceof TestClass)) {
+                return false;
+            }
+            return value == ((TestClass) o).value;
+        }
+
+        public int hashCode() {
+            return Long.hashCode(value);
+        }
+    }
+}