test(cookbook): add unit tests for 6 recipes and extract shared test utility#3632
Draft
itsjustriley wants to merge 5 commits intomainfrom
Draft
test(cookbook): add unit tests for 6 recipes and extract shared test utility#3632itsjustriley wants to merge 5 commits intomainfrom
itsjustriley wants to merge 5 commits intomainfrom
Conversation
… recipes #1130: Express recipe tests — validate server.mjs structure (Express setup, express.static middleware, EADDRINUSE handling), entry.server streaming change, vite config oxygen removal, and AppSession class. #1131: GTM recipe tests — validate recipe structure, CSP domain configuration in entry.server patch, GoogleTagManager component dataLayer integration and useAnalytics usage. #1132: Partytown recipe tests — test maybeProxyRequest pure function (script proxying, non-script bypass, already-proxied bypass) and partytownAtomicHeaders (COOP/COEP headers). #1133: Third-party-api recipe tests — test minifyQuery pure function (comment removal, whitespace collapsing, structure preservation) and display name extraction regex pattern. Partytown and third-party-api have real pure functions to unit test. GTM and express are primarily template/config, so tests validate structural integrity and key configuration properties. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Export minifyQuery from createRickAndMortyClient.server.ts and import in test (eliminates duplicated implementation) - Mock @Shopify/hydrogen to resolve import in test context - Replace ! non-null assertions with proper narrowing guards - Use beforeAll + shared variables to avoid repeated file reads - Import readdir directly instead of dynamic reimport - Use method signature patterns for more precise string matching Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
Three changes to improve test quality: 1. Express AppSession test: scope method assertions to extracted class body rather than the entire server.mjs file. Previously, get( could match app.get(/favicon.ico...) unrelated to AppSession, meaning the test could pass even if AppSession lost its get() method. 2. GTM test: extract findPatchFile helper (matching express test pattern) instead of inlining the patch-file lookup. Moves readdir to beforeAll for consistency and reusability. 3. Partytown test: rename misleading test from returns the original URL for non-proxy domains to proxies all domains when nonProxyDomains is empty - the test asserts the URL IS proxied, not that it is returned unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace `classMatch![0]` non-null assertion with proper narrowing guard
(`if (!classMatch) throw`) in express.test.ts, removing the last remaining
`!` assertion that the earlier commit claimed to have eliminated.
- Add comments to `minifyQuery` negative assertions (`.not.toContain('#')`,
`.not.toMatch(/\s{2,}/)`) explaining they are supplementary to the exact
`.toBe()` match assertions that serve as the primary regression guards.
- Expand `extractDisplayName` regex comment to explain the intentional
duplication from `createRickAndMortyClient.server.ts` and why exporting
the regex is not worth the added public API surface.
- Improve `findPatchFile` error messages in express and GTM tests to include
the actual files found in the patches directory, enabling faster debugging
when a patch file is renamed.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Three review fixes from consensus review: - Export OPERATION_NAME_PATTERN constant from createRickAndMortyClient.server.ts to eliminate regex duplication between source and test - Extract shared loadRecipePatch utility into __test-utils__/index.ts to replace duplicated findPatchFile helpers in express and GTM tests - Fix misleading test name "removes oxygen plugin" to accurately describe the structural assertion New coverage additions: - createRickAndMortyClient query method: 3 tests covering success, data.error, and HTTP error response branches - combined-listings recipe: 9 tests covering isCombinedListing type guard with all input types, settings snapshot, and filter query generation - markets recipe i18n utilities: 25 tests covering all 6 pure functions (getLocaleFromRequest, getPathWithoutLocale, localeMatchesPrefix, normalizePrefix, findLocaleByPrefix, cleanPath) plus exported constants 68 recipe tests across 7 files, 155 total cookbook tests, all passing. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
itsjustriley
changed the title
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
1 participant
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.
WHY are these changes introduced?
Fixes https://github.com/Shopify/developer-tools-team/issues/1130
Fixes https://github.com/Shopify/developer-tools-team/issues/1131
Fixes https://github.com/Shopify/developer-tools-team/issues/1132
Fixes https://github.com/Shopify/developer-tools-team/issues/1133
Six cookbook recipes had no unit tests, meaning structural regressions (broken exports, missing config, invalid CSP domains) and behavioral regressions (broken query minification, incorrect locale parsing, wrong type guards) would only be caught by slower E2E tests or in production.
WHAT is this pull request doing?
Recipe tests (6 recipes, 68 tests across 7 files):
setupcommand #1130 — Express: validate server.mjs structure, Express setup, renderToPipeableStream, vite config, and AppSession classmaybeProxyRequestpure function (script proxying, non-script bypass, already-proxied bypass) andpartytownAtomicHeaders(COOP/COEP headers)minifyQuery, display name extraction viaOPERATION_NAME_PATTERN, andcreateRickAndMortyClientquery method error handling (success, data.error, HTTP error)isCombinedListingtype guard with all input types (valid product, missing tag, null, undefined, non-object), settings snapshot, and filter query generationgetLocaleFromRequest,getPathWithoutLocale,localeMatchesPrefix,normalizePrefix,findLocaleByPrefix,cleanPath— plus exported constantsShared infrastructure:
loadRecipePatchutility intocookbook/recipes/__test-utils__/index.tsto replace duplicatedfindPatchFilehelpersOPERATION_NAME_PATTERNconstant fromcreateRickAndMortyClient.server.tsso the test imports the real regex instead of duplicating itRecipes not tested (and why):
parseSectiondepends on@shopify/hydrogen'sparseMetafieldmultipassify.server.tsneedscrypto-js;multipass.tsneeds fetch + browser envHOW to test your changes?
Full cookbook suite:
npx vitest run --pool forks— 155 passed, 13 filesChecklist