Add PerfKit integration contract tests#3774
Conversation
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
Lock down the deterministic Hydrogen <-> PerfKit contract with dedicated tests, without changing runtime behavior: - Export the pinned PERF_KIT_URL constant (shopify-perf-kit-spa.min.js) so the exact SPA script URL is asserted in tests; bumping it is now a deliberate, reviewed change. - Memoize the data-* attributes object (stable identity; lets tests assert the exact attributes). No behavior change. - PerfKit.test.tsx covers: SPA script URL; exact data-* attributes; shop GID parsing; storefront id from hydrogenSubchannelId; Internal_Shopify_Perf_Kit registration; no wiring while loading/error; wiring only after done; all five view subscriptions; ready() once after wiring; no double-wire across a loading->done transition; page_viewed -> navigate(); product/collection/ search/cart -> setPageType(...); no throw when window.PerfKit is absent. Non-user-facing, nothing exported from the package entrypoint, no semver change (empty changeset). Requested by Dan Gayle <dan.gayle@shopify.com>
d6e6b62 to
da534b6
Compare
Per PR review: the PR touches PerfKit.tsx (exports PERF_KIT_URL, memoizes attributes), so it needs a patch changeset rather than an empty one.
|
Remove which part? There is a small public change to perfkit itself that
isn't a test.
…On Mon, Jun 8, 2026 at 9:32 AM freddie ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In .changeset/perfkit-contract-tests.md
<#3774 (comment)>:
> @@ -0,0 +1,5 @@
+---
***@***.***/hydrogen": patch
+---
+
+Add PerfKit integration contract tests and export the internal `PERF_KIT_URL` constant; the PerfKit script `data-*` attributes are now memoized. This locks down the deterministic PerfKit contract (pinned SPA script URL, required `data-*` attributes, subscription wiring/readiness, and `navigate()` / `setPageType(...)` on the view events) so it can't regress. No change to runtime behavior.
tests are not user facing changes, pls remove
—
Reply to this email directly, view it on GitHub
<#3774?email_source=notifications&email_token=AAA7US6BCDYWTEIIFNPH6PL463TAJA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINBVGE2DINRQGQY2M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-4451446041>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA7US5ANCPVAIDOJFA7W23463TAJAVCNFSM6AAAAACZWKB4UKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DINJRGQ2DMMBUGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Co-authored-by: freddie <freddieshaihulud@gmail.com>
| expect(subscribedEvents).toHaveLength(5); | ||
| }); | ||
|
|
||
| it('calls ready() once, after subscriptions are wired', () => { |
There was a problem hiding this comment.
blocking: this test says ready() happens after subscriptions are wired, but it only checks that ready() was called once.
If the implementation moved ready() before the subscribe(...) calls, this test would still pass. Since this is new contract coverage, let's assert the ordering too, using Vitest's mock invocation order.
|
|
||
| // A subsequent re-render must not re-wire — the loadedEvent.current guard | ||
| // is what prevents it once deps stop changing. | ||
| rerender(<PerfKit shop={SHOP} />); |
There was a problem hiding this comment.
blocking: this doesn't actually exercise the loadedEvent.current guard.
On the final rerender, the effect deps are unchanged (subscribe, ready, and scriptStatus are the same values), so React doesn't rerun the effect at all. If someone removed the guard, this test would still pass.
Let's force a dependency change after the first successful wiring - e.g. done -> loading -> done - and assert the subscription count stays at 5 on the second done.
| expect(setPageType).toHaveBeenCalledWith(pageType); | ||
| }); | ||
|
|
||
| it('does not throw when window.PerfKit is absent (script-load race)', () => { |
There was a problem hiding this comment.
non-blocking: this only exercises the page_viewed callback while window.PerfKit is absent.
The implementation uses optional chaining for all five callbacks, so it would be nice to iterate through every subscribed callback here and assert none of them throw. Otherwise a future unsafe setPageType(...) callback could slip through while this test still passes.
|
|
||
| // A subsequent re-render must not re-wire — the loadedEvent.current guard | ||
| // is what prevents it once deps stop changing. | ||
| rerender(<PerfKit shop={SHOP} />); |
There was a problem hiding this comment.
blocking: this does not actually exercise the loadedEvent.current guard.
On the final rerender, the effect deps are unchanged (subscribe, ready, and scriptStatus are the same values), so React does not rerun the effect at all. If someone removed the guard, this test would still pass.
We should force a dependency change after the first successful wiring - e.g. done -> loading -> done - and assert the subscription count stays at 5 on the second done.
What this PR does
PerfKit.tsxhad zero tests, so if someone accidentally changed a label, broke the load order, or stopped pinging PerfKit on navigation, nothing would catch it — it would just silently ship broken.This PR adds a safety net. Three small things:
PerfKit.test.tsxchecks the adapter still does exactly what it should — loads the right script URL, sets the rightdata-*labels, waits until the script is loaded before wiring anything up, and calls PerfKit on the five view events. If anyone breaks the contract later, the test fails.PERF_KIT_URL) so the test can point at it, and so changing that URL becomes a deliberate, reviewed edit instead of a quiet one.data-*labels into a single object so the test can read them. No behavior change.That's the whole PR. No new features, nothing users see, no version bump. It's purely a guard so Hydrogen can't accidentally break its own PerfKit wiring.
What the tests lock down
PerfKit.test.tsx(PerfKit is mocked at the network level — the real script is never downloaded):data-*labels exactly:data-application=hydrogen,data-spa-mode=true, parseddata-shop-id,data-storefront-id(fromhydrogenSubchannelId),data-monorail-region=global,data-resource-timing-sampling-rate=100.Internal_Shopify_Perf_Kit.loading, and not onerror— only after it'sdone.page_viewed,product_viewed,collection_viewed,search_viewed,cart_viewed).ready()exactly once, after wiring.loading → donetransition (the re-render guard).page_viewed → window.PerfKit.navigate(); product/collection/search/cart →setPageType(...).window.PerfKitisn't there yet (script-load race).About versions
The PerfKit script URL is pinned in
PERF_KIT_URLand asserted exactly in the test:https://cdn.shopify.com/shopifycloud/perf-kit/shopify-perf-kit-spa.min.jsHeads-up for reviewers: this URL is not stable across Hydrogen versions (older builds used a versioned URL like
shopify-perf-kit-1.0.1.min.js; the current build uses the unversioned-spabundle), andwindow.PerfKitexposes no runtime version. This PR just pins the current-spabuild. Giving PerfKit an explicit version/capability signal is a separate, cross-team follow-up.Scope / impact
PERF_KIT_URLis module-local — nothing new is exported from the package entrypoint. No runtime behavior change. No semver bump (empty changeset).PerfKit.tsx,PerfKit.test.tsx(new), and an empty changeset.Checks run
pnpm --dir packages/hydrogen typecheck✅pnpm --dir packages/hydrogen test✅ — 58 files, 548 testspnpm --dir packages/hydrogen exec vitest run src/analytics-manager/PerfKit.test.tsx✅ — 16 testseslint+prettieron changed files ✅Follow-ups (separate, not in this PR)
Requested by Dan Gayle dan.gayle@shopify.com