From 0f1d4a2a91af1251847e3f0a42acc99361959328 Mon Sep 17 00:00:00 2001 From: Cornelius Roemer Date: Sun, 12 May 2024 17:26:55 +0200 Subject: [PATCH 01/17] Clearer error messages --- website/tests/pages/review/index.spec.ts | 13 +++++------- website/tests/pages/review/review.page.ts | 22 ++++++++++++++------- website/tests/util/preprocessingPipeline.ts | 14 ++++++++++++- 3 files changed, 33 insertions(+), 16 deletions(-) diff --git a/website/tests/pages/review/index.spec.ts b/website/tests/pages/review/index.spec.ts index 55412748a4..65db76f0cd 100644 --- a/website/tests/pages/review/index.spec.ts +++ b/website/tests/pages/review/index.spec.ts @@ -2,6 +2,7 @@ import { test, testSequenceCount } from '../../e2e.fixture'; import { submitViaApi } from '../../util/backendCalls.ts'; import { prepareDataToBe } from '../../util/prepareDataToBe.ts'; +// This test must not be run in parallel with other tests that submit, approve or delete sequences. test.describe('The review page', () => { test('should show the total sequences and an increase when new submission occurs', async ({ reviewPage, @@ -15,9 +16,7 @@ test.describe('The review page', () => { await submitViaApi(testSequenceCount, token, groupId); - await reviewPage.waitForTotalSequencesFulfillPredicate( - (totalSequenceCount) => totalSequenceCount === total + testSequenceCount, - ); + await reviewPage.waitForTotalSequenceCountCorrect(total + testSequenceCount); }); test('should allow bulk approval', async ({ reviewPage, loginAsTestUser }) => { @@ -29,13 +28,11 @@ test.describe('The review page', () => { await prepareDataToBe('awaitingApproval', token, groupId); - await reviewPage.waitForTotalSequencesFulfillPredicate( - (totalSequenceCount) => totalSequenceCount === total + testSequenceCount, - ); + await reviewPage.waitForTotalSequenceCountCorrect(total + testSequenceCount); await reviewPage.approveAll(); - await reviewPage.waitForTotalSequencesFulfillPredicate((totalSequenceCount) => totalSequenceCount === total); + await reviewPage.waitForTotalSequenceCountCorrect(total); }); test('should allow bulk deletion', async ({ reviewPage, loginAsTestUser }) => { @@ -49,6 +46,6 @@ test.describe('The review page', () => { await reviewPage.deleteAll(); - await reviewPage.waitForTotalSequencesFulfillPredicate((totalSequenceCount) => totalSequenceCount < total); + await reviewPage.waitForTotalSequenceCountCorrect(total, 'less'); }); }); diff --git a/website/tests/pages/review/review.page.ts b/website/tests/pages/review/review.page.ts index 6cbd12da85..d5fc53b897 100644 --- a/website/tests/pages/review/review.page.ts +++ b/website/tests/pages/review/review.page.ts @@ -53,21 +53,29 @@ export class ReviewPage { } } - public async waitForTotalSequencesFulfillPredicate( - predicate: (totalSequenceCount: number) => boolean, + public async waitForTotalSequenceCountCorrect( + expectedTotal: number, + comparator: 'less' | 'equal' = 'equal', retries: number = 10, delayInSeconds: number = 1, ) { + let currentTotal; for (let i = 0; i < retries; i++) { await new Promise((resolve) => setTimeout(resolve, delayInSeconds * 1000)); - const { total: currentTotal } = await this.getReviewPageOverview(); - if (predicate(currentTotal)) { - return true; + currentTotal = (await this.getReviewPageOverview()).total; + switch (comparator) { + case 'equal': + if (currentTotal === expectedTotal) { + return true; + } + case 'less': + if (currentTotal < expectedTotal) { + return true; + } } } - - throw new Error(`Waiting for total count of sequences to match predicate, but total did not match predicate`); + throw new Error(`Sequence count ${currentTotal} not ${comparator} expected count ${expectedTotal}`); } public async approveAll() { diff --git a/website/tests/util/preprocessingPipeline.ts b/website/tests/util/preprocessingPipeline.ts index ee9e92f130..52b2c3f05d 100644 --- a/website/tests/util/preprocessingPipeline.ts +++ b/website/tests/util/preprocessingPipeline.ts @@ -84,7 +84,19 @@ async function query(numberOfSequenceEntries: number): Promise line.length > 0) .map((line: string): UnprocessedData => { - return JSON.parse(line) as UnprocessedData; + try { + return JSON.parse(line) as UnprocessedData; + } catch (error) { + if (error instanceof SyntaxError) { + throw new Error( + 'Invalid JSON syntax error: ' + + error.message + + ' for response:\n ' + + unprocessedDataAsNdjson, + ); + } + throw error; + } }); }, (error) => { From 40d8dbc4949475c4bbc86fc35df0ba8fc15e1f9a Mon Sep 17 00:00:00 2001 From: Cornelius Roemer Date: Sun, 12 May 2024 20:42:24 +0200 Subject: [PATCH 02/17] Deflake seqset tests --- .../tests/pages/seqsets/[seqSetId].spec.ts | 14 +++++------ website/tests/pages/seqsets/index.spec.ts | 4 ++-- website/tests/pages/seqsets/seqset.page.ts | 23 +++++++------------ 3 files changed, 17 insertions(+), 24 deletions(-) diff --git a/website/tests/pages/seqsets/[seqSetId].spec.ts b/website/tests/pages/seqsets/[seqSetId].spec.ts index 977913fadb..6b2a2e519d 100644 --- a/website/tests/pages/seqsets/[seqSetId].spec.ts +++ b/website/tests/pages/seqsets/[seqSetId].spec.ts @@ -40,7 +40,7 @@ test.describe('The seqSet item page', () => { await seqSetPage.gotoDetail(testSeqSetName); await expect(seqSetPage.page.getByRole('button', { name: 'Export' })).toBeVisible(); await seqSetPage.page.getByRole('button', { name: 'Export' }).click(); - await seqSetPage.waitForLoad(); + await seqSetPage.page.waitForLoadState(); await seqSetPage.page.getByTestId('json-radio').waitFor(); await seqSetPage.page.getByTestId('json-radio').click(); @@ -56,7 +56,7 @@ test.describe('The seqSet item page', () => { await seqSetPage.gotoDetail(testSeqSetName); await expect(seqSetPage.page.getByRole('button', { name: 'Export' })).toBeVisible(); await seqSetPage.page.getByRole('button', { name: 'Export' }).click(); - await seqSetPage.waitForLoad(); + await seqSetPage.page.waitForLoadState(); await seqSetPage.page.getByTestId('tsv-radio').waitFor(); await seqSetPage.page.getByTestId('tsv-radio').click(); @@ -72,11 +72,11 @@ test.describe('The seqSet item page', () => { await seqSetPage.gotoDetail(testSeqSetName); await expect(seqSetPage.page.getByRole('button', { name: 'Delete' })).toBeVisible(); await seqSetPage.page.getByRole('button', { name: 'Delete' }).click(); - await seqSetPage.waitForLoad(); + await seqSetPage.page.waitForLoadState(); await expect(async () => { await seqSetPage.page.getByRole('button', { name: 'Cancel' }).click(); - await seqSetPage.waitForLoad(); + await seqSetPage.page.waitForLoadState(); await expect(seqSetPage.page.getByRole('heading', { name: testSeqSetName })).toBeVisible(); }).toPass(); }); @@ -86,14 +86,14 @@ test.describe('The seqSet item page', () => { await seqSetPage.gotoDetail(testSeqSetName); await expect(seqSetPage.page.getByRole('button', { name: 'Edit' })).toBeVisible(); await seqSetPage.page.getByRole('button', { name: 'Edit' }).click(); - await seqSetPage.waitForLoad(); + await seqSetPage.page.waitForLoadState(); await expect(async () => { await expect(seqSetPage.page.getByText('Edit SeqSet')).toBeVisible(); await seqSetPage.page.locator('#seqSet-name').fill(editSeqSetName); await seqSetPage.page.getByRole('button', { name: 'Save' }).click(); - await seqSetPage.waitForLoad(); - await expect(seqSetPage.page.getByText(editSeqSetName)).toBeVisible(); + await seqSetPage.page.waitForLoadState(); + await expect(await seqSetPage.page.getByText(editSeqSetName).locator('visible=true')).toBeVisible(); }).toPass(); await seqSetPage.deleteTestSeqSet(editSeqSetName); diff --git a/website/tests/pages/seqsets/index.spec.ts b/website/tests/pages/seqsets/index.spec.ts index b4aa5e7e20..f40a3fbecc 100644 --- a/website/tests/pages/seqsets/index.spec.ts +++ b/website/tests/pages/seqsets/index.spec.ts @@ -31,7 +31,7 @@ test.describe('The seqSets list page', () => { await expect(async () => { await seqSetPage.page.getByTestId('AddIcon').click(); - await seqSetPage.waitForLoad(); + await seqSetPage.page.waitForLoadState(); await expect(seqSetPage.page.getByText('Create a SeqSet')).toBeVisible(); }).toPass(); }); @@ -45,7 +45,7 @@ test.describe('The seqSets list page', () => { await seqSetPage.gotoList(); await expect(async () => { await seqSetPage.page.getByText(testSeqSetName).first().click(); - await seqSetPage.waitForLoad(); + await seqSetPage.page.waitForLoadState(); await expect(seqSetPage.page.getByRole('heading', { name: testSeqSetName })).toBeVisible(); }).toPass(); }); diff --git a/website/tests/pages/seqsets/seqset.page.ts b/website/tests/pages/seqsets/seqset.page.ts index 1a3e73e44c..24cd8e8def 100644 --- a/website/tests/pages/seqsets/seqset.page.ts +++ b/website/tests/pages/seqsets/seqset.page.ts @@ -7,32 +7,25 @@ export class SeqSetPage { constructor(public readonly page: Page) {} public async gotoList() { - await this.page.goto(`${baseUrl}/seqsets`, { waitUntil: 'load' }); - await this.waitForLoad(); + await this.page.goto(`${baseUrl}/seqsets`); } public async gotoDetail(seqSetName: string = testSeqSet.name) { await this.gotoList(); await this.page.getByText(seqSetName).first().click(); - await this.waitForLoad(); - } - - public async waitForLoad() { - await this.page.waitForLoadState('domcontentloaded', { timeout: 15000 }); - await this.page.waitForLoadState('load', { timeout: 30000 }); - await this.page.waitForLoadState('networkidle', { timeout: 5000 }); + await this.page.waitForLoadState(); } public async createTestSeqSet(seqSetName: string = testSeqSet.name) { await this.gotoList(); await this.page.getByTestId('AddIcon').waitFor(); await this.page.getByTestId('AddIcon').click(); - await this.page.locator('#seqSet-name').fill(seqSetName); - await this.page.locator('#seqSet-description').fill(testSeqSet.description); - await this.page.locator('#loculus-focal-accession-input').fill(testSeqSet.focalLoculusAccessions); - await this.page.locator('#loculus-background-accession-input').fill(testSeqSet.backgroundLoculusAccessions); + await this.page.getByLabel('SeqSet name').fill(seqSetName); + await this.page.getByLabel('SeqSet description').fill(testSeqSet.description); + await this.page.getByLabel('Focal accessions').fill(testSeqSet.focalLoculusAccessions); + await this.page.getByLabel('Background accessions').fill(testSeqSet.backgroundLoculusAccessions); await this.page.getByRole('button', { name: 'Save' }).click(); - await this.waitForLoad(); + await this.page.waitForLoadState(); } public async deleteTestSeqSet(seqSetName: string = testSeqSet.name) { @@ -40,6 +33,6 @@ export class SeqSetPage { await this.page.getByText(seqSetName).first().click(); await this.page.getByRole('button', { name: 'Delete' }).click(); await this.page.getByRole('button', { name: 'Confirm' }).click(); - await this.waitForLoad(); + await this.page.waitForLoadState(); } } From e1f318257e1a4c109958984d7e5bb934e17e447a Mon Sep 17 00:00:00 2001 From: Cornelius Roemer Date: Sun, 12 May 2024 20:59:42 +0200 Subject: [PATCH 03/17] Allow triggering of all browsers manually --- .github/workflows/e2e-k3d.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/e2e-k3d.yml b/.github/workflows/e2e-k3d.yml index 98537b8179..5f1cc5c563 100644 --- a/.github/workflows/e2e-k3d.yml +++ b/.github/workflows/e2e-k3d.yml @@ -2,6 +2,12 @@ name: E2E test (on kubernetes) on: workflow_dispatch: + inputs: + all_browsers: + description: "Run tests on all browsers" + required: false + default: false + type: boolean pull_request: paths: - "backend/**" @@ -31,7 +37,7 @@ jobs: timeout-minutes: 30 env: - ALL_BROWSERS: ${{ github.ref == 'refs/heads/main' && 'true' || 'false' }} + ALL_BROWSERS: ${{ github.ref == 'refs/heads/main' || github.event.inputs.all_browsers && 'true' || 'false' }} steps: - name: Collect Workflow Telemetry From e88c9f2d2a60eb8ea92998166abb347a7627e673 Mon Sep 17 00:00:00 2001 From: Cornelius Roemer Date: Sun, 12 May 2024 21:36:00 +0200 Subject: [PATCH 04/17] Use correct sha --- .github/workflows/e2e-k3d.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/e2e-k3d.yml b/.github/workflows/e2e-k3d.yml index 5f1cc5c563..382399f2df 100644 --- a/.github/workflows/e2e-k3d.yml +++ b/.github/workflows/e2e-k3d.yml @@ -59,7 +59,7 @@ jobs: - name: Template with helm uses: WyriHaximus/github-action-helm3@v4 with: - exec: ./deploy.py --verbose helm --branch ${{ github.ref_name }} --sha ${{ github.sha }} --for-e2e --template > /tmp/helm_template.yaml + exec: ./deploy.py --verbose helm --branch ${{ github.ref_name }} --sha ${{ github.sha }} --for-e2e --template > /tmp/helm_template.yaml - name: Upload default helm template uses: actions/upload-artifact@v4 @@ -67,8 +67,10 @@ jobs: name: helm-template path: /tmp/helm_template.yaml - name: Deploy with helm + env: + SHA_ARG: --sha ${{ github.sha }} run: | - ./deploy.py --verbose helm --branch ${{ github.ref_name }} --sha ${{ github.sha }} --for-e2e + ./deploy.py --verbose helm --branch ${{ github.ref_name }} ${{ github.event_name != 'workflow_dispatch' && env.SHA_ARG || '' }} --for-e2e - uses: actions/setup-node@v4 with: From 791deac51daf7cedee71aac0593a712c725b32db Mon Sep 17 00:00:00 2001 From: Cornelius Roemer Date: Sun, 12 May 2024 22:27:33 +0200 Subject: [PATCH 05/17] Make seqset tests more robust --- .../components/SeqSetCitations/SeqSetItemActions.tsx | 12 +++++++++--- .../src/components/SeqSetCitations/SeqSetList.tsx | 8 +++++++- website/tests/pages/seqsets/seqset.page.ts | 2 +- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/website/src/components/SeqSetCitations/SeqSetItemActions.tsx b/website/src/components/SeqSetCitations/SeqSetItemActions.tsx index 9421a93efc..3a554e90aa 100644 --- a/website/src/components/SeqSetCitations/SeqSetItemActions.tsx +++ b/website/src/components/SeqSetCitations/SeqSetItemActions.tsx @@ -1,4 +1,4 @@ -import { type FC, useState } from 'react'; +import { type FC, useState, useEffect } from 'react'; import { ExportSeqSet } from './ExportSeqSet'; import { SeqSetForm } from './SeqSetForm'; @@ -32,6 +32,11 @@ const SeqSetItemActionsInner: FC = ({ const [editModalVisible, setEditModalVisible] = useState(false); const [exportModalVisible, setExportModalVisible] = useState(false); const { errorMessage, isErrorOpen, openErrorFeedback, closeErrorFeedback } = useErrorFeedbackState(); + const [isClient, setIsClient] = useState(false); + + useEffect(() => { + setIsClient(true); + }, []); const { mutate: deleteSeqSet } = useDeleteSeqSetAction( clientConfig, @@ -51,13 +56,13 @@ const SeqSetItemActionsInner: FC = ({
-
{isAdminView ? ( - ) : null} @@ -72,6 +77,7 @@ const SeqSetItemActionsInner: FC = ({ onConfirmation: handleDeleteSeqSet, }) } + disabled={!isClient} > Delete diff --git a/website/src/components/SeqSetCitations/SeqSetList.tsx b/website/src/components/SeqSetCitations/SeqSetList.tsx index 94b58dd4ec..a5b149ccba 100644 --- a/website/src/components/SeqSetCitations/SeqSetList.tsx +++ b/website/src/components/SeqSetCitations/SeqSetList.tsx @@ -1,5 +1,5 @@ import MUIPagination from '@mui/material/Pagination'; -import { type FC, type MouseEvent, useState, useMemo } from 'react'; +import { type FC, type MouseEvent, useState, useMemo, useEffect } from 'react'; import { routes } from '../../routes/routes'; import type { SeqSet } from '../../types/seqSetCitation'; @@ -85,8 +85,13 @@ export const SeqSetList: FC = ({ seqSets, username }) => { const [order, setOrder] = useState('desc'); const [orderBy, setOrderBy] = useState('createdAt'); const [page, setPage] = useState(1); + const [isClient, setIsClient] = useState(false); const rowsPerPage = 5; + useEffect(() => { + setIsClient(true); + }, []); + const handleRequestSort = (_: MouseEvent, property: keyof SeqSet) => { const isAsc = orderBy === property && order === 'asc'; setOrder(isAsc ? 'desc' : 'asc'); @@ -174,6 +179,7 @@ export const SeqSetList: FC = ({ seqSets, username }) => { className='hover:bg-primary-100 border-gray-100 cursor-pointer' onClick={(event) => handleClick(event, row.seqSetId, row.seqSetVersion.toString())} key={row.seqSetId} + data-testid={isClient ? row.name : 'disabled'} > {formatDate(row.createdAt)} diff --git a/website/tests/pages/seqsets/seqset.page.ts b/website/tests/pages/seqsets/seqset.page.ts index 24cd8e8def..89dcd029cd 100644 --- a/website/tests/pages/seqsets/seqset.page.ts +++ b/website/tests/pages/seqsets/seqset.page.ts @@ -12,7 +12,7 @@ export class SeqSetPage { public async gotoDetail(seqSetName: string = testSeqSet.name) { await this.gotoList(); - await this.page.getByText(seqSetName).first().click(); + await this.page.getByTestId(seqSetName).first().click(); await this.page.waitForLoadState(); } From ba55450c23ae33527c3c4c488703a6457a9be3f1 Mon Sep 17 00:00:00 2001 From: Cornelius Roemer Date: Sun, 12 May 2024 22:42:23 +0200 Subject: [PATCH 06/17] Deflake search button test --- .../src/components/SearchPage/SearchForm.spec.tsx | 6 ++---- website/src/components/SearchPage/SearchForm.tsx | 14 ++++++++++---- website/tests/pages/search/index.spec.ts | 2 +- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/website/src/components/SearchPage/SearchForm.spec.tsx b/website/src/components/SearchPage/SearchForm.spec.tsx index 3ce049ec00..5fbfd2130b 100644 --- a/website/src/components/SearchPage/SearchForm.spec.tsx +++ b/website/src/components/SearchPage/SearchForm.spec.tsx @@ -3,7 +3,7 @@ import { render, screen, act } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { beforeEach, describe, expect, test, vi } from 'vitest'; -import { SearchForm } from './SearchForm'; +import { SearchForm, searchButtonText } from './SearchForm'; import { testConfig, testOrganism } from '../../../vitest.setup.ts'; import { routes, SEARCH } from '../../routes/routes.ts'; import type { MetadataFilter } from '../../types/config.ts'; @@ -20,8 +20,6 @@ global.ResizeObserver = class FakeResizeObserver { unobserve() {} }; -const searchButtonText = 'Search sequences'; - vi.mock('../../config', () => ({ fetchAutoCompletion: vi.fn().mockResolvedValue([]), getLapisUrl: vi.fn().mockReturnValue('lapis.dummy.url'), @@ -173,7 +171,7 @@ describe('SearchForm', () => { await userEvent.type(dateField, '2025'); - await userEvent.click(screen.getByRole('button', { name: 'Search sequences' })); + await userEvent.click(screen.getByRole('button', { name: searchButtonText })); expect(window.location.href).toContain(`${dateFieldName}=2025-01-25`); }); diff --git a/website/src/components/SearchPage/SearchForm.tsx b/website/src/components/SearchPage/SearchForm.tsx index ac5f84ac26..eb02f1515d 100644 --- a/website/src/components/SearchPage/SearchForm.tsx +++ b/website/src/components/SearchPage/SearchForm.tsx @@ -1,7 +1,7 @@ import CircularProgress from '@mui/material/CircularProgress'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { sentenceCase } from 'change-case'; -import { type FC, type FormEventHandler, useMemo, useState, useCallback } from 'react'; +import { type FC, type FormEventHandler, useMemo, useState, useCallback, useEffect } from 'react'; import { CustomizeModal } from './CustomizeModal.tsx'; import { AccessionField } from './fields/AccessionField.tsx'; @@ -61,6 +61,10 @@ export const SearchForm: FC = ({ const [isLoading, setIsLoading] = useState(false); const { isOpen: isMobileOpen, close: closeOnMobile, toggle: toggleMobileOpen } = useOffCanvas(); const [isCustomizeModalOpen, setIsCustomizeModalOpen] = useState(false); + const [isClient, setIsClient] = useState(false); + useEffect(() => { + setIsClient(true); + }, []); const handleFieldChange = useCallback( (metadataName: string, filter: string) => { @@ -234,7 +238,7 @@ export const SearchForm: FC = ({ style={{ background: 'linear-gradient(to bottom, transparent, white)' }} />
- +
@@ -274,9 +278,11 @@ const SearchField: FC = (props) => { } }; -const SearchButton: FC<{ isLoading: boolean }> = ({ isLoading }) => ( +export const searchButtonText = 'Search sequences'; + +const SearchButton: FC<{ isLoading: boolean; isClient: boolean }> = ({ isLoading }) => ( ); diff --git a/website/tests/pages/search/index.spec.ts b/website/tests/pages/search/index.spec.ts index c2a1a43a2a..7d3b91f3ab 100644 --- a/website/tests/pages/search/index.spec.ts +++ b/website/tests/pages/search/index.spec.ts @@ -47,7 +47,7 @@ test.describe('The search page', () => { await expect(rowLocator.getByText('B.1.1.7')).toBeVisible(); await accessionLink.click(); - await expect(searchPage.page.getByText('Amino acid mutations')).toBeVisible(); + await expect(searchPage.page.getByText('Amino acid mutations')).toBeVisible({ timeout: 30000 }); }); test('should search a few sequence entries by accession', async ({ searchPage }) => { From 1d161546aa4ace9b9bbeea48406be9b1a59791ef Mon Sep 17 00:00:00 2001 From: Cornelius Roemer Date: Mon, 13 May 2024 00:45:32 +0200 Subject: [PATCH 07/17] fix #1887 This isn't a real fix. I don't know why the `field.label` value switched between SSR and client Super weird. Suggests there is something else going on here. --- website/src/components/SearchPage/SearchForm.tsx | 4 +--- website/src/components/SearchPage/fields/DateField.tsx | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/website/src/components/SearchPage/SearchForm.tsx b/website/src/components/SearchPage/SearchForm.tsx index eb02f1515d..4f465d921a 100644 --- a/website/src/components/SearchPage/SearchForm.tsx +++ b/website/src/components/SearchPage/SearchForm.tsx @@ -127,9 +127,7 @@ export const SearchForm: FC = ({ if (field.grouped === true) { return (
-

- {field.displayName !== undefined ? field.displayName : field.label} -

+

{field.displayName}

{field.groupedFields.map((groupedField) => ( = ({ return (
-