From 576d78c7b066962539ba04d3d51789d1d68c7d34 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Thu, 30 Jul 2026 12:48:13 +0200 Subject: [PATCH] test: resolve potential flaky Playwright tests - UnifiedSearchPage: expose the popover's polite status region and add waitForSettledResults(), which waits for the settled result count. That is the component's own "all providers answered" signal, so no arbitrary delay is needed. activeOptionId() asserts the attribute before reading it so a mid-flight list cannot yield null. - header-unified-search: the three selection-dependent tests settle the search before reading the selection; Enter's navigation is awaited with a pre-armed waitForURL, since the /f/ redirect can outlive the default assertion timeout. - admin-settings-limit-to-same-group: create the shared groups once in beforeAll instead of per test, moving two occ round-trips out of each test's timeout. - FilesListPage: bound the previously unbounded waits, so a row or list that never renders fails fast and names the missing element instead of surfacing as a bare test timeout. - playwright.config: allow 1.5x the default test timeout on CI, where the server is shared and every step is slower. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Ferdinand Thiessen --- playwright.config.ts | 5 +- .../e2e/core/header-unified-search.spec.ts | 27 +++++--- ...admin-settings-limit-to-same-group.spec.ts | 64 ++++++++++--------- .../support/sections/FilesListPage.ts | 12 +++- .../support/sections/UnifiedSearchPage.ts | 42 ++++++++++++ 5 files changed, 106 insertions(+), 44 deletions(-) diff --git a/playwright.config.ts b/playwright.config.ts index ef6258fa715e0..079c33ef445b3 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -11,6 +11,7 @@ export default defineConfig({ forbidOnly: !!process.env.CI, retries: process.env.CI ? 1 : 0, workers: process.env.CI ? 1 : undefined, + timeout: process.env.CI ? 45_000 : undefined, // on CI allow 1.5x the default timeout to compensate for shared server resources reporter: process.env.CI ? [['blob'], ['dot'], ['github']] : 'html', use: { baseURL: 'http://localhost:8042/index.php/', @@ -57,10 +58,10 @@ export default defineConfig({ stdout: 'pipe', gracefulShutdown: { signal: 'SIGTERM', - timeout: 10000, + timeout: 10_000, }, reuseExistingServer: !process.env.CI, - timeout: 5 * 60 * 1000, + timeout: 300_000, wait: { stdout: /Nextcloud container ready to run Playwright tests/, }, diff --git a/tests/playwright/e2e/core/header-unified-search.spec.ts b/tests/playwright/e2e/core/header-unified-search.spec.ts index 452d4fc86d877..a7cd2cd0414de 100644 --- a/tests/playwright/e2e/core/header-unified-search.spec.ts +++ b/tests/playwright/e2e/core/header-unified-search.spec.ts @@ -48,6 +48,9 @@ test.describe('Header: unified search keyboard navigation', () => { test('typing auto-selects the first result', async ({ page }) => { const search = new UnifiedSearchPage(page) await search.input().fill(TOKEN) + // A row rendering is not enough: a provider settling later re-renders the + // list and re-establishes the selection, so read it only once it is final. + await search.waitForSettledResults() const firstOption = search.options().first() await expect(firstOption).toBeVisible() @@ -60,37 +63,43 @@ test.describe('Header: unified search keyboard navigation', () => { test('arrow keys move the selection while focus stays in the input', async ({ page }) => { const search = new UnifiedSearchPage(page) await search.input().fill(TOKEN) + await search.waitForSettledResults() // Need at least two rows to have somewhere to move to. await expect(search.options().nth(1)).toBeVisible() - await expect(search.input()).toHaveAttribute('aria-activedescendant', /\w/) - const firstId = await search.input().getAttribute('aria-activedescendant') + const firstId = await search.activeOptionId() await page.keyboard.press('ArrowDown') // Selection advanced to another row and the input never lost focus. - await expect(search.input()).not.toHaveAttribute('aria-activedescendant', firstId!) + await expect(search.input()).not.toHaveAttribute('aria-activedescendant', firstId) await expect(search.input()).toBeFocused() await page.keyboard.press('ArrowUp') - await expect(search.input()).toHaveAttribute('aria-activedescendant', firstId!) + await expect(search.input()).toHaveAttribute('aria-activedescendant', firstId) await expect(search.input()).toBeFocused() }) test('Enter opens the selected result', async ({ page }) => { const search = new UnifiedSearchPage(page) await search.input().fill(TOKEN) - await expect(search.options().first()).toBeVisible() + // Enter activates whatever is selected *at that moment* and does nothing at + // all when the list is mid-flight (no selection), so the search has to have + // settled before the key is pressed. + await search.waitForSettledResults() - const activeId = await search.input().getAttribute('aria-activedescendant') + const activeId = await search.activeOptionId() // The row is an NcListItem link to the file's short URL (/f/), which the // server redirects into the Files viewer. Grab the id and assert we land on it. - const href = await search.option(activeId!).getByRole('link').getAttribute('href') + const href = await search.option(activeId).getByRole('link').getAttribute('href') const fileId = href?.match(/\/f\/(\d+)/)?.[1] expect(fileId).toBeTruthy() + // Arm the wait before the key press: the URL is only reached after a + // server-side redirect, which outlives the default assertion timeout on a + // loaded CI machine. + const opened = page.waitForURL(new RegExp(`/files/${fileId}(?:[/?]|$)`), { timeout: 15_000 }) await page.keyboard.press('Enter') - - await expect(page).toHaveURL(new RegExp(`/files/${fileId}(?:[/?]|$)`)) + await opened }) test('Escape closes the open results popover', async ({ page }) => { diff --git a/tests/playwright/e2e/files_sharing/admin-settings-limit-to-same-group.spec.ts b/tests/playwright/e2e/files_sharing/admin-settings-limit-to-same-group.spec.ts index 1695a6c2c91e4..402abe50c6d50 100644 --- a/tests/playwright/e2e/files_sharing/admin-settings-limit-to-same-group.spec.ts +++ b/tests/playwright/e2e/files_sharing/admin-settings-limit-to-same-group.spec.ts @@ -25,6 +25,11 @@ test.describe('files_sharing: Sharing limited to members of the same group', () test.beforeAll(async () => { await runOcc(['config:app:set', '--value', 'yes', 'core', 'shareapi_only_share_with_group_members']) + // The groups are shared by both tests, so create them once here rather than + // spending two container round-trips inside every test's own timeout. + for (const group of groups) { + await runOcc(['group:add', group], { failOnError: false }) + } }) test.afterAll(async () => { @@ -34,36 +39,6 @@ test.describe('files_sharing: Sharing limited to members of the same group', () } }) - /** - * Put both accounts in two shared groups, then share a file each way. - * - * @returns the file names, `fromUser` shared by `user`, `fromRecipient` by `recipient` - */ - async function seedMutualShares( - user: User, - recipient: User, - userRequest: Parameters[0], - recipientRequest: Parameters[0], - ): Promise<{ fromUser: string, fromRecipient: string }> { - for (const group of groups) { - await runOcc(['group:add', group], { failOnError: false }) - await runOcc(['group:adduser', group, user.userId]) - await runOcc(['group:adduser', group, recipient.userId]) - } - - const fromUser = 'shared-by-user.txt' - const fromRecipient = 'shared-by-recipient.txt' - await uploadContent(userRequest, user, 'share to recipient', 'text/plain', `/${fromUser}`) - await uploadContent(recipientRequest, recipient, 'share to user', 'text/plain', `/${fromRecipient}`) - await createShare(userRequest, `/${fromUser}`, recipient.userId) - await createShare(recipientRequest, `/${fromRecipient}`, user.userId) - - await waitForShare(recipientRequest, recipient, '', fromUser) - await waitForShare(userRequest, user, '', fromRecipient) - - return { fromUser, fromRecipient } - } - test('keeps the shares while one common group is left', async ({ page, user, recipient, recipientRequest, filesListPage }) => { const { fromUser, fromRecipient } = await seedMutualShares(user, recipient, page.request, recipientRequest) @@ -92,4 +67,33 @@ test.describe('files_sharing: Sharing limited to members of the same group', () await filesListPage.open() await expect(filesListPage.getRowForFile(fromUser)).toHaveCount(0) }) + + /** + * Put both accounts in two shared groups, then share a file each way. + * + * @returns the file names, `fromUser` shared by `user`, `fromRecipient` by `recipient` + */ + async function seedMutualShares( + user: User, + recipient: User, + userRequest: Parameters[0], + recipientRequest: Parameters[0], + ): Promise<{ fromUser: string, fromRecipient: string }> { + for (const group of groups) { + await runOcc(['group:adduser', group, user.userId]) + await runOcc(['group:adduser', group, recipient.userId]) + } + + const fromUser = 'shared-by-user.txt' + const fromRecipient = 'shared-by-recipient.txt' + await uploadContent(userRequest, user, 'share to recipient', 'text/plain', `/${fromUser}`) + await uploadContent(recipientRequest, recipient, 'share to user', 'text/plain', `/${fromRecipient}`) + await createShare(userRequest, `/${fromUser}`, recipient.userId) + await createShare(recipientRequest, `/${fromRecipient}`, user.userId) + + await waitForShare(recipientRequest, recipient, '', fromUser) + await waitForShare(userRequest, user, '', fromRecipient) + + return { fromUser, fromRecipient } + } }) diff --git a/tests/playwright/support/sections/FilesListPage.ts b/tests/playwright/support/sections/FilesListPage.ts index eaa4ae42c8d04..df63836c775f2 100644 --- a/tests/playwright/support/sections/FilesListPage.ts +++ b/tests/playwright/support/sections/FilesListPage.ts @@ -25,9 +25,11 @@ export class FilesListPage { return this.page.locator('[data-cy-files-list]') } - /** Wait for the file list container to be rendered (e.g. after a direct goto). */ + /** + * Wait for the file list container to be rendered (e.g. after a direct goto). + */ async waitForList(): Promise { - await this.getFilesList().waitFor({ state: 'visible' }) + await this.getFilesList().waitFor({ state: 'visible', timeout: 20_000 }) } /** @@ -176,6 +178,8 @@ export class FilesListPage { * row Locator so it serves both name- and fileid-addressed rows. */ private async openActionsMenuForRow(row: Locator): Promise { + await expect(row).toBeVisible({ timeout: 15_000 }) + await row.hover() const actionsButton = row.getByRole('button', { name: 'Actions' }) @@ -431,7 +435,9 @@ export class FilesListPage { // the row's buttons by the folder name is ambiguous for shared folders, // whose row also carries a "Shared by …" action button that can contain // the same text. - await this.getRowNameLinkForFile(directory).click() + const link = this.getRowNameLinkForFile(directory) + await expect(link).toBeVisible() + await link.click() // Assert the deepest segment of the `dir` query param matches the folder // we just opened. Comparing the decoded value (URLSearchParams decodes diff --git a/tests/playwright/support/sections/UnifiedSearchPage.ts b/tests/playwright/support/sections/UnifiedSearchPage.ts index 4c14da0b01bf5..5100c25ff2160 100644 --- a/tests/playwright/support/sections/UnifiedSearchPage.ts +++ b/tests/playwright/support/sections/UnifiedSearchPage.ts @@ -5,6 +5,8 @@ import type { Locator, Page } from '@playwright/test' +import { expect } from '@playwright/test' + /** * The unified search in the top header: the combobox input and the results * popover it controls. Desktop layout only (the mobile header collapses to a @@ -63,6 +65,46 @@ export class UnifiedSearchPage { return this.panel().getByRole('option') } + /** + * The popover's polite status region (WCAG 4.1.3). It reports the search + * progress: "Searching …" while any provider is still in flight, then the + * settled outcome — "No matching results" or the result count. + */ + status(): Locator { + return this.panel().getByRole('status') + } + + /** + * Wait for the result set to stop changing, i.e. for every provider to have + * answered. + * + * Results stream in per provider and the rows re-render on every batch, which + * clears and re-establishes the selection in between — so a visible first row + * does not mean the selection is settled. Anything that reads + * `aria-activedescendant`, or activates the selected row with Enter, has to + * wait for this first: on a mid-flight list the id is momentarily absent and + * Enter is a no-op. + * + * The status region is the app's own settled signal, so this needs no + * arbitrary delay. + */ + async waitForSettledResults(): Promise { + // The count, not just "results": "No matching results" is a settled state too, + // but one no selection assertion can work with. Anchored (with the template's + // surrounding whitespace, which a regex match does not normalize away) so a + // still-running search cannot slip through. + await expect(this.status()).toHaveText(/^\s*\d+ results?\s*$/) + } + + /** + * The id of the currently selected row, once the input names one. Waits for the + * attribute so a still-settling list cannot yield `null`. + */ + async activeOptionId(): Promise { + await expect(this.input()).toHaveAttribute('aria-activedescendant', /\S/) + return (await this.input().getAttribute('aria-activedescendant'))! + } + /** * A single result row addressed by its DOM id (the value the input carries in * aria-activedescendant). Attribute selector so provider ids with dots or colons