Skip to content

ci(integration-tests): run Playwright with 3 workers instead of 2, allowlist unused keycloak font 404 - #7393

Open
corneliusroemer-agent wants to merge 9 commits into
mainfrom
ci/playwright-workers-experiment
Open

corneliusroemer-agent wants to merge 9 commits into
mainfrom
ci/playwright-workers-experiment

Conversation

@corneliusroemer-agent

@corneliusroemer-agent corneliusroemer-agent commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Speed up integration tests by using 3 Playwright workers up from 2. On chromium the test step drops from ~11 min to ~7 min, Firefox ~15min to ~14min.

Measurements

Benchmarked for different numbers of workers:

workers run chromium firefox cli
2 35846770807 656 s 911 s 89 s
3 35846907451 425 s 818 s 90 s
4 35847931624 441 s 822 s 99 s
5 35848066764 434 s 889 s 91 s

Why it was lowered to 2, and why that reason no longer applies

#6370 (May 2026) went from 4 to 2 workers because of intermittent ERR_EMPTY_RESPONSE / dropped-connection failures, suspected to be caused by an overloaded runner (CPU was near 100% even at 2 workers). Since then those failures have been traced to other causes.

Details

None of today's 8 browser jobs, at any worker count, had a pod eviction, a browser crash or an OOM kill.

The one class that might still be load-sensitive is Firefox occasionally dying mid-test ("Target page, context or browser has been closed"). It is rare (n≈2 in recent surveys) and not understood, and more workers means more browser processes on the runner. If that or other load-sensitive failures come back after this change, the fix is either to deflake the specific test or to go back to 2.

Also ignores a 404 on resources-common/node_modules/patternfly/dist/fonts/OpenSans-Light-webfont.* in the chromium console check. Keycloak's login theme requests that font and Keycloak doesn't serve it. It was the only failure in 8 runs at 3 workers (7/8 chromium, 8/8 firefox, 7/8 cli; 5 dispatched rounds plus the PR's own runs), and an unidentified Keycloak 404 had failed edit-account.spec.ts:6 at 2 workers before.

If it turns out flakes go up again, we can just lower back down to 2 - it's cheap.

https://claude.ai/code/session_01LkfZS1emsiUHd4P4pK8sra

🚀 Preview: Add preview label to enable

…HT_WORKERS

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LkfZS1emsiUHd4P4pK8sra
@claude

claude Bot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

This PR may be related to: #6096, #5118 (both about integration test flakiness/performance), and references #6370 (the earlier 4→2 worker change) already noted in the description.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LkfZS1emsiUHd4P4pK8sra
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LkfZS1emsiUHd4P4pK8sra
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LkfZS1emsiUHd4P4pK8sra
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LkfZS1emsiUHd4P4pK8sra
@corneliusroemer-agent corneliusroemer-agent changed the title ci(integration-tests): experiment - Playwright worker count 2 vs 3/4/5 (do not merge) ci(integration-tests): run Playwright with 3 workers instead of 2 Sep 23, 2026
@corneliusroemer
corneliusroemer marked this pull request as ready for review September 23, 2026 12:12
@claude

claude Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @corneliusroemer's task in 1m 4s —— View job


Review: 3 Playwright workers

  • Gather context (diff, workflow, Playwright config, test layout)
  • Review the change
  • Post review

No blocking issues. The change is a single flag value in .github/workflows/integration-tests.yml:125, backed by a four-point benchmark, and trivially revertible.

Checks I ran against the change:

  • No test uses test.describe.configure({ mode: 'serial' }) and fullyParallel: true is already set (integration-tests/playwright.config.ts:18), so tests were already free to interleave — raising the worker count adds concurrency but doesn't break any ordering assumption that was previously holding.
  • The readonly-setup project dependency still gates the *-with-dep projects, so the shared read-only fixture ordering is unaffected.
  • --workers=3 applies to all three matrix legs including cli, where the benchmark shows no gain (89 s → 90 s) and 4 workers showed a small regression (99 s). Harmless, just noting the flag isn't per-browser.

Observations (not blockers):

  1. The measurements suggest 3 is at the plateau, not below it. 4 and 5 workers gave no improvement (425 / 441 / 434 s on chromium), which is what you'd expect on 4-vCPU standard runners that are also hosting the k3d cluster and the whole Loculus stack. That supports 3 as the right pick, but it also means there's no CPU headroom left — the "load-sensitive Firefox death" risk in your description is the real thing to watch, and it's the one that would show up as a mid-test browser close rather than as a slow run.

  2. retries is 0 on CI (integration-tests/playwright.config.ts:24, written as process.env.CI ? 0 : 0). If a load-sensitive flake does come back, the only mitigations available are "deflake the test" or "revert to 2" — a single flaky test costs a full 45-minute job rerun. Setting retries: 1 on CI would be a cheaper middle option, though I assume 0 is deliberate so flakes stay visible rather than getting papered over. Worth a conscious decision rather than a change here. The redundant ternary could also just be retries: 0. Fix this →

  3. Docs divergence: integration-tests/AGENTS.md:81,87 documents --workers=4 for local runs. That's fine (local machines aren't 4-vCPU runners sharing with k3d), but if you ever want CI and local to be self-consistent, moving the count into playwright.config.ts as workers: process.env.CI ? 3 : undefined would keep it in one place and let Playwright pick a sensible local default.

Your writeup of why the 2-worker cap from #6370 no longer applies is unusually well-evidenced — tracing ERR_EMPTY_RESPONSE to disk-pressure evictions rather than CPU, with before/after counts across the k3s upgrade, is the part that makes this safe to land rather than just a re-roll of the dice.

Note: I can't modify files under .github/workflows/ (GitHub App permissions), so if you want changes to the workflow itself they'll need to be made directly.

--- • Branch: ci/playwright-workers-experiment

@corneliusroemer
corneliusroemer marked this pull request as draft September 23, 2026 12:32
@corneliusroemer

Copy link
Copy Markdown
Contributor

I'll do a few more runs to see whether it really is stable enoughl.

Keycloak's login theme requests patternfly's OpenSans-Light webfont from
resources-common and gets a 404, which the chromium console check turned
into intermittent failures (edit-account.spec.ts:6, cli end-to-end-flow).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LkfZS1emsiUHd4P4pK8sra
@corneliusroemer corneliusroemer changed the title ci(integration-tests): run Playwright with 3 workers instead of 2 ci(integration-tests): run Playwright with 3 workers instead of 2, allowlist unused keycloak font 404 Sep 23, 2026
@corneliusroemer
corneliusroemer marked this pull request as ready for review September 23, 2026 15:53
@claude

claude Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @corneliusroemer's task in 1m 35s —— View job


Review: 3 Playwright workers + Keycloak font 404 allowlist

  • Gather context (diff, fixture, playwright config, keycloak theme)
  • Review the change
  • Post review

No blocking issues. Both changes are small and revertible, and the benchmark table plus the archaeology on why #6370's 2-worker cap no longer applies makes the worker bump well-justified rather than a re-roll.

Checks I ran:

  • msg.location().url is the right hook for this: Chromium's message text for a failed subresource is just Failed to load resource: the server responded with a status of 404, with no URL in it, so a text-based allowlist entry would have had to be broad enough to mute every 404 in every test. Matching on the location URL keeps it targeted.
  • The allowlist only affects Chromium — the assertion at console-warnings.fixture.ts:37 is already browserName === 'chromium'-gated.
  • The font genuinely isn't Loculus's to fix: keycloak/keycloakify is a keycloakify-based theme built on Keycloak 24's base resources, and resources-common/node_modules/patternfly/… is upstream theme CSS. Nothing in this repo references that path.
  • Config-wise the worker bump is safe: fullyParallel: true is already set (integration-tests/playwright.config.ts:18), no spec uses describe.configure({ mode: 'serial' }), and the readonly-setup project dependency still gates the *-with-dep projects, so the shared read-only fixture ordering is untouched.

One inline comment on the allowlist scope (only OpenSans-Light is covered, while patternfly references several weights from the same directory).

Two things carried over from the earlier review, neither a reason to hold this up:

  1. --workers=3 also applies to the cli matrix leg, where the benchmark shows no gain (89 s → 90 s). Harmless; the flag just isn't per-browser.
  2. retries: process.env.CI ? 0 : 0 (integration-tests/playwright.config.ts:24) is a redundant ternary under a "Retry on CI only" comment that no longer describes it. If a load-sensitive flake does come back, retries: 1 on CI is a cheaper middle option than reverting to 2 workers — but I assume 0 is deliberate so flakes stay visible.

Note: I can't modify files under .github/workflows/ (GitHub App permissions), so any change to the workflow itself needs to be made directly.
• Branch: ci/playwright-workers-experiment

Comment on lines +21 to +24
// Keycloak's login theme CSS references a font Keycloak itself 404s on
const harmlessUrls = [
'/resources-common/node_modules/patternfly/dist/fonts/OpenSans-Light-webfont.',
];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The allowlist covers exactly one font file, but patternfly's CSS references several OpenSans faces (Regular, Semibold, Bold, Italic, …) from the same dist/fonts/ directory, and the browser only fetches the ones a rendered page actually needs — which is presumably why this showed up as a 1-in-8 flake rather than a deterministic failure. If a different weight gets requested on some other login page, you'll hit the identical failure with a different filename and need another round of PR + benchmark.

Since none of these fonts come from Loculus code (they're upstream Keycloak/keycloakify theme resources), allowlisting the directory loses nothing that this check could meaningfully catch:

Suggested change
// Keycloak's login theme CSS references a font Keycloak itself 404s on
const harmlessUrls = [
'/resources-common/node_modules/patternfly/dist/fonts/OpenSans-Light-webfont.',
];
// Keycloak's login theme CSS references patternfly fonts Keycloak itself 404s on
const harmlessUrls = [
'/resources-common/node_modules/patternfly/dist/fonts/',
];

Fix this →

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants