Port TraceMapperV1 payload test from Spock to JUnit 5 - #12124
Port TraceMapperV1 payload test from Spock to JUnit 5#12124AlexeyKuznetsov-DD wants to merge 3 commits into
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AlexeyKuznetsov-DD
added
tag: ai generated
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
![datadog-prod-us1-6[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The Java migration preserves all original Spock scenarios and the shared reader changes only package visibility for test constants; no diff-only behavioral regression was identified. The targeted test could not execute because the checkout requires a Java 25 toolchain, while this sandbox provides JDK 8/11/17/21.
🤖 Datadog Autotest · Commit 6074405 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 607440569b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| long spanId = 0; | ||
| long parentId = 0; | ||
| long start = 0; | ||
| long duration = 0; | ||
| boolean error = false; |
There was a problem hiding this comment.
Initializing parentId to 0 and error to false weakens the wire-format verification because both are valid expected values. If encoding the zero-parent or non-error case writes the wrong field ID, these values can remain unchanged and the assertions still pass; the map-size assertion does not establish that every field ID is unique. The Spock test used nullable boxed values, so retain that behavior or track explicit seenParentId and seenError flags.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, and it was worse than described for parentId: TraceGenerator.randomSpan passes DDSpanId.ZERO, so the expected parent id is 0 for every span in the data-driven test, which made that assertion inert rather than merely weakened.
Confirmed by mutating TraceMapperV1 to write field id 6 twice and omit field 5 (16 entries preserved, start written twice with its correct value). Before the fix only spanIdsAreEncodedAsUnsignedValues failed — the one place that kept a boxed Long. Every test going through verifySpan passed with the parent-id field entirely unwritten.
Fixed in d6bf61b by pinning the decoded field-id set, rather than restoring boxed values or adding per-field flags:
assertEquals(EXPECTED_SPAN_FIELD_IDS, spanFieldsSeen);Together with the existing 16-entry count assertion, set equality guarantees each id appears exactly once, so this covers all 16 fields instead of just parentId and error, and the failure message names the missing id. It also matches the EXPECTED_PAYLOAD_FIELD_IDS pattern already used for the payload header. The same mutation now fails 4 tests instead of 1.
verifyChunk was already safe — it keeps boxed values with assertNotNull on all five fields plus a seenSpans flag.
verifySpan asserted a 16-entry span map but not that each field id appeared exactly once, so a repeated id plus an omitted one left the omitted field's decoded value at its initialiser. For parentId (0) and error (false) those sentinels equal the expected values, and TraceGenerator always uses DDSpanId.ZERO as the parent id, so the parent-id check was inert. Assert the decoded field-id set instead, which covers all 16 fields and names the missing id on failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
What Does This Do
Replaces
TraceMapperV1PayloadTest.groovywith a JUnit 5 / Java equivalent. All 25 Spock test instances are preserved; the count goes to 31 becausetest span kind value conversion(oneexpectblock with 7 assertions) became a 7-row@TableTest.While porting, duplicated helpers were dropped in favour of the existing shared ones:
traceIdBytes,newStringTableand thereadFirstSpan(byte[])/readFirstChunk(byte[])overloads now come fromV1PayloadReader, collapsing the hand-rolled unpacker walks in most tests to a single line.PayloadVerifiers.CapturingChannelandPayloadVerifiers.assertEqualsWithNullAsEmptyreplace the test's ownByteArrayChanneland null-as-empty helper.V1PayloadReader.PayloadField/ChunkField/SpanField. This required droppingprivatefrom those three nested constant classes (test-only file, same package).PojoSpanconstructions are behind small named factories (span,spanWithBaggage,spanWithIds,spanWithLinks).Two behaviour-visible tightenings:
spanKindValueConversionasserts the literal wire values0–5instead of comparingTraceMapperV1.SPAN_KIND_*to itself, so a protocol-value change now fails the test.attributes.size()assertions to the attribute tests, pinning that the encoder emits nothing beyond the tags under test plusthread.id/thread.name.Motivation
The equivalent V0.4 and V0.5 payload tests were already migrated in #11871, and
V1PayloadReaderwas written with this port in mind. This brings V1 in line with its siblings and lets the three tests share the same decode helpers.Additional Notes
SpanLink's constructor isprotected, which Groovy could call across packages but Java cannot, hence the smallTestSpanLinksubclass. Config isolation moves fromDDSpecificationto@ExtendWith(WithConfigExtension.class)plus aProcessTags.reset(Config.get())@BeforeEach, matching the V0.4/V0.5 tests.No production code is touched.
🤖 Generated with Claude Code