Merge pull request #5216 from hallieswan/SWC-6556

SWC-6556, SWC-6560
Sage-Bionetworks · Oct 27, 2023 · 960336a · 960336a
2 parents eace0e4 + cb2a479
commit 960336a
Show file tree

Hide file tree

Showing 6 changed files with 149 additions and 42 deletions.
diff --git a/.github/workflows/build-test-e2e.yml b/.github/workflows/build-test-e2e.yml
@@ -9,6 +9,9 @@ env:
   PW_ALL_BLOBS_DIR: all-blob-reports
 jobs:
   build:
+    # Run in Sage repo on develop or release- branches
+    # and on all branches in user-owned forks
+    if: ${{ github.ref_name == 'develop' || startsWith(github.ref_name, 'release-') || github.actor == github.repository_owner }}
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v3
@@ -21,7 +24,6 @@ jobs:
           path: '${{ env.BUILD_DIR }}/${{ env.BUILD_NAME }}'
           retention-days: 1
   playwright-tests:
-    if: ${{ github.ref_name == 'develop' || startsWith(github.ref_name, 'release-') || github.actor == github.repository_owner }}
     needs: [build]
     runs-on: ${{ github.repository_owner == 'Sage-Bionetworks' && 'ubuntu-22.04-4core-16GBRAM-150GBSSD' || 'macos-latest' }}
     timeout-minutes: 60
@@ -79,7 +81,8 @@ jobs:
         run: colima stop
   merge-reports:
     # Merge reports after playwright-tests, even if some shards have failed
-    if: always()
+    # But skip this job if the previous job was cancelled
+    if: ${{ !cancelled() }}
     needs: [playwright-tests]
     runs-on: ubuntu-latest
     steps:

diff --git a/e2e/discussions.spec.ts b/e2e/discussions.spec.ts
@@ -40,6 +40,7 @@ const expectDiscussionPageLoaded = async (page: Page, projectId: string) => {
     await expect(
       page.getByRole('button', { name: 'Discussion Tools' }),
     ).toBeVisible()
+    await expect(page.getByPlaceholder('Search discussions')).toBeVisible()
   })
 }
 
@@ -71,20 +72,35 @@ const expectThreadTableLoaded = async (
   })
 }
 
-const expectDiscussionThreadLoaded = async (page: Page, threadId: number) => {
+const expectDiscussionThreadLoaded = async (
+  page: Page,
+  threadId: number,
+  threadTitle: string,
+  threadBody: string,
+) => {
   await testAuth.step('Discussion thread has loaded', async () => {
     await page.waitForURL(
       `${entityUrlPathname(userProject.id)}/discussion/threadId=${threadId}`,
     )
     await expect(
-      page.getByRole('button', { name: 'Show All Threads' }),
+      page.getByRole('heading', { name: 'Discussion' }),
     ).toBeVisible()
+
+    await expect(
+      page.getByRole('button', { name: /show all threads/i }),
+    ).toBeVisible({ timeout: 60_000 })
     await expect(
       page.getByRole('button', { name: 'Date Posted' }),
     ).toBeVisible()
     await expect(
       page.getByRole('button', { name: 'Most Recent' }),
     ).toBeVisible()
+
+    const discussionThread = page.locator(discussionThreadSelector)
+    await expect(discussionThread.getByText(threadTitle)).toBeVisible()
+    await expect(discussionThread.getByText(threadBody)).toBeVisible({
+      timeout: 60_000,
+    })
   })
 }
 
@@ -163,6 +179,8 @@ test.describe('Discussions', () => {
   testAuth(
     'should allow discussion and reply CRUD',
     async ({ userPage, validatedUserPage }) => {
+      test.slow()
+
       const threadTitle = 'The Title of the Thread'
       const threadBody = 'The body of the Thread'
       const threadReply = 'A really interesting reply to the Thread'
@@ -182,12 +200,18 @@ test.describe('Discussions', () => {
         })
 
         await testAuth.step('Enter thread information', async () => {
-          await userPage.getByPlaceholder('Title').fill(threadTitle)
-          await getThreadTextbox(userPage).fill(threadBody)
+          const threadTitleInput = userPage.getByPlaceholder('Title')
+          await threadTitleInput.fill(threadTitle)
+          await expect(threadTitleInput).toHaveValue(threadTitle)
+
+          const threadTextbox = getThreadTextbox(userPage)
+          await threadTextbox.fill(threadBody)
+          await expect(threadTextbox).toHaveValue(threadBody)
         })
 
         await testAuth.step('Post new thread', async () => {
           await userPage.getByRole('button', { name: 'Post' }).click()
+          await dismissAlert(userPage, 'A new thread has been created.')
         })
 
         await expectThreadTableLoaded(
@@ -211,7 +235,12 @@ test.describe('Discussions', () => {
 
           await testAuth.step('Go to thread', async () => {
             await threadLink.click()
-            await expectDiscussionThreadLoaded(userPage, threadId)
+            await expectDiscussionThreadLoaded(
+              userPage,
+              threadId,
+              threadTitle,
+              threadBody,
+            )
           })
 
           await testAuth.step('Check thread info', async () => {
@@ -226,28 +255,32 @@ test.describe('Discussions', () => {
               }),
             ).toBeVisible()
             await expect(discussionThread.getByText('Moderator')).toBeVisible()
-            await expect(discussionThread.getByText(threadTitle)).toBeVisible()
-            await expect(discussionThread.getByText(threadBody)).toBeVisible()
           })
 
           return threadId
         },
       )
 
       await testAuth.step('View count is updated', async () => {
-        await userPage.getByRole('button', { name: 'Show All Threads' }).click()
-
-        // reload is necessary for view count to update
-        await userPage.reload()
+        await userPage
+          .getByRole('button', { name: /show all threads/i })
+          .click()
 
         await expectDiscussionPageLoaded(userPage, userProject.id)
-        await expectThreadTableLoaded(
-          userPage,
-          threadTitle,
-          userConfigs['swc-e2e-user'].username,
-          '0',
-          '1',
-        )
+
+        // 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 expectThreadTableLoaded(
+            userPage,
+            threadTitle,
+            userConfigs['swc-e2e-user'].username,
+            '0',
+            '1',
+          )
+        }).toPass()
       })
 
       await testAuth.step('Second user can view discussion', async () => {
@@ -264,7 +297,12 @@ test.describe('Discussions', () => {
 
       await testAuth.step('Second user can view thread', async () => {
         await validatedUserPage.getByRole('link', { name: threadTitle }).click()
-        await expectDiscussionThreadLoaded(validatedUserPage, threadId)
+        await expectDiscussionThreadLoaded(
+          validatedUserPage,
+          threadId,
+          threadTitle,
+          threadBody,
+        )
       })
 
       const secondUserReplyPostActions = await testAuth.step(
@@ -282,10 +320,18 @@ test.describe('Discussions', () => {
           })
 
           await testAuth.step('Post a reply', async () => {
-            await validatedUserPage
-              .getByRole('textbox', { name: 'Write a reply...' })
-              .click()
-            await getThreadTextbox(validatedUserPage).fill(threadReply)
+            const replyInput = discussionThread.getByRole('textbox', {
+              name: 'Write a reply...',
+            })
+
+            // A second reply input briefly appears when the network
+            // connection is slow. Wait for the second input to be hidden.
+            await expect(replyInput).toHaveCount(1)
+
+            await replyInput.click()
+            const threadTextbox = getThreadTextbox(validatedUserPage)
+            await threadTextbox.fill(threadReply)
+            await expect(threadTextbox).toHaveValue(threadReply)
             await validatedUserPage
               .getByRole('button', { name: 'Post', exact: true })
               .click()
@@ -347,7 +393,12 @@ test.describe('Discussions', () => {
         async () => {
           await testAuth.step('Go to discussion thread', async () => {
             await userPage.getByRole('link', { name: threadTitle }).click()
-            await expectDiscussionThreadLoaded(userPage, threadId)
+            await expectDiscussionThreadLoaded(
+              userPage,
+              threadId,
+              threadTitle,
+              threadBody,
+            )
           })
 
           await expectThreadReplyVisible(
@@ -394,6 +445,7 @@ test.describe('Discussions', () => {
           const threadTextbox = getThreadTextbox(validatedUserPage)
           await threadTextbox.clear()
           await threadTextbox.fill(threadReplyEdited)
+          await expect(threadTextbox).toHaveValue(threadReplyEdited)
           await validatedUserPage.getByRole('button', { name: 'Save' }).click()
         })
 

diff --git a/e2e/files.spec.ts b/e2e/files.spec.ts
@@ -114,9 +114,19 @@ const uploadFile = async (
         ? 'Upload or Link to a File'
         : 'Upload a New Version of File'
     await page.getByRole('button', { name: uploadButtonText }).click()
+
+    await expect(page.getByRole('link', { name: 'Upload File' })).toBeVisible()
+    await expect(page.getByRole('link', { name: 'Link to URL' })).toBeVisible()
+    await expect(page.getByRole('button', { name: 'Browse...' })).toBeVisible()
   })
 
   await testAuth.step('choose file', async () => {
+    // Ensure that upload type is specified before attempting upload
+    // to prevent upload error: "Unsupported external upload type specified: undefined"
+    await expect(
+      page.getByText(/all uploaded files will be stored in synapsestorage/i),
+    ).toBeVisible()
+
     const fileChooserPromise = page.waitForEvent('filechooser')
     await page.getByRole('button', { name: 'Browse...' }).click()
     if (uploadType === 'initialUpload') {
@@ -140,10 +150,7 @@ const uploadFile = async (
   await testAuth.step('ensure there was not an upload error', async () => {
     const uploadError = page.getByRole('heading', { name: 'Upload Error' })
     if (await uploadError.isVisible()) {
-      const uploadErrorText = await page
-        .getByRole('dialog')
-        .getByText('Unable to upload the file')
-        .textContent()
+      const uploadErrorText = await page.getByRole('dialog').textContent()
       throw new Error(`Upload Error: ${uploadErrorText}`)
     }
   })

diff --git a/e2e/helpers/teams.ts b/e2e/helpers/teams.ts
@@ -0,0 +1,22 @@
+import { Page } from '@playwright/test'
+import { BackendDestinationEnum, doDelete } from './http'
+
+const teamHashBang = '#!Team'
+
+export function getTeamIdFromPathname(pathname: string) {
+  if (!pathname.includes(teamHashBang)) {
+    return undefined
+  }
+
+  return pathname.replace(new RegExp(`.*${teamHashBang}:`), '')
+}
+
+// https://rest-docs.synapse.org/rest/DELETE/team/id.html
+export async function deleteTeam(
+  teamId: string,
+  accessToken: string,
+  page: Page,
+) {
+  const url = `/repo/v1/team/${teamId}`
+  await doDelete(page, url, accessToken, BackendDestinationEnum.REPO_ENDPOINT)
+}
diff --git a/e2e/teams.spec.ts b/e2e/teams.spec.ts
@@ -5,6 +5,7 @@ import {
   deleteTeamInvitationMessage,
   deleteTeamInviteAcceptanceMessage,
 } from './helpers/messages'
+import { deleteTeam, getTeamIdFromPathname } from './helpers/teams'
 import {
   getAccessTokenFromCookie,
   getAdminPAT,
@@ -19,6 +20,7 @@ import {
 
 const TEAM_NAME = 'swc-e2e-team-' + uuidv4()
 const INVITATION_MESSAGE = 'swc-e2e-invite'
+let teamId: string | undefined = undefined
 
 // Run multiple describes in parallel, but run tests inside each describe in order
 // ...tests within describe expect afterAll to be run with the same users, i.e. on the same worker
@@ -29,6 +31,8 @@ test.describe('Teams', () => {
   testAuth(
     'should exercise team lifecycle',
     async ({ userPage, validatedUserPage }, testInfo) => {
+      test.slow()
+
       const { userName, validatedUserName } = await testAuth.step(
         'should get user names',
         async () => {
@@ -61,6 +65,10 @@ test.describe('Teams', () => {
         },
       )
 
+      await testAuth.step('user should get teamId', async () => {
+        teamId = getTeamIdFromPathname(userPage.url())
+      })
+
       await testAuth.step(
         'user should invite validated user to team',
         async () => {
@@ -161,22 +169,26 @@ test.describe('Teams', () => {
           }),
         ).toBeVisible()
       })
+
+      await testAuth.step('user deletes team', async () => {
+        await goToDashboard(userPage)
+        await userPage.getByLabel('Teams').click()
+        await userPage.getByRole('link', { name: TEAM_NAME }).click()
+        await userPage.getByRole('button', { name: 'Team Actions' }).click()
+        await userPage.getByRole('link', { name: 'Delete Team' }).click()
+        await userPage.getByRole('button', { name: 'Delete' }).click()
+        await expect(
+          userPage.getByText('Team successfully deleted'),
+        ).toBeVisible()
+        await expect(
+          userPage.getByRole('link', { name: TEAM_NAME }),
+        ).not.toBeVisible()
+        teamId = undefined
+      })
     },
   )
 
   testAuth.afterAll(async ({ userPage, validatedUserPage }) => {
-    // delete team
-    await goToDashboard(userPage)
-    await userPage.getByLabel('Teams').click()
-    await userPage.getByRole('link', { name: TEAM_NAME }).click()
-    await userPage.getByRole('button', { name: 'Team Actions' }).click()
-    await userPage.getByRole('link', { name: 'Delete Team' }).click()
-    await userPage.getByRole('button', { name: 'Delete' }).click()
-    await expect(userPage.getByText('Team successfully deleted')).toBeVisible()
-    await expect(
-      userPage.getByRole('link', { name: TEAM_NAME }),
-    ).not.toBeVisible()
-
     // get credentials
     const adminPAT = getAdminPAT()
 
@@ -190,6 +202,11 @@ test.describe('Teams', () => {
     const userName = userConfigs[userPrefix].username
     expect(userName).not.toBeNull()
 
+    // delete team if it was created but not cleaned up during the test
+    if (teamId) {
+      await deleteTeam(teamId, userAccessToken, userPage)
+    }
+
     // delete team invitation: user -> validated user
     await deleteTeamInvitationMessage(
       [validatedUserId],

diff --git a/e2e_workflow/README.md b/e2e_workflow/README.md
@@ -110,6 +110,12 @@ await cdpSession.send('Network.emulateNetworkConditions', {
 await cdpSession.send('Emulation.setCPUThrottlingRate', { rate: 3 })
 ```
 
+Alternately, manually run the test with network throttling (e.g. Slow 3G) applied to the browser. This can be useful for identifying states that occur when there is a slow network connection.
+
+#### Repeats
+
+A flaky test in CI can be difficult to reproduce locally. However, running the test many times in a row (via Playwright's `--repeat-each` flag) can help reveal the flake.
+
 #### Hardware Resources
 
 A developer's local machine may have more powerful hardware resources compared to GitHub Actions workflow runners. If a VM is overbooked, tests may fail. Alternately, the remote machine may be more powerful than the local machine.