From 18147183bb55d9fd383c1b6f7f5a2f660355e954 Mon Sep 17 00:00:00 2001 From: John McLear Date: Wed, 1 Apr 2026 09:07:45 +0100 Subject: [PATCH] Fix frontend test failures across all browsers (#7416) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix frontend test failures across all browsers - Fix home button using fragile relative URL (window.location.href + "/../..") that WebKit doesn't resolve correctly. Use window.location.origin instead. - Wait for #editorcontainer.initialized in goToNewPad/goToPad/ appendQueryParams so toolbar, chat, and cookie handlers are fully set up before tests interact with them. - Clear cookies in chat test beforeEach to prevent chatAndUsers cookie from prior tests disabling the sticky chat checkbox. - Wait for navigation to complete in editbar home button test. Fixes #7405 Co-Authored-By: Claude Opus 4.6 (1M context) * Run frontend tests on pull requests Playwright runs locally and doesn't need Sauce Labs secrets, so there's no reason to limit frontend tests to push events only. Also remove stale Sauce Labs references from workflow names/comments. Co-Authored-By: Claude Opus 4.6 (1M context) * Fix sticky chat test: use click() instead of check()/uncheck() The stickToScreen() handler manages checkbox state internally with its own toggle logic and a setTimeout. Playwright's check()/uncheck() methods verify state after clicking, but race with the async toggle, causing "Clicking the checkbox did not change its state" errors. Using click() avoids this — the waitForSelector calls already verify the final state. Co-Authored-By: Claude Opus 4.6 (1M context) * Fix sticky chat handler and reduce parallel workers - Remove force:true from sticky chat checkbox clicks — it can bypass jQuery event handlers preventing stickToScreen() from firing. - Wait for chatbox stickyChat class instead of checkbox state, since stickToScreen() manages the checkbox asynchronously via setTimeout. - Reduce workers from 5 to 2 to avoid overloading the single Etherpad server instance, which causes goToNewPad timeouts on CI. Co-Authored-By: Claude Opus 4.6 (1M context) * Clean up workflows: remove Sauce Labs, load test push-only - Remove all Sauce Labs references (steps, comments, secrets) from frontend test workflows — Playwright replaced Sauce Labs - Remove unused set-output steps and GIT_HASH exports - Remove stale commented-out code from admin tests - Restrict load test to push events only (no need on PRs) - Fix artifact names to not reference undefined matrix.node Co-Authored-By: Claude Opus 4.6 (1M context) * Fix sticky chat test: click label instead of checkbox The label element intercepts pointer events on the checkbox (reported by Webkit). On Chrome/Firefox the checkbox is "not stable" due to animations. Clicking the label is how a real user interacts with it and properly triggers the jQuery click handler. Co-Authored-By: Claude Opus 4.6 (1M context) * Fix home button to preserve subpath installations Use URL API to resolve '../..' relative to current URL instead of hardcoding origin + '/'. This preserves any configured base path (e.g. /etherpad) for reverse-proxy installations. Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: Claude Opus 4.6 (1M context) --- .github/workflows/frontend-admin-tests.yml | 61 ++----------------- .github/workflows/frontend-tests.yml | 45 +++----------- .github/workflows/load-test.yml | 4 -- src/playwright.config.ts | 2 +- src/static/js/pad_editbar.ts | 2 +- src/tests/frontend-new/helper/padHelper.ts | 3 + .../frontend-new/helper/settingsHelper.ts | 8 +-- src/tests/frontend-new/specs/chat.spec.ts | 3 +- src/tests/frontend-new/specs/editbar.spec.ts | 1 + 9 files changed, 24 insertions(+), 105 deletions(-) diff --git a/.github/workflows/frontend-admin-tests.yml b/.github/workflows/frontend-admin-tests.yml index 45e56963f..f28483325 100644 --- a/.github/workflows/frontend-admin-tests.yml +++ b/.github/workflows/frontend-admin-tests.yml @@ -1,13 +1,15 @@ -# Leave the powered by Sauce Labs bit in as this means we get additional concurrency name: "Frontend admin tests" on: push: paths-ignore: - 'doc/**' + pull_request: + paths-ignore: + - 'doc/**' permissions: - contents: read # to fetch code (actions/checkout) + contents: read jobs: withplugins: @@ -22,12 +24,6 @@ jobs: node: [20, 22, 24] steps: - - - name: Generate Sauce Labs strings - id: sauce_strings - run: | - printf %s\\n '::set-output name=name::${{ github.workflow }} - ${{ github.job }} - Node ${{ matrix.node }}' - printf %s\\n '::set-output name=tunnel_id::${{ github.run_id }}-${{ github.run_number }}-${{ github.job }}-node${{ matrix.node }}' - name: Checkout repository uses: actions/checkout@v6 @@ -54,31 +50,9 @@ jobs: path: | ~/.cache/ms-playwright key: ${{ runner.os }}-playwright-${{ env.PLAYWRIGHT_VERSION }} - #- - # name: Install etherpad plugins - # # We intentionally install an old ep_align version to test upgrades to - # # the minor version number. The --legacy-peer-deps flag is required to - # # work around a bug in npm v7: https://github.com/npm/cli/issues/2199 - # run: pnpm install --workspace-root ep_align@0.2.27 - # Etherpad core dependencies must be installed after installing the - # plugin's dependencies, otherwise npm will try to hoist common - # dependencies by removing them from src/node_modules and installing them - # in the top-level node_modules. As of v6.14.10, npm's hoist logic appears - # to be buggy, because it sometimes removes dependencies from - # src/node_modules but fails to add them to the top-level node_modules. - # Even if npm correctly hoists the dependencies, the hoisting seems to - # confuse tools such as `npm outdated`, `npm update`, and some ESLint - # rules. - name: Install all dependencies and symlink for ep_etherpad-lite run: gnpm i --runtimeVersion="${{ matrix.node }}" - #- - # name: Install etherpad plugins - # run: rm -Rf node_modules/ep_align/static/tests/* - - - name: export GIT_HASH to env - id: environment - run: echo "::set-output name=sha_short::$(git rev-parse --short ${{ github.sha }})" - name: Create settings.json run: cp settings.json.template settings.json @@ -96,33 +70,6 @@ jobs: working-directory: admin run: | gnpm run build --runtimeVersion="${{ matrix.node }}" - # name: Run the frontend admin tests - # shell: bash - # env: - # SAUCE_USERNAME: ${{ secrets.SAUCE_USERNAME }} - # SAUCE_ACCESS_KEY: ${{ secrets.SAUCE_ACCESS_KEY }} - # SAUCE_NAME: ${{ steps.sauce_strings.outputs.name }} - # TRAVIS_JOB_NUMBER: ${{ steps.sauce_strings.outputs.tunnel_id }} - # GIT_HASH: ${{ steps.environment.outputs.sha_short }} - # run: | - # src/tests/frontend/travis/adminrunner.sh - #- - # uses: saucelabs/sauce-connect-action@v2.3.6 - # with: - # username: ${{ secrets.SAUCE_USERNAME }} - # accessKey: ${{ secrets.SAUCE_ACCESS_KEY }} - # tunnelIdentifier: ${{ steps.sauce_strings.outputs.tunnel_id }} - #- - # name: Run the frontend admin tests - # shell: bash - # env: - # SAUCE_USERNAME: ${{ secrets.SAUCE_USERNAME }} - # SAUCE_ACCESS_KEY: ${{ secrets.SAUCE_ACCESS_KEY }} - # SAUCE_NAME: ${{ steps.sauce_strings.outputs.name }} - # TRAVIS_JOB_NUMBER: ${{ steps.sauce_strings.outputs.tunnel_id }} - # GIT_HASH: ${{ steps.environment.outputs.sha_short }} - # run: | - # src/tests/frontend/travis/adminrunner.sh - name: Run the frontend admin tests shell: bash run: | diff --git a/.github/workflows/frontend-tests.yml b/.github/workflows/frontend-tests.yml index a99118cb6..da091923e 100644 --- a/.github/workflows/frontend-tests.yml +++ b/.github/workflows/frontend-tests.yml @@ -1,13 +1,15 @@ -# Leave the powered by Sauce Labs bit in as this means we get additional concurrency -name: "Frontend tests powered by Sauce Labs" +name: "Frontend tests" on: push: paths-ignore: - 'doc/**' + pull_request: + paths-ignore: + - 'doc/**' permissions: - contents: read # to fetch code (actions/checkout) + contents: read jobs: playwright-chrome: @@ -16,12 +18,6 @@ jobs: name: Playwright Chrome runs-on: ubuntu-latest steps: - - - name: Generate Sauce Labs strings - id: sauce_strings - run: | - printf %s\\n '::set-output name=name::${{ github.workflow }} - ${{ github.job }}' - printf %s\\n '::set-output name=tunnel_id::${{ github.run_id }}-${{ github.run_number }}-${{ github.job }}' - name: Checkout repository uses: actions/checkout@v6 @@ -45,10 +41,6 @@ jobs: - name: Install all dependencies and symlink for ep_etherpad-lite run: gnpm install --frozen-lockfile - - - name: export GIT_HASH to env - id: environment - run: echo "::set-output name=sha_short::$(git rev-parse --short ${{ github.sha }})" - name: Create settings.json run: cp ./src/tests/settings.json settings.json @@ -72,7 +64,7 @@ jobs: - uses: actions/upload-artifact@v7 if: always() with: - name: playwright-report-${{ matrix.node }}-chrome + name: playwright-report-chrome path: src/playwright-report/ retention-days: 30 playwright-firefox: @@ -81,11 +73,6 @@ jobs: name: Playwright Firefox runs-on: ubuntu-latest steps: - - name: Generate Sauce Labs strings - id: sauce_strings - run: | - printf %s\\n '::set-output name=name::${{ github.workflow }} - ${{ github.job }}' - printf %s\\n '::set-output name=tunnel_id::${{ github.run_id }}-${{ github.run_number }}-${{ github.job }}' - name: Checkout repository uses: actions/checkout@v6 - uses: actions/cache@v5 @@ -107,9 +94,6 @@ jobs: version: 0.0.12 - name: Install all dependencies and symlink for ep_etherpad-lite run: gnpm install --frozen-lockfile - - name: export GIT_HASH to env - id: environment - run: echo "::set-output name=sha_short::$(git rev-parse --short ${{ github.sha }})" - name: Create settings.json run: cp ./src/tests/settings.json settings.json - name: Run the frontend tests @@ -132,7 +116,7 @@ jobs: - uses: actions/upload-artifact@v7 if: always() with: - name: playwright-report-${{ matrix.node }}-firefox + name: playwright-report-firefox path: src/playwright-report/ retention-days: 30 playwright-webkit: @@ -141,12 +125,6 @@ jobs: env: PNPM_HOME: ~/.pnpm-store steps: - - - name: Generate Sauce Labs strings - id: sauce_strings - run: | - printf %s\\n '::set-output name=name::${{ github.workflow }} - ${{ github.job }}' - printf %s\\n '::set-output name=tunnel_id::${{ github.run_id }}-${{ github.run_number }}-${{ github.job }}' - name: Checkout repository uses: actions/checkout@v6 @@ -169,10 +147,6 @@ jobs: - name: Install all dependencies and symlink for ep_etherpad-lite run: gnpm install --frozen-lockfile - - - name: export GIT_HASH to env - id: environment - run: echo "::set-output name=sha_short::$(git rev-parse --short ${{ github.sha }})" - name: Create settings.json run: cp ./src/tests/settings.json settings.json @@ -196,9 +170,6 @@ jobs: - uses: actions/upload-artifact@v7 if: always() with: - name: playwright-report-${{ matrix.node }}-webkit + name: playwright-report-webkit path: src/playwright-report/ retention-days: 30 - - - diff --git a/.github/workflows/load-test.yml b/.github/workflows/load-test.yml index 12bacdcf8..f590dd931 100644 --- a/.github/workflows/load-test.yml +++ b/.github/workflows/load-test.yml @@ -1,13 +1,9 @@ name: "Loadtest" -# any branch is useful for testing before a PR is submitted on: push: paths-ignore: - "doc/**" - pull_request: - paths-ignore: - - "doc/**" permissions: contents: read diff --git a/src/playwright.config.ts b/src/playwright.config.ts index 583c4f920..003645f7b 100644 --- a/src/playwright.config.ts +++ b/src/playwright.config.ts @@ -17,7 +17,7 @@ export default defineConfig({ expect: { timeout: defaultExpectTimeout }, timeout: defaultTestTimeout, retries: 0, - workers: 5, + workers: 2, /* Shared settings for all the projects below. See https://playwright.dev/docs/api/class-testoptions. */ use: { /* Base URL to use in actions like `await page.goto('/')`. */ diff --git a/src/static/js/pad_editbar.ts b/src/static/js/pad_editbar.ts index 1a5ee1dc0..cb8dae2a7 100644 --- a/src/static/js/pad_editbar.ts +++ b/src/static/js/pad_editbar.ts @@ -356,7 +356,7 @@ exports.padeditbar = new class { this.registerDropdownCommand('import_export'); this.registerDropdownCommand('embed'); this.registerCommand('home', ()=>{ - window.location.href = window.location.href + "/../.." + window.location.href = new URL('../..', window.location.href).href }) this.registerCommand('settings', () => { diff --git a/src/tests/frontend-new/helper/padHelper.ts b/src/tests/frontend-new/helper/padHelper.ts index f52cd0a35..1be987d23 100644 --- a/src/tests/frontend-new/helper/padHelper.ts +++ b/src/tests/frontend-new/helper/padHelper.ts @@ -111,6 +111,7 @@ export const appendQueryParams = async (page: Page, queryParameters: MapArrayTyp }); await page.goto(page.url()+"?"+ searchParams.toString()); await page.waitForSelector('iframe[name="ace_outer"]'); + await page.waitForSelector('#editorcontainer.initialized'); } export const goToNewPad = async (page: Page) => { @@ -118,12 +119,14 @@ export const goToNewPad = async (page: Page) => { const padId = "FRONTEND_TESTS"+randomUUID(); await page.goto('http://localhost:9001/p/'+padId); await page.waitForSelector('iframe[name="ace_outer"]'); + await page.waitForSelector('#editorcontainer.initialized'); return padId; } export const goToPad = async (page: Page, padId: string) => { await page.goto('http://localhost:9001/p/'+padId); await page.waitForSelector('iframe[name="ace_outer"]'); + await page.waitForSelector('#editorcontainer.initialized'); } diff --git a/src/tests/frontend-new/helper/settingsHelper.ts b/src/tests/frontend-new/helper/settingsHelper.ts index 729dd48f6..c35ccf346 100644 --- a/src/tests/frontend-new/helper/settingsHelper.ts +++ b/src/tests/frontend-new/helper/settingsHelper.ts @@ -22,14 +22,14 @@ export const enableStickyChatviaSettings = async (page: Page) => { const stickyChat = page.locator('#options-stickychat') const checked = await stickyChat.isChecked() if(checked) return - await stickyChat.check({force: true}) - await page.waitForSelector('#options-stickychat:checked') + await page.locator('label[for="options-stickychat"]').click() + await page.waitForFunction(() => document.querySelector('#chatbox')?.classList.contains('stickyChat')) } export const disableStickyChat = async (page: Page) => { const stickyChat = page.locator('#options-stickychat') const checked = await stickyChat.isChecked() if(!checked) return - await stickyChat.uncheck({force: true}) - await page.waitForSelector('#options-stickychat:not(:checked)') + await page.locator('label[for="options-stickychat"]').click() + await page.waitForFunction(() => !document.querySelector('#chatbox')?.classList.contains('stickyChat')) } diff --git a/src/tests/frontend-new/specs/chat.spec.ts b/src/tests/frontend-new/specs/chat.spec.ts index 4d4f1bd1c..fac0e915d 100644 --- a/src/tests/frontend-new/specs/chat.spec.ts +++ b/src/tests/frontend-new/specs/chat.spec.ts @@ -14,7 +14,8 @@ import { import {disableStickyChat, enableStickyChatviaSettings, hideSettings, showSettings} from "../helper/settingsHelper"; -test.beforeEach(async ({ page })=>{ +test.beforeEach(async ({ page, context })=>{ + await context.clearCookies(); await goToNewPad(page); }) diff --git a/src/tests/frontend-new/specs/editbar.spec.ts b/src/tests/frontend-new/specs/editbar.spec.ts index c2dc56fda..154d79180 100644 --- a/src/tests/frontend-new/specs/editbar.spec.ts +++ b/src/tests/frontend-new/specs/editbar.spec.ts @@ -12,6 +12,7 @@ test('should go to home on pad', async ({page}) => { expect(attribute).toBe('pad.toolbar.home.title'); await homeButton.click(); + await page.waitForURL((url) => !url.pathname.includes('/p/')); const url = page.url(); expect(url).not.toContain('/p/'); })