Migrated Data Collector functional tests to Vitest+Playwright+MSW#1539
Migrated Data Collector functional tests to Vitest+Playwright+MSW#1539carterworks wants to merge 38 commits into
Conversation
|
|
| Filename | Overview |
|---|---|
| packages/browser/test/integration/specs/Data Collector/dataCollector.spec.js | New integration spec migrating 13 Data Collector functional tests to Vitest+Playwright+MSW. Four tests are correctly skipped (sendBeacon not interceptable). Issues flagged in prior review rounds include DOM element accumulation, window.___getLinkDetails leaking across tests, consoleSpy not restored on unhappy paths, and a misleading third C81183 test name with an unused result variable. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph Fixture["Test Fixture (extend.js, auto:true)"]
direction TB
NR_RESET["networkRecorder.reset()"]
WORKER_START["worker.start() (once)"]
SETUP_BASE["setupBaseCode()"]
SETUP_ALLOY["setupAlloy()"]
USE["yield alloy to test"]
CLEAN["cleanAlloy() (deletes __alloyMonitors, alloy)"]
WORKER_RESET["worker.resetHandlers()"]
NR_RESET2["networkRecorder.reset()"]
end
subgraph TestBody["Test Body"]
WORKER_USE["worker.use(handler)"]
CONFIGURE["alloy('configure', ...)"]
ACTION["sendEvent / link.click()"]
FIND_CALL["networkRecorder.findCall(pattern)"]
ASSERT["expect(...)"]
end
subgraph MSW["MSW Service Worker"]
INTERCEPT["Intercept fetch()"]
RECORD_REQ["networkRecorder.captureRequest()"]
HANDLE["handler responds"]
RECORD_RESP["networkRecorder.captureResponse()"]
end
NR_RESET --> WORKER_START --> SETUP_BASE --> SETUP_ALLOY --> USE
USE --> WORKER_USE --> CONFIGURE --> ACTION --> FIND_CALL --> ASSERT
ASSERT --> CLEAN --> WORKER_RESET --> NR_RESET2
ACTION -- "fetch to edge" --> INTERCEPT --> RECORD_REQ --> HANDLE --> RECORD_RESP
FIND_CALL -- "polls calls[]" --> RECORD_RESP
Reviews (2): Last reviewed commit: "test(integration): migrate data collecto..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Skipped: tests that assert collect-vs-interact routing depend on sendBeacon | ||
| // interception via MSW, which is not reliably supported in browser mode, and | ||
| // this test was a baseline failure in the functional suite. | ||
| // See FUNCTIONAL_MIGRATION_PLAN.md §1. | ||
| test.skip("C8118 - link click routes to interact (no identity) then collect (identity established)", () => {}); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // C9369211 – sendEvent includes a Referer header | ||
| // --------------------------------------------------------------------------- | ||
| // Skipped: the collect-endpoint portion of this test uses sendBeacon, which | ||
| // MSW cannot intercept in browser mode. The interact portion relies on | ||
| // inspecting request headers that MSW/networkRecorder may not surface | ||
| // consistently. This was a baseline failure in the functional suite. | ||
| // See FUNCTIONAL_MIGRATION_PLAN.md §1. | ||
| test.skip("C9369211 - sendEvent includes a Referer header on interact and collect requests", () => {}); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // C81182 – onBeforeLinkClickSend with personalization metric | ||
| // --------------------------------------------------------------------------- | ||
| // Skipped: all sub-tests in the source functional file are already marked | ||
| // test.skip because they require a specific personalization response from the | ||
| // live edge that is difficult to reproduce deterministically with MSW mocks. | ||
| test.skip("C81182 - onBeforeLinkClickSend interacts with personalization metric on link (source tests skipped)", () => {}); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // C81183 – getLinkDetails monitoring hook | ||
| // --------------------------------------------------------------------------- | ||
| describe("C81183 - getLinkDetails monitoring hook via __alloyMonitors", () => { | ||
| test("getLinkDetails returns correct link info for a visible link element", async ({ | ||
| alloy, | ||
| }) => { | ||
| // Install monitor BEFORE configure so onInstanceConfigured fires | ||
| window.__alloyMonitors = window.__alloyMonitors || []; | ||
| window.__alloyMonitors.push({ | ||
| onInstanceConfigured(data) { | ||
| window.___getLinkDetails = data.getLinkDetails; | ||
| }, | ||
| }); | ||
|
|
||
| await alloy("configure", { | ||
| ...alloyConfig, | ||
| clickCollectionEnabled: true, | ||
| }); | ||
|
|
||
| const link = document.createElement("a"); | ||
| link.id = "alloy-link-test"; | ||
| link.href = "https://example.com/valid.html"; | ||
| link.textContent = "Test Link"; | ||
| document.body.appendChild(link); | ||
|
|
||
| const result = window.___getLinkDetails(link); | ||
| expect(result).toBeTruthy(); | ||
| expect(result.linkName).toBeTruthy(); | ||
| expect(result.linkUrl).toContain("example.com"); | ||
| expect(result.linkType).toBe("exit"); | ||
| }); | ||
|
|
||
| test("getLinkDetails returns results even when clickCollectionEnabled is false", async ({ | ||
| alloy, | ||
| }) => { | ||
| window.__alloyMonitors = window.__alloyMonitors || []; | ||
| window.__alloyMonitors.push({ | ||
| onInstanceConfigured(data) { | ||
| window.___getLinkDetails = data.getLinkDetails; | ||
| }, | ||
| }); | ||
|
|
||
| await alloy("configure", { | ||
| ...alloyConfig, | ||
| clickCollectionEnabled: false, | ||
| }); | ||
|
|
||
| const link = document.createElement("a"); | ||
| link.id = "alloy-link-test-disabled"; | ||
| link.href = "https://example.com/"; | ||
| link.textContent = "External Link"; | ||
| document.body.appendChild(link); | ||
|
|
||
| const result = window.___getLinkDetails(link); | ||
| expect(result).toBeTruthy(); | ||
| expect(result.linkName).toBeTruthy(); | ||
| expect(result.linkType).toBe("exit"); | ||
| }); | ||
|
|
||
| test("getLinkDetails returns falsy for an element that would not produce a click event", async ({ | ||
| alloy, | ||
| }) => { | ||
| window.__alloyMonitors = window.__alloyMonitors || []; | ||
| window.__alloyMonitors.push({ | ||
| onInstanceConfigured(data) { | ||
| window.___getLinkDetails = data.getLinkDetails; | ||
| }, | ||
| }); | ||
|
|
||
| await alloy("configure", { | ||
| ...alloyConfig, | ||
| clickCollectionEnabled: true, | ||
| onBeforeLinkClickSend: (options) => { | ||
| const { clickedElement } = options; | ||
| if (clickedElement.id === "cancel-alloy-link-test") { | ||
| return false; | ||
| } | ||
| return true; | ||
| }, | ||
| }); | ||
|
|
||
| const cancelLink = document.createElement("a"); | ||
| cancelLink.id = "cancel-alloy-link-test"; | ||
| cancelLink.href = "https://example.com/canceled.html"; | ||
| cancelLink.textContent = "Canceled Link"; | ||
| document.body.appendChild(cancelLink); | ||
|
|
||
| // getLinkDetails itself doesn't invoke onBeforeLinkClickSend — it returns | ||
| // the raw link details regardless of the callback. This just proves the | ||
| // monitor hook is accessible and returns a value. | ||
| const result = window.___getLinkDetails(cancelLink); | ||
| // The result may or may not be defined depending on implementation, but | ||
| // the call itself must not throw. | ||
| expect(() => window.___getLinkDetails(cancelLink)).not.toThrow(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
window.___getLinkDetails never cleaned up between tests
cleanAlloy() deletes window.__alloyMonitors and window.alloy, but window.___getLinkDetails (set inside each onInstanceConfigured callback) is never removed. If configure fails or onInstanceConfigured does not fire in a later test, the test would silently execute against the getLinkDetails function from the previous alloy instance rather than fail loudly. Adding delete window.___getLinkDetails to the teardown in clean.js would make the dependency explicit.
|
|
||
| const result = window.___getLinkDetails(link); | ||
| expect(result).toBeTruthy(); | ||
| expect(result.linkName).toBeTruthy(); | ||
| expect(result.linkType).toBe("exit"); | ||
| }); | ||
|
|
||
| test("getLinkDetails returns falsy for an element that would not produce a click event", async ({ | ||
| alloy, | ||
| }) => { | ||
| window.__alloyMonitors = window.__alloyMonitors || []; | ||
| window.__alloyMonitors.push({ | ||
| onInstanceConfigured(data) { | ||
| window.___getLinkDetails = data.getLinkDetails; | ||
| }, | ||
| }); | ||
|
|
||
| await alloy("configure", { | ||
| ...alloyConfig, | ||
| clickCollectionEnabled: true, | ||
| onBeforeLinkClickSend: (options) => { | ||
| const { clickedElement } = options; | ||
| if (clickedElement.id === "cancel-alloy-link-test") { | ||
| return false; | ||
| } | ||
| return true; | ||
| }, | ||
| }); | ||
|
|
||
| const cancelLink = document.createElement("a"); | ||
| cancelLink.id = "cancel-alloy-link-test"; | ||
| cancelLink.href = "https://example.com/canceled.html"; | ||
| cancelLink.textContent = "Canceled Link"; | ||
| document.body.appendChild(cancelLink); | ||
|
|
||
| // getLinkDetails itself doesn't invoke onBeforeLinkClickSend — it returns |
There was a problem hiding this comment.
The test is titled "getLinkDetails returns falsy for an element that would not produce a click event", but the only assertion is expect(() => window.___getLinkDetails(cancelLink)).not.toThrow(). The comment directly contradicts the name ("The result may or may not be defined"). The variable result on line 649 is also unused. Either the test should assert the return value or the name and comment should be updated to accurately describe what is being verified.
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!
|
|
||
| test("callback returning false cancels the event and sendEvent resolves with {}", async ({ | ||
| alloy, | ||
| worker, | ||
| networkRecorder, | ||
| }) => { | ||
| worker.use(sendEventHandler); | ||
|
|
||
| let callbackInvoked = false; | ||
|
|
||
| const consoleSpy = vi.spyOn(console, "info"); | ||
|
|
||
| await alloy("configure", { | ||
| ...alloyConfig, | ||
| debugEnabled: true, | ||
| onBeforeEventSend: () => { | ||
| callbackInvoked = true; | ||
| return false; | ||
| }, | ||
| }); | ||
|
|
||
| const result = await alloy("sendEvent"); | ||
|
|
||
| expect(callbackInvoked).toBe(true); | ||
| expect(result).toEqual({}); | ||
|
|
||
| const interactCalls = networkRecorder.calls.filter((c) => | ||
| /v1\/interact/.test(c.request?.url ?? ""), | ||
| ); | ||
| expect(interactCalls.length).toBe(0); | ||
|
|
||
| expect(searchForLogMessage(consoleSpy, "Event was canceled")).toBe(true); | ||
|
|
||
| consoleSpy.mockRestore(); | ||
| }); | ||
| }); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // C8119 – Click collection disabled: link click does not send an event |
There was a problem hiding this comment.
document.body are never removed
Every test that creates an <a> element appends it to document.body but does not remove it in a teardown. Multiple tests reuse id="alloy-link-test", so after a few tests have run there are several elements with the same ID in the document — invalid HTML that some browsers de-duplicate in unexpected ways. Because click collection is listening on document, accumulated anchors from prior tests remain live targets. Adding an afterEach (or using document.body.innerHTML = "" in the existing alloy fixture's cleanup) would prevent this drift across the suite.
carterworks
force-pushed
the
migrate-integration/06-config-overrides
branch
from
fe8d1b8 to
15655d5
Compare
carterworks
force-pushed
the
migrate-integration/07-data-collector
branch
from
758f9f6 to
21d9792
Compare
carterworks
force-pushed
the
migrate-integration/06-config-overrides
branch
from
15655d5 to
c1b8db1
Compare
carterworks
force-pushed
the
migrate-integration/07-data-collector
branch
from
21d9792 to
e563a97
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.
The third C81183 test only asserted not.toThrow(), silently dropping the
coverage from original test 2 which verified getLinkDetails returns no
meaningful data for a non-existent (null) element.
Restore it: call getLinkDetails(null) and assert linkName, linkType, and
linkUrl are all undefined, matching the original eql({linkName: undefined,
...}) assertion.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The original C81181 functional file had 8 tests; the initial migration included only 3. The 5 dropped tests do not depend on sendBeacon/collect endpoint interception — they are deferred scope, not blocked. Add test.skip stubs so the gap is documented rather than silently blessed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…order Adds a permanent navigator.sendBeacon recorder (installed before the library loads, since alloy binds sendBeacon once at instance creation) so collect-vs- interact routing can be observed in-page — the same approach the functional suite used. Keeps the suite hermetic: the recorder returns true and no request hits the network. Adds sendEventWithIdentityHandler to establish identity from the interact response so the routing transition can be exercised. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Verified empirically that networkRecorder never captures the Referer header: it is a browser-set forbidden header added after MSW's service worker sees the request. Replaces the speculative skip rationale with the confirmed reason. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Removes comments that duplicated the NO_REQUEST_WAIT_MS constant's rationale or restated their assertion, condenses the verbose ones, and corrects the C8118 skip note (collect routing is now feasible via the sendBeacon recorder, not blocked as the stale comment claimed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The functional original sent a datasetId and asserted meta.configOverrides, but its `.event` key was undefined so the check was a no-op. Restores the meaningful assertion: the datasetId propagates to meta.configOverrides.com_adobe_experience_platform.datasets.event.datasetId. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Match the functional original, which pinned the entire webInteraction object; the migration had dropped name, region, and linkClicks. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The functional original pinned the whole webInteraction object; the migration checked only the augmented name. Adds region/type/linkClicks and a loose URL origin check (the URL resolves against the localhost test page). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Note that the collect-side referer was already a TODO in the functional source and that collect routing is covered by C455258, rather than implying C2592 covers everything. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Moves DOM cleanup and the sendBeacon-recorder reset out of the spec and into the shared alloy fixture, and the ___getLinkDetails teardown into cleanAlloy (next to __alloyMonitors). installSendBeaconRecorder now only installs; the fixture owns the per-test reset. Removes the unusual top-level afterEach from the spec and documents why the recorder install lives in setupBaseCode. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The __alloyMonitors install was copy-pasted in all three C81183 tests. A beforeEach installs it once; cleanAlloy already clears it between tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Groups the three loose top-level test.skip stubs into a "Not migrated" describe so they read as intentional, and replaces the inline setTimeout promises with the existing waitFor util. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The migration only counted one sendBeacon call. Restore the original's intent by asserting the beacon hit /v1/collect and carries a valid events payload. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The loose /v1\/interact/ match would count any URL containing that substring. Anchor it to the edge host and the configId query so a wrong host or malformed path is not mistaken for a valid interact call. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add appendHtmlToBody so anchor structures from the functional originals (spans, download/data-* attributes, custom-region wrappers) can be built as-is, and a preventNavigation option on clickLink so a test can let real hash navigation proceed. Default click behavior is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The migration clicked via the navigation-preventing clickLink, so a regression that blocked default navigation would have passed. Let the click navigate and assert the hash changed, matching the original's getLocation().contains(#foo). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The migration loosened these to truthy/contains and changed the link to external (exit). Restore the original's exact internal-link pinning — linkName, linkRegion, linkType:other, pageIDType, resolved linkUrl and pageName (adapted to the localhost page) — and assert all five fields are undefined for a null element. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The migration only checked the augmented webInteraction name plus customField. Restore the original's full coverage: eventType, the complete webInteraction object, and the activity-map data (whose link stays "Test Link" since the callback augmented only the xdm). Adds reusable link-assertion helpers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
These were stubbed as skips; none depends on sendBeacon, so port them: no-callback collection, conditional cancel via onBeforeLinkClickSend and via filterClickDetails, filterClickDetails augmentation, and sessionStorageEnabled:false. Each pins the full webInteraction (and activity map where the original did), adapted to localhost URLs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the wholesale C8118 skip with the full suite: the interact-then-collect routing case (via the sendBeacon recorder once the interact response establishes identity) plus the 14 interact-only cases — enabled/disabled download, internal and external links, event grouping, cached click on page view, custom region, custom link type, custom activity map, multiple grouped clicks, and custom XDM. URLs are adapted to the localhost page; the grouped-clicks payload shape stays tolerant, matching the original's hedge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reviewer flagged the global sendBeacon override as potentially masking collect/fallback behavior in unrelated specs. Opt-in per spec was attempted and verified to fail: alloy binds navigator.sendBeacon by value when it creates its network service at bundle load, before any per-test beforeEach runs, so a later swap is ignored (the routing assertions saw zero beacons). Document that the global install is therefore required and that always returning true is the deliberate hermetic default mirroring the functional suite. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
For consistency with the other restored assertions, pin the whole webPageDetails object (URL, name, pageViews) instead of just name and pageViews. webPageDetails.URL is window.location.href, the same value already used for the activity-map page. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CI failed 10 link-click assertions with findCall returning undefined. Unlike the sendEvent tests, which await the command (so the call is already complete), clickLink returns before the request is sent, so findCall must outwait the click → event → fetch → MSW round-trip. Its default (5 retries × 10ms ≈ 50ms) is too short under CI load — the latent race only surfaced once this PR added ~20 more click tests. Route all click-driven interact lookups through a findInteractCall helper that polls up to ~2s, returning as soon as the call completes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the retry-tuned findCall with an event-driven networkRecorder.waitForCall: it resolves the instant the matching response is captured (driven by the MSW request/response events the recorder already listens to), so there is no polling interval to tune and no CI-load race. Link clicks are fire-and-forget — clickLink returns before the request is sent — so this is the right tool where there is no command promise to await. Falls back to undefined after a safety timeout so presence is still asserted with toBeDefined(). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
carterworks
force-pushed
the
migrate-integration/07-data-collector
branch
from
2719f9c to
276d06e
Compare
Summary table
Parity matrix
Deletion alignment: The §1c — Assertion fidelityC2592 // Old (testcafe): await t.expect(request.meta.state.domain).ok();
// New (vitest): expect(typeof body.meta.state.domain).toBe("string");Old configOverrides shape ( §2 — Skip justificationC9369211 ( The rationale is plausible: C81182 ( §3 — Mock fidelity
§4 — Timing / flakeNegative assertions ("no request fired") all use Fire-and-forget link-click positives use One pattern in C8118's second click test uses §5 — Isolation
Clean. §6 — Hygiene
RecommendationApprove with the following comments:
No blockers. The migration is solid. This is a PR comment, not a formal approval or request-for-changes review. |
Rebasing onto 00-infra collided 07's sendEventWithIdentityHandler with the identically named handler main added (used by platformServicesCookieWiring). 07's handler was renamed to sendEventWithIdentityCookieHandler during conflict resolution; the C8118 link-click test still referenced the old name.
carterworks
force-pushed
the
migrate-integration/07-data-collector
branch
from
c685d7a to
2689764
Compare
1 participant
Changed Packages
Description
Migrates the Data Collector functional tests to the new Vitest+Playwright+MSW harness. C455258, C8118, and C9369211 are preserved as documented test.skip (sendBeacon not interceptable by MSW in browser mode). C81182 is preserved as test.skip (originally skipped in TestCafe).
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/Data Collector/C2592.jspackages/browser/test/functional/specs/Data Collector/C75372.jspackages/browser/test/functional/specs/Data Collector/C225010.jspackages/browser/test/functional/specs/Data Collector/C1715149.jspackages/browser/test/functional/specs/Data Collector/C8119.jspackages/browser/test/functional/specs/Data Collector/C81181.jspackages/browser/test/functional/specs/Data Collector/C81182.jspackages/browser/test/functional/specs/Data Collector/C81183.jspackages/browser/test/functional/specs/Data Collector/C81184.jspackages/browser/test/functional/specs/Data Collector/C11693274.jspackages/browser/test/functional/specs/Data Collector/C455258.jspackages/browser/test/functional/specs/Data Collector/C8118.jspackages/browser/test/functional/specs/Data Collector/C9369211.jsTypes of changes
Checklist:
Stack