From 3a927df5b8a06a4affc0016236337ce20d28c741 Mon Sep 17 00:00:00 2001 From: Jordan Wong Date: Fri, 4 Sep 2026 09:25:38 -0400 Subject: [PATCH] skill(apm-integrations): split tests.md into mandatory rules + situational style guide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tests.md mixed always-applicable rules (DSL choice, error-test coverage, no-sleep hygiene) with situational rules that only apply to specific circumstances (version-mismatch comments, mutual-exclusion deps, latestDepTest source-set routing, banner-comment style) and their code examples — 179 lines every reader loaded via SKILL.md's Step 9.1 pointer, regardless of whether their module hit any of those cases. Split into: - tests.md (28 lines) — the mandatory checklist: DSL rule, error-test requirement, ForkedTest naming, integration-name registration, no-Thread.sleep/no-default-jvmArgs hygiene. This is what SKILL.md Step 9.1 points to. - tests-style.md (148 lines) — situational rules + all code examples, linked from tests.md for the specific cases that need them. Also folds in the DSL-rule clarification from #12359 ("Full Java instrumentation test support is not yet available" read as an absolute ban that the repo's own Java-DSL test suites contradict — restated as unconditional-unless-already-Java-DSL). Updated SKILL.md's two references/tests.md pointers (Step 9.1, Step 9.3's latestDepTest pointer) to match the new file split. Co-Authored-By: Claude Opus 4.8 (1M context) --- .agents/skills/apm-integrations/SKILL.md | 4 +- .../references/tests-style.md | 148 ++++++++++++++++ .../apm-integrations/references/tests.md | 167 +----------------- 3 files changed, 158 insertions(+), 161 deletions(-) create mode 100644 .agents/skills/apm-integrations/references/tests-style.md diff --git a/.agents/skills/apm-integrations/SKILL.md b/.agents/skills/apm-integrations/SKILL.md index 5bacedebeaa..c575230fad2 100644 --- a/.agents/skills/apm-integrations/SKILL.md +++ b/.agents/skills/apm-integrations/SKILL.md @@ -93,7 +93,7 @@ Cover all mandatory test types: ### 1. Instrumentation test (mandatory) -**Read [Writing Tests](references/tests.md).** Instrumentation tests are Groovy/Spock (`src/test/groovy/`) — add the `tag: override groovy enforcement` label to suppress the `Enforce Groovy Migration` CI check (which blocks new `.groovy` files by default — instrumentation tests are intentionally Groovy/Spock). Must cover error/exception scenarios. When adding new integration names, register them per [Supported Configurations](references/supported-configurations.md). When `compileOnly` and `testImplementation` use different versions, comment the specific class that requires the higher version. Include sibling version modules as `testImplementation` dependencies for mutual-exclusion tests. +**Read [Writing Tests](references/tests.md) — it defines the Groovy-vs-Java DSL rule; follow it before creating any test files.** Adding new `.groovy` files to a PR triggers the `Enforce Groovy Migration` CI check — add the `tag: override groovy enforcement` label to suppress it. Must cover error/exception scenarios. When adding new integration names, register them per [Supported Configurations](references/supported-configurations.md). Situational rules (version-mismatch comments, mutual-exclusion deps, etc.) are in [tests-style.md](references/tests-style.md) — check it if your module hits one of those cases. ### 2. Muzzle directives (mandatory) @@ -101,7 +101,7 @@ Cover all mandatory test types: ### 3. Latest dependency test (mandatory) -If the library's API surface changes across minor versions (deprecated/removed methods, changed signatures), see [Writing Tests](references/tests.md)'s "Version-sensitive tests belong in a separate `latestDepTest` source set" section for which tests belong in `src/test/` vs `src/latestDepTest/`. +If the library's API surface changes across minor versions (deprecated/removed methods, changed signatures), see [tests-style.md](references/tests-style.md)'s "Version-sensitive tests belong in a separate `latestDepTest` source set" section for which tests belong in `src/test/` vs `src/latestDepTest/`. Use `latestDepTestImplementation` in `build.gradle` to pin the latest available version. Run with: ```bash diff --git a/.agents/skills/apm-integrations/references/tests-style.md b/.agents/skills/apm-integrations/references/tests-style.md new file mode 100644 index 00000000000..1c2f6dbdbbf --- /dev/null +++ b/.agents/skills/apm-integrations/references/tests-style.md @@ -0,0 +1,148 @@ +# Test Style & Situational Rules + +> Referenced from [tests.md](tests.md). These rules are situational — read the specific section you need, not the whole file top to bottom. + +## Error test example + +```groovy +// Example error test (Groovy/Spock) +def "exception sets error tags"() { + when: + client.execute(badRequest) + + then: + thrown(SomeException) + assertTraces(1) { + trace(1) { + span { + errored true + errorTags(SomeException, "expected error message") + } + } + } +} +``` + +## compileOnly and testImplementation may use different versions — explain why + +When `compileOnly` and `testImplementation` use different versions of the same library, add a +comment that explains the specific API or class that requires the higher version, and why. +Do not just state the fact — state the reason. + +```groovy +// WRONG — states the fact without explaining why +// compileOnly=2.3, testImplementation=2.4 +compileOnly group: 'com.sparkjava', name: 'spark-core', version: '2.3' +testImplementation group: 'com.sparkjava', name: 'spark-core', version: '2.4' + +// CORRECT — explains the specific class and why it requires the higher version +// compileOnly=2.3 (module targets this version) but testImplementation=2.4: +// JettyHandler, which Spark uses internally to dispatch HTTP requests to route handlers, +// is not accessible as a public class in 2.3 — it was exposed starting in 2.4. +// The instrumentation hooks into JettyHandler via Jetty's existing instrumentation, +// so tests require 2.4 at minimum to exercise the code path. +compileOnly group: 'com.sparkjava', name: 'spark-core', version: '2.3' +testImplementation group: 'com.sparkjava', name: 'spark-core', version: '2.4' +``` + +**How to discover this during development**: install the library at the `compileOnly` version and run +your instrumentation test. If a specific class raises `ClassNotFoundException` or is inaccessible, that +class is the reason — check when it became public and use that version for `testImplementation`. +Name the class in the comment. + +## Include sibling version modules in testImplementation for mutual exclusion + +When two modules instrument the same library at non-overlapping version ranges, each module should include the other as a `testImplementation` dependency to confirm they don't double-instrument. The rule is symmetric — both the older and the newer module should carry this dependency: + +```groovy +// jedis-3.0/build.gradle +dependencies { + testImplementation project(':dd-java-agent:instrumentation:jedis:jedis-1.4') +} + +// jedis-1.4/build.gradle +dependencies { + testImplementation project(':dd-java-agent:instrumentation:jedis:jedis-3.0') +} +``` + +This ensures `:test` in each module validates that only the correct module fires for its version range. + +## Embedded servers use a static field — do not recreate per test + +For tests that start an embedded server (Jetty, Netty, Undertow, Spark, etc.), initialize the server once as a `@Shared` or `static` field and reuse it across test methods. Do NOT construct a new server in each `setup:` / `@Before` unless you have a concrete reason (e.g. per-test configuration). Recreating the server per test multiplies test wall-time and adds a startup-race surface for no benefit. Follow the pattern of existing server-instrumentation tests in the same framework family. + +## Factor shared test scaffolding into a base class + +If two sibling test classes (e.g. `FooTest` and `FooForkedTest`) need the same setup, request builder, or assertion helpers, extract them into a shared abstract base — do NOT copy-paste between the two files. Duplicated helper code across a handful of test classes is how bespoke JUnit scaffolding metastasizes across the codebase. + +## ForkedTest variants must have a concrete isolation reason + +The `ForkedTest` suffix runs a test in its own JVM via the `forkedTest` task. Only add a `ForkedTest` variant when the test genuinely needs JVM isolation — e.g. a system property that must be set before class-loading, an agent-level configuration that cannot be reset between tests, or a class-loader state that leaks. Do NOT mechanically add a `ForkedTest` alongside every `Test` class; each fork adds JVM startup cost to CI. + +State the isolation reason in a comment on the `ForkedTest` class. + +## Version-sensitive tests belong in a separate latestDepTest source set + +For libraries whose API surface changes across minor versions (Reactor deprecates and removes APIs; Netty changes handler signatures; gRPC's generated code evolves), route each test to the source set whose classpath actually satisfies its imports. First check how the module wires `latestDepTest` in `build.gradle`: + +- **`addTestSuite('latestDepTest')`** — `latestDepTest` has its own sources at `src/latestDepTest/`, separate from `src/test/`. In this shape: put latest-only APIs (added after your pinned min) in `src/latestDepTest/`; put removed-in-latest APIs (e.g. Reactor's `Schedulers.elastic()`, removed in 3.5+) in `src/test/`, testing the replacement API (`Schedulers.boundedElastic()`) in `latestDepTest/` instead. +- **`addTestSuiteForDir('latestDepTest', 'test')`** — `latestDepTest` reuses `src/test/`'s sources and compiles them against the latest classpath too. In this shape, `src/test/` is NOT a safe place for a removed-in-latest-API test — it still gets compiled against `latestDepTestImplementation` and will fail the same way. A test that exercises a removed API needs the module to declare a real separate `latestDepTest` source set instead (switch to `addTestSuite('latestDepTest')`), or the test needs to avoid the removed API entirely (e.g. call the replacement API and assert equivalent behavior). + +Common libraries where this split matters: Reactor, Netty, gRPC, Kafka clients (consumer API changed at 3.0), Cassandra driver (3.x vs 4.x largely incompatible). + +**Editing an existing module:** check for `src/latestDepTest/` and the exact `addTestSuite(...)`/`addTestSuiteForDir(...)` declaration in `build.gradle` before touching tests, and preserve whichever shape master uses. + +## No banner/separator comments in test files + +Do NOT insert banner-style separator comments (e.g. `// --------- Successful completion ---------`) inside test files to group related test methods. Banner comments have unclear scope, don't render usefully in IDEs, and add review burden without a benefit that justifies the noise. + +**If a group of related tests warrants its own heading**, extract them into a separate test class with a focused class-level Javadoc: + +```java +// Java example, style only — the same DSL rule in tests.md applies regardless of language +// ❌ Banner comments +class RxJava3ResultExtensionTest extends AbstractInstrumentationTest { + // --------------------------------------------------------------------------- + // Successful async completion: span finishes when reactive type completes + // --------------------------------------------------------------------------- + @ParameterizedTest + void successfulCompletion(...) { ... } + + // --------------------------------------------------------------------------- + // Error paths: span records error and finishes + // --------------------------------------------------------------------------- + @ParameterizedTest + void errorPath(...) { ... } +} + +// ✅ Either omit the banner +class RxJava3ResultExtensionTest extends AbstractInstrumentationTest { + @ParameterizedTest + void successfulCompletion(...) { ... } + + @ParameterizedTest + void errorPath(...) { ... } +} + +// OR extract into focused classes with class Javadoc +/** + * Successful async completion — verifies the extension finishes the span + * when the reactive type emits a terminal signal. + */ +class RxJava3ResultExtensionSuccessTest extends AbstractInstrumentationTest { + @ParameterizedTest + void successfulCompletion(...) { ... } +} + +/** + * Error paths — verifies the extension records the error and finishes the span + * when the reactive type emits an onError signal. + */ +class RxJava3ResultExtensionErrorTest extends AbstractInstrumentationTest { + @ParameterizedTest + void errorPath(...) { ... } +} +``` + +Source: @ygree review on PR #11939. diff --git a/.agents/skills/apm-integrations/references/tests.md b/.agents/skills/apm-integrations/references/tests.md index c1be9b8cc07..5dff4707e38 100644 --- a/.agents/skills/apm-integrations/references/tests.md +++ b/.agents/skills/apm-integrations/references/tests.md @@ -1,10 +1,10 @@ # Writing Tests -> Referenced from `SKILL.md` Step 9.1 (Instrumentation test). For muzzle directives (Step 9.2), see `muzzle.md` in this directory. +> Referenced from `SKILL.md` Step 9.1 (Instrumentation test). For muzzle directives (Step 9.2), see `muzzle.md` in this directory. For situational rules that don't apply to every module (version-mismatch comments, mutual-exclusion deps, `latestDepTest` routing, style conventions), see [Test Style & Situational Rules](tests-style.md). ## 1. Instrumentation test (mandatory) -**Write Groovy/Spock tests for instrumentation tests** (per `AGENTS.md`: "Only use Groovy / Spock tests for instrumentation and smoke tests"). Full Java instrumentation test support is not yet available. Adding new `.groovy` files to a PR will trigger the `Enforce Groovy Migration` bot — add the `tag: override groovy enforcement` label to bypass it. +**Write Groovy/Spock tests for instrumentation tests** (per `AGENTS.md`: "Only use Groovy / Spock tests for instrumentation and smoke tests"). This is unconditional — including for modules whose existing siblings happen to use Java/JUnit — an existing Java-DSL sibling is NOT license to add more Java tests; do not migrate a Groovy family to Java either. Confirm what the family is on with `ls src/test/` on the module's master version and its version-siblings (e.g. `jedis-1.4/`, `jedis-4.0/` for `jedis-3.0`) before writing tests. Java examples in [tests-style.md](tests-style.md) are style-only illustrations for modules ALREADY on the Java/JUnit DSL — NOT a license to introduce Java into a Groovy family. Adding new `.groovy` files to a PR will trigger the `Enforce Groovy Migration` bot — add the `tag: override groovy enforcement` label to bypass it. - Groovy/Spock test class in `src/test/groovy/datadog/trace/instrumentation//` - Verify: spans created, tags set, errors propagated, resource names correct @@ -12,168 +12,17 @@ - Use `TEST_WRITER.waitForTraces(N)` for setup/teardown flushing (not for assertions) - Use `runUnderTrace("root") { ... }` from `TraceUtils` for synchronous code (trailing Groovy closure) -**Tests must cover error/exception scenarios, not just the happy path.** At minimum, add a test that exercises an exception or error condition and asserts the span's error tags (`error.type`, `error.message`, `error.stack`) are set correctly: +**Tests must cover error/exception scenarios, not just the happy path.** At minimum, add a test that exercises an exception or error condition and asserts the span's error tags (`error.type`, `error.message`, `error.stack`) are set correctly. See [tests-style.md](tests-style.md#error-test-example) for an example. -```groovy -// Example error test (Groovy/Spock) -def "exception sets error tags"() { - when: - client.execute(badRequest) - - then: - thrown(SomeException) - assertTraces(1) { - trace(1) { - span { - errored true - errorTags(SomeException, "expected error message") - } - } - } -} -``` - -For tests that need a separate JVM, suffix the test class with `ForkedTest` and run via the `forkedTest` task. +For tests that need a separate JVM, suffix the test class with `ForkedTest` and run via the `forkedTest` task — see [tests-style.md](tests-style.md#forkedtest-variants-must-have-a-concrete-isolation-reason) for when this is actually warranted. ### Register new integration names in `metadata/supported-configurations.json` See [Supported Configurations](supported-configurations.md) for the key shapes, CI checks, and JSON format. -### compileOnly and testImplementation may use different versions — explain why - -When `compileOnly` and `testImplementation` use different versions of the same library, add a -comment that explains the specific API or class that requires the higher version, and why. -Do not just state the fact — state the reason. - -```groovy -// WRONG — states the fact without explaining why -// compileOnly=2.3, testImplementation=2.4 -compileOnly group: 'com.sparkjava', name: 'spark-core', version: '2.3' -testImplementation group: 'com.sparkjava', name: 'spark-core', version: '2.4' - -// CORRECT — explains the specific class and why it requires the higher version -// compileOnly=2.3 (module targets this version) but testImplementation=2.4: -// JettyHandler, which Spark uses internally to dispatch HTTP requests to route handlers, -// is not accessible as a public class in 2.3 — it was exposed starting in 2.4. -// The instrumentation hooks into JettyHandler via Jetty's existing instrumentation, -// so tests require 2.4 at minimum to exercise the code path. -compileOnly group: 'com.sparkjava', name: 'spark-core', version: '2.3' -testImplementation group: 'com.sparkjava', name: 'spark-core', version: '2.4' -``` - -**How to discover this during development**: install the library at the `compileOnly` version and run -your instrumentation test. If a specific class raises `ClassNotFoundException` or is inaccessible, that -class is the reason — check when it became public and use that version for `testImplementation`. -Name the class in the comment. - -### Include sibling version modules in testImplementation for mutual exclusion - -When two modules instrument the same library at non-overlapping version ranges, each module should include the other as a `testImplementation` dependency to confirm they don't double-instrument. The rule is symmetric — both the older and the newer module should carry this dependency: - -```groovy -// jedis-3.0/build.gradle -dependencies { - testImplementation project(':dd-java-agent:instrumentation:jedis:jedis-1.4') -} - -// jedis-1.4/build.gradle -dependencies { - testImplementation project(':dd-java-agent:instrumentation:jedis:jedis-3.0') -} -``` - -This ensures `:test` in each module validates that only the correct module fires for its version range. - -## Test hygiene - -### No `Thread.sleep()` in tests — use deterministic waits - -`Thread.sleep(...)` is a recipe for flake. Use a deterministic mechanism instead: - -- `TEST_WRITER.waitForTraces(N)` — waits until at least N traces have been recorded (`traceCount >= N`), with a bounded timeout of 20s (see `dd-trace-core/src/main/java/datadog/trace/common/writer/ListWriter.java`). Use `TEST_WRITER.size()` afterwards to assert the exact count you expect. -- `CountDownLatch` / `CompletableFuture.get(timeout, TimeUnit)` — for signalling from async callbacks -- Spock's `PollingConditions` — for polling an assertion until it holds - -If you catch yourself writing `Thread.sleep(...)`, name the specific signal you're waiting for and wait on that signal directly. - -### Embedded servers use a static field — do not recreate per test - -For tests that start an embedded server (Jetty, Netty, Undertow, Spark, etc.), initialize the server once as a `@Shared` or `static` field and reuse it across test methods. Do NOT construct a new server in each `setup:` / `@Before` unless you have a concrete reason (e.g. per-test configuration). Recreating the server per test multiplies test wall-time and adds a startup-race surface for no benefit. Follow the pattern of existing server-instrumentation tests in the same framework family. - -### Factor shared test scaffolding into a base class - -If two sibling test classes (e.g. `FooTest` and `FooForkedTest`) need the same setup, request builder, or assertion helpers, extract them into a shared abstract base — do NOT copy-paste between the two files. Duplicated helper code across a handful of test classes is how bespoke JUnit scaffolding metastasizes across the codebase. - -### `ForkedTest` variants must have a concrete isolation reason - -The `ForkedTest` suffix runs a test in its own JVM via the `forkedTest` task. Only add a `ForkedTest` variant when the test genuinely needs JVM isolation — e.g. a system property that must be set before class-loading, an agent-level configuration that cannot be reset between tests, or a class-loader state that leaks. Do NOT mechanically add a `ForkedTest` alongside every `Test` class; each fork adds JVM startup cost to CI. - -State the isolation reason in a comment on the `ForkedTest` class. - -### Do not add default jvmArgs to test tasks - -`dd.trace.enabled=true` is the default; adding `jvmArgs '-Ddd.trace.enabled=true'` to a `Test` task in `build.gradle` is noise. Only add jvmArgs that meaningfully diverge from defaults (e.g. enabling a specific integration that's off by default, or a debug flag). If you're tempted to copy a `jvmArgs` block from a sibling module, check whether each flag is actually needed for this module. - -## Version-sensitive tests belong in a separate `latestDepTest` source set - -For libraries whose API surface changes across minor versions (Reactor deprecates and removes APIs; Netty changes handler signatures; gRPC's generated code evolves), route each test to the source set whose classpath actually satisfies its imports. First check how the module wires `latestDepTest` in `build.gradle`: - -- **`addTestSuite('latestDepTest')`** — `latestDepTest` has its own sources at `src/latestDepTest/`, separate from `src/test/`. In this shape: put latest-only APIs (added after your pinned min) in `src/latestDepTest/`; put removed-in-latest APIs (e.g. Reactor's `Schedulers.elastic()`, removed in 3.5+) in `src/test/`, testing the replacement API (`Schedulers.boundedElastic()`) in `latestDepTest/` instead. -- **`addTestSuiteForDir('latestDepTest', 'test')`** — `latestDepTest` reuses `src/test/`'s sources and compiles them against the latest classpath too. In this shape, `src/test/` is NOT a safe place for a removed-in-latest-API test — it still gets compiled against `latestDepTestImplementation` and will fail the same way. A test that exercises a removed API needs the module to declare a real separate `latestDepTest` source set instead (switch to `addTestSuite('latestDepTest')`), or the test needs to avoid the removed API entirely (e.g. call the replacement API and assert equivalent behavior). - -Common libraries where this split matters: Reactor, Netty, gRPC, Kafka clients (consumer API changed at 3.0), Cassandra driver (3.x vs 4.x largely incompatible). - -**Editing an existing module:** check for `src/latestDepTest/` and the exact `addTestSuite(...)`/`addTestSuiteForDir(...)` declaration in `build.gradle` before touching tests, and preserve whichever shape master uses. - -## No banner/separator comments in test files - -Do NOT insert banner-style separator comments (e.g. `// --------- Successful completion ---------`) inside test files to group related test methods. Banner comments have unclear scope, don't render usefully in IDEs, and add review burden without a benefit that justifies the noise. - -**If a group of related tests warrants its own heading**, extract them into a separate test class with a focused class-level Javadoc: - -```java -// ❌ Banner comments -class RxJava3ResultExtensionTest extends AbstractInstrumentationTest { - // --------------------------------------------------------------------------- - // Successful async completion: span finishes when reactive type completes - // --------------------------------------------------------------------------- - @ParameterizedTest - void successfulCompletion(...) { ... } - - // --------------------------------------------------------------------------- - // Error paths: span records error and finishes - // --------------------------------------------------------------------------- - @ParameterizedTest - void errorPath(...) { ... } -} - -// ✅ Either omit the banner -class RxJava3ResultExtensionTest extends AbstractInstrumentationTest { - @ParameterizedTest - void successfulCompletion(...) { ... } - - @ParameterizedTest - void errorPath(...) { ... } -} - -// OR extract into focused classes with class Javadoc -/** - * Successful async completion — verifies the extension finishes the span - * when the reactive type emits a terminal signal. - */ -class RxJava3ResultExtensionSuccessTest extends AbstractInstrumentationTest { - @ParameterizedTest - void successfulCompletion(...) { ... } -} +## Test hygiene (always applies) -/** - * Error paths — verifies the extension records the error and finishes the span - * when the reactive type emits an onError signal. - */ -class RxJava3ResultExtensionErrorTest extends AbstractInstrumentationTest { - @ParameterizedTest - void errorPath(...) { ... } -} -``` +- **No `Thread.sleep()`** — use `TEST_WRITER.waitForTraces(N)`, a `CountDownLatch`/`CompletableFuture.get(timeout, ...)`, or Spock's `PollingConditions`. If you catch yourself writing `Thread.sleep(...)`, name the specific signal you're waiting for and wait on that signal directly. +- **Do not add default `jvmArgs`** — `dd.trace.enabled=true` is already the default; only add jvmArgs that meaningfully diverge from defaults. -Source: @ygree review on PR #11939. +For the rest — version-mismatch comments, mutual-exclusion `testImplementation` deps, embedded-server reuse, shared base classes, `ForkedTest` isolation criteria, `latestDepTest` source-set routing, and no-banner-comment style — see [tests-style.md](tests-style.md); those are situational and don't come up on every module.