Migrated Personalization functional tests to Vitest+Playwright+MSW#1546
Migrated Personalization functional tests to Vitest+Playwright+MSW#1546carterworks wants to merge 4 commits into
Conversation
|
|
| Filename | Overview |
|---|---|
| packages/browser/test/integration/specs/Personalization/personalization.spec.js | New 700-line integration test file migrating 23 functional tests to the Vitest+Playwright+MSW harness. Contains one unused helper (makeInteractHandler) and inline mbox-cookie teardown that is skipped when a test fails. |
Sequence Diagram
sequenceDiagram
participant Test as Vitest Test
participant Fixture as alloy fixture (extend.js)
participant MSW as MSW Worker (browser)
participant Alloy as Alloy SDK
participant NR as NetworkRecorder
Test->>Fixture: auto-initialize (beforeEach)
Fixture->>Fixture: clear kndctr_/AMCV_ cookies
Fixture->>Alloy: setupBaseCode() + setupAlloy()
Test->>MSW: worker.use(sendEventHandler / formBasedHandler)
Test->>Alloy: alloy("configure", alloyConfig)
Test->>Alloy: alloy("sendEvent", options)
Alloy->>MSW: "POST /v1/interact?configId=…"
MSW->>MSW: match configId → return mock JSON
MSW-->>Alloy: mocked edge response
Alloy-->>Test: result (propositions[])
Test->>NR: "networkRecorder.findCalls(/v1\/interact/, {retries, minCalls})"
NR-->>Test: recorded calls[]
Test->>Test: assert request shape and response propositions
Fixture->>Fixture: cleanAlloy() (afterEach)
Fixture->>MSW: worker.resetHandlers() (afterEach)
Reviews (1): Last reviewed commit: "test(integration): migrate personalizati..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| * Covered by personalizationInteractions.spec.js. | ||
| * | ||
| * C21636439 — Conflict resolution: show one in-app message sorted by rank. | ||
| * Covered by AJO/inapp_messages.spec.js. | ||
| * | ||
| * C21886916 — Shadow-DOM click tracking. Skipped per migration plan. | ||
| * | ||
| * C22098199 — Non-idempotent actions not applied multiple times. | ||
| * Covered by personalizationRender.spec.js. | ||
| */ | ||
|
|
||
| import { http, HttpResponse } from "msw"; | ||
| // eslint-disable-next-line import/no-unresolved | ||
| import { server } from "vitest/browser"; |
There was a problem hiding this comment.
makeInteractHandler is defined at the top of the file but is never called anywhere in the 700-line spec. Every test that needs an interact endpoint uses the shared sendEventHandler or the module-level formBasedHandler instead. If this factory was scaffolded for a future test or as an experiment for the C6364800 tests (which ultimately relied on sendEventHandler + the inline responseBody), it should be removed to avoid confusion about its intended use.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| ]); | ||
|
|
||
| const applyResult = await alloy("applyResponse", { | ||
| renderDecisions: true, | ||
| responseBody, | ||
| }); | ||
|
|
||
| expect(applyResult).toHaveProperty("decisions"); | ||
| expect(Array.isArray(applyResult.decisions)).toBe(true); | ||
| // propositions should have renderAttempted=true | ||
| expect(applyResult).toHaveProperty("propositions"); | ||
| applyResult.propositions.forEach((p) => { | ||
| expect(p.renderAttempted).toBe(true); | ||
| }); |
There was a problem hiding this comment.
mbox cookie cleanup bypassed on test failure
The mbox cookie is planted inline inside the test body and deleted at the very end (document.cookie = "mbox=; …expires…"). If any assertion or await between those two lines throws, the test exits early and the cookie is never removed. The alloy fixture in extend.js only wipes kndctr_ and AMCV_ cookies, so the stale mbox cookie persists into the next test.
The immediate neighbour, C7498683, also plants an mbox cookie, so in practice it is overwritten — but any test further down that doesn't explicitly set targetMigrationEnabled could pick up unexpected Target-migration state. Moving the cookie setup and teardown into a beforeEach / afterEach pair (or a Vitest fixture) guarantees cleanup even when a test fails mid-way. The same concern applies to the matching cookie in C7498683 (around line 430).
carterworks
force-pushed
the
migrate-integration/13-personalization-spa
branch
from
55c07c6 to
b0f403c
Compare
carterworks
force-pushed
the
migrate-integration/14-personalization
branch
from
10498fd to
952fcaf
Compare
There was a problem hiding this comment.
carterworks has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
carterworks
force-pushed
the
migrate-integration/13-personalization-spa
branch
from
b0f403c to
3ca0c49
Compare
carterworks
force-pushed
the
migrate-integration/14-personalization
branch
from
952fcaf to
3b728c8
Compare
There was a problem hiding this comment.
carterworks has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
carterworks
changed the base branch from
migrate-integration/13-personalization-spa
to
migrate-integration/00-infra
There was a problem hiding this comment.
carterworks has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
carterworks
force-pushed
the
migrate-integration/14-personalization
branch
4 times, most recently
from
d902a8a to
8510d7a
Compare
carterworks
force-pushed
the
migrate-integration/14-personalization
branch
from
8510d7a to
bf16112
Compare
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
carterworks
force-pushed
the
migrate-integration/14-personalization
branch
from
bf16112 to
1fdff92
Compare
…y integration suite Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PR #1546 —
|
| Criterion | Status |
|---|---|
| 1a. C-file coverage (9 deleted, 9 covered) | 🟢 |
| 1b. Subtest counts | 🟢 C14317242 1→3 (improvement), all others match |
| 1c. Assertion fidelity | 🔴 2 gaps (vacuous loops; mbox entries) |
| 2. Skip justifications | 🟢 all verified against old source |
| 3. Mock fidelity | 🟡 makeInteractHandler is defined but never called |
| 4. Timing / flake | 🟢 findCalls with retries + minCalls throughout |
| 5. Isolation | 🟢 handlers reset, recorder reset, cookies cleared, DOM cleaned |
| 6. Hygiene | 🟢 license ✓, vitest.projects.js auto-discovers glob ✓ |
Parity Matrix
| Old C-file | Migrated | Subtests old→new | Notes |
|---|---|---|---|
| C28755.js | ✅ | 1→1 | |
| C28756.js | ✅ | 1→1 | scope order .eql → .toContain (weakened, low risk) |
| C205529.js | ✅ | 1→1 | |
| C6364800.js | ✅ | 2→2 | DOM content assertions weakened vs live-edge original |
| C6984408.js | ✅ | 1→1 | mbox entries assertion dropped (see §1c) |
| C7498683.js | ✅ | 1→1 | |
| C7878996.js | ✅ | 1→1 | |
| C14317242.js | ✅ | 1→3 | split is an improvement |
| C15325239.js | ✅ | 1→1 | type top-level → xdm.eventType (new form is more correct) |
1c. Assertion Fidelity
🔴 Finding 1 — Vacuous proposition loops: sendEventResponse.json has empty payloads
sendEventResponse.json returns two personalization:decisions handles, both with "payload": []. Alloy traces these through createFetchDataHandler.js:56-69 — getPayloadsByType("personalization:decisions") returns the empty arrays; handles.map(createProposition) yields nothing; returnedPropositions = []. Every test using sendEventHandler that then loops over propositions is silently vacuous:
// personalization.spec.js:253-255 (C28755)
result.propositions.forEach((p) => {
expect(p.renderAttempted).toBe(false); // never executes — array is empty
});Same issue at lines 533–535 (C7878996), 627–629 (C14317242 subtest 2), 656–658 (C14317242 subtest 3).
Note: C28756 is fine — it uses formBasedHandler → personalizationFormBasedResponse.json which returns two real proposition payloads. The expect(matchingProposition).toBeDefined() at line 297 would fail on an empty array, so that test legitimately exercises the proposition path.
Also a deeper issue for C7878996: because eventResult.propositions = [], the notificationPropositions array sent in the second sendEvent is empty ([]). The test verifies two requests succeed, but the notification payload carries no propositions — it doesn't exercise the "notification with proposition IDs" path the old test intended.
Fix: Enrich sendEventResponse.json with at least one real personalization:decisions payload item, or add expect(result.propositions.length).toBeGreaterThan(0) guard before each loop.
🔴 Finding 2 — C6984408: mbox cookie in meta.state.entries not verified
Old spec (C6984408.js):
await t.expect(request.meta.state.entries.find(({ key }) => key === "mbox")).ok();
await t.expect(request.meta.target.migration).eql(true);New spec (personalization.spec.js:474-475):
expect(body.meta).toBeDefined();
expect(body.meta.target?.migration).toBe(true);The meta.target.migration flag is tested, but the core assertion — that the mbox cookie value was actually read from document.cookie and forwarded in meta.state.entries — is absent. These test different things: the flag confirms migration mode is active; the entries check confirms the cookie was plumbed through.
Fix: Add at line 476:
expect(body.meta.state.entries.find(({ key }) => key === "mbox")).toBeDefined();2. Skip Justifications
All verified against old source via git show migrate-integration/00-infra:
- C7494472 —
test.skip("Test C7494472: AJO offers should be delivered")✅ - C7638574 —
test.skip("Test C7638574: AJO offers for custom surface are delivered")✅ - C9932846 —
test.skip("Test C9932846: AJO click-tracking offers are delivered")✅ - C11389844 —
test.skip("Test C11389844: AJO SPA support")✅ - C28758 (ShadowDOM), C44363 (QA Mode), C753469/70 (CSP nonce), C3272624 (profile attrs), C8631576/77 (client hints), C205528 (redirect) — all plausible env/infra constraints, original files are not
test.skipbut the integration constraints are real. 🟢
3. Mock Fidelity
🟡 Dead code: makeInteractHandler is defined at personalization.spec.js:155-166 but never called anywhere in the file. It's likely a leftover from an earlier draft. No behavioral impact, but it should be removed.
formBasedHandler correctly serves personalizationFormBasedResponse.json with a real alloy-test-scope-1 proposition containing "<h3>welcome to TARGET AWESOME WORLD!!! </h3>" — matching the old C28756 content assertion verbatim. 🟢
buildEdgeResponse (inline helper for C6364800) produces a synthetic dom-action/setHtml proposition. The old spec used a live edge response and asserted DOM text matching /Greetings.+|Thanks.+/. The new spec checks renderAttempted=true and innerHTML containment of the injected content ("Second apply content") in the second subtest. Adequate for unit-style integration. 🟢
4. Timing / Flake
All network waits use networkRecorder.findCalls with retries and minCalls. Multi-call tests use appropriately higher retry counts (C7878996: retries:20, minCalls:2; C15325239: retries:20, minCalls:3; C14317242: retries:15, minCalls:2). No bare setTimeout or unbounded polls. 🟢
5. Isolation
worker.resetHandlers()runs after each test via{ auto: true }fixture (extend.js:41). 🟢networkRecorder.reset()runs before and after each test (extend.js:48–51). 🟢- Cookies cleared wholesale via
cookieStore.getAll()before each test (extend.js:59–62). C6984408/C7498683 also defensively expire the mbox cookie after the test — belt-and-suspenders, fine. 🟢 - C6364800 DOM fixture uses
beforeEachreturning a cleanup function — correct Vitest pattern. 🟢
6. Hygiene
- License header: 2026 ✅
test.metatags: absent — correct, those are TestCafe-only.vitest.projects.jsalready includespackages/browser/test/integration/**/*.{test,spec}.*— new spec is auto-discovered, no config change needed. ✅- C15325239: old spec used top-level
type: "decisioning.propositionFetch"as a sendEvent option; new spec usesxdm: { eventType: "decisioning.propositionFetch" }. The SDK readsxdm.eventType(src/components/Context/injectOneTimeAnalyticsReferrer.js:25), so the new form is semantically more correct. Since the test only counts 3 network calls (not the event type in the body), this doesn't introduce a false pass. 🟢 - Migration notes comment block is thorough. The rationales are specific and cite the original constraints clearly.
1 participant
Changed Packages
Description
Migrates the Personalization functional tests to the new Vitest+Playwright+MSW harness.
Related Issue
Part of the functional test → integration test migration. See
packages/browser/test/FUNCTIONAL_MIGRATION_PLAN.md.Motivation and Context
The existing TestCafe functional test suite is being migrated to Vitest+Playwright+MSW to enable faster, more reliable CI testing without a running server. This PR is part of a stacked series — each PR migrates one test file.
Functional tests replaced:
packages/browser/test/functional/specs/Personalization/C28755.jspackages/browser/test/functional/specs/Personalization/C28756.jspackages/browser/test/functional/specs/Personalization/C28757.jspackages/browser/test/functional/specs/Personalization/C28758.jspackages/browser/test/functional/specs/Personalization/C28759.jspackages/browser/test/functional/specs/Personalization/C28760.jspackages/browser/test/functional/specs/Personalization/C3272624.jspackages/browser/test/functional/specs/Personalization/C5298194.jspackages/browser/test/functional/specs/Personalization/C5805675.jspackages/browser/test/functional/specs/Personalization/C5805676.jspackages/browser/test/functional/specs/Personalization/C6364797.jspackages/browser/test/functional/specs/Personalization/C6364798.jspackages/browser/test/functional/specs/Personalization/C6364799.jspackages/browser/test/functional/specs/Personalization/C6364800.jspackages/browser/test/functional/specs/Personalization/C6984408.jspackages/browser/test/functional/specs/Personalization/C7494472.jspackages/browser/test/functional/specs/Personalization/C7498683.jspackages/browser/test/functional/specs/Personalization/C7638574.jspackages/browser/test/functional/specs/Personalization/C7878996.jspackages/browser/test/functional/specs/Personalization/C8631576.jspackages/browser/test/functional/specs/Personalization/C8631577.jspackages/browser/test/functional/specs/Personalization/C9932846.jspackages/browser/test/functional/specs/Personalization/C17409728.jsTypes of changes
Checklist:
Stack