SWC-6555 #5200

hallieswan · 2023-10-13T16:13:46Z

add end-to-end test for File sharing
- certify test users by default
- add methods for uploading/deleting a file and creating/deleting projects
bump playwright version
update CI workflow to build SWC once, then shard tests per browser across different machines

…es, so deletion order follows backend integration tests and multiple projects/files can be created per test, add e2e test for file sharing workflow, bump playwright version

…shift

…nu in teams test

…hort-lived saving button

hallieswan · 2023-10-13T16:14:28Z

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

+  build:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+      - name: Build SWC
+        uses: ./.github/workflows/build
+      - name: Upload build to GitHub Actions Artifacts
+        uses: actions/upload-artifact@v3
+        with:
+          name: ${{ env.BUILD_ARTIFACT_NAME }}
+          path: '${{ env.BUILD_DIR }}/${{ env.BUILD_NAME }}'
+          retention-days: 1


Build SWC once, then upload WAR as an artifact

Specify the full file path, so that other files in the target directory are not included in the artifact

This is unique per-workflow, right? i.e. if you have 2 PRs running this job, could you have this race condition?

Job 1 builds + uploads WAR artifact 1

Job 2 builds + uploads WAR artifact 2

Job 1 shard(s) downloads the artifact, which ends up incorrectly being WAR artifact 2

That's a good question! I think artifacts are unique per workflow run (e.g. each e2e workflow run has its own 'playwright-report' artifact), but I couldn't find something in the docs that explicitly states this.

Either way, I don't think we'd run into that race condition because per the GH docs, "You can only download artifacts that were uploaded during the same workflow run." So the Job 1 shard should only be able to download WAR artifact 1, even if another WAR artifact exists.

hallieswan · 2023-10-13T16:16:31Z

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

+    strategy:
+      fail-fast: false
+      matrix:
+        shard: [1/3, 2/3, 3/3]


Shard e2e tests for each browser across machines. Set fail-fast to false, so that all shards will finish running tests, even if tests on one shard fail.

Just curious, is it smart enough to shard by browser engine?

Good question! I don't see their sharding strategy explicitly defined in the docs, but from my tests, sharding is done by test, where tests are ordered by the project declaration order in the config file, then the alphabetized file name, then the test declaration order within each file. The tests are then distributed as evenly as possible across shards. In a simple example, running three projects (10 chromium tests, 10 firefox tests, and 5 webkit tests) on three shards results in shard 1 running 9 tests (all chromium), shard 2 running 8 tests (1 chromium, 7 firefox), and shard 3 running 8 tests (3 firefox, 5 webkit).

Since we have one project per browser that runs all tests, using an equal number of shards as browsers ensures that each shard only runs tests for one browser. However, if we start writing browser-specific tests or change how the projects are defined, we may need a more complicated strategy to ensure that each shard runs one browser (or decide whether that's necessary).

I've seen issues requesting other sharding strategies (e.g. sharding by test time), so it's also possible that more sophisticated strategies will be available in the future.

hallieswan · 2023-10-13T16:16:58Z

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

-          # workaround for https://github.com/actions/runner-images/issues/8104
-          # ...and handle 8.1.0 regression: https://github.com/Homebrew/homebrew-core/issues/140244
-          export HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK=1
-          brew uninstall --ignore-dependencies qemu
-          curl -OSL https://raw.githubusercontent.com/Homebrew/homebrew-core/dc0669eca9479e9eeb495397ba3a7480aaa45c2e/Formula/qemu.rb
-          brew install ./qemu.rb
-


Workaround no longer needed -- qemu has been updated in the GitHub runners

hallieswan · 2023-10-13T16:27:40Z

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

-        run: yarn playwright test
-      - name: Upload Playwright Report
+        run: yarn playwright test --shard ${{ matrix.shard }}
+      - name: Upload blob report to GitHub Actions Artifacts


Each shard will produce its own blob report, so upload these as an artifact

hallieswan · 2023-10-13T16:28:04Z

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

+      - name: Download blob reports from GitHub Actions Artifacts
+        uses: actions/download-artifact@v3
+        with:
+          name: ${{ env.PW_ALL_BLOBS_DIR }}
+          path: ${{ env.PW_ALL_BLOBS_DIR }}
+      - name: Merge into HTML Report
+        run: yarn playwright merge-reports --reporter html ./"${{ env.PW_ALL_BLOBS_DIR }}"


Combine the blob reports created by each shard into a single HTML report

hallieswan · 2023-10-13T16:30:36Z

e2e/files.spec.ts

+    const userContext = await browser.newContext({
+      storageState: storageStatePaths['swc-e2e-user'],
+    })
+    const userPage = await userContext.newPage()
+    const userAccessToken = await getAccessTokenFromCookie(userPage)
+
+    // create project
+    const userProjectName = generateEntityName('project')
+    const userProjectId = await createProject(
+      userProjectName,
+      userAccessToken,
+      userPage,
+    )


Create the Project with the test user's access token so that the project is initially only accessible by the test user

hallieswan · 2023-10-13T16:32:59Z

e2e/files.spec.ts

+      if (browserName === 'webkit') {
+        test.info().annotations.push({
+          type: 'very slow',
+          description: `webkit in CI only. May be related to the InviteWidget. 
+            Evaluate if timeout can be removed after addressing 
+            https://sagebionetworks.jira.com/browse/SWC-6569.`,
+        })
+        test.setTimeout(testInfo.timeout * 5)
+      }


The files test is intermittently very slow in webkit in CI (5-7 minutes in CI vs 35 seconds locally), but does eventually pass. Increase overall test timeout and timeout of particularly slow expectations to help this test pass when it is slow.

That's really weird. This annotations system is cool though, so thanks for adding all of this potentially helpful information.

hallieswan · 2023-10-13T16:35:34Z

e2e/files.spec.ts

+            name: 'Enter name...',
+          })
+          await userSuggestBox.fill(validatedUserName)
+          await userSuggestBox.press('Shift')


press and pressSequentially are particularly slow in webkit in CI, so fill the validated username to avoid calling press for each letter of the validated user's name, then press Shift to trigger the user suggestion dropdown, as discussed in SWC-6569

hallieswan · 2023-10-13T16:36:21Z

e2e/fixtures/authenticatedUserPages.ts

-    const validatedUserPage = await context.newPage()
-    await use(validatedUserPage)
+    const userPage = await context.newPage()
+    await use(userPage)


General method, so use a general variable name

hallieswan · 2023-10-13T16:37:57Z

e2e/fixtures/authenticatedUserPages.ts

+        await base.step(`certify test user - ${user.username}`, async () => {
+          await setCertifiedUserStatus(userId, true, getAdminPAT(), page)


Users must be certified to upload a file -- certify new users by default, similar to backend integration tests

hallieswan · 2023-10-13T16:41:54Z

e2e/helpers/entities.ts

+// Get a FileHandle using its ID.
+// Note: Only the user that created the FileHandle can access it directly.
+export async function getFileHandle(
+  accessToken: string,
+  handleId: string,
+  page: Page,
+) {
+  return (await doGet(
+    page,
+    `/file/v1/fileHandle/${handleId}`,
+    accessToken,
+    BackendDestinationEnum.REPO_ENDPOINT,
+  )) as FileHandle
+}
+
+// Delete a FileHandle using its ID.
+// Note: Only the user that created the FileHandle can delete it.
+//   Also, a FileHandle cannot be deleted if it is associated with a FileEntity or WikiPage
+export async function deleteFileHandle(
+  accessToken: string,
+  handleId: string,
+  page: Page,
+) {
+  await doDelete(
+    page,
+    `/file/v1/fileHandle/${handleId}`,
+    accessToken,
+    BackendDestinationEnum.REPO_ENDPOINT,
+  )
+  return handleId
+}


Moved from e2e/helpers/messages.ts so these methods are colocated with other entity methods

hallieswan · 2023-10-13T16:42:54Z

e2e/helpers/messages.ts

-// Get a FileHandle using its ID.
-// Note: Only the user that created the FileHandle can access it directly.
-async function getFileHandle(
-  accessToken: string,
-  handleId: string,
-  page: Page,
-) {
-  return (await doGet(
-    page,
-    `/file/v1/fileHandle/${handleId}`,
-    accessToken,
-    BackendDestinationEnum.REPO_ENDPOINT,
-  )) as FileHandle
-}
-
-// Delete a FileHandle using its ID.
-// Note: Only the user that created the FileHandle can delete it.
-//   Also, a FileHandle cannot be deleted if it is associated with a FileEntity or WikiPage
-async function deleteFileHandle(
-  accessToken: string,
-  handleId: string,
-  page: Page,
-) {
-  await doDelete(
-    page,
-    `/file/v1/fileHandle/${handleId}`,
-    accessToken,
-    BackendDestinationEnum.REPO_ENDPOINT,
-  )
-  return handleId
-}


Moved into e2e/helpers/entities.ts

hallieswan · 2023-10-13T16:48:24Z

e2e/helpers/testUser.ts

-  const loadingButton = page.getByRole('button', { name: 'Logging you in' })
-  await expect(loadingButton).toBeVisible()
-  await expect(loadingButton).not.toBeVisible()


This state is very short-lived, so was intermittently missed and caused tests to fail, even when wrapped in a Promise.all. This expectation was originally added to help us understand why login intermittently failed, but removing it since actually caused tests to be flakier and the check for the URL and the username/password alert should help us evaluate, if needed

hallieswan · 2023-10-13T16:49:41Z

package.json

-    "e2e:report": "yarn playwright show-report"
+    "e2e:report": "yarn playwright show-report",
+    "e2e:report:blob": "yarn playwright merge-reports --reporter html ./blob-report && yarn e2e:report",
+    "playwright:update": "yarn add -D @playwright/test@latest; yarn playwright install"


Update Playwright browsers after updating the Playwright version

hallieswan · 2023-10-13T16:50:27Z

playwright.config.ts

@@ -32,7 +32,7 @@ export default defineConfig({
  workers: process.env.CI ? 1 : 2,
  /* Reporter to use. See https://playwright.dev/docs/test-reporters */
  // Concise 'dot' for CI, default 'list' when running locally
-  reporter: process.env.CI ? [['list'], ['html']] : 'html',
+  reporter: process.env.CI ? [['list'], ['blob']] : 'html',


Use 'blob' format in CI, so that the report from each shard can be merged into a single HTML report

nickgros

This looks great, though I had a few questions

nickgros · 2023-10-13T18:42:46Z

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

+  build:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+      - name: Build SWC
+        uses: ./.github/workflows/build
+      - name: Upload build to GitHub Actions Artifacts
+        uses: actions/upload-artifact@v3
+        with:
+          name: ${{ env.BUILD_ARTIFACT_NAME }}
+          path: '${{ env.BUILD_DIR }}/${{ env.BUILD_NAME }}'
+          retention-days: 1


This is unique per-workflow, right? i.e. if you have 2 PRs running this job, could you have this race condition?

Job 1 builds + uploads WAR artifact 1

Job 2 builds + uploads WAR artifact 2

Job 1 shard(s) downloads the artifact, which ends up incorrectly being WAR artifact 2

nickgros · 2023-10-13T18:44:57Z

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

+    strategy:
+      fail-fast: false
+      matrix:
+        shard: [1/3, 2/3, 3/3]


Just curious, is it smart enough to shard by browser engine?

nickgros · 2023-10-13T18:51:22Z

e2e/files.spec.ts

+      if (browserName === 'webkit') {
+        test.info().annotations.push({
+          type: 'very slow',
+          description: `webkit in CI only. May be related to the InviteWidget. 
+            Evaluate if timeout can be removed after addressing 
+            https://sagebionetworks.jira.com/browse/SWC-6569.`,
+        })
+        test.setTimeout(testInfo.timeout * 5)
+      }


That's really weird. This annotations system is cool though, so thanks for adding all of this potentially helpful information.

nickgros · 2023-10-13T18:54:32Z

e2e/files.spec.ts

+          // Cookies banner covers the button to save settings
+          // ...so close banner by accepting cookies
+          await acceptSiteCookies(userPage)


Would it make sense to move this into a fixture, and just auto-accept for every new page (user?) instance? I don't think we should have to think about this in this test.

Sure, I'll add it in to the user page fixtures!

nickgros · 2023-10-13T18:55:56Z

e2e/files.spec.ts

+
+        await fileLink.click()
+        await expect(
+          userPage.getByText(`Discussion about ${fileName}`),


Just curious, why look for "Discussion about" ?

Ah, good question -- this was an easy locator to confirm that the File page had loaded, but I'll add some other expectations about the File page for completeness!

nickgros · 2023-10-13T18:56:16Z

e2e/files.spec.ts

+      })
+    },
+  )
+})


This test suite is exceptionally readable!!

…n user page fixture, per PR

hallieswan added 15 commits October 13, 2023 08:24

SWC-6555: certify test users

24ddd60

SWC-6555: add project fixture

2c7b0f8

SWC-6555: create e2e project and files in methods, rather than fixtur…

1f4d882

…es, so deletion order follows backend integration tests and multiple projects/files can be created per test, add e2e test for file sharing workflow, bump playwright version

SWC-6555: remove workaround for qemu

fbf5d38

SWC-6555: try entering username with fill, then triggering menu with …

6d29b18

…shift

SWC-6555: try to decrease flakiness by waiting for intermediate states

6ffe0dd

SWC-6555: update to match successful locator for SynapseSuggestBox me…

d53f7b4

…nu in teams test

SWC-6555: try using fill and shift to trigger keydown event

a642a9e

SWC-6555: try increasing timeout for webkit

ac8b980

SWC-6555: stop awaiting short-lived logging you in button

9b7f2f6

SWC-6555: increase timeout for slow steps and remove wait for flaky s…

a0e5f61

…hort-lived saving button

SWC-6555: shard projects in CI, increase files test timeout for webkit

73de03d

SWC-6555: add test annotation

d0499c6

SWC-6555: build once, then use on each shard

1294f7b

SWC-6555: bump playwright version, specify node version

9b17d99

hallieswan commented Oct 13, 2023

View reviewed changes

hallieswan marked this pull request as ready for review October 13, 2023 16:50

hallieswan requested a review from nickgros October 13, 2023 16:50

hallieswan self-assigned this Oct 13, 2023

nickgros approved these changes Oct 13, 2023

View reviewed changes

SWC-6555: improve file page loading expectations and accept cookies i…

be61832

…n user page fixture, per PR

hallieswan merged commit 2eae71c into Sage-Bionetworks:develop Oct 16, 2023

hallieswan deleted the SWC-6555 branch October 16, 2023 16:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SWC-6555 #5200

SWC-6555 #5200

hallieswan commented Oct 13, 2023

hallieswan Oct 13, 2023

hallieswan Oct 13, 2023

nickgros Oct 13, 2023

hallieswan Oct 13, 2023

hallieswan Oct 13, 2023

nickgros Oct 13, 2023

hallieswan Oct 13, 2023

hallieswan Oct 13, 2023

hallieswan Oct 13, 2023

hallieswan Oct 13, 2023

hallieswan Oct 13, 2023

hallieswan Oct 13, 2023

nickgros Oct 13, 2023

hallieswan Oct 13, 2023

hallieswan Oct 13, 2023

hallieswan Oct 13, 2023

hallieswan Oct 13, 2023

hallieswan Oct 13, 2023

hallieswan Oct 13, 2023

hallieswan Oct 13, 2023

hallieswan Oct 13, 2023

nickgros left a comment

nickgros Oct 13, 2023

nickgros Oct 13, 2023

nickgros Oct 13, 2023

nickgros Oct 13, 2023

hallieswan Oct 13, 2023

nickgros Oct 13, 2023

hallieswan Oct 13, 2023

nickgros Oct 13, 2023

		await base.step(`certify test user - ${user.username}`, async () => {
		await setCertifiedUserStatus(userId, true, getAdminPAT(), page)

SWC-6555 #5200

SWC-6555 #5200

Conversation

hallieswan commented Oct 13, 2023

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

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

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

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