From 6bcd7dd1642423311cdf397fc1e8171119f6c6f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Rzepecki?= Date: Wed, 16 Sep 2026 16:32:28 +0200 Subject: [PATCH 1/2] 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: but was:" 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 --- .../com/appland/appmap/output/v1/Value.java | 6 + .../com/appland/appmap/util/MockDetector.java | 170 ++++++++++++++++++ agent/test/mockito/.gitignore | 3 + agent/test/mockito/appmap.yml | 3 + agent/test/mockito/build.gradle | 65 +++++++ agent/test/mockito/mockito.bats | 47 +++++ agent/test/mockito/settings.gradle | 6 + .../java/com/example/mockito/Calculator.java | 8 + .../java/com/example/mockito/Service.java | 36 ++++ .../example/mockito/MockRecordingTest.java | 112 ++++++++++++ 10 files changed, 456 insertions(+) create mode 100644 agent/src/main/java/com/appland/appmap/util/MockDetector.java create mode 100644 agent/test/mockito/.gitignore create mode 100644 agent/test/mockito/appmap.yml create mode 100644 agent/test/mockito/build.gradle create mode 100644 agent/test/mockito/mockito.bats create mode 100644 agent/test/mockito/settings.gradle create mode 100644 agent/test/mockito/src/main/java/com/example/mockito/Calculator.java create mode 100644 agent/test/mockito/src/main/java/com/example/mockito/Service.java create mode 100644 agent/test/mockito/src/test/java/com/example/mockito/MockRecordingTest.java diff --git a/agent/src/main/java/com/appland/appmap/output/v1/Value.java b/agent/src/main/java/com/appland/appmap/output/v1/Value.java index c486e87c..fc611f0b 100644 --- a/agent/src/main/java/com/appland/appmap/output/v1/Value.java +++ b/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(); diff --git a/agent/src/main/java/com/appland/appmap/util/MockDetector.java b/agent/src/main/java/com/appland/appmap/util/MockDetector.java new file mode 100644 index 00000000..658b07a4 --- /dev/null +++ b/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. + * + *

+ * 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. + * + *

+ * 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. + * + *

+ * 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> NEVER_MOCKABLE = new HashSet>(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 MOCKITO = new ClassValue() { + @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. + * + *

+ * 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. + * + *

+ * 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 candidateLoaders(Class type) { + List loaders = new ArrayList(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 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; + } + } +} diff --git a/agent/test/mockito/.gitignore b/agent/test/mockito/.gitignore new file mode 100644 index 00000000..f74f81ac --- /dev/null +++ b/agent/test/mockito/.gitignore @@ -0,0 +1,3 @@ +tmp/ +build/ +.gradle/ diff --git a/agent/test/mockito/appmap.yml b/agent/test/mockito/appmap.yml new file mode 100644 index 00000000..772d1c65 --- /dev/null +++ b/agent/test/mockito/appmap.yml @@ -0,0 +1,3 @@ +name: mockito +packages: +- path: com.example.mockito diff --git a/agent/test/mockito/build.gradle b/agent/test/mockito/build.gradle new file mode 100644 index 00000000..d47abb72 --- /dev/null +++ b/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}" + ] +} diff --git a/agent/test/mockito/mockito.bats b/agent/test/mockito/mockito.bats new file mode 100644 index 00000000..aa2f4caf --- /dev/null +++ b/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]' +} diff --git a/agent/test/mockito/settings.gradle b/agent/test/mockito/settings.gradle new file mode 100644 index 00000000..7e56c33e --- /dev/null +++ b/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' diff --git a/agent/test/mockito/src/main/java/com/example/mockito/Calculator.java b/agent/test/mockito/src/main/java/com/example/mockito/Calculator.java new file mode 100644 index 00000000..bb031baa --- /dev/null +++ b/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); +} diff --git a/agent/test/mockito/src/main/java/com/example/mockito/Service.java b/agent/test/mockito/src/main/java/com/example/mockito/Service.java new file mode 100644 index 00000000..4a9ebe6e --- /dev/null +++ b/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"; + } +} diff --git a/agent/test/mockito/src/test/java/com/example/mockito/MockRecordingTest.java b/agent/test/mockito/src/test/java/com/example/mockito/MockRecordingTest.java new file mode 100644 index 00000000..8fee25a3 --- /dev/null +++ b/agent/test/mockito/src/test/java/com/example/mockito/MockRecordingTest.java @@ -0,0 +1,112 @@ +package com.example.mockito; + +import static org.junit.Assert.assertEquals; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; +import static org.mockito.Mockito.when; + +import java.util.ArrayList; +import java.util.List; + +import org.junit.Test; + +/** + * Recording a mock must not disturb it. + * + *

+ * Mockito keeps thread-local state between one call and the next: argument + * matchers go on a stack and are bound to the next invocation it sees, and + * {@code when()} applies to the last invocation. Calling a method on a mock is + * an invocation, so if AppMap calls one while recording -- {@code toString()}, + * say, to get a value for the AppMap -- Mockito attributes that state to the + * wrong call. The stubbing the test set up then silently never applies, and the + * method returns the Java type default instead. + */ +public class MockRecordingTest { + + /** Baseline: recording must not change what a stubbed mock returns. */ + @Test + public void testStubbedValueSurvivesRecording() { + Calculator calc = mock(Calculator.class); + when(calc.add(1, 2)).thenReturn(3); + + Service service = new Service(calc); + assertEquals(3, service.compute(1, 2)); + } + + /** Recording must not register invocations of its own. */ + @Test + public void testRecordingAMockAddsNoInteractions() { + Calculator calc = mock(Calculator.class); + Service service = new Service(calc); + + assertEquals("same", service.identify(calc)); + + verifyNoMoreInteractions(calc); + } + + /** + * anyInt() pushes a matcher, then the instrumented tag() call hands AppMap a + * mock to record, then eq() pushes the second matcher. If AppMap calls the + * mock in between, Mockito binds a matcher to that call instead and reports + * "2 matchers expected, 1 recorded". + */ + @Test + public void testRecordingDoesNotStealPendingMatcherWhenStubbing() { + Calculator calc = mock(Calculator.class); + Service service = new Service(calc); + + when(calc.add(anyInt(), eq(service.tag(calc)))).thenReturn(5); + + assertEquals(5, calc.add(1, 2)); + } + + /** The same window, on the verification side. */ + @Test + public void testRecordingDoesNotStealPendingMatcherWhenVerifying() { + Calculator calc = mock(Calculator.class); + Service service = new Service(calc); + + calc.add(1, 2); + + verify(calc).add(anyInt(), eq(service.tag(calc))); + } + + /** + * The inline mock maker mocks a class by retransforming it, so a mock of a + * bootstrap-loaded type like ArrayList *is* an ArrayList, loaded by the + * bootstrap loader. Recognizing it means looking for Mockito somewhere other + * than the mock's own class loader, which can't see it. + * + *

+ * On Java 8 the default mock maker is the subclass one, which generates an + * ordinary subclass with a normal class loader, so this only exercises the + * bootstrap path from Java 11 on. + */ + @Test + public void testRecordingDoesNotStealPendingMatcherForBootstrapTypeMock() { + Calculator calc = mock(Calculator.class); + @SuppressWarnings("unchecked") + List list = mock(ArrayList.class); + Service service = new Service(calc); + + when(calc.add(anyInt(), eq(service.tagAny(list)))).thenReturn(5); + + assertEquals(5, calc.add(1, 2)); + } + + /** A mocked bootstrap-loaded type must not be called while recording. */ + @Test + public void testMockOfBootstrapTypeAddsNoInteractions() { + @SuppressWarnings("unchecked") + List list = mock(ArrayList.class); + Service service = new Service(mock(Calculator.class)); + + assertEquals(2, service.tagAny(list)); + + verifyNoMoreInteractions(list); + } +} From 5d2dd772d2264af510bdeeb9a0848070a1e406fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Rzepecki?= Date: Thu, 17 Sep 2026 21:47:12 +0200 Subject: [PATCH 2/2] Refine comment on Mockito mock-detection API Update comment to clarify Mockito's mock-detection behavior. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- agent/src/main/java/com/appland/appmap/util/MockDetector.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/agent/src/main/java/com/appland/appmap/util/MockDetector.java b/agent/src/main/java/com/appland/appmap/util/MockDetector.java index 658b07a4..c30ab8ee 100644 --- a/agent/src/main/java/com/appland/appmap/util/MockDetector.java +++ b/agent/src/main/java/com/appland/appmap/util/MockDetector.java @@ -60,8 +60,8 @@ public class MockDetector { /** * 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. + * 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 MOCKITO = new ClassValue() { @Override