fix(test): unbreak vtex_segment cookie-forward tests on Node 22#49
Merged
Conversation
The two failing cases in `client-segment-cookie.test.ts` had been
silently broken since the test was first written — they pass the
default NOOP `RequestStore` so `RequestContext.current` returns null
inside the test callback and the production code never sees the
cookie under test. The five passing cases happen to expect
`headers.cookie === undefined`, which the broken setup also produces,
masking the bug. The release pipeline on `next` finally caught it
because the merged observability work bumped the Node target enough
that another spec wedged the runner into surfacing all failures
instead of one.
There are two compounding issues:
1. `new Request(url, { headers: { cookie } })` silently drops the
`cookie` header under Node 22 / undici because the Request
headers guard is "request", which strips forbidden request
headers. The cookie never reached `request.headers.get("cookie")`.
2. `RequestContext` is backed by a `RequestStore` that defaults to
a NOOP. The ALS-backed store is wired only at worker boot in
site code; in unit tests the run callback executes without any
context propagation, so `RequestContext.current` returns null.
Fix replaces the `RequestContext.run(new Request(...))` fixture with
a fresh `Headers` (which uses the "none" guard, so `set("cookie", ...)`
works) wrapped in a minimal Ctx-shaped object, and overrides the
`RequestContext.current` getter via `vi.spyOn`. The spy is restored
after `fn` resolves to keep tests isolated. No reliance on undici
internals or ALS plumbing.
Result: 413/413 tests pass. Unblocks the Release pipeline on `next`,
which runs `npm test` before `npx semantic-release` and was failing
on the post-merge run of PR #48 (the observability work).
Co-authored-by: Cursor <cursoragent@cursor.com>
|
🎉 This PR is included in version 1.15.0-next.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
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.
What broke
The Release workflow on
nextfailed after PR #48 was merged because two tests invtex/__tests__/client-segment-cookie.test.tsfailed:Both were checking that
vtexFetchResponseforwards thevtex_segmentcookie when one is present on the incomingRequestContext.Why it was actually broken from day one (not introduced by #48)
Two compounding issues, both pre-existing:
new Request(url, { headers: { cookie } })stripscookieunder Node 22 / undici. Request headers are in "request" guard mode, which silently drops forbidden request headers includingcookie. The cookie never reachesrequest.headers.get("cookie")inside the production code.@decocms/start'sRequestContextdefaults to a NOOPRequestStore. The ALS-backed store is installed by site code at worker boot, not in unit tests. SoRequestContext.run(req, fn)callsfn()without any propagation, andRequestContext.currentinsidefnreturnsnull— production code under test never sees the test's cookie.The five other cases in the file happen to expect
headers.cookie === undefined, which the broken setup also produces, masking the bug. PR #48 is the trigger but not the cause —git stash-ing all of #48's changes and running the test on stockmainreproduces the same 2 failures.Fix
Replace the
RequestContext.run(new Request(...))fixture with:Headers(which uses the "none" guard, soset("cookie", ...)works);Ctx-shaped object ({ request: { headers }, signal, responseHeaders, bag, startedAt });vi.spyOn(RequestContext, "current", "get").mockReturnValue(fakeCtx)so the static getter returns the fake instead of consulting the noop store.The spy is restored after
fnresolves to keep tests isolated. Nothing here depends on undici internals or ALS plumbing.Test plan
npm test→ 413/413 passing (was 411/413)npm run typecheck→ cleannpx biome check vtex/__tests__/client-segment-cookie.test.ts→ cleannextrunsnpm testcleanly and semantic-release publishes the next1.x.0-next.Xprerelease (which carries the PR feat(observability): URL routers + per-call operation names + commerce histograms #48 observability work)Followups
When the next push to
nextsucceeds, this PR plus #48 will be packaged into a single prerelease on the@nextdist-tag. After validating on a real storefront, both can be promoted tomainvia the normalnext → mainmerge.Made with Cursor
Summary by cubic
Unbreaks the
vtex_segmentcookie-forwarding tests on Node 22 by simulating a real request context and setting the cookie on a standaloneHeaders. Restores forwarding behavior and auth header preservation, unblocking the Release pipeline onnext.RequestContext.run(new Request(...))with a minimal fake context using a standaloneHeadersto setcookie.RequestContext.currentviavi.spyOn, restoring it after each run.cookieand the default NOOP store in@decocms/startthat hid the cookie.Written for commit 427bc73. Summary will update on new commits. Review in cubic