test: refactor cart tests#3514
Conversation
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
61e2798 to
4378a85
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the e2e cart tests to follow best practices by using accessibility-based selectors instead of dynamic element selection, and introduces a new CartUtil helper class to simplify test code. The changes also improve accessibility of the skeleton template's cart components by adding proper ARIA labels and semantic roles.
Changes:
- Introduced CartUtil helper class with accessibility-based assertion methods
- Refactored skeleton and smoke cart tests to use stable product names instead of dynamic selection
- Enhanced cart accessibility with ARIA labels on cart badge and improved semantic structure in CartSummary
- Changed test store from 'hydrogenPreviewStorefront' to 'mockShop' for skeleton tests
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| templates/skeleton/app/components/Header.tsx | Added ARIA label to cart badge for accessibility testing |
| templates/skeleton/app/components/CartSummary.tsx | Added id to h4 and role="group" to dl for better accessibility |
| e2e/specs/smoke/cart.spec.ts | Simplified test using CartUtil and accessibility selectors |
| e2e/specs/skeleton/cart.spec.ts | Completely refactored to use CartUtil, accessibility selectors, and stable product names |
| e2e/fixtures/index.ts | Exported CartUtil for use in tests |
| e2e/fixtures/cart-utils.ts | New utility class with accessibility-based cart assertion methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
🤖 Code Review · #projects-dev-ai for questions ✅ Complete - 1 findings 📋 History✅ 1 findings → ✅ 1 findings |
7855b4b to
92ebcb9
Compare
92ebcb9 to
d87a4d0
Compare
| <h4>Totals</h4> | ||
| <dl className="cart-subtotal"> | ||
| <h4 id="cart-summary">Totals</h4> | ||
| <dl role="group" className="cart-subtotal"> |
There was a problem hiding this comment.
Duplicate DOM id cart-summary can break accessibility + test selectors
CartSummary hard-codes id="cart-summary" and also uses aria-labelledby="cart-summary". If two CartSummary components are ever rendered on the same page (easy to do in composed layouts, portals, or future refactors), the DOM will contain duplicate IDs. That breaks aria-labelledby resolution (assistive tech can pick the wrong label) and can make Playwright queries ambiguous/flaky.
Evidence:
<div aria-labelledby="cart-summary" ...>
<h4 id="cart-summary">Totals</h4>
<dl role="group" ...>Impact:
- User impact: Screen readers may announce the wrong label or fail to associate the label correctly in pages with multiple summaries.
- Infra/scale: Affects any rendered page; failure mode depends on composition, so it can surface unexpectedly as the app evolves.
- Consequences: Accessibility regression; flaky e2e tests when relying on ARIA labeling.
There was a problem hiding this comment.
agreed but not now, if possible create a ticket for this tech debt alongside other things
fredericoo
left a comment
There was a problem hiding this comment.
lgtm! looking at flakiness now to see if we can improve it and avoid 'holding it wrong' issues
Part of https://github.com/Shopify/developer-tools-team/issues/1075
Referring to Freddie's example test, this refactors e2e cart tests and improves skeleton accessibility.
Note: duplicates some of freddie's work (skeleton accessibility updates, base utils) so will need to be rebased
To validate passing locally:
npx playwright test --project=skeleton e2e/specs/skeleton/cart.spec.ts(use --ui flag if desired)