Conversation
Contributor
size-limit report 📦
|
JPeer264
force-pushed
the
jp/cf-int-tests-span-helpers
branch
4 times, most recently
from
September 8, 2026 07:39
7a50715 to
013f716
Compare
JPeer264
force-pushed
the
jp/cf-int-tests-span-helpers
branch
from
September 18, 2026 19:05
013f716 to
a8adff1
Compare
JPeer264
marked this pull request as ready for review
September 21, 2026 06:44
JPeer264
requested review from
isaacs,
msonnb and
mydea
and removed request for
a team
September 21, 2026 06:44
Contributor
Member
|
This mostly looks pretty good, but it has some conflicts with |
JPeer264
force-pushed
the
jp/cf-int-tests-span-helpers
branch
from
September 27, 2026 09:54
a8adff1 to
9050d9b
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Member
Author
Rebased and tests work |
… test runner Adds `spanUtils.ts` next to `runner.ts`, and two span collection methods on the runner. The suites ported to span streaming then do not each write their own envelope handling. `spanUtils.ts` reads a single envelope: `getSpanContainer` and `getSpansFromEnvelope`. It re-exports `getSpanOp` from `@sentry-internal/test-utils`, which this package already depends on. That avoids a third copy of the function in the repo. The runner reads across envelopes: `collectStreamedSpans` and `collectStreamedSpansUntilSegment`. The names follow `dev-packages/test-utils/src/event-proxy-server.ts`, so a streamed span assertion reads the same in this package and in the E2E apps. A trace does not arrive in one envelope when several isolates send spans. The collecting helpers therefore group the spans by trace, and resolve on the first trace that satisfies the predicate. Span waiters observe the envelope stream and never consume it. `.expect(...)` stays usable for error envelopes at the same time. Span waiters also receive the runner rejection, so a worker that fails to boot gives the real error instead of a Vitest timeout. `collectStreamedSpansUntilSegment` is for asserting on the segment span alone. Each envelope is its own request to the mock server, so the segment can be received before the envelope carrying its children even though it ends last. A suite that asserts on the children waits for those children instead, by name or by count. A runner now tears down its own workers from `onTestFinished`, and that teardown kills only its own workers rather than running the shared cleanup. Both halves are needed. A suite that asserts on streamed spans never calls `completed()`, so its runner never settles and its `wrangler dev` would otherwise stay alive until the Vitest process exits. A full run left 32 of them behind. Running the shared cleanup instead would kill the worker the next test had already started. The process-exit cleanup still covers every worker. Renames `public-api/startSpan-streamed` to `public-api/startSpan`, and `tracing/ignoreSpans-streamed` to `tracing/ignoreSpans`. Ports both suites onto the helpers, which proves the shape before the bulk of the port. Span streaming is the default, so a `-streamed` suffix no longer marks a difference, and the explicit `traceLifecycle: 'stream'` those four suites carried says nothing either. It goes with the suffix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JPeer264
force-pushed
the
jp/cf-int-tests-span-helpers
branch
from
September 27, 2026 13:28
9050d9b to
20f51f7
Compare
isaacs
approved these changes
Sep 29, 2026
isaacs
left a comment
isaacs
left a comment
Member
There was a problem hiding this comment.
Two small suggestions to add a little bit of resilience, but this is fine.
Comment on lines
+259
to
+263
| onTestFinished(cleanupThisRunner); | ||
|
|
||
| let closeMockServer: (() => void) | undefined; | ||
|
|
||
| const { resolve, reject, promise: isComplete } = deferredPromise(cleanupThisRunner); |
Member
There was a problem hiding this comment.
A runner now has three teardown paths:
onTestFinished(cleanupThisRunner)(line 259)deferredPromise(cleanupThisRunner), whenisCompletesettles (line 263)CLEANUP_STEPS, two entries per runner, at process exit (lines 440, 528)
This causes a few problems:
- With
onTestFinishedin place, thedeferredPromise(cleanupThisRunner)is redundant. CLEANUP_STEPSnever drops entries. It grows by two closures per runner and callskill()/close()on dead objects at exit. It's harmless, but it's extra.- The PR description says span-only runners settle "from the abort signal." In vitest 3 the context
signalaborts only on timeout, not on normal test end. For a passing span-only test,onTestFinishedis the only trigger. The comment at lines 242-247 says the same thing.
Suggestion: Keep onTestFinished. Add cleanupThisRunner to CLEANUP_STEPS once, and have it remove itself when it runs. Drop the deferredPromise(cleanupThisRunner) argument and the two ad hoc CLEANUP_STEPS.add calls. One function, two triggers.
Comment on lines
+248
to
+253
| function cleanupThisRunner(): void { | ||
| child?.kill(); | ||
| childSubWorker?.kill(); | ||
| closeMockServer?.(); | ||
| closeMockServer = undefined; | ||
| } |
Member
There was a problem hiding this comment.
If cleanupThisRunner runs while createBasicSentryServer is still pending (the test throws after start() but before its first await), closeMockServer and child are both undefined. The .then later opens the server and spawns wrangler dev. Nothing kills it until process exit. This is the leak the PR sets out to fix, on a narrower path.
We could set a disposed flag in cleanupThisRunner. At the top of the .then, and before each spawn, close the server and return if it is set.
This branch has not been deployed
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.
closes #24146
Adds the shared span assertion helpers
cloudflare-integration-testsis missing, so the ~80 suites still pinned totraceLifecycle: 'static'can be ported without each one hand-rolling its owngetSpanContainer(envelope). Three copies of that function exist in the package today.The new helpers are inspired by the E2E tests:
collectStreamedSpanscollectStreamedSpansUntilSegmentWe don't need
waitForStreamedSpanas given in the ticket, as we already have the.expectRenamed tests
public-api/startSpan-streamedis renamed topublic-api/startSpantracing/ignoreSpans-streamedtotracing/ignoreSpansBoth are ported onto the helpers so the shape is proven before the bulk work starts.
Per-runner teardown
A runner now tears down its own worker and its own mock server when it settles, instead of running the shared
cleanupChildProcesses. A suite that asserts only on streamed spans never callscompleted(), so its runner settles from the abort signal after its own test has ended. The shared cleanup would then kill the worker the next test had already started, and leaving the mock server open would keep one server per scenario listening for the whole run. The ported suites failed intermittently in large runs until both halves of this were fixed. The process-exit cleanup still covers every runner.