fix: don't call methods on mock objects while recording

Recording a value called toString() on it. When the value is a mock,
that call is an invocation as far as the mocking framework is concerned,
and mocking frameworks keep thread-local state between one call and the
next. Mockito collects argument matchers on a stack and binds them to
the next invocation it sees, and when() applies to the most recent
invocation. A toString() from the agent lands in the middle of that
handshake, so Mockito attributes the matcher or the stubbing to the
agent's call instead of the test's.

The result is that the stubbing a test set up silently never applies,
and the stubbed method returns the Java type default. A test that does

    when(service.getData()).thenReturn("expectedValue");

fails with "expected:<expectedValue> but was:<null>" under the agent and
passes without it. Where all arguments use matchers, Mockito reports
"2 matchers expected, 1 recorded" instead.

Recognize mocks and record a placeholder rather than asking them for a
value. Detection goes through the mocking framework's own API, because
the class name isn't a reliable signal: Mockito's inline mock maker
(the default since 5.0) mocks a class by retransforming it, so a mock
of com.example.Foo is an instance of com.example.Foo. The lookup is
resolved against the class loader of the object being inspected and
cached per class, so it costs nothing when no mocking framework is on
the class path.

The new test project has a control task that runs the same tests with
no agent attached, so a failure under the agent is attributable.

Assisted-by: Claude:claude-opus-5

Author: dividedmind

agent/src/main/java/com/appland/appmap/output/v1/Value.java

@@ -4,6 +4,7 @@
 import com.alibaba.fastjson.serializer.ToStringSerializer;
 import com.appland.appmap.config.Properties;
 import com.appland.appmap.util.Logger;
+import com.appland.appmap.util.MockDetector;
 import org.apache.commons.lang3.StringUtils;
 
 /**
@@ -117,6 +118,11 @@ public Value freeze() {
     if (this.value != null) {
       if (Properties.DisableValue) {
         this.value = "< disabled >";
+      } else if (MockDetector.isMock(this.value)) {
+        // Calling toString() on a mock is an interaction, and it consumes the
+        // mocking framework's pending thread-local state. That breaks the
+        // user's stubbing, so record a placeholder instead.
+        this.value = MockDetector.MOCK_VALUE;
       } else {
         try {
           this.value = this.value.toString();

agent/src/main/java/com/appland/appmap/util/MockDetector.java

@@ -0,0 +1,170 @@
+package com.appland.appmap.util;
+
+import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import org.tinylog.TaggedLogger;
+
+import com.appland.appmap.config.AppMapConfig;
+
+/**
+ * Recognizes mock objects, so that recording them doesn't call their methods.
+ *
+ * <p>
+ * Calling a method on a mock is an interaction as far as the mocking framework
+ * is concerned, and mocking frameworks keep thread-local state between one call
+ * and the next. Mockito, for example, collects argument matchers on a
+ * thread-local stack and binds them to the next invocation it sees. A stray
+ * {@code toString()} from the agent binds that matcher to the wrong call, so
+ * the stubbing the test set up never applies and the test fails with a value
+ * the user never configured.
+ *
+ * <p>
+ * Detection goes through the mocking framework's own API, because the class
+ * name isn't a reliable signal: Mockito's inline mock maker mocks a class by
+ * retransforming it, so a mock of {@code com.example.Foo} is an instance of
+ * {@code com.example.Foo}.
+ */
+public class MockDetector {
+  private static final TaggedLogger logger = AppMapConfig.getLogger(null);
+
+  /** Placeholder recorded in place of a mock's value. */
+  public static final String MOCK_VALUE = "[mocked]";
+
+  /**
+   * Types no mock maker will mock, so an instance of one is never a mock.
+   *
+   * <p>
+   * These are the values AppMap records most often, and skipping the lookup for
+   * them keeps the common path as cheap as it was before mock detection
+   * existed. The list is deliberately conservative -- a type wrongly listed
+   * here would reintroduce the bug this class exists to prevent -- and matches
+   * what Mockito's {@code MockMaker.isTypeMockable} rejects. Note that enums
+   * are absent on purpose: Mockito mocks those quite happily.
+   */
+  private static final Set<Class<?>> NEVER_MOCKABLE = new HashSet<Class<?>>(Arrays.asList(
+      String.class,
+      Class.class,
+      Boolean.class,
+      Byte.class,
+      Character.class,
+      Short.class,
+      Integer.class,
+      Long.class,
+      Float.class,
+      Double.class));
+
+  /**
+   * Mockito's mock-detection API, or null if it can't be reached for a given
+   * class. Cached per class, so applications with no mocking framework on the
+   * class path pay one {@link ClassValue} read and nothing else.
+   */
+  private static final ClassValue<Method[]> MOCKITO = new ClassValue<Method[]>() {
+    @Override
+    protected Method[] computeValue(Class<?> type) {
+      for (ClassLoader loader : candidateLoaders(type)) {
+        Method[] api = resolve(loader);
+        if (api != null) {
+          return api;
+        }
+      }
+      return null;
+    }
+  };
+
+  /**
+   * Class loaders that might be able to see Mockito, most specific first.
+   *
+   * <p>
+   * The object's own loader isn't enough. The inline mock maker mocks a class
+   * by retransforming it, so a mock can be an instance of any mockable type --
+   * including a bootstrap type like {@code java.util.ArrayList}, whose loader
+   * can't see Mockito at all.
+   *
+   * <p>
+   * Only loaders that are fixed for the lifetime of the JVM are considered,
+   * because the result is cached per class. Including the thread context loader
+   * would make the cached answer depend on whichever thread happened to record
+   * the first value of that type.
+   */
+  private static List<ClassLoader> candidateLoaders(Class<?> type) {
+    List<ClassLoader> loaders = new ArrayList<ClassLoader>(3);
+    addLoader(loaders, type.getClassLoader());
+    addLoader(loaders, MockDetector.class.getClassLoader());
+    try {
+      addLoader(loaders, ClassLoader.getSystemClassLoader());
+    } catch (Throwable t) {
+      // Can happen very early in startup, or under a security manager.
+      logger.debug(t, "couldn't get the system class loader");
+    }
+    return loaders;
+  }
+
+  /** Adds a loader, skipping the bootstrap loader and duplicates. */
+  private static void addLoader(List<ClassLoader> loaders, ClassLoader loader) {
+    // The bootstrap loader can't see a mocking framework, so there's no point
+    // asking it.
+    if (loader != null && !loaders.contains(loader)) {
+      loaders.add(loader);
+    }
+  }
+
+  private static Method[] resolve(ClassLoader loader) {
+    try {
+      Method mockingDetails = Class.forName("org.mockito.Mockito", false, loader)
+          .getMethod("mockingDetails", Object.class);
+      Method isMock = Class.forName("org.mockito.MockingDetails", false, loader)
+          .getMethod("isMock");
+      return new Method[] {mockingDetails, isMock};
+    } catch (ClassNotFoundException e) {
+      // Mockito isn't visible from this loader. Nothing to do, and not worth
+      // logging: it's the normal case outside of tests.
+      return null;
+    } catch (Throwable t) {
+      // A Mockito we don't recognize. Degrade to treating objects as
+      // non-mocks rather than failing.
+      logger.debug(t, "couldn't find Mockito's mock detection API");
+      return null;
+    }
+  }
+
+  private MockDetector() {
+  }
+
+  /**
+   * Checks whether an object is a mock. Only inspects the mocking framework's
+   * bookkeeping; never calls a method on {@code o} itself.
+   *
+   * @param o the object to check, may be null
+   * @return true if {@code o} is known to be a mock
+   */
+  public static boolean isMock(Object o) {
+    if (o == null) {
+      return false;
+    }
+
+    Class<?> type = o.getClass();
+    if (type.isArray() || NEVER_MOCKABLE.contains(type)) {
+      return false;
+    }
+
+    Method[] api = MOCKITO.get(type);
+    if (api == null) {
+      return false;
+    }
+
+    try {
+      Object details = api[0].invoke(null, o);
+      return details != null && Boolean.TRUE.equals(api[1].invoke(details));
+    } catch (Throwable t) {
+      // If we can't tell, say no: recording a value is less important than not
+      // guessing wrong about the user's objects.
+      logger.debug(t, "failed to check whether {} is a mock", type.getName());
+      return false;
+    }
+  }
+}

agent/test/mockito/.gitignore

@@ -0,0 +1,3 @@
+tmp/
+build/
+.gradle/

agent/test/mockito/appmap.yml

@@ -0,0 +1,3 @@
+name: mockito
+packages:
+- path: com.example.mockito

agent/test/mockito/build.gradle

@@ -0,0 +1,65 @@
+import org.gradle.api.tasks.testing.logging.TestLogEvent
+
+plugins {
+  id 'java'
+}
+
+repositories {
+  mavenCentral()
+}
+
+def buildAppmapJar = "$System.env.AGENT_JAR"
+def buildAnnotationJar = "$System.env.ANNOTATION_JAR"
+
+def agentJar = findProperty("agentJar") ?: buildAppmapJar
+def annotationJar = findProperty("annotationJar") ?: buildAnnotationJar
+
+// Mockito 5 requires Java 11. On Java 8 stay on 4.x, where the default mock
+// maker is the subclass one rather than the inline one.
+//
+// Don't go below 5.19 on the Mockito 5.x line: the Byte Buddy bundled with
+// earlier releases rejects Java 25 class files ("Java 25 (69) is not supported
+// by the current version of Byte Buddy"), which the inline mock maker hits as
+// soon as it mocks anything.
+def jdkVersion = System.getProperty('java.version').split('\\.')[0] as Integer
+def mockitoVersion = findProperty('mockitoVersion') ?: (jdkVersion <= 8 ? '4.11.0' : '5.19.0')
+
+dependencies {
+  testImplementation 'junit:junit:4.13.2'
+  testImplementation "org.mockito:mockito-core:${mockitoVersion}"
+  testImplementation files(annotationJar)
+}
+
+def commonTestConfig = {
+  testClassesDirs = sourceSets.test.output.classesDirs
+  classpath = sourceSets.test.runtimeClasspath
+
+  testLogging {
+    events TestLogEvent.STANDARD_OUT
+    exceptionFormat = 'full'
+  }
+}
+
+// Control task: the same tests with no agent attached. These tests have to
+// pass without AppMap, otherwise a failure under AppMap proves nothing.
+tasks.register('test_noagent', Test) {
+  description = 'Runs the Mockito tests with no AppMap agent (control)'
+  group = 'verification'
+
+  configure commonTestConfig
+}
+
+tasks.register('test_appmap', Test) {
+  description = 'Runs the Mockito tests under the AppMap agent'
+  group = 'verification'
+
+  configure commonTestConfig
+
+  systemProperty 'appmap.config.file', "$projectDir/appmap.yml"
+
+  jvmArgs += [
+      System.env.JAVA_OUTPUT_OPTIONS,
+      "-javaagent:${agentJar}",
+      "-Djava.util.logging.config.file=${System.env.JUL_CONFIG}"
+    ]
+}

agent/test/mockito/mockito.bats

@@ -0,0 +1,47 @@
+#!/usr/bin/env bats
+
+load ../helper
+
+setup_file() {
+  export AGENT_JAR="$(find_agent_jar)"
+  export ANNOTATION_JAR="$(find_annotation_jar)"
+  _configure_logging
+}
+
+setup() {
+  cd "$(dirname "$BATS_TEST_FILENAME")"
+  rm -rf tmp/appmap
+}
+
+# The tests have to pass with no agent attached, otherwise a failure under the
+# agent doesn't tell us anything.
+@test "control: mockito tests pass with no agent" {
+  run gradlew cleanTest test_noagent
+  assert_success
+}
+
+# Recording a mock used to call toString() on it, which Mockito counts as an
+# invocation. That stole pending argument matchers ("2 matchers expected, 1
+# recorded") and silently broke stubbing, so stubbed calls returned the Java
+# type default.
+@test "recording does not disturb mockito" {
+  run gradlew cleanTest test_appmap
+  assert_success
+  refute_output --partial "InvalidUseOfMatchersException"
+}
+
+# The flip side of the fix: a mock's value is a placeholder, because asking the
+# mock for its value is what caused the trouble.
+@test "a mock is recorded as a placeholder, not by calling it" {
+  run gradlew cleanTest test_appmap --tests '*MockRecordingTest.testRecordingAMockAddsNoInteractions'
+  assert_success
+
+  output="$(< tmp/appmap/junit/com_example_mockito_MockRecordingTest_testRecordingAMockAddsNoInteractions.appmap.json)"
+
+  # The mock really is recorded as a parameter, so this isn't vacuous...
+  assert_json_contains \
+    '[.events[] | select(.method_id=="identify") | .parameters[0].class] | first' 'MockitoMock'
+  # ...and its value came from us, not from the mock.
+  assert_json_eq \
+    '[.events[] | select(.method_id=="identify") | .parameters[0].value] | first' '[mocked]'
+}

agent/test/mockito/settings.gradle

@@ -0,0 +1,6 @@
+plugins {
+    // Apply the foojay-resolver plugin to allow automatic download of JDKs
+    id 'org.gradle.toolchains.foojay-resolver-convention' version '0.4.0'
+}
+
+rootProject.name = 'mockito'

agent/test/mockito/src/main/java/com/example/mockito/Calculator.java

@@ -0,0 +1,8 @@
+package com.example.mockito;
+
+/** A public collaborator interface, the most common thing a project mocks. */
+public interface Calculator {
+  int add(int a, int b);
+
+  String describe(String label);
+}

agent/test/mockito/src/main/java/com/example/mockito/Service.java

@@ -0,0 +1,36 @@
+package com.example.mockito;
+
+/**
+ * Instrumented app code that handles mocks. AppMap records the parameters and
+ * return values of these methods, which is how it comes into contact with the
+ * mocks a test passes around.
+ */
+public class Service {
+  private final Calculator calculator;
+
+  public Service(Calculator calculator) {
+    this.calculator = calculator;
+  }
+
+  public int compute(int a, int b) {
+    return calculator.add(a, b);
+  }
+
+  /**
+   * Takes a mock and returns an int, so it can be called from inside the
+   * argument list of a stubbed or verified call.
+   */
+  public int tag(Calculator other) {
+    return other == calculator ? 2 : 0;
+  }
+
+  /** Like tag(), for a collaborator that isn't a Calculator. */
+  public int tagAny(Object other) {
+    return other == null ? 0 : 2;
+  }
+
+  /** Takes a mock, so AppMap records a mock as a parameter. */
+  public String identify(Calculator other) {
+    return other == calculator ? "same" : "different";
+  }
+}