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

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: for each distinct recorded type, Mockito resolution
+   * is attempted once and the result (including "unavailable") is reused.
+   */
+  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";
+  }
+}