Skip to content

Parallelize Playwright accessibility tests and isolate functional suite - #2786

Open
abn wants to merge 1 commit into
lemonade-sdk:GUI3_mergingfrom
abn:perf/speed-up-playwright-tests
Open

Parallelize Playwright accessibility tests and isolate functional suite#2786
abn wants to merge 1 commit into
lemonade-sdk:GUI3_mergingfrom
abn:perf/speed-up-playwright-tests

Conversation

@abn

@abn abn commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Enables parallel test execution in src/app/tests/a11y.spec.ts.
  • Configures separate a11y (parallelized) and functional (isolated sequential execution to avoid shared local storage/server state collisions) test projects in playwright.config.ts.
  • Updates src/app/package.json test script to run --project=a11y && --project=functional, accelerating full suite runtime from ~9 minutes to ~60 seconds.

@github-actions github-actions Bot added area::ci CI / GitHub Actions / self-hosted runner infrastructure enhancement New feature or request labels Jul 23, 2026
@abn
abn requested a review from fl0rianr July 23, 2026 00:17
@abn

abn commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author

Docs CI failure should be resolved by 5f40d5f from #2783.

@fl0rianr fl0rianr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the optimization work — splitting the accessibility and functional suites makes sense, and the runtime improvement is valuable.

I found two issues that should be addressed before merging:

  1. The new npm test script breaks argument forwarding:
"test": "npx playwright test --project=a11y && npx playwright test --project=functional"

Arguments passed via commands such as:

npm test -- --grep "Model Tuning"

are appended only to the second command. This means filters, --list, --last-failed, debugging options, snapshot updates, and similar arguments affect only the functional suite, while the complete accessibility suite always runs.

A small Node runner that invokes both projects with the same forwarded arguments would avoid this regression.

  1. The intended isolation applies only to npm test.

npm run test:headed and direct npx playwright test calls still start both projects in the same Playwright run. workers: 1 limits the functional project itself, but does not guarantee that it cannot overlap with the accessibility project.

All supported test entry points should use the same ordered runner, or it should be demonstrated that concurrent execution of the two projects is safe.

There is also a smaller artifact issue: both sequential Playwright invocations use the same outputDir. The second invocation clears test-results, so screenshots and other artifacts from the accessibility run are removed.

Regarding the currently failing check: this is not a docs-generation failure. The app-regression-tests job fails before reaching Playwright because npm run typecheck was missing after the old GUI2 workflow was merged. Commit 5f40d5f from #2783 adds that script and has now been merged into GUI3_merging, so this PR needs to be updated or re-run against the new base.

Also note that the current CI workflow still does not execute the Playwright regression suite; its regression-test step only prints that GUI3 tests are skipped.

@abn
abn force-pushed the perf/speed-up-playwright-tests branch from 6346e0d to a46e265 Compare July 24, 2026 00:52
@abn

abn commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the feedback!

  1. Sequential Runner & Argument Forwarding: Created a Node runner script (src/app/scripts/run-playwright-tests.mjs) and updated the test and test:headed package scripts to use it. The script sequentially runs the accessibility and functional test projects while forwarding all CLI arguments (such as filters, debug flags, and headed mode) to both invocations. If a specific project is explicitly passed (e.g. --project=a11y) or --help is requested, it bypasses sequencing and executes a single Playwright run directly.
  2. Artifact Separation: Configured project-specific output directories in playwright.config.ts (./test-results/a11y and ./test-results/functional) so sequential invocations do not overwrite or clear each other's traces and screenshots. staging files have also been added to .gitignore.
  3. Route Interception Wildcard Fix: While testing, I noticed that the authoritative server snapshot removes stale paused renderer state test was failing because /api/v1/health and /api/v1/downloads** were missing wildcard prefixes, causing the mock server intercepts to miss requests pointing to alternative localhost ports and leaving the app offline. I corrected these to **/api/v1/health** and **/api/v1/downloads**, and the full suite now successfully passes locally.

@abn
abn requested a review from fl0rianr July 24, 2026 22:30

@fl0rianr fl0rianr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update — the project split, sequential runner, separate output directories, and route interception fixes all move the PR in the right direction.

There is still one blocking issue in run-playwright-tests.mjs:

spawnSync('npx', ['playwright', 'test', ...args], {
  stdio: 'inherit',
  shell: true,
});

Using shell: true breaks reliable argument forwarding because arguments containing spaces can be split again by the shell. For example:

npm test -- --grep "Model Tuning"

may reach Playwright as separate Model and Tuning arguments instead of one grep value. It also introduces unnecessary shell interpretation.

Please invoke the platform-specific executable directly and disable the shell, for example:

const npxCommand = process.platform === 'win32' ? 'npx.cmd' : 'npx';

const result = spawnSync(
  npxCommand,
  ['playwright', 'test', ...args],
  {
    stdio: 'inherit',
    shell: false,
  },
);

The runner should also treat process startup failures as failures. Currently, these paths can return success when status is null:

process.exit(result.status ?? 0);
process.exit(functionalResult.status ?? 0);

Please check result.error and use a non-zero fallback such as status ?? 1.

One smaller clarification remains: direct npx playwright test still runs both projects in the same Playwright invocation, so the strict ordering applies only to the npm runner scripts. Either ensure direct full-suite execution is safe or document npm test and npm run test:headed as the supported isolated entry points.

@abn
abn force-pushed the perf/speed-up-playwright-tests branch from a46e265 to ae42313 Compare July 28, 2026 13:26
@abn
abn requested a review from fl0rianr July 28, 2026 13:26
@abn

abn commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator Author

I've updated the PR to address these points:

  1. Direct Execution & Shell Splitting: Switched from shell: true to shell: false in run-playwright-tests.mjs and resolved the platform-specific executable (npx.cmd on Windows, npx on macOS/Linux) directly. This ensures argument forwarding (such as filters with spaces) works correctly without being split by the shell.
  2. Process Startup Validation: Added validation for process startup errors (result.error) and fall back to a non-zero exit code if the process fails to start.
  3. Documentation: Documented in src/app/README.md that direct execution via npx playwright test runs the projects concurrently and that npm test / npm run test:headed are the recommended entry points for isolated, sequential execution (ensuring separate project outputDir directories are utilized to prevent artifact overwrites).
  4. Rebase: Rebased the branch on the latest upstream/GUI3_merging.

@fl0rianr fl0rianr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor topic that if started with a non existent test all are passed green. But normally we use the wohle suite anyway.

Comment thread src/app/scripts/run-playwright-tests.mjs Outdated
Comment thread src/app/scripts/run-playwright-tests.mjs Outdated
Parallelizes accessibility (a11y) tests and configures functional tests to run sequentially, preventing shared state collisions. Adds a Node script runner to forward CLI arguments, separates project output directories, and corrects route wildcards.
@abn
abn force-pushed the perf/speed-up-playwright-tests branch from ae42313 to fe8e852 Compare July 28, 2026 20:01
@abn
abn requested a review from fl0rianr July 28, 2026 20:02

@fl0rianr fl0rianr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, ready to merge right away.

@kpoineal getting in the way of your work or merge now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area::ci CI / GitHub Actions / self-hosted runner infrastructure enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants