Merge pull request #46 from shopcanal/dgattey/fixes

feat: rewrite all tests to work without flakiness + add sharding

.github/workflows/test.yml

@@ -7,16 +7,19 @@ jobs:
     runs-on: ubuntu-latest
     strategy:
       matrix:
-        # Each of these are browser names we want to run tests against in parallel
+        # Each of these are browser names we want to run tests against in parallel - Firefox is disabled until https://github.com/microsoft/playwright/issues/13027 is resolved
         browser: ['chromium', 'webkit']
+        # We split tests into 4 different shards to run in parallel for speed
+        shard: [1, 2, 3, 4]
     steps:
       - uses: actions/checkout@v2
 
       # Uses the internal Github action for testing the tester!
       - uses: ./
-        name: test ${{ matrix.browser }}
+        name: test ${{ matrix.browser }} [${{ matrix.shard }}/4]
         env:
           APP_TEST_PASSWORD: ${{ secrets.APP_TEST_PASSWORD }}
           E2E_DATA_SETUP_URL: ${{ secrets.E2E_DATA_SETUP_URL }}
         with:
           browser: ${{ matrix.browser }}
+          shard: '${{ matrix.shard }}/4'

Dockerfile

@@ -1,4 +1,5 @@
-FROM mcr.microsoft.com/playwright:focal
+FROM mcr.microsoft.com/playwright:v1.20.0-focal
+COPY *.ts ./
 COPY action.sh /action.sh
 COPY package.json /package.json
 COPY tests /tests

README.md

@@ -12,11 +12,18 @@ to all browsers. Use `main` to keep up to date against the latest E2E tests writ
 to that version. Usually you want `main`. Github action is written in Dockerfile + action.yml + action.sh if you need
 to change it.
 
+The tests can be sharded, but default to only one shard! Here's how both options would work:
+
 ```yml
+strategy:
+  matrix:
+    browser: ['chromium', 'webkit', 'firefox']
+    shard: [1, 2, 3, 4]
 steps:
   - uses: shopcanal/e2e-tests@main
     with:
       browser: ${{ matrix.browser }}
+      shard: '${{ matrix.shard }} / 4'
 ```
 
 ## Committing changes
@@ -33,11 +40,11 @@ https://playwright.dev/docs/test-intro/ for more information about how it functi
 
 ### Running tests manually
 
-Use `yarn test` to run all tests, or `yarn test tests/x` to run test `x.spec.ts` in the tests folder. You can add
+Use `yarn test` to run all tests, or `yarn test x` to run test `x.spec.ts` in the tests folder. You can add
 as many tests to run as you want instead of running all them. Running all tests on all browsers can be done with
-`yarn test.all`.
+`yarn test.all`. If you need to get more specific, you can add folder names too like `yarn test shopkeep/x`, etc.
 
-To run tests and debug with Playwright Inspector, use `yarn debug`.
+To run tests and debug with Playwright Inspector, use `yarn debug`. You'll need to comment out the `test.describe.configure({ mode: 'parallel' })` lines to be able to debug properly.
 
 All tests will eventually be run on PRs in other repos, but for now these tests can be run alone. Test screenshots are
 published as artifacts on action runs.
@@ -46,3 +53,57 @@ published as artifacts on action runs.
 
 Add a new test file to `tests`, written in Typescript and with a `.spec.ts` ending. Screenshots should go in
 `screenshots.spec.ts`, otherwise test structure is up to you and pretty flexible, as long as things stay organized.
+
+#### Tips and Tricks
+
+Best practices for non-flaky and easy to debug tests:
+
+- **NO TIMEOUTS**. Period. If you find yourself waiting for an arbitrary amount of time, there's _always_ a better way to do it. Familiarize yourself with the [`page` API](https://playwright.dev/docs/api/class-page). Usually a `waitForNavigation` or `waitFor({ state: 'visible' })` on a locator clears up your issue. There's a few other tips about this below.
+
+- Don't use things like `page.$` or `page.$$` or `page.waitForSelector` to get elements! Instead use Playwright's [`locator` objects](https://playwright.dev/docs/api/class-locator). They are executed every time they're used vs. just once when you create a selector, so they're more resilient to change + less flaky as a result. You can use these like selectors.
+
+```typescript
+const tabButton = page.locator('text=Discover');
+
+await tabButton.click(); // simply click on the button once it's visible, for example
+await tabButton.waitFor({ state: 'hidden' }); // wait for it to be hidden, for example
+```
+
+- If you need to await a navigation to a new page when there's a button click (for example), do it in a `Promise.all` so that there's no race condition between the navigation and the action that opens the new page:
+
+```typescript
+const button = page.locator('text=Hi');
+await Promise.all([button.click(), page.waitForNavigation()]);
+```
+
+However, if you're needing the `waitForNavigation` for the test pass, **there's usually a better way to do it**. Usually, this just means you need a locator on the next page that you `waitFor` after the button click. That could look like this:
+
+```typescript
+const tab = page.locator('nav button:has-text("Discover")');
+const discoverHeader = page.locator('text="Discover products to feature on your store"');
+
+// Shouldn't be visible
+await discoverHeader.waitFor({ state: 'detached' });
+
+// Clicking the tab should navigate us to discover and show the header
+await tab.click();
+await discoverHeader.waitFor();
+```
+
+- But if the action opens a new tab/window instead of navigates the existing page, you do need to capture the new page instead of waiting for navigation/some other locator on the page. Treat the new page object like you would the normal `page` variable (you can test title, url, etc, get elements, etc).
+
+```typescript
+const button = page.locator('text=Hi');
+
+const [newPage] = await Promise.all([context.waitForEvent('page'), button.click()]);
+const newPageHeader = newPage.locator('text="Discover Products"');
+await newPageHeader.waitFor();
+```
+
+### Turning off tests/addressing flaky tests
+
+If there's a flaky test/etc that can't be immediately fixed, add a `test.skip(true, 'reason why here')` at the top of the test - it'll show up as a skipped test, but won't be run. This is better than commenting it out, as it provides an indication of how many we're skipping/how quickly we need to address the backlog of flaky tests.
+
+If a test appears to be flaky, try removing all need to `waitForNavigation` or `waitForURL` from it. Instead, follow the tips and tricks above to switch to waiting for locators. That's a surefire way to avoid race conditions with navigation.
+
+Testing for flakiness can be done by running all tests with `--workers=20` or some other high number. Having this many workers at once may expose problems faster than with fewer workers running tests in parallel. You can also try running the tests in a loop in code to see how often the flaky ones fail.

action.sh

@@ -1,10 +1,11 @@
 #!/bin/sh -l
 
 BROWSER="$1"
+SHARD="$2"
 
 echo "Installing packages..."
 yarn
 
-echo "Running tests on $BROWSER..."
-HOME=/root yarn test --browser=$BROWSER
+echo "Running tests on $BROWSER (shard $SHARD)..."
+HOME=/root DEBUG=pw:api,pw:browser yarn test --project=$BROWSER --shard=$SHARD
 

action.yml

@@ -9,8 +9,13 @@ inputs:
     description: 'The browser against which to run tests, defaults to all if not provided'
     required: false
     default: 'all'
+  shard:
+    description: 'The test shard so we can execute only part at a time. Will be 1/3 or 4/7, etc. Defaults to 1/1'
+    required: false
+    default: '1/1'
 runs:
   using: 'docker'
   image: 'Dockerfile'
   args:
     - '${{ inputs.browser }}'
+    - '${{ inputs.shard }}'

helpers/intercept.ts

@@ -0,0 +1,20 @@
+import type { Page } from '@playwright/test';
+
+/**
+ * Intercepts requests on the page to disable requesting fonts, or any third
+ * party resource. We do this to better control the flakiness of the tests and
+ * avoid some third party request from affecting things. EVERY test should call
+ * this function via a before each.
+ */
+export const intercept = async (page: Page): Promise<void> => {
+  await page.route(
+    /.*(?:clarity.ms)|(?:crisp.chat)|(?:cdn.shopify)|(?:shopify.com)|(?:heapanalytics)|(?:facebook)|(?:googletagmanager)|(?:google-analytics)|(?:bing.com)|(?:stripe)|(?:sentry).*/,
+    // eslint-disable-next-line @typescript-eslint/no-misused-promises
+    (route) => route.abort(),
+  );
+  await page.route(
+    '**.{woff2}',
+    // eslint-disable-next-line @typescript-eslint/no-misused-promises
+    (route) => route.abort(),
+  );
+};

helpers/login.ts

@@ -1,14 +1,6 @@
-import type {
-  BrowserContext,
-  Page,
-  PlaywrightTestArgs,
-  PlaywrightTestOptions,
-  PlaywrightWorkerArgs,
-  PlaywrightWorkerOptions,
-  TestType,
-} from '@playwright/test';
+import type { BrowserContext, Page } from '@playwright/test';
 import { expect } from '@playwright/test';
-import { LOGIN_PAGE, SHOPKEEP_ROUTES } from './routes';
+import { SHOPKEEP_ROUTES, SUPPLIER_ROUTES } from './routes';
 
 /**
  * Selectors used for logging in
@@ -19,44 +11,59 @@ export const LOGIN_BUTTON_SELECTOR = 'button#login';
 
 const E2E_ACCOUNT_LOGIN = 'e2e_tester@shopcanal.com';
 
-export const logInSuccessfully = async (
+/**
+ * Use this to log a user in and make sure they land on a given page.
+ */
+const logIn = async (
   page: Page,
   context: BrowserContext,
-  test?: TestType<
-    PlaywrightTestArgs & PlaywrightTestOptions,
-    PlaywrightWorkerArgs & PlaywrightWorkerOptions
-  >,
+  loginUrl: string,
+  firstLoggedInPageUrl: string,
 ): Promise<void> => {
+  const loginButton = page.locator(LOGIN_BUTTON_SELECTOR);
+  const profileDropdown = page.locator('button:has-text("e2e_tester")');
+
   if (process.env.APP_TEST_PASSWORD) {
     // Make sure browser is logged out before attempting to go through login flow
-    await logout(context);
+    await logout(page, context);
 
     // Navigate to login page and wait for login button to load
-    await page.goto(LOGIN_PAGE);
-    await page.waitForSelector(LOGIN_BUTTON_SELECTOR);
+    await page.goto(loginUrl);
+    await loginButton.waitFor();
 
     // Fill out email and password
     await page.fill(LOGIN_EMAIL_INPUT_SELECTOR, E2E_ACCOUNT_LOGIN);
     await page.fill(LOGIN_PASSWORD_INPUT_SELECTOR, process.env.APP_TEST_PASSWORD || '');
 
-    // Then click the login button
-    await page.click(LOGIN_BUTTON_SELECTOR);
+    // Then click the login button and make sure we navigate
+    await loginButton.click();
 
-    // Wait 10 seconds to give time for the next page to load
-    await page.waitForTimeout(10000);
+    // Make sure the profile dropdown shows, as this appears on both sides once logged in
+    await profileDropdown.waitFor();
 
-    // See if the nav has loaded - if it hasn't, then skip the test since login was flaky
-    const overviewButton = await page.$('a#navOverview');
-    if (test) test.skip(!overviewButton, 'Login was not successful - skipping test');
-
-    // Ensure that the URL is now the URL of the Shopkeep Inventory page
-    expect(page.url().includes(SHOPKEEP_ROUTES.INVENTORY)).toBeTruthy();
+    // Make sure this is the page we want
+    expect(page.url().includes(firstLoggedInPageUrl)).toBeTruthy();
   } else {
     console.warn('Could not log in because no APP_TEST_PASSWORD was provided. Failing test.');
     expect(true).toBe(false);
   }
 };
 
-export const logout = async (context: BrowserContext): Promise<void> => {
+/**
+ * Logs a user into the shopkeep landing page
+ */
+export const logIntoShopkeep = async (page: Page, context: BrowserContext): Promise<void> =>
+  logIn(page, context, SHOPKEEP_ROUTES.LOGIN, SHOPKEEP_ROUTES.INVENTORY);
+
+/**
+ * Logs a user into the supplier landing page
+ */
+export const logIntoSupplier = async (page: Page, context: BrowserContext): Promise<void> =>
+  logIn(page, context, SUPPLIER_ROUTES.LOGIN, SUPPLIER_ROUTES.OVERVIEW);
+
+/**
+ * Simulates a log out by clearing cookies. On next page nav, we'll refresh.
+ */
+export const logout = async (page: Page, context: BrowserContext): Promise<void> => {
   await context.clearCookies();
 };

helpers/routes.ts

@@ -2,31 +2,28 @@ const DOMAIN = 'https://develop.shopcanal.com';
 const SHOPKEEP = `${DOMAIN}/shopkeep`;
 const SUPPLIER = `${DOMAIN}/supplier`;
 
-/**
- * App URLs used across our various test files
- */
-export const LOGIN_PAGE = `${DOMAIN}/login`;
-
 /**
  * This must match the "App Url" setting of our Shopify App in order to test the
  * userflow of embedded users.
  */
 export const SK_APP_URL = `${DOMAIN}/shopkeep/inventory?embedded=true&canal_app_name=storefront`;
 
 export const SHOPKEEP_ROUTES = {
-  INVENTORY: `${SHOPKEEP}/inventory`,
+  LOGIN: `${DOMAIN}/login`,
   DISCOVER: `${SHOPKEEP}/discover`,
-  REQUESTS: `${SHOPKEEP}/requests`,
-  ORDERS: `${SHOPKEEP}/orders`,
+  INVENTORY: `${SHOPKEEP}/inventory`,
+  SUPPLIERS: `${SHOPKEEP}/suppliers`,
+  PROPOSALS: `${SHOPKEEP}/proposals`,
+  INVITE: `${SHOPKEEP}/invite`,
   SETTINGS: `${SHOPKEEP}/settings`,
-  FAQ: `${SHOPKEEP}/faq`,
 };
 
 export const SUPPLIER_ROUTES = {
+  LOGIN: `${DOMAIN}/login?next=/supplier`,
   OVERVIEW: `${SUPPLIER}/overview`,
-  INVENTORY: `${SUPPLIER}/inventory`,
   DISCOVER: `${SUPPLIER}/discover`,
-  REQUESTS: `${SUPPLIER}/requests`,
+  INVENTORY: `${SUPPLIER}/inventory`,
+  STOREFRONTS: `${SUPPLIER}/storefronts`,
+  PROPOSALS: `${SUPPLIER}/proposals`,
   SETTINGS: `${SUPPLIER}/settings`,
-  FAQ: `${SUPPLIER}/faq`,
 };

package.json

@@ -8,29 +8,29 @@
   "license": "MIT",
   "homepage": "https://github.com/shopcanal/e2e-tests#readme",
   "devDependencies": {
-    "@commitlint/cli": "^12.1.4",
-    "@commitlint/config-conventional": "^12.1.4",
-    "@playwright/test": "^1.16.1",
-    "@semantic-release/changelog": "^5.0.1",
-    "@semantic-release/git": "^9.0.0",
-    "@typescript-eslint/eslint-plugin": "^4.28.3",
-    "@typescript-eslint/parser": "^4.28.3",
-    "eslint": "^7.30.0",
-    "eslint-config-prettier": "^8.3.0",
-    "eslint-import-resolver-typescript": "^2.4.0",
-    "eslint-plugin-import": "^2.23.4",
-    "eslint-plugin-react": "^7.24.0",
+    "@commitlint/cli": "^16.2.3",
+    "@commitlint/config-conventional": "^16.2.1",
+    "@playwright/test": "^1.20.1",
+    "@semantic-release/changelog": "^6.0.1",
+    "@semantic-release/git": "^10.0.1",
+    "@typescript-eslint/eslint-plugin": "^5.16.0",
+    "@typescript-eslint/parser": "^5.16.0",
+    "eslint": "^8.12.0",
+    "eslint-config-prettier": "^8.5.0",
+    "eslint-import-resolver-typescript": "^2.7.0",
+    "eslint-plugin-import": "^2.25.4",
+    "eslint-plugin-react": "^7.29.4",
     "eslint-plugin-tsc": "^2.0.0",
-    "husky": "^7.0.0",
-    "lint-staged": "^11.0.1",
-    "playwright": "^1.16.1",
-    "prettier": "^2.3.2",
-    "semantic-release": "^17.4.4",
-    "typescript": "^4.3.5"
+    "husky": "^7.0.4",
+    "lint-staged": "^12.3.7",
+    "playwright": "^1.20.1",
+    "prettier": "^2.6.1",
+    "semantic-release": "^19.0.2",
+    "typescript": "^4.6.3"
   },
   "scripts": {
     "prepare": "husky install",
-    "test": "playwright test tests",
+    "test": "playwright test",
     "test.all": "yarn test --browser=all",
     "debug": "PWDEBUG=1 yarn test",
     "help": "playwright --help",