search

SalesforceCommerceCloud / pwa-kit

React-based JavaScript frontend framework to create a progressive web app (PWA) storefront for Salesforce B2C Commerce.
https://developer.salesforce.com/docs/commerce/pwa-kit-managed-runtime/guide/pwa-kit-overview.html
BSD 3-Clause "New" or "Revised" License
283 stars 130 forks source link

[Product Tile] Fix flaky e2e tests when selecting a different swatch (@W-16116401@) #1870

Closed vmarta closed 3 months ago

vmarta commented 3 months ago

This PR attempts at fixing the flaky E2E tests for Product Tile. We noticed on selecting a different swatch, the tests sometimes failed at detecting whether the image src has changed. It looks like we need to wait a bit for this image src to change.

How to Test-Drive This PR

What you can test right now:

  1. Checkout this PR branch
  2. Run npm run test:e2e:ui in your terminal
  3. Click on the Play button in the resulting GUI window
  4. It will run the E2E tests against https://scaffold-pwa-e2e-tests-pwa-kit.mobify-storefront.com/

But yes, the real test would happen tonight when the E2E tests are run nightly.

Checklists

General

Accessibility Compliance

You must check off all items in one of the follow two lists:

or...

Localization

vcua-mobify commented 3 months ago

I left a comment in our slack thread about the mobile tests still appearing to be flaky.

vcua-mobify commented 3 months ago

Mobile tests appear to be much more reliable now after the changes.

Now the other semi-frequent failure I am seeing tends to come from the desktop tests after they successfully submit an order.

shethj commented 3 months ago

Mobile tests appear to be much more reliable now after the changes.

Now the other semi-frequent failure I am seeing tends to come from the desktop tests after they successfully submit an order.

@vmarta The mobile checkout test sometimes hits a locator conflict:

 1) [Mobile Chrome] › tests/mobile/guest-shopper.spec.js:17:1 › Guest shopper can checkout items as guest

    Error: locator.waitFor: Error: strict mode violation: getByRole('link', { name: 'Tops' }) resolved to 2 elements:
        1) <a href="/category/womens-clothing-tops" class="cha…>Tops</a> aka getByTestId('sf-product-list-page').getByRole('link', { name: 'Tops' })
        2) <a type="button" data-index="2" aria-expanded="true…>…</a> aka getByLabel('Womens').getByRole('link', { name: 'Tops' })

    Call log:
      - waiting for getByRole('link', { name: 'Tops' }) to be hidden
      -   locator resolved to visible <a type="button" data-index="2" aria-expanded="true…>…</a>
      -   locator resolved to visible <a type="button" data-index="2" aria-expanded="true…>…</a>

      38 |   await topsLink.click();
      39 |   // Wait for the nav menu to close first
    > 40 |   await topsLink.waitFor({state: 'hidden'})
         |                  ^
      41 |
      42 |   await expect(page.getByRole("heading", { name: "Tops" })).toBeVisible();
      43 |

        at /Users/j.sheth/Documents/Salesforce/pwa/pwa-kit/e2e/tests/mobile/guest-shopper.spec.js:40:18
vmarta commented 3 months ago

Mobile tests appear to be much more reliable now after the changes. Now the other semi-frequent failure I am seeing tends to come from the desktop tests after they successfully submit an order.

@vmarta The mobile checkout test sometimes hits a locator conflict:

 1) [Mobile Chrome] › tests/mobile/guest-shopper.spec.js:17:1 › Guest shopper can checkout items as guest

    Error: locator.waitFor: Error: strict mode violation: getByRole('link', { name: 'Tops' }) resolved to 2 elements:
        1) <a href="/category/womens-clothing-tops" class="cha…>Tops</a> aka getByTestId('sf-product-list-page').getByRole('link', { name: 'Tops' })
        2) <a type="button" data-index="2" aria-expanded="true…>…</a> aka getByLabel('Womens').getByRole('link', { name: 'Tops' })

    Call log:
      - waiting for getByRole('link', { name: 'Tops' }) to be hidden
      -   locator resolved to visible <a type="button" data-index="2" aria-expanded="true…>…</a>
      -   locator resolved to visible <a type="button" data-index="2" aria-expanded="true…>…</a>

      38 |   await topsLink.click();
      39 |   // Wait for the nav menu to close first
    > 40 |   await topsLink.waitFor({state: 'hidden'})
         |                  ^
      41 |
      42 |   await expect(page.getByRole("heading", { name: "Tops" })).toBeVisible();
      43 |

        at /Users/j.sheth/Documents/Salesforce/pwa/pwa-kit/e2e/tests/mobile/guest-shopper.spec.js:40:18

Thanks for catching and fixing that @shethj

Now I see how the Playwright's Locators really work. The locator is dynamic: before every action/assertion on it, Playwright will re-fetch the latest DOM element.

As per documentation: https://playwright.dev/docs/locators#locating-elements

Every time a locator is used for an action, an up-to-date DOM element is located in the page. In the snippet below, the underlying DOM element will be located twice, once prior to every action. This means that if the DOM changes in between the calls due to re-render, the new element corresponding to the locator will be used.

vmarta commented 3 months ago

Mobile tests appear to be much more reliable now after the changes.

Now the other semi-frequent failure I am seeing tends to come from the desktop tests after they successfully submit an order.

@vcua-mobify unfortunately I'm not able to reproduce the failures you saw with the desktop tests. I think let's ignore that for now. I'm hoping that our default 3 retries will be able to resolve that.

What I'm seeing on CI, though, is that the current e2e tests on develop branch consistently fail at asserting the new product tile image once you've selected a different swatch. The PR will address that failure.

So I'm hopeful that once we merge this PR, things will be much less flaky.

Update: actually, I think I know what happened: I forgot to await some of the waitFor. So that's probably why you were seeing the error after the entire test has ended.

vmarta commented 3 months ago

For the most part the playwright tests pass for me, but every once in a while I'll get a timeout fail, can we consider increasing the default timeout of 30000ms by adding the timeout property in playwright.config.js?

@joeluong-sfcc I think for now, we may not need to increase the timeout value. Let's see what happens after we merge this PR.

My guess is that our current setup of doing 3 retries (i.e. the same test can possibly be run for 4 times) is good enough to eat up the intermittent timeout fail we sometimes see.