Skip to content

fix(test): repair broken cold-start tests from #1254#1255

Merged
lucaspimentel merged 2 commits into
mainfrom
lpimentel/fix-cold-start-test-runtime
Jun 15, 2026
Merged

fix(test): repair broken cold-start tests from #1254#1255
lucaspimentel merged 2 commits into
mainfrom
lpimentel/fix-cold-start-test-runtime

Conversation

@lucaspimentel

@lucaspimentel lucaspimentel commented Jun 12, 2026

Copy link
Copy Markdown
Member

Overview

Two tests added in #1254 were failing on main. The Rust "Test Suite" CI job is not a required check (only devflow/mergegate is), so the red suite did not block #1254 from merging.

1. enrich_ctx_keeps_tracer_set_cold_start_trace_id_without_tracer_detected

Annotated #[test] but calls setup(), which spawns a Tokio task via tokio::spawn(service.run()). Running it panicked with there is no reactor running, must be called from the context of a Tokio 1.x runtime. Changed to #[tokio::test] async fn to match its sibling tests.

2. 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 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.

"A test that drops its own sender and then complains the line went dead: the unit-testing equivalent of cutting the phone cord and filing a complaint about the silence." — Claude 🤖

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]>
@datadog-official

datadog-official Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Pipelines

Fix all issues with BitsAI

⚠️ Warnings

🚦 1 Pipeline job failed

DataDog/datadog-lambda-extension | e2e-test-status (amd64, fips)   View in Datadog   GitLab

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: f397009 | Docs | Datadog PR Page | Give us feedback!

@lucaspimentel
lucaspimentel marked this pull request as ready for review June 12, 2026 22:45
@lucaspimentel
lucaspimentel requested a review from a team as a code owner June 12, 2026 22:45
@lucaspimentel
lucaspimentel requested review from Copilot and shreyamalpani and removed request for Copilot June 12, 2026 22:45
@lucaspimentel
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 lucaspimentel changed the title fix(test): add Tokio runtime to cold-start test fix(test): repair broken cold-start tests from #1254 Jun 12, 2026
@lucaspimentel
lucaspimentel marked this pull request as ready for review June 12, 2026 23:17
Copilot AI review requested due to automatic review settings June 12, 2026 23:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 into on_platform_runtime_done and keeping the original alive through the assertion.
  • Converts a sync #[test] to #[tokio::test] async fn so it can call setup() (which uses tokio::spawn) without panicking due to a missing Tokio runtime.

Comment thread bottlecap/src/lifecycle/invocation/processor.rs

@duncanista duncanista left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MB!

@lucaspimentel
lucaspimentel merged commit aef51ba into main Jun 15, 2026
59 of 60 checks passed
@lucaspimentel
lucaspimentel deleted the lpimentel/fix-cold-start-test-runtime branch June 15, 2026 14:04
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 🤖
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants