Draft
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.
Contributor
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
Add E2E tests for delivery address management in the skeleton template, using MSW to mock the Customer Account API with mutable closure state. The MSW scenario seeds 2 addresses and tracks mutations so subsequent queries reflect CRUD changes. The DeliveryAddressUtil fixture follows the deep module pattern — entity locators are public, button mechanics and form-filling are hidden behind action methods. 8 serial tests cover all meaningful user flows: - Read: existing addresses render, create form visible, default checkbox - Create: fill and submit new address, verify count and data - Update: modify city field, verify change - Default Address: toggle default to a different address - Delete: remove one address, delete all to empty state Design decisions: - Serial mode because MSW mutable state accumulates across tests - FIELD_LABEL_MAP as single source of truth for field-to-label mapping - DELIVERY_ADDRESS_SEED_COUNT exported from handlers so the spec derives the count from the source of truth - FORM_SUBMISSION_TIMEOUT_IN_MS constant for completion signal timeouts - Forms identified by button presence (Create vs Save) not hardcoded IDs - assertAddressVisible accepts Partial<AddressFormData> for flexibility - All selectors use getByRole/getByLabel per project conventions
The address form's territory code input had aria-label="territoryCode" (a raw developer string) instead of a meaningful label for assistive technology. A screen reader would announce "territoryCode" which is meaningless to users. Changed to "Country code" to match the visible label's intent.
itsjustriley
force-pushed
the
test/e2e-delivery-addresses
branch
from
4ca6de0 to
fa84d86
Compare
The update and default-address-toggle tests were asserting against DOM
state that persists regardless of mutation success. The form uses
uncontrolled inputs (defaultValue/defaultChecked), so user-typed values
survive React re-renders even when the MSW mutation fails silently.
Both tests now re-navigate after the mutation, forcing fresh component
mounts that can only render data from MSW closure state. This ensures
the assertions verify server-side persistence, not stale DOM values.
The completion signal (button text check) was also a race condition -
it could pass immediately before React re-rendered to the loading state.
Replaced with waitForLoadState('networkidle') which reliably waits for
the full action + revalidation cycle to complete.
Also uses MSW_SCENARIOS constant in smoke test for consistency.
The delete method's waitForLoadState('networkidle') fires prematurely
in CI because React Router's action-then-revalidation has a brief
network gap between steps. The original not.toBeVisible() signal is
stronger: it waits for the form to disappear from the DOM, which
proves the full delete -> revalidate -> re-render cycle completed.
…erts The delete completion signal (not.toBeVisible on the delete button) fails in multi-delete loops because Playwright locators are lazy - after the first form is deleted, .first() shifts to the next form whose delete button IS visible. Replaced with a count-based signal: capture count before, click delete, wait for count to decrease by 1. Also strengthened assertions to match gift card/discount util patterns: - deleteAddress: assert button toBeVisible before clicking (before assert) - empty state test: assert empty state toHaveCount(0) before deleting (not.toBeVisible can pass on hidden elements; toHaveCount(0) asserts non-existence in the DOM)
networkidle is an antipattern for Playwright tests - it's timing- dependent and fires prematurely in React Router's multi-step request chain (action response -> gap -> revalidation request). Each mutation now uses an appropriate signal: - create: count-based (new form appears in existing addresses list) - update: waitForResponse (intercepts the action response, proving the server processed the mutation before tests re-navigate) - delete: count-based (already using assertAddressCount)
The e2e guidelines say to wait for visible effects, not network requests. But the skeleton's AddressForm has no visible success feedback and uses uncontrolled inputs (defaultValue), so there is no user-visible effect to wait for after a successful update. Added a DEVIATION comment explaining why waitForResponse is used as a pragmatic compromise, and noting that callers must re-navigate afterward to verify persistence via fresh mount from MSW state.
Follows the established POM pattern from discounts.spec.ts and
giftCards.spec.ts: register the utility in base.extend, destructure
from test args ({addresses}), and use beforeEach for shared navigation
setup. This replaces the manual `new DeliveryAddressUtil(page)` +
`navigateToAddresses()` repetition in every test.
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
2 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.
WHY are these changes introduced?
Adds E2E test coverage for delivery address CRUD in the skeleton template. This is a gap in our test suite — the
account.addresses.tsxroute had zero E2E coverage. Uses MSW to mock the Customer Account API, enabling fast, deterministic tests without a real storefront.Depends on MSW infrastructure from PR #3537.
Related: https://github.com/Shopify/developer-tools-team/issues/1038
WHAT is this pull request doing?
New files:
e2e/fixtures/delivery-address-utils.ts—DeliveryAddressUtilfixture following the deep module pattern. Hides form-filling, button mechanics, and completion signals behind a simple action interface.e2e/specs/skeleton/deliveryAddresses.spec.ts— 8 serial tests covering all CRUD flows + default address toggling.Modified files:
e2e/fixtures/msw/handlers.ts— Newdelivery-addressesMSW scenario with mutable closure state. MocksCustomerDetailsQuery,CustomerOrdersQuery, and all three address mutations. Seed data exported asDELIVERY_ADDRESS_SEED_COUNTfor spec consumption.e2e/fixtures/msw/scenarios.ts— AddeddeliveryAddressesscenario constant.e2e/fixtures/index.ts— Removed orphanedAccountUtilexport.templates/skeleton/app/routes/account.addresses.tsx— Fixedaria-label="territoryCode"→aria-label="Country code"(was a raw developer string, meaningless to screen readers).Test coverage:
HOW to test your changes?
npx playwright test --project=skeleton --workers=1 e2e/specs/skeleton/deliveryAddresses.spec.tsChecklist