Skip to content

test(node,browser): Add integration tests for continueTrace and startNewTrace - #22804

Open
mydea wants to merge 1 commit into
fn/remove-node-otel-e2efrom
fn/continue-start-trace-tests
Open

test(node,browser): Add integration tests for continueTrace and startNewTrace#22804
mydea wants to merge 1 commit into
fn/remove-node-otel-e2efrom
fn/continue-start-trace-tests

Conversation

@mydea

@mydea mydea commented Jul 29, 2026

Copy link
Copy Markdown
Member

Adds integration tests that directly exercise the public Sentry.continueTrace() and Sentry.startNewTrace() APIs, in both node and browser integration test packages. Before this, no integration test called either API directly — the real-world incoming-trace path (HTTP instrumentation → propagator extract) was covered, but the manual API surface was not.

These land first on develop so the suites are validated against the current implementation. A follow-up stacks the SentryPropagator simplification PR on top, where the same node suites guard the refactor (which reroutes these APIs through @sentry/core and a custom SentryTracer).

node-integration-tests

continueTrace is covered across four incoming-trace variants — sampled (-1), unsampled (-0), deferred sampling decision (no trailing flag), and no incoming trace — times three tracing configs (no tracesSampleRate, =1, =0). It asserts the continued trace id reaches both the error event and the transaction, that an incoming sampling decision overrides the local sample rate, and that outgoing propagation (via getTraceData()) carries the continued trace.

startNewTrace asserts that every root span created within a single callback shares the one new trace id — two startInactiveSpan plus a startSpan — that the new trace is detached from an ambient active span, and that outgoing propagation carries the new trace id, across all three tracing configs. The multi-root-span assertion specifically guards the OTEL behavior where each root span re-enters startNewTrace (tracer.ts _startSentrySpan): without a shared remote span context, each root would otherwise mint its own trace id.

Both node suites run in ESM and CJS via createEsmAndCjsTests, with the config/variant matrices driven by describe.each/test.each.

browser-integration-tests

The browser SDK is non-OTEL, so continueTrace/startNewTrace run core's implementations directly. startNewTrace was already covered for the single-span case; continueTrace had no direct coverage.

  • New continueTrace suites cover the same four incoming variants across tracesSampleRate: 1, tracesSampleRate: 0, and tracing-without-performance, asserting trace continuation on transactions/errors and outgoing request propagation.
  • A new startNewTrace-multiple-spans suite asserts multiple root spans in one callback share the single new trace id and remain independent root spans.

Comment thread dev-packages/node-integration-tests/suites/tracing/continueTrace/test.ts Outdated
@mydea mydea changed the title test(node): Add integration tests for continueTrace and startNewTrace test(node,browser): Add integration tests for continueTrace and startNewTrace Jul 29, 2026
@mydea
mydea force-pushed the fn/continue-start-trace-tests branch from f8a1d7d to 9a6ddf1 Compare July 29, 2026 09:52
@mydea
mydea marked this pull request as ready for review July 29, 2026 10:08
@mydea
mydea requested review from a team as code owners July 29, 2026 10:08
@mydea
mydea requested review from andreiborza, isaacs, logaretm and msonnb and removed request for a team July 29, 2026 10:08
@mydea
mydea force-pushed the fn/continue-start-trace-tests branch from 9a6ddf1 to 9a9c4f5 Compare July 29, 2026 13:36

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 9a9c4f5. Configure here.

const headers = await outgoingRequest.allHeaders();
expect(headers['sentry-trace']).toMatch(new RegExp(`^${UNSAMPLED_TRACE_ID}-[a-f0-9]{16}-0$`));
},
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing no-transaction assertions

Medium Severity

Several cases claim a transaction is not emitted (unsampled, deferred with tracesSampleRate=0, TwP / negative parent decisions) but only assert on the error event and outgoing headers. Nothing fails if a transaction is still sent. Flagged because the review rules require an explicit assertion when a payload is expected to be absent.

Additional Locations (2)
Fix in Cursor Fix in Web

Triggered by project rule: PR Review Guidelines for Cursor Bot

Reviewed by Cursor Bugbot for commit 9a9c4f5. Configure here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Valid, but this feels pretty minor to me.

@mydea
mydea force-pushed the fn/continue-start-trace-tests branch 2 times, most recently from 4fbbdeb to 4f3087d Compare July 29, 2026 14:15
Adds node-integration-tests directly exercising the public `Sentry.continueTrace()`
and `Sentry.startNewTrace()` APIs, which previously had no direct coverage in
node-integration-tests.

`continueTrace` is covered across four incoming-trace variants (sampled, unsampled,
deferred sampling decision, and no incoming trace) and three tracing configs (no
`tracesSampleRate`, `=1`, `=0`), asserting the continued trace id reaches both the
error event and the transaction, that the incoming sampling decision overrides the
local rate, and that outgoing propagation carries the continued trace.

`startNewTrace` asserts that every root span created within a single callback shares
the one new trace id (multiple `startInactiveSpan` plus a `startSpan`), that the new
trace is detached from an ambient active span, and that outgoing propagation carries
the new trace id — across all three tracing configs.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

test(browser): Add continueTrace + multi-span startNewTrace coverage

Adds browser-integration-tests for `Sentry.continueTrace()`, which had no direct
coverage (only `dist/` bundle artifacts referenced it). Covers four incoming-trace
variants (sampled, unsampled, deferred sampling decision, no incoming trace) across
three tracing configs — `tracesSampleRate: 1`, `tracesSampleRate: 0`, and
tracing-without-performance — asserting trace-id continuation on transactions and
error events, incoming-sampling-decision precedence over the local rate, and outgoing
request propagation.

`startNewTrace` was already covered for the single-span case, so this adds a focused
suite asserting that multiple root spans created within one `startNewTrace` callback
all share the single new trace id and remain independent root spans.

Also refactors the node `continueTrace`/`startNewTrace` suites to drive the config and
variant matrices via `describe.each`/`test.each` instead of nested loops, and widens
the runner's `withEnv` to accept `undefined` values so an unset config maps cleanly to
TwP mode.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

adjustments and fixes
@mydea
mydea force-pushed the fn/continue-start-trace-tests branch from 4f3087d to c730c52 Compare July 29, 2026 14:47
@mydea
mydea changed the base branch from develop to fn/remove-node-otel-e2e July 29, 2026 14:50

@isaacs isaacs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like this indeed would've caught the issues in the other PR 👍

The failing e2e looks unrelated.

const headers = await outgoingRequest.allHeaders();
expect(headers['sentry-trace']).toMatch(new RegExp(`^${UNSAMPLED_TRACE_ID}-[a-f0-9]{16}-0$`));
},
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Valid, but this feels pretty minor to me.

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.

2 participants