From 8234aecdc7ce4474908b34c60044da9096b668c5 Mon Sep 17 00:00:00 2001 From: Hallie Swan <26949006+hallieswan@users.noreply.github.com> Date: Wed, 25 Oct 2023 11:18:29 -0700 Subject: [PATCH 1/7] SWC-6556: always run playwright tests on macos runner so environment is consistent between forks and in Sage repo, move conditional to first job so that subsequent jobs are skipped when first job is skipped --- .github/workflows/build-test-e2e.yml | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build-test-e2e.yml b/.github/workflows/build-test-e2e.yml index fa1be15d1c..8ceab32315 100644 --- 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,9 +24,8 @@ 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' }} + runs-on: 'macos-latest' timeout-minutes: 60 strategy: fail-fast: false @@ -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: From ddfc8ed389347ab44a3751d483d07fa42c211209 Mon Sep 17 00:00:00 2001 From: Hallie Swan <26949006+hallieswan@users.noreply.github.com> Date: Thu, 26 Oct 2023 11:40:47 -0700 Subject: [PATCH 2/7] SWC-6556: continue using macos-latest runner until usage limit is investigated --- .github/workflows/build-test-e2e.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build-test-e2e.yml b/.github/workflows/build-test-e2e.yml index 8ceab32315..21f840395a 100644 --- a/.github/workflows/build-test-e2e.yml +++ b/.github/workflows/build-test-e2e.yml @@ -25,7 +25,7 @@ jobs: retention-days: 1 playwright-tests: needs: [build] - runs-on: 'macos-latest' + runs-on: ${{ github.repository_owner == 'Sage-Bionetworks' && 'ubuntu-22.04-4core-16GBRAM-150GBSSD' || 'macos-latest' }} timeout-minutes: 60 strategy: fail-fast: false From c052c25a4261b47469c334e368a1ff3c222a2092 Mon Sep 17 00:00:00 2001 From: Hallie Swan <26949006+hallieswan@users.noreply.github.com> Date: Thu, 26 Oct 2023 15:36:12 -0700 Subject: [PATCH 3/7] SWC-6556: prevent intermittent error where file upload is attempted before upload type is specified --- e2e/files.spec.ts | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/e2e/files.spec.ts b/e2e/files.spec.ts index c9083f9f0c..133ae3c213 100644 --- 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}`) } }) From f35e58b0b884cbe44034aaba06a7c1bb70c3962e Mon Sep 17 00:00:00 2001 From: Hallie Swan <26949006+hallieswan@users.noreply.github.com> Date: Thu, 26 Oct 2023 18:08:20 -0700 Subject: [PATCH 4/7] SWC-6560: add more specific and additional checks to hopefully reduce flake --- e2e/discussions.spec.ts | 53 ++++++++++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 16 deletions(-) diff --git a/e2e/discussions.spec.ts b/e2e/discussions.spec.ts index f8a84864f2..2cc14534b5 100644 --- 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() }) } @@ -77,8 +78,12 @@ const expectDiscussionThreadLoaded = async (page: Page, threadId: number) => { `${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() @@ -163,6 +168,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 +189,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( @@ -235,19 +248,24 @@ test.describe('Discussions', () => { ) 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', - ) + + 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 () => { @@ -282,10 +300,12 @@ test.describe('Discussions', () => { }) await testAuth.step('Post a reply', async () => { - await validatedUserPage + await discussionThread .getByRole('textbox', { name: 'Write a reply...' }) .click() - await getThreadTextbox(validatedUserPage).fill(threadReply) + const threadTextbox = getThreadTextbox(validatedUserPage) + await threadTextbox.fill(threadReply) + await expect(threadTextbox).toHaveValue(threadReply) await validatedUserPage .getByRole('button', { name: 'Post', exact: true }) .click() @@ -394,6 +414,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() }) From af1c9fc44c6ddb3f7cb4fa874fba5cc6e302aa6a Mon Sep 17 00:00:00 2001 From: Hallie Swan <26949006+hallieswan@users.noreply.github.com> Date: Thu, 26 Oct 2023 18:10:23 -0700 Subject: [PATCH 5/7] SWC-6554: delete team via API, if not cleaned up during test --- e2e/helpers/teams.ts | 22 ++++++++++++++++++++++ e2e/teams.spec.ts | 39 +++++++++++++++++++++++++++------------ 2 files changed, 49 insertions(+), 12 deletions(-) create mode 100644 e2e/helpers/teams.ts diff --git a/e2e/helpers/teams.ts b/e2e/helpers/teams.ts new file mode 100644 index 0000000000..698c19902d --- /dev/null +++ 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 index 4e11feb8d1..014466f294 100644 --- 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 @@ -61,6 +63,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 +167,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 +200,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], From 99f4821e7ca1ca6a55ac6d7cfa4bc7a6f8133dff Mon Sep 17 00:00:00 2001 From: Hallie Swan <26949006+hallieswan@users.noreply.github.com> Date: Fri, 27 Oct 2023 10:01:46 -0700 Subject: [PATCH 6/7] SWC-6560: wait for discussion thread to load, handle state that appears when network connection is slow --- e2e/discussions.spec.ts | 49 +++++++++++++++++++++++++++++++++-------- e2e_workflow/README.md | 6 +++++ 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/e2e/discussions.spec.ts b/e2e/discussions.spec.ts index 2cc14534b5..c945c34343 100644 --- a/e2e/discussions.spec.ts +++ b/e2e/discussions.spec.ts @@ -72,7 +72,12 @@ 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}`, @@ -90,6 +95,12 @@ const expectDiscussionThreadLoaded = async (page: Page, threadId: number) => { 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, + }) }) } @@ -224,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 () => { @@ -239,8 +255,6 @@ test.describe('Discussions', () => { }), ).toBeVisible() await expect(discussionThread.getByText('Moderator')).toBeVisible() - await expect(discussionThread.getByText(threadTitle)).toBeVisible() - await expect(discussionThread.getByText(threadBody)).toBeVisible() }) return threadId @@ -254,6 +268,7 @@ test.describe('Discussions', () => { await expectDiscussionPageLoaded(userPage, userProject.id) + // retry if the view count hasn't updated await expect(async () => { // reload is necessary for view count to update await userPage.reload() @@ -282,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( @@ -300,9 +320,15 @@ test.describe('Discussions', () => { }) await testAuth.step('Post a reply', async () => { - await discussionThread - .getByRole('textbox', { name: 'Write a reply...' }) - .click() + 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) @@ -367,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( diff --git a/e2e_workflow/README.md b/e2e_workflow/README.md index 658bf9dbec..9b47b0bea3 100644 --- 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. From cb2a4796cc3dd5d114ac408b71866254e925a4e7 Mon Sep 17 00:00:00 2001 From: Hallie Swan <26949006+hallieswan@users.noreply.github.com> Date: Fri, 27 Oct 2023 10:41:50 -0700 Subject: [PATCH 7/7] SWC-6554: mark teams test as slow to handle intermittent test timeout --- e2e/teams.spec.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/e2e/teams.spec.ts b/e2e/teams.spec.ts index 014466f294..e2d27c5ced 100644 --- a/e2e/teams.spec.ts +++ b/e2e/teams.spec.ts @@ -31,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 () => {