test: improve E2E test quality across recipe specs#3629
Draft
itsjustriley wants to merge 11 commits intomainfrom
Draft
test: improve E2E test quality across recipe specs#3629itsjustriley wants to merge 11 commits intomainfrom
itsjustriley wants to merge 11 commits intomainfrom
Conversation
#1155: Extract shared KNOWN_PRODUCT constant to e2e/fixtures/known-products.ts, eliminating duplication across 5 spec files. Each file imports with a context-appropriate alias (KNOWN_REGULAR_PRODUCT, KNOWN_PRODUCT_WITH_VARIANTS). #1153: Replace bare getAttribute('href') calls with expect.poll() in custom-cart-method.spec.ts for auto-retry after async cart updates. #1151: Add case-insensitive flag to negative locale assertion regex in markets-utils.ts. Add comment explaining the |\/$ branch in waitForURL (handles default locale where pathPrefix is '/'). #1150: Consolidate redundant page.goto('/') calls into beforeEach in B2B Location Selector tests. Add comment explaining why CustomerLocations uses raw graphql.query() (recipe-specific query, no generated types). #1116: Improve metaobjects spec: replace CSS child selector with getByRole('region'), extract getStoreLinks() helper for 4 repeated locator patterns, replace page.waitForURL with expect(page).toHaveURL, strengthen keyboard tests with Tab navigation instead of .focus(). #1152: Replace .or() assertion in subscriptions-auth cancel test with definitive expect(cancelButton).not.toBeVisible(). Add mock data drift risk documentation to subscriptions handler comment. #1154: Add comment clarifying tag-based parent-product filter mechanism in combined-listings spec. #1118: Add test.describe.configure({retries: 1}) for third-party-api tests (live external API dependency). Add explicit 15s timeout to expect.poll() calls via EXTERNAL_API_TIMEOUT_IN_MS constant. #1135: Add skip/success summary log at end of E2E upgrade test to surface changelog data quality drift. #1136: Verify cumulative intermediate dependencies after upgrade, not just toRelease dependencies. Exercises getCumulativeRelease logic for multi-version jumps. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Extract maxTabPresses to MAX_TAB_PRESSES_TO_REACH_CONTENT constant - Remove unnecessary scrollIntoViewIfNeeded before Tab navigation - Remove dead getCancellingButton method from SubscriptionsUtil - Add cumulative devDependencies verification in upgrade E2E test 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. |
The cancel test was asserting `not.toBeVisible()` after clicking the cancel button, but never verified the button was visible before clicking. This means the test could vacuously pass if the mock data drifted and the cancel button was never rendered. Adding `await expect(cancelButton).toBeVisible()` before the click ensures the test fails if the button is absent, following the E2E testing guideline of asserting presence before asserting absence. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Widen validateDependencyVersion depType param from union to string, fixing the blocking type mismatch when called with template literals for cumulative dependency context - Add comment documenting known limitation where cumulative dep loop may produce false failures when multiple intermediate releases update the same dependency - Add comments explaining why the if(actualVersion) guard intentionally skips deps not present in the upgraded project - Add diagnostic Tab-loop-exhaustion assertions to metaobjects keyboard navigation tests for clearer failure messages - Rename KNOWN_PRODUCT to KNOWN_SKELETON_PRODUCT to eliminate aliasing ceremony at every import site Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The test loop iterated through intermediate releases (newest-first from changelog order) and validated each release's expected dep version independently. When multiple intermediate releases updated the same dep (e.g., react-router), the loop would assert against the earlier version even though the production code correctly installs the latest. Fix: build last-write-wins maps (iterating oldest-first) to determine the final expected version for each dep, then validate once. This mirrors the production getCumulativeRelease logic. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ents #1125: PaginatedResourceSection renders region wrapper when ariaLabel is provided even without resourcesClassName. Previously the ariaLabel prop was silently ignored unless className was also passed. #1139: Add aria-live="polite" region to PaginatedResourceSection for loading state and product count announcements (WCAG 4.1.3). Also add aria-live announcement to infinite-scroll recipe ProductsGrid for IO auto-loaded content. #1140: Infinite scroll recipe: separate IO ref target from focusable NextLink into a non-focusable <div aria-hidden="true"> sentinel. Prevents keyboard tabbing from triggering IO and creating an infinite loading cycle (WCAG 2.1.1, 2.2.2). #1115: Metaobjects recipe: change <h1> to <h2> in SectionHero, SectionStores, and SectionStoreProfile to fix heading hierarchy (WCAG 1.3.1). Replace <div role="region"> with <section> in Sections.tsx per first rule of ARIA. #1111: Markets country selector: add Escape key handler to close dropdown; add tabIndex={-1} to hidden form inputs to prevent them from appearing in tab sequence. #1114: Note — subscriptions ProductPrice and infinite-scroll ProductsGrid hardcoded ARIA attributes are documented as pre-existing patterns that require recipe regeneration to align with ariaLabel prop. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Round 1 review fixes from Gray, John, Tina:
- Remove redundant tabIndex={-1} on hidden inputs (all agents)
- Remove role="group" on <details> to preserve native disclosure
semantics (Tina: WCAG 4.1.2)
- Fix SectionStores nested h2→h3 for store name headings
(Tina: WCAG 1.3.1 heading hierarchy)
- Fix SectionStoreProfile h5→h3 for Address/Opening Hours
sub-headings (Tina: WCAG 1.3.1 heading level skip)
- Extract PaginationStatus component to defer aria-live
announcements until after initial render (Tina: WCAG 4.1.3)
- Use explicit '1px' in sentinel style (John: clarity)
- Add changeset for skeleton template changes (Gray: blocking)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Wrap decorative unicode arrows (↑/↓) in infinite-scroll patch with
aria-hidden="true" to prevent screen readers from announcing "upwards
arrow" / "downwards arrow". This matches the existing pattern in the
skeleton template's PaginatedResourceSection.
Use the idiomatic HTMLDetailsElement.open property instead of
removeAttribute('open') in CountrySelector's Escape key handler,
since the <details> element exposes a dedicated boolean property
for controlling its open/closed state.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Apply the deferred aria-live pattern (useState('') + useEffect) to the
infinite-scroll ProductsGrid, matching the PaginationStatus approach in
PaginatedResourceSection. This prevents potential unprompted screen reader
announcements on initial mount.
Replace `state: any` with the actual Pagination state type shape to
restore TypeScript safety in the recipe patch.
Update changeset description to reflect the full PR scope: heading
hierarchy fixes, CountrySelector Escape key, IO sentinel separation,
and aria-live announcements — not just PaginatedResourceSection changes.
Add clarifying comment in CountrySelector explaining that `<input
type="hidden">` elements are not focusable per HTML spec, addressing
the concern raised in issue #1111 about hidden input tabIndex.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The helper was only used once, on the root URL after page.goto('/').
A direct pathname assertion is more readable and precise — it verifies
the exact expected state (pathname is '/') rather than a negation
(pathname doesn't match a locale pattern).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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/1155
Fixes https://github.com/Shopify/developer-tools-team/issues/1153
Fixes https://github.com/Shopify/developer-tools-team/issues/1151
Fixes https://github.com/Shopify/developer-tools-team/issues/1150
Fixes https://github.com/Shopify/developer-tools-team/issues/1116
Fixes https://github.com/Shopify/developer-tools-team/issues/1152
Fixes https://github.com/Shopify/developer-tools-team/issues/1154
Fixes https://github.com/Shopify/developer-tools-team/issues/1118
Fixes https://github.com/Shopify/developer-tools-team/issues/1135
Fixes https://github.com/Shopify/developer-tools-team/issues/1136
E2E recipe specs had duplicated constants, race conditions, missing documentation, and insufficient assertions that could mask real failures.
WHAT is this pull request doing?
KNOWN_SKELETON_PRODUCTconstant to eliminate duplication across 6 spec filesgetAttribute('href')withexpect.poll()for auto-retry in custom-cart-method speccreateWithCache#1151: Add case-insensitive flag to locale regex; document|\/$URL pattern branchbeforeEach; document raw handler usagegetStoreLinks()helper, consistent URL assertions, Tab keyboard tests.or()assertion with definitive check in subscriptions-auth cancel test; document mock data drift riskretries: 1and explicit 15s timeouts for third-party-api external API dependencyHOW to test your changes?
npx playwright test e2e/specs/recipes/custom-cart-method.spec.ts --workers=1npx playwright test e2e/specs/recipes/metaobjects.spec.ts --workers=1npx playwright test e2e/specs/recipes/b2b.spec.ts --workers=1npx playwright test e2e/specs/recipes/third-party-api.spec.ts --workers=1npx playwright test e2e/specs/recipes/subscriptions-auth.spec.ts --workers=1npx vitest run packages/cli/src/commands/hydrogen/upgrade-e2e.test.ts --pool=forks --poolOptions.forks.singleForkChecklist