Conversation
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
e6b046e to
efbdbba
Compare
|
🤖 Code Review · #projects-dev-ai for questions ✅ Complete - 1 findings 📋 History✅ No issues → ✅ 1 findings |
| await expect(cartToggle.getByLabel('Items: 1')).toBeVisible(); | ||
|
|
||
| expect(parseInt(updatedCount)).toBeGreaterThan(parseInt(initialCount)); | ||
| await cart.assertInCart(page, productName); |
There was a problem hiding this comment.
If we follow the POM pattern and make the object into a class or singleton, each domains assertion doesn't have to keep passing in the Playwright's page as an arg.
const cartUtil = new CartUtil(page);
cartUtil.assertInCart(productName)
There was a problem hiding this comment.
it’s a decision we can make together! i particularly dont like POM because it needs to be declared at the top all to avoid passing page as a param to functions
we can do it though, especially if there are more advantages than skipping page
|
|
||
| // Wait for the cart update network request to complete | ||
| await page.waitForLoadState('networkidle'); | ||
| await expect(cartToggle.getByLabel('items: 0')).toBeVisible(); |
There was a problem hiding this comment.
Flaky selector due to inconsistent label casing (items: 0 vs Items: 1)
The test asserts two different accessible labels for the cart badge:
await expect(cartToggle.getByLabel('items: 0')).toBeVisible();
...
await expect(cartToggle.getByLabel('Items: 1')).toBeVisible();This is likely to fail depending on the actual accessible name casing/format and is inconsistent within the same test. If the UI label is Items: 0, the first assertion fails; if it’s items: 1, the second fails. Even if matching is case-insensitive in some contexts, relying on that is brittle across locator strategies.
This PR is an example of good practices when writing e2e tests with playwright
highlights: