Skip to content

Commit 4215789

Browse files
jordan-wongclaude
andcommitted
eval: prepare blind test for jedis
Remove existing jedis instrumentation (1.4, 3.0, 4.0) so the agent generates from scratch without reference. Append collected review rules (R1-R11) to the add-apm-integrations skill file. Also includes spotless formatting fixes for unrelated files caught by pre-commit hook. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 5580c61 commit 4215789

21 files changed

Lines changed: 165 additions & 1936 deletions

File tree

.claude/skills/add-apm-integrations/SKILL.md

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,3 +230,57 @@ After the instrumentation is complete (or abandoned), review the full session an
230230

231231
Keep each change minimal and targeted. Do not rewrite sections that worked correctly.
232232
After editing, confirm to the user which improvements were made to the skill.
233+
234+
## Appendix: Collected review rules
235+
236+
These rules are derived from expert feedback on generated PRs. They serve as a checklist for self-review before finishing. Some overlap with guidance in the steps above; they are collected here for completeness.
237+
238+
### R1: No lambdas in advice classes (error)
239+
Search for lambda expressions (`->` or `::`) in any file with "Advice" or "Instrumentation" in its name. Advice methods are inlined into bytecode by ByteBuddy, and lambdas create invokedynamic instructions that break when inlined into a different classloader context.
240+
*Source: mcculls, PR #10579*
241+
242+
### R2: Assign wrapped future back with `@Advice.Return(readOnly=false)` (error)
243+
In async advice exit methods that wrap futures (CompletableFuture, ListenableFuture, etc.), verify the wrapped result is assigned back to the return value using `@Advice.Return(readOnly=false)`. Do not discard the return of `future.whenComplete`/`thenApply`/etc.
244+
*Applies to: async instrumentations only. Source: mcculls, PR #10579*
245+
246+
### R3: Single InstrumenterModule per integration (error)
247+
There should be exactly one class extending `InstrumenterModule` (with `@AutoService(InstrumenterModule.class)`) per module. If the integration needs to instrument multiple classes, list them as separate `Instrumenter` implementations within the same module.
248+
*Source: mcculls, PR #10579*
249+
250+
### R4: Module name must end with version (error)
251+
The module directory under `dd-java-agent/instrumentation/` must follow `{library}-{version}` (e.g., `feign-8.0`, `okhttp-3`). Do not use the Maven artifact name (e.g., `feign-core`).
252+
*Source: mcculls, PR #10579*
253+
254+
### R5: CallDepthThreadLocalMap must be reset on the same thread (error)
255+
If `CallDepthThreadLocalMap` is used for reentrancy protection, `increment()` and `reset()` must be called on the same thread. In async code paths, callbacks may run on a different thread, so `CallDepthThreadLocalMap` cannot be used across async boundaries.
256+
*Applies to: async instrumentations only. Source: mcculls, PR #10579*
257+
258+
### R6: Code passes codeNarc checks (warning)
259+
If there are Groovy test files, avoid common codeNarc violations: unused imports, unnecessary semicolons, missing spaces after keywords. Run `codenarcTest` in addition to `spotlessCheck`.
260+
*Source: Runs 0, 1*
261+
262+
### R7: Test against minimum supported version, not just latest (warning)
263+
Compare the `testCompile`/`testImplementation` dependency version in `build.gradle` against the minimum version in the muzzle `pass` directive. If the test uses APIs that only exist in newer versions, it does not actually verify minimum version compatibility.
264+
*Source: Run 1 (auto-detected)*
265+
266+
### R8: Test extends correct base test class (warning)
267+
The test superclass should match the integration type:
268+
- HTTP clients: extend `HttpClientTest`
269+
- HTTP servers: extend `HttpServerTest`
270+
- Database clients: extend `DatabaseClientTest`
271+
- Messaging: use the appropriate messaging base test
272+
273+
If no base test class exists for the type, custom tests are acceptable but should cover span creation, error handling, and tag verification.
274+
*Source: Run 1 (auto-detected)*
275+
276+
### R9: Disabled base test methods must have justification (warning)
277+
Methods that override base test class methods to return `false` (e.g., `testRedirects() { return false }`) must have a comment explaining why — what specific library limitation prevents the test from working.
278+
*Source: Run 1 (auto-detected)*
279+
280+
### R10: Test exercises real library, not just mocks (error)
281+
The test must instantiate real library objects and make real calls (e.g., a real HTTP client calling a test server). Tests that only use mocks without exercising actual library code do not verify the instrumentation works. Base test classes typically provide real server infrastructure.
282+
*Source: PR #10317 (Resilience4j)*
283+
284+
### R11: Test compiles and runs against minimum supported version (warning)
285+
Verify that the test code only uses APIs available in the minimum supported version. If the test uses a newer API (e.g., a static factory method added in a later version), it cannot verify minimum version compatibility. This extends R7 with API-level checking.
286+
*Source: Run 1 (extends R7)*

components/json/src/test/java/datadog/json/JsonMapperTest.java

Lines changed: 26 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,12 @@
2424

2525
class JsonMapperTest {
2626
@TableTest({
27-
"Scenario | Input | Expected ",
28-
"null input | | '{}' ",
29-
"empty map | [:] | '{}' ",
30-
"single entry | [key1: value1] | '{\"key1\":\"value1\"}' ",
31-
"two entries | [key1: value1, key2: value2] | '{\"key1\":\"value1\",\"key2\":\"value2\"}' ",
32-
"quoted entries | [key1: va\"lu\"e1, ke\"y2: value2] | '{\"key1\":\"va\\\"lu\\\"e1\",\"ke\\\"y2\":\"value2\"}'"
27+
"Scenario | Input | Expected ",
28+
"null input | | '{}' ",
29+
"empty map | [:] | '{}' ",
30+
"single entry | [key1: value1] | '{\"key1\":\"value1\"}' ",
31+
"two entries | [key1: value1, key2: value2] | '{\"key1\":\"value1\",\"key2\":\"value2\"}' ",
32+
"quoted entries | [key1: va\"lu\"e1, ke\"y2: value2] | '{\"key1\":\"va\\\"lu\\\"e1\",\"ke\\\"y2\":\"value2\"}'"
3333
})
3434
@ParameterizedTest(name = "test mapping to JSON object: {0}")
3535
@MethodSource("testMappingToJsonObjectArguments")
@@ -94,12 +94,12 @@ void testMappingToMapFromNonObjectJson(String json) {
9494
}
9595

9696
@TableTest({
97-
"Scenario | Input | Expected ",
98-
"null input | | '[]' ",
99-
"empty list | [] | '[]' ",
100-
"single value | [value1] | '[\"value1\"]' ",
101-
"two values | [value1, value2] | '[\"value1\",\"value2\"]' ",
102-
"quoted values | [va\"lu\"e1, value2] | '[\"va\\\"lu\\\"e1\",\"value2\"]'"
97+
"Scenario | Input | Expected ",
98+
"null input | | '[]' ",
99+
"empty list | [] | '[]' ",
100+
"single value | [value1] | '[\"value1\"]' ",
101+
"two values | [value1, value2] | '[\"value1\",\"value2\"]' ",
102+
"quoted values | [va\"lu\"e1, value2] | '[\"va\\\"lu\\\"e1\",\"value2\"]'"
103103
})
104104
@ParameterizedTest(name = "test mapping iterable to JSON array: {0}")
105105
void testMappingIterableToJsonArray(List<String> input, String expected) throws IOException {
@@ -111,12 +111,12 @@ void testMappingIterableToJsonArray(List<String> input, String expected) throws
111111
}
112112

113113
@TableTest({
114-
"Scenario | Input | Expected ",
115-
"null input | | '[]' ",
116-
"empty array | [] | '[]' ",
117-
"single element | [value1] | '[\"value1\"]' ",
118-
"two elements | [value1, value2] | '[\"value1\",\"value2\"]' ",
119-
"escaped quotes | [va\"lu\"e1, value2] | '[\"va\\\"lu\\\"e1\",\"value2\"]'"
114+
"Scenario | Input | Expected ",
115+
"null input | | '[]' ",
116+
"empty array | [] | '[]' ",
117+
"single element | [value1] | '[\"value1\"]' ",
118+
"two elements | [value1, value2] | '[\"value1\",\"value2\"]' ",
119+
"escaped quotes | [va\"lu\"e1, value2] | '[\"va\\\"lu\\\"e1\",\"value2\"]'"
120120
})
121121
@ParameterizedTest(name = "test mapping array to JSON array: {0}")
122122
void testMappingArrayToJsonArray(String ignoredScenario, String[] input, String expected)
@@ -137,14 +137,14 @@ void testMappingToListFromEmptyJsonObject(String json) throws IOException {
137137
}
138138

139139
@TableTest({
140-
"Scenario | input | expected ",
141-
"null value | | '' ",
142-
"empty string | '' | '' ",
143-
"\\b | '\b' | '\"\\b\"'",
144-
"\\t | '\t' | '\"\\t\"'",
145-
"\\f | '\f' | '\"\\f\"'",
146-
"a | 'a' | '\"a\"' ",
147-
"/ | '/' | '\"\\/\"'"
140+
"Scenario | input | expected ",
141+
"null value | | '' ",
142+
"empty string | '' | '' ",
143+
"\\b | '\b' | '\"\\b\"'",
144+
"\\t | '\t' | '\"\\t\"'",
145+
"\\f | '\f' | '\"\\f\"'",
146+
"a | 'a' | '\"a\"' ",
147+
"/ | '/' | '\"\\/\"'"
148148
})
149149
@ParameterizedTest(name = "test mapping to JSON string: {0}")
150150
@MethodSource("testMappingToJsonStringArguments")

dd-java-agent/instrumentation/jedis/jedis-1.4/build.gradle

Lines changed: 0 additions & 25 deletions
This file was deleted.

0 commit comments

Comments
 (0)