fix(test): repair broken cold-start tests from #1254#1255
Merged
Conversation
enrich_ctx_keeps_tracer_set_cold_start_trace_id_without_tracer_detected calls setup(), which spawns a Tokio task, but was annotated with #[test] instead of #[tokio::test], causing a "no reactor running" panic. Match its sibling test and use #[tokio::test] async fn. 🤖 Co-Authored-By: Claude Code <[email protected]>
Contributor
|
lucaspimentel
marked this pull request as ready for review
June 12, 2026 22:45
lucaspimentel
requested review from
Copilot and
shreyamalpani
and removed request for
Copilot
June 12, 2026 22:45
lucaspimentel
marked this pull request as draft
June 12, 2026 23:01
timeout_without_tracer_does_not_send_cold_start_span passed its trace_sender by value into on_platform_runtime_done, dropping the only Arc<SendingTraceProcessor> before try_recv(). The receiver then reported Disconnected, but the test asserted Empty, so it failed even though the cold start span was correctly never sent. Pass a clone and hold the sender until after the assertion so the channel stays connected. 🤖 Co-Authored-By: Claude Code <[email protected]>
lucaspimentel
marked this pull request as ready for review
June 12, 2026 23:17
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes two recently added cold-start regression tests in bottlecap that were failing on main, without changing production behavior.
Changes:
- Fixes a test failure caused by dropping the only
Arc<SendingTraceProcessor>too early by passing a clone intoon_platform_runtime_doneand keeping the original alive through the assertion. - Converts a sync
#[test]to#[tokio::test] async fnso it can callsetup()(which usestokio::spawn) without panicking due to a missing Tokio runtime.
1 task
lucaspimentel
added a commit
that referenced
this pull request
Jun 15, 2026
…ake (#1256) - [x] ~Note: stacked on #1255. Merge that PR first.~ ## Overview Extends the in-process fake-intake APM tests (added in #1194) with **value-level** coverage of trace stats aggregation. The existing stats tests assert serialization fidelity (a hand-built `ClientStatsPayload` survives the roundtrip) and stats presence/suppression (`_dd.compute_stats` truth table). None assert that the *computed* aggregates reaching the intake are correct. This PR routes concrete spans through the real pipeline (`SendingTraceProcessor` → `StatsConcentratorService` → `StatsFlusher` → fake-intake) and asserts on the decoded `ClientGroupedStats`: - `e2e_stats_count_aggregates_identical_spans` — 3 identical top-level spans collapse into one group with `hits == 3`, `top_level_hits == 3`. - `e2e_stats_counts_errors_separately_from_hits` — `errors` counts only `error != 0`; `hits` counts all. - `e2e_stats_sums_span_durations` — `duration` is the nanosecond sum of aggregated spans. - `e2e_stats_groups_by_resource` — distinct resources stay in separate groups. - `e2e_stats_groups_by_span_kind` — distinct `span.kind` stay in separate groups. - `e2e_stats_groups_by_http_status_code` — distinct `http.status_code` stay in separate groups. These verify bottlecap's wiring and serialization end-to-end (a span entering the trace path comes out the intake correctly aggregated), which libdatadog's concentrator unit tests cannot see. The aggregation *math* itself is owned and exhaustively tested upstream in `libdd-trace-stats`. To support multiple spans, `run_processor_pipeline` was generalized into `run_processor_pipeline_with_traces(.., traces)`, with a thin single-span wrapper preserving the existing `_dd.compute_stats` tests unchanged. ### Out of scope: time-bucketing Deliberately not added. Bucket assignment lives entirely inside `SpanConcentrator` and is thoroughly tested in `libdd-trace-stats` (`test_concentrator_stats_counts` over multiple buckets, `test_concentrator_oldest_timestamp_cold/hot`, `test_force_flush`). bottlecap's wiring does not touch bucketing, so a test here would duplicate upstream coverage and require a production clock seam (the concentrator seeds `oldest_timestamp` from wall-clock) for no unique coverage. ## Testing This PR is only adding more tests. If they pass, we're good. Ran locally: - `cargo test --test apm_integration_test` - `cargo fmt --all -- --check` - `cargo clippy --workspace --all-targets --features default` > *"Six tests walk into a bucket. They all collapse into one group — turns out they were identical the whole time."* — Claude 🤖
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
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.
Overview
Two tests added in #1254 were failing on
main. The Rust "Test Suite" CI job is not a required check (onlydevflow/mergegateis), so the red suite did not block #1254 from merging.1.
enrich_ctx_keeps_tracer_set_cold_start_trace_id_without_tracer_detectedAnnotated
#[test]but callssetup(), which spawns a Tokio task viatokio::spawn(service.run()). Running it panicked withthere is no reactor running, must be called from the context of a Tokio 1.x runtime. Changed to#[tokio::test] async fnto match its sibling tests.2.
timeout_without_tracer_does_not_send_cold_start_spanPassed its
trace_senderby value intoon_platform_runtime_done, dropping the onlyArc<SendingTraceProcessor>beforetry_recv(). The receiver then reportedDisconnected, but the test assertedEmpty, so it failed even though the production code correctly never sent the cold start span. Fixed by passing a clone and holding the sender until after the assertion so the channel stays connected.No production code changes; both fixes are test-only.