test(e2e): legacy customer account flow e2e tests#3602
Draft
itsjustriley wants to merge 47 commits intomainfrom
Draft
test(e2e): legacy customer account flow e2e tests#3602itsjustriley wants to merge 47 commits intomainfrom
itsjustriley wants to merge 47 commits intomainfrom
Conversation
Why: PR review identified a latent bug (localStorage polyfill skipped in workerd), type safety gaps, magic numbers, dead code, and structural inconsistencies with existing fixture patterns. How: Fix hasWorkingLocalStorage null detection by adding an explicit null check before the try/catch — optional chaining silently no-ops when localStorage is undefined, causing the function to return true and skip the polyfill in exactly the environment (workerd) where it's needed. Augment the global Env interface with HYDROGEN_E2E_MSW_SCENARIO to remove the unsafe double cast in getMswScenario. Extract E2E_TUNNEL_HOSTNAME and SESSION_TTL_IN_MS as named constants with "why" comments. Add JSDoc to ensureNodeProcessForMsw explaining the NODE_ENV/versions.node strategy, and a comment on module-level currentMswScenarioMeta explaining the architectural constraint. Convert AccountUtil to a Playwright fixture (matching CartUtil, DiscountUtil, GiftCardUtil patterns), remove the pass-through expectLoggedInState method, and update account.spec.ts to use the fixture. Add explicit MswScenarioMeta return type and defensive throw after Map.get() in getHandlersForScenario. Remove dead code guard in graphql.ts parseOperation (regex only captures 'query'|'mutation'). Remove unused env option from DevServer.
…ation We've agreed to prioritise test utilities that take a `page` arg over Playwright fixtures that override the test function signature. Revert the AccountUtil fixture registration while keeping all other review feedback changes from the prior commit.
Validation was invoking npm commands inside the skeleton template, which fails on this repo because dependencies use catalog specifiers and pnpm workspace behavior. Detect the repository package manager and run install/build/typecheck/codegen with the matching command, and remove an orphaned markets patch file that caused recipe validation to fail before execution.
Validates 8 Hydrogen cookbook recipes against current skeleton template to identify which recipes can have E2E tests created. Results: ✅ Working (1): gtm ❌ Broken (7): bundles, combined-listings, custom-cart-method, metaobjects, partytown, subscriptions, third-party-api All broken recipes have patch conflicts requiring maintainer attention to regenerate patches against current skeleton structure. Most conflicts stem from import formatting changes and skeleton reorganization. E2E tests will only be created for recipes that validate cleanly.
…pe tests WHY: Recipe tests were experiencing race conditions because multiple tests would mutate templates/skeleton in-place simultaneously, causing patch conflicts and test failures. WHAT: - Add --template flag to cookbook apply command to support custom template paths - Update recipe fixture to copy skeleton first, then apply recipe to the copy - Remove fragile git cleanup logic from fixture generation HOW: - The --template flag allows applying recipes to any directory, not just templates/skeleton - Recipe fixture now creates isolated copies for each recipe test - This prevents race conditions and ensures true test isolation - No cleanup needed since each fixture has its own isolated directory
Addresses binks-code-reviewer feedback on PR #3492: Issue 1: --template path resolution always relative to REPO_ROOT - Changed to respect absolute paths and CWD-relative paths - Now checks path.isAbsolute() first, then resolves relative to process.cwd() - Previously all paths were forced relative to repo root, breaking absolute paths and making relative paths confusing Issue 2: cp -r command fails on Windows - Replaced execSync('cp -r') with fs.cpSync() in tests - Cross-platform solution (works on Windows, Mac, Linux) - No shell subprocess overhead - Available since Node.js v16.7.0 Both tests passing locally (2/2).
Implements all blocking and non-blocking fixes identified by the 4-agent consensus review (Sheldon, John, Gray, Dave). Blocking fixes: - B1: switchToFirstAvailableLocale now requires currentLocalePrefix param, iterates buttons to find a DIFFERENT locale, and throws if none found — prevents silent no-op when first button targets the current locale - B2: Add COOKBOOK_APPLY_TIMEOUT_IN_MS (2min) and PNPM_INSTALL_TIMEOUT_IN_MS (3min) to prevent indefinite hangs in CI - B3: Add @shopify/create-hydrogen to changeset, reword for user-facing context Non-blocking fixes: - NB0: Build workspace package map once in resolveWorkspaceProtocols instead of per-dependency, eliminating O(N×P) filesystem reads - NB1: Add invalid locale 404 test asserting /ZZ-ZZ/ returns 404 - NB2: Throw descriptive error for unsupported workspace: specifiers (workspace:^ and workspace:~) instead of silent pass-through - NB3: Extract configureDevServer() shared lifecycle helper used by both setTestStore and setRecipeFixture, eliminating server lifecycle duplication Additional: - Migrate pnpm install from exec to execFile (no shell needed) - Extract magic number into LOCALE_SWITCH_NAVIGATION_TIMEOUT_IN_MS
…incomplete fixtures Recipe fixture generation now builds into a staging directory (.tmp suffix) and atomically renames to the final path only after install completes. Without this, parallel Playwright workers could see pathExists() === true on a fixture directory that was still mid-install, then launch a dev server against an incomplete node_modules tree.
…ection
Screen readers were announcing the "↑" and "↓" arrow characters in the
"Load previous" / "Load more" links as "up-pointing arrow" / "down-pointing
arrow", adding noise to the navigation experience.
Wraps each arrow in <span aria-hidden="true"> so they remain visible
but are excluded from the accessible name. Also fixes JSDoc grammar
("encapsulate" → "encapsulates").
Adds 8 Playwright tests validating the infinite-scroll recipe's collection page behavior against the preview storefront fixture store. Coverage: - Initial product loading and "Load more" button visibility - Manual load-more interaction (product count increase + URL param update) - Direct URL navigation — simulates a shared link to "page 2" and verifies the "Load previous" link appears (no accumulated location.state) - Automatic loading via Intersection Observer when scrolling load-more into view - History replace mode (no history length growth after scroll-triggered load) - Scroll position preservation across pagination loads Key design decisions: - Viewport constrained to 400px height (SCROLL_TEST_VIEWPORT) for scroll tests so the load-more button reliably starts below the fold on any CI runner - InfiniteScrollUtil fixture hides URL polling, locator details, and param knowledge behind named methods (waitForUrlToContainPaginationParam, etc.) - Custom poll error messages for CI debuggability - Shared-link test captures a real paginated URL via load-more cycle rather than hardcoding a cursor value
…updates Regenerates all recipe patch files to align with the latest skeleton template state after the rebase of setup-clean and infinite-scroll-clean branches. The patch file hash suffixes update to reflect the new file paths. Recipes updated: b2b, bundles, combined-listings, custom-cart-method, express, infinite-scroll, legacy-customer-account-flow, metaobjects, multipass, partytown, subscriptions, third-party-api
# Conflicts: # e2e/fixtures/index.ts
Add e2e tests for the legacy-customer-account-flow cookbook recipe covering unauthenticated flows: login/register/recover page rendering, cross-page navigation, unauthenticated redirects, and header account link. 16 tests across 5 describe blocks. Uses role-based selectors per e2e testing guidelines. No MSW mocking needed — tests verify form rendering and client-side behavior. Co-authored-by: Claude <noreply@anthropic.com>
Extend the MSW testing infrastructure to support the legacy Storefront
API auth flow alongside the existing Customer Account API mocking.
The legacy recipe stores `customerAccessToken` (with `{accessToken,
expiresAt}` shape) in the session, unlike the Customer Account API
which uses `customerAccount` (with additional `refreshToken`). The
`expiresAt` field is an ISO date string rather than a Unix timestamp.
Changes:
- Add `mocksLegacyCustomerAuth` to MswScenarioMeta interface so session
injection can branch on auth type
- Add `legacy-customer-account-logged-in` MSW scenario with Storefront
API `Customer` and `CustomerOrders` query handlers
- Branch session injection in entry.ts: legacy auth writes
`customerAccessToken`, CAAPI auth writes `customerAccount`
- Extend LegacyCustomerAccountUtil with authenticated page helpers
- Add 12 authenticated flow tests covering orders, profile, addresses,
navigation, and logout
Co-authored-by: Claude <noreply@anthropic.com>
Marketing checkbox aria-label is "Accept marketing" not "Subscribed to marketing". Password confirm field aria-label is "New password confirm" without parentheses. Co-authored-by: Claude <noreply@anthropic.com>
The aria-label on the password confirm input is "New password confirm" (no parentheses), not "New password (confirm)". Co-authored-by: Claude <noreply@anthropic.com>
…ests Code quality improvements identified by 4-agent consensus review: 1. Export LEGACY_CUSTOMER_MOCK from handlers.ts as single source of truth — eliminates mock data duplication between handlers and spec file that could silently drift 2. Scope getAccountMenuLink to navigation region — the previous implementation was identical to getLink (a pass-through), now it meaningfully narrows to the nav element 3. Replace .orders CSS class selector with semantic locator — was the sole CSS selector in the utility, violating project guidelines 4. Replace #email CSS ID selector with getByLabel — consistent with all other form field locators in the utility 5. Add mocksLegacyCustomerAuth: false to subscriptions scenario — fixes interface compliance after MswScenarioMeta was extended 6. Add 4 new tests for authenticated redirects on public routes: login/register/recover redirect to /account when session token exists, and catch-all redirects unknown sub-routes Co-authored-by: Claude <noreply@anthropic.com>
…orts) Changes split from prior commit due to worktree branch-switching: - Export LEGACY_CUSTOMER_MOCK from handlers.ts (single source of truth) - Update index.ts to re-export LEGACY_CUSTOMER_MOCK - Add mocksLegacyCustomerAuth: false to subscriptions scenario - Scope getAccountMenuLink to navigation region - Replace .orders CSS class selector with semantic locator - Replace #email CSS ID selector with getByLabel Co-authored-by: Claude <noreply@anthropic.com>
Contributor
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
What: Fix blocking test weakness, remove dead code, improve consistency. Why: The catch-all redirect test used a regex that matched the starting URL, meaning it would pass even if the redirect was broken. The utility class had an unused method, and the addresses test bypassed the Page Object pattern. Recipe yaml step numbers were strings instead of numbers. How: - Strengthen catch-all redirect assertion to check /account/orders (the actual final destination) and update test name to match - Remove dead assertOrdersPageRendered method from utils - Add getCityInput locator and use Page Object in addresses form test - Convert recipe.yaml step numbers from strings to numeric values
What: Eliminate repeated instantiation of the stateless utility class.
Why: Both test files created a new LegacyCustomerAccountUtil(page) in
every single test body despite beforeEach hooks already creating one for
navigation. Since the class is stateless (just a wrapper around page),
re-instantiating per test adds cognitive noise without isolation benefit.
How: Declare a let-scoped variable in each describe block, assign it
in beforeEach, and reuse it across tests. Tests that only use recipe
methods (no direct page assertions) drop the unused {page} destructure.
Tests that still need page for URL assertions keep it. Logout and Header
Navigation blocks are left as-is since they have 1-2 tests each.
itsjustriley
changed the title
itsjustriley
force-pushed
the
auth/legacy-customer-account-flow-all-tests
branch
from
0676363 to
f788bcc
Compare
Collaborator
|
@itsjustriley looks like the |
YAML parses bare `step: 1` as a JavaScript number, but the Zod schema (StepSchema.step) expects z.string().regex(/^\d+(?:\.\d+)?$/). This type mismatch caused all 6 legacy-customer-account-flow e2e tests to fail at fixture generation time with: RecipeSchema: Invalid input: expected string, received number Quoting the values (`step: "1"`) forces YAML to preserve them as strings, matching what the schema requires.
The cookbook CLAUDE.md documented step values as bare integers (`step: 1`) and listed quoted strings as an error. This was backwards — the Zod schema (StepSchema.step) is z.string().regex(/^\d+(?:\.\d+)?$/), which requires quoted strings. Bare YAML integers get parsed as JavaScript numbers and fail validation. Corrected all five references: requirements list, common errors section, code style section, YAML structure example, and the validate error format.
…rver setRecipeFixture accepted a `mock` option in its call sites but silently discarded it — RecipeFixtureOptions had no `mock` property and the destructuring never extracted it. Since the e2e directory lacks a tsconfig, TypeScript couldn't catch the extra property. Without `mock` reaching configureDevServer: - createMockEnvFile was never called (no scenario env var) - The MSW entry point was never set (no fetch interception) - No session cookie injection occurred - account.tsx's isLoggedIn check always returned false - All authenticated tests redirected to /account/login The fix adds `mock` to RecipeFixtureOptions and forwards it through to configureDevServer, which already handles it correctly.
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
3 participants
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.
Part of https://github.com/Shopify/developer-tools-team/issues/1057
Depends on: MSW infrastructure and recipe test framework PRS (not yet on
main). This PR targetsmainbut should not be merged until the base infrastructure ships.WHY are these changes introduced?
The legacy customer account flow recipe had zero e2e test coverage. This recipe replaces the Customer Account API with Storefront API-based authentication — a fundamentally different auth mechanism that stores
customerAccessTokenin the session instead ofcustomerAccount. Without tests, regressions from skeleton template updates or recipe regeneration would go undetected.WHAT is this pull request doing?
Adds 33 e2e tests (17 unauthenticated + 16 authenticated) covering the legacy customer account flow recipe:
Unauthenticated tests (no MSW):
Authenticated tests (MSW-mocked):
/account→/account/orderswhen logged in)/account)MSW infrastructure extensions:
legacyCustomerAccountLoggedInscenario withCustomerandCustomerOrdersStorefront API handlersmocksLegacyCustomerAuthboolean onMswScenarioMetato branch session injectioncustomerAccessTokenwith ISOexpiresAt(vs CAAPI'scustomerAccountwith Unix timestamp)LEGACY_CUSTOMER_MOCKexported as single source of truth for test assertionsHOW to test your changes?
Checklist