test: refactor discount e2e tests#3515
Conversation
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
itsjustriley
force-pushed
the
refactor-skeleton-discount
branch
3 times, most recently
from
c0aad6c to
32e6470
Compare
itsjustriley
force-pushed
the
refactor-skeleton-discount
branch
from
3e3a075 to
807256d
Compare
![binks-code-reviewer[bot]](https://avatars.githubusercontent.com/in/2788732?s=60&v=4){kind=small}
|
🤖 Code Review · #projects-dev-ai for questions 🚫 PR closed 📋 History✅ 1 findings → ✅ 1 findings → ✅ No issues → ✅ 2 findings → ✅ No issues → ✅ No issues → 🚫 PR closed |
itsjustriley
force-pushed
the
refactor-skeleton-discount
branch
from
64f9684 to
f751d84
Compare
| export function CartSummary({cart, layout}: CartSummaryProps) { | ||
| const className = | ||
| layout === 'page' ? 'cart-summary-page' : 'cart-summary-aside'; | ||
| const summaryId = useId(); |
There was a problem hiding this comment.
praise: the old code had aria-labelledby="cart-summary" on the container but the referenced id="cart-summary" was missing from the <h4> - so the aria-labelledby was broken. Using useId() both fixes this broken reference and prevents duplicate IDs when multiple cart summaries render on the same page. nice catch and fix.
| const appliedCodes = await storefront.getAppliedDiscountCodes(); | ||
| expect(appliedCodes).not.toContain(inactiveDiscountCode); | ||
| await discount.assertNoDiscounts(); | ||
| }); |
There was a problem hiding this comment.
non-blocking: this test accesses page.getByLabel('Discounts') and getByRole('group') directly instead of using DiscountUtil. The whole point of the refactor is centralizing selectors. I think this logic should be encapsulated as a new method on DiscountUtil (e.g., assertDiscountCount(n)) for consistency.
| await cart.addItem(PRODUCT_NAME); | ||
| await cart.closeCartAside(); | ||
| await cart.navigateToCartPage(); | ||
| }); |
There was a problem hiding this comment.
non-blocking: the old version computed subtotalBefore - subtotalAfter === 10, verifying the exact discount amount regardless of product price. The new version hardcodes both prices. Both approaches hardcode assumptions about the discount amount (the old toBe(10) is equally a magic number), but the new version requires updating two interdependent constants if the product price changes. I think at minimum a comment would help: // Product: $749.95, discount: $10.00 off, expected: $739.95
| setTestStore('mockShop'); | ||
|
|
||
| const PRODUCT_NAME = "Women's T-shirt"; | ||
| const PRODUCT_HANDLE = 'women-t-shirt'; |
There was a problem hiding this comment.
praise: switching from "go to home, find product, click" to page.goto('/products/${PRODUCT_HANDLE}') is a good simplification - tests are faster and more focused on cart behavior.
| await page.getByRole('button', {name: 'Add to cart'}).click(); | ||
| await page.goto(`/products/${productHandle}`); | ||
| await cart.addItem(productName); | ||
|
|
There was a problem hiding this comment.
nit: the removed comment // Validate that the backend has returned the correct subtotal explained that assertSubtotal verifies backend state, not just UI. I think we should keep that comment or something equivalent - it's a "why" comment that helps future readers understand intent.
There was a problem hiding this comment.
We validate the back end state in the addItem util now, which I think is self-documenting.
| @@ -853,7 +853,7 @@ export class StorefrontPage { | |||
| async openCartAside(): Promise<void> { | |||
| const cartLink = this.page | |||
| .getByRole('banner') | |||
There was a problem hiding this comment.
nit: /cart/i to 'Cart' is good - exact matches are more precise. Note that Playwright's getByRole({name: 'Cart'}) uses substring matching by default, so if the cart link text includes a badge count (e.g., "Cart 2"), it would still match. This should be safe.
itsjustriley
force-pushed
the
refactor-skeleton-discount
branch
from
61da714 to
c1759db
Compare
3 participants
Part of https://github.com/Shopify/developer-tools-team/issues/1075
Referring to Freddie's example test, this refactors e2e discount 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 tests pass locally:
npx playwright test e2e/specs/skeleton/discounts.spec.ts(use --ui flag if desired)