SWC-6605 #5235

hallieswan · 2023-11-27T23:47:17Z

Use a macOS runner for e2e tests to reduce network flakiness, but only run workflow on release-** branches in the Sage repo and only run one shard at a time
Fix common failure points in e2e tests, e.g. adding functions for reloading page that wait for SWC to be ready for interaction and deleting project via UI if possible, otherwise deleting via API
bump Playwright version

…kipped when previous job was skipped

…slow network requests, increase timeout at key steps with known slow responses, decrease expect timeout when using page reloads

…hboard

…me bug in webkit

…un and number of runners used due to GitHub restricting number of concurrent macOS jobs

hallieswan · 2023-11-27T23:48:22Z

.github/workflows/build-test-e2e.yml

    # and on all branches in user-owned forks
-    if: ${{ github.ref_name == 'develop' || startsWith(github.ref_name, 'release-') || github.actor == github.repository_owner }}
+    if: ${{ startsWith(github.ref_name, 'release-') || github.actor == github.repository_owner }}


Only run e2e workflow for release-** branches in Sage repo, so macOS runners are available for other teams' workflows

hallieswan · 2023-11-27T23:49:56Z

.github/workflows/build-test-e2e.yml

@@ -25,9 +25,10 @@ jobs:
          retention-days: 1
  playwright-tests:
    needs: [build]
-    runs-on: ${{ github.repository_owner == 'Sage-Bionetworks' && 'ubuntu-22.04-4core-16GBRAM-150GBSSD' || 'macos-latest' }}
+    runs-on: 'macos-latest'


Use macOS runner for all jobs to decrease flakiness vs linux runners -- see 10x passing runs using macOS runner here.

hallieswan · 2023-11-27T23:50:26Z

.github/workflows/build-test-e2e.yml

    timeout-minutes: 60
    strategy:
+      max-parallel: ${{ github.repository_owner == 'Sage-Bionetworks' && 1 || 3 }}


Only run one shard at a time in the Sage repo, so macOS runners are available for other teams' workflows

hallieswan · 2023-11-27T23:52:04Z

.github/workflows/build-test-e2e.yml

+    # But skip this job if the previous job was cancelled or skipped
+    if: ${{ !cancelled() && needs.playwright-tests.result != 'skipped' }}


"skipped" !== "cancelled" -- skip the job if the workflow was "skipped", as in this case.

hallieswan · 2023-11-27T23:53:19Z

e2e/discussions.spec.ts

@@ -58,38 +61,6 @@ const expectThreadTableLoaded = async (
  })
 }

-const expectDiscussionThreadLoaded = async (


Moved into testUser.ts, so this can be reused in other tests

hallieswan · 2023-11-27T23:53:54Z

e2e/discussions.spec.ts

@@ -153,6 +126,7 @@ test.describe('Discussions', () => {
  })

  testAuth.afterAll(async ({ browser }) => {
+    test.slow()


Allow extra time for the project and file handle to be cleaned up

hallieswan · 2023-11-27T23:58:46Z

e2e/discussions.spec.ts

-        await userPage.goto(getDefaultDiscussionPath(userProject.id))
+        await goToDashboardPage(
+          userPage,
+          getDefaultDiscussionPath(userProject.id),
+        )
+


Playwright's goto method waits for the 'load' event, which may be fired before SWC is ready for interaction. Instead, use custom goTo*** methods that wait for SWC to fully load, e.g. SynapseNavDrawer is loaded for authenticated users.

hallieswan · 2023-11-27T23:59:52Z

e2e/discussions.spec.ts

@@ -257,8 +236,12 @@ test.describe('Discussions', () => {
        // retry if the view count hasn't updated
        await expect(async () => {
          // reload is necessary for view count to update
-          await userPage.reload()
-          await expectDiscussionPageLoaded(userPage, userProject.id)
+          await reloadDashboardPage(userPage)


Similar to the custom goTo*** methods, use a custom reload*** method that waits for SWC to fully load

hallieswan · 2023-11-28T00:04:57Z

e2e/files.spec.ts

-            .click({ timeout: testInfo.timeout * 3 })
+            .click({ timeout: defaultExpectTimeout * 3 })


testInfo.timeout is the test timeout. The expect timeout is not currently available in the testInfo fixture, but Playwright plans to expose the timeout in the future

hallieswan · 2023-11-28T00:05:26Z

e2e/files.spec.ts

-      test.fail(
-        browserName === 'webkit',
-        `Playwright is not preserving the File's lastModified time in webkit, so 
-        file upload fails because it seems like the file was modified during 
-        upload. See https://github.com/microsoft/playwright/issues/27452. Fix 
-        planned for Playwright v1.40.`,
-      )


Fixed in Playwright v1.40

hallieswan · 2023-11-28T00:07:24Z

e2e/fixtures/authenticatedUserPages.ts

-    { scope: 'worker', timeout: 2 * 60 * 1000 },
+    { scope: 'worker', timeout: defaultTestTimeout * 2 },


Fixtures intermittently timed out on linux runner -- allow time to create/setup two users

hallieswan · 2023-11-28T00:10:11Z

e2e/helpers/testUser.ts

-  // Perform authentication steps
-  await page.goto('/')
-  await waitForInitialPageLoad(page)


This page reload is unnecessary -- the page has already navigated to the base URL when this method is called

hallieswan · 2023-11-28T00:11:53Z

e2e/projects.spec.ts

-    await expect(
-      userPage.getByRole('heading', { name: 'Your Projects' }),
-    ).toBeVisible()
+    await testAuth.step('should delete project', async () => {


Delete the project via the UI

hallieswan · 2023-11-28T00:12:11Z

e2e/projects.spec.ts

+  testAuth.afterAll(async ({ browser }) => {
+    test.slow()
+    // if test failed before project was deleted, then delete project here
+    if (projectId) {


Clean up the project, if the test failed before deleting the project via the UI

I really like the pattern of deleting using admin credentials only if the UI path failed

hallieswan · 2023-11-28T00:12:45Z

e2e/teams.spec.ts

+          await testAuth.step('user should get teamId', async () => {
+            await userPage.waitForURL(`/${teamHashBang}:**`)
+            teamId = getTeamIdFromPathname(userPage.url())
+          })


Get the teamId before dismissing the alert and waiting for the team page to load, so that the team can be cleaned up in the afterAll if the test fails during these steps

hallieswan · 2023-11-28T00:13:21Z

e2e/teams.spec.ts

-          const loadInvitedUser = userPage.getByText('Loading...')
-          await expect(loadInvitedUser).toBeVisible()
-          await expect(loadInvitedUser).not.toBeVisible()
+          await expect(userPage.getByText('Loading...')).not.toBeVisible()


Remove flaky wait for "Loading..." to be visible

hallieswan · 2023-11-28T00:15:03Z

e2e/teams.spec.ts

@@ -203,6 +218,8 @@ test.describe('Teams', () => {
  )

  testAuth.afterAll(async ({ userPage, validatedUserPage }) => {
+    test.slow()


Allow extra time to clean up objects associated with the teams test

nickgros · 2023-11-28T15:36:14Z

e2e/files.spec.ts

-            name: 'Trash Can',
-          })
-          await expect(trashCanHeading).toBeVisible()
+          // reload page if trash can deferred js does not load


Safe to assume this probably intermittently fails because of resource constraints in CI, and not because of some other bug right?

Yes I think so -- I saw this behavior (and similar timeouts waiting for network responses) on the linux runners, but haven't seen it on the macOS runners

nickgros

Looks great!

nickgros · 2023-11-28T15:39:29Z

e2e/helpers/testUser.ts

+  await successfulLoginResponsePromise
  await expect(async () => {
    expect(page.url()).not.toContain('LoginPlace')
  }).toPass()

  // Wait until the page reaches a state where all cookies are set
-  await expect(page.getByLabel('Search')).toBeVisible()
-  await expect(page.getByLabel('Projects')).toBeVisible()
-  await expect(page.getByLabel('Your Account')).toBeVisible({
-    timeout: 30 * 1000,
-  })
+  await authenticatedUserBundleResponsePromise
+  await expectAuthenticatedNavDrawerLoaded(page)


Interesting pattern to get the promises first and hold off on the await until you need them, but it makes sense since they aren't necessarily sequential--I like it.

nickgros · 2023-11-28T15:48:52Z

e2e/projects.spec.ts

+  testAuth.afterAll(async ({ browser }) => {
+    test.slow()
+    // if test failed before project was deleted, then delete project here
+    if (projectId) {


I really like the pattern of deleting using admin credentials only if the UI path failed

nickgros · 2023-11-28T16:04:17Z

playwright.config.ts

+// FIXME: expect timeout not available in testInfo fixture in v1.39
+// change tests to use testInfo fixture once expect settings are exposed,
+// see: https://github.com/microsoft/playwright/issues/27915
+export const defaultExpectTimeout = process.env.CI ? 60 * 1000 : 5 * 1000


Wow, huge difference in CI to get this to be reliable.

hallieswan added 11 commits November 27, 2023 09:57

SWC-6554: skipped is not the same as cancelled - ensure that job is s…

da4743b

…kipped when previous job was skipped

SWC-6605: small tweaks to decrease flake

90eff2c

SWC-6605: increase test, worker, and expect timeouts in CI to handle …

f4e90fa

…slow network requests, increase timeout at key steps with known slow responses, decrease expect timeout when using page reloads

SWC-6605: await key network responses when logging in and loading das…

bec84a2

…hboard

SWC-6605: increase timeout for slow tests

ae69fa9

SWC-6605: bump playwright version, which fixes File's lastModified ti…

dce739d

…me bug in webkit

SWC-6605: delete project via ui, otherwise delete via API

fb22de1

SWC-6605: allow time for teams afterAll to finish

62cb016

SWC-6605: attempt fixes for common failure points

7452af7

SWC-6605: use macOS runner in Sage repo, but limit when workflow is r…

7812a0f

…un and number of runners used due to GitHub restricting number of concurrent macOS jobs

SWC-6605: adjust timeouts based on test runs

c2f3cd7

hallieswan commented Nov 27, 2023

View reviewed changes

hallieswan commented Nov 28, 2023

View reviewed changes

SWC-6605: fix handling of intermittently appearing file upload alert

6fdcb49

hallieswan marked this pull request as ready for review November 28, 2023 01:34

hallieswan requested a review from nickgros November 28, 2023 01:37

hallieswan self-assigned this Nov 28, 2023

nickgros reviewed Nov 28, 2023

View reviewed changes

nickgros approved these changes Nov 28, 2023

View reviewed changes

hallieswan merged commit 880b7f2 into Sage-Bionetworks:develop Nov 28, 2023

hallieswan deleted the SWC-6605 branch November 28, 2023 16:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SWC-6605 #5235

SWC-6605 #5235

hallieswan commented Nov 27, 2023 •

edited

Loading

hallieswan Nov 27, 2023

hallieswan Nov 27, 2023

hallieswan Nov 27, 2023

hallieswan Nov 27, 2023

hallieswan Nov 27, 2023

hallieswan Nov 27, 2023

hallieswan Nov 27, 2023

hallieswan Nov 27, 2023

hallieswan Nov 28, 2023

hallieswan Nov 28, 2023

hallieswan Nov 28, 2023

hallieswan Nov 28, 2023

hallieswan Nov 28, 2023

hallieswan Nov 28, 2023

nickgros Nov 28, 2023

hallieswan Nov 28, 2023 •

edited

Loading

hallieswan Nov 28, 2023

hallieswan Nov 28, 2023

nickgros Nov 28, 2023

hallieswan Nov 28, 2023

nickgros left a comment

nickgros Nov 28, 2023

nickgros Nov 28, 2023

nickgros Nov 28, 2023

		# But skip this job if the previous job was cancelled or skipped
		if: ${{ !cancelled() && needs.playwright-tests.result != 'skipped' }}

		.click({ timeout: testInfo.timeout * 3 })
		.click({ timeout: defaultExpectTimeout * 3 })

		{ scope: 'worker', timeout: 2 * 60 * 1000 },
		{ scope: 'worker', timeout: defaultTestTimeout * 2 },

SWC-6605 #5235

SWC-6605 #5235

Conversation

hallieswan commented Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hallieswan Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickgros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hallieswan commented Nov 27, 2023 •

edited

Loading

hallieswan Nov 28, 2023 •

edited

Loading