Fix vacuous passes in v1 payload tests - #7450
Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 3 commits intoAug 7, 2026
Merged
Fix vacuous passes in v1 payload tests#7450gh-worker-dd-mergequeue-cf854d[bot] merged 3 commits into
gh-worker-dd-mergequeue-cf854d[bot] merged 3 commits into
Conversation
…pans `interfaces.library.get_root_spans()` and `get_traces()` are generators that yield nothing when no matching data is collected. Two tests in tests/test_v1_payloads.py only iterated them without asserting non-emptiness, so they PASSED when zero spans/traces were found, contradicting their own docstrings: - Test_V1TopLevelSpans::test_root_span_is_top_level - Test_V1PayloadByDefault::test_main Note the asymmetry that made this easy to miss: the singular `get_root_span()` does `assert spans, "No root spans found"`, but the plural `get_root_spans()` has no such guard. Sibling classes Test_V1SpanLinks, Test_V1SpanEvents and Test_V1Payloads already assert non-emptiness and are unaffected. This was not theoretical. The easy win activation script observed Test_V1TopLevelSpans "passing" for the 6 nodejs weblogs and activated it in manifests/nodejs.yml, even though dd-trace-js ships no v1 trace encoder: packages/dd-trace/src/encode/ contains only 0.4.js and 0.5.js on both master and v6.8.0, and getEncoder() falls back to the 0.4 encoder for any value other than '0.5'. So DD_TRACE_AGENT_PROTOCOL_VERSION=1.0 produces no v1 payload, zero v1 root spans are collected, and the test reported success. Deactivated for the 6 nodejs weblogs with a comment recording why. Blast radius checked with Manifest(...).get_declarations(...) across every library/weblog rather than by reading the YAML, since manifest declarations only ever disable. Test_V1TopLevelSpans was active for nodejs (6 weblogs), java (15) and golang (net-http, net-http-orchestrion); off everywhere else. Test_V1PayloadByDefault is off for every library/weblog, so its guard is zero-risk. java and golang genuinely support v1 (ProtocolVersion.V1_0 in dd-trace-java, tracesAPIPathV1 in dd-trace-go) and already run other classes in the same scenario that assert non-emptiness and are green, so the new guard is a no-op there. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
|
|
The Test_V1TopLevelSpans deactivation added here overlapped #7447, which removes the bogus file-level version gate for tests/test_v1_payloads.py and collapses the whole block to a single `missing_feature` entry. That entry already covers Test_V1TopLevelSpans for all six nodejs weblogs, so the per-class block added here was redundant, and it attached to context lines that #7447 deletes -- so whichever PR merged second would have conflicted. Keep this PR test-only and let #7447 own all manifest deactivation. This creates a merge-order dependency: #7447 must land first, otherwise the new "Expected at least one root span" assertion turns nodejs Test_V1TopLevelSpans red, since it is still activated on main. Co-Authored-By: Claude <noreply@anthropic.com>
darccio
changed the title
5 tasks
🎉 All green!🧪 All tests passed 🔗 Commit SHA: c197411 | Docs | Datadog PR Page | Give us feedback! |
darccio
marked this pull request as ready for review
August 6, 2026 13:00
nccatoni
approved these changes
Aug 7, 2026
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
dario.castane/busy-hofstadter-60817c
branch
August 7, 2026 15:28
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Two tests in
tests/test_v1_payloads.pycould pass without asserting anything.Test_V1TopLevelSpans::test_root_span_is_top_leveliteratesinterfaces.library.get_root_spans(self.r)with no non-empty guard.get_root_spans(utils/interfaces/_library/core.py:145) is a generator that yields nothing when no root spans match, so when the tracer sends v0.4 instead of v1 the loop body never executes and the test passes — despite its own docstring asserting that a root span must exist and be marked top-level. Compareget_root_spansingular (core.py:159), which doesassert spans, "No root spans found".Test_V1PayloadByDefault::test_mainhas the same shape overget_traces().This is not theoretical. It caused a real misclassification: the easy-win auto-activation script observed
Test_V1TopLevelSpans"passing" for the 6 nodejs weblogs and activated it inmanifests/nodejs.yml, even though dd-trace-js has no v1 trace encoder at any version —packages/dd-trace/src/encode/ships only0.4.jsand0.5.json every ref includingmasterand tagv6.8.0. The tracer sends v0.4, zero v1 root spans are collected, and the test reports success.Changes
Collect each generator into a list and assert it is non-empty before looping, so an empty result is a failure rather than a silent pass.
Test-only — no manifest changes.
Merge order: this must land after #7447
Test_V1TopLevelSpansis currently activated for the 6 nodejs weblogs on main, off the back of the vacuous pass described above. Once the guard lands, that test genuinely fails for nodejs.#7447 removes the bogus file-level version gate for
tests/test_v1_payloads.pyand collapses the block to a singlemissing_featureentry, which deactivatesTest_V1TopLevelSpansfor all six nodejs weblogs (andTest_V1SpanEventsfor rubyrack). #7447 must merge first.An earlier revision of this PR carried its own per-class deactivation block to stay independently mergeable, but it duplicated what #7447's file-level entry already covers and attached to context lines #7447 deletes, so the two conflicted. Dropped it in favour of a clean split: #7447 owns manifests, this PR owns test logic.
Reviewer notes
Test_V1SpanLinksandTest_V1SpanEventsalready assert non-emptiness and are unchanged.java and golang have
Test_V1TopLevelSpansactivated too (java:ratpack,uds-spring-boot,spring-boot-wildfly,spring-boot-undertow; golang:net-http,net-http-orchestrion). The guard should be a no-op there — both also haveTest_V1Payloadsactivated, which asserts real v1 payload structure, so spans are demonstrably being collected. Worth confirming on CI rather than assuming.The upstream cause is the auto-activation script treating a vacuous pass as an easy win. This PR removes the vacuous passes; whether the script needs its own fix is a question for #apm-shared-testing.
Workflow
Once your PR is reviewed and the CI green, you can merge it!
#apm-shared-testing
Reviewer checklist
tests/ormanifests/is modified ? I have the approval from R&P team — onlytests/is modifiedbuild-XXX-imagelabel is present🤖 Generated with Claude Code