Warm Playwright cache on develop and bump workers to 10#456
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR updates CI pipeline conditions to run tests on the ChangesCI Workflow Update
E2E Test Infrastructure and Reorganization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
nitrobass24
changed the title
Two CI throughput improvements rolled up. 1. Drop the develop-push skip on Build and Test (amd64). With it in place the job never ran on develop merges, so the actions/cache step never populated a develop-scoped Playwright browser cache. PRs targeting develop fall back to looking at their own branch (empty for fresh PRs) and at develop (also empty), and pay the full browser+system-deps install cost every run. Letting the job run on develop pushes warms the cache once per merge so subsequent PRs hit it. 2. Set workers: 5 in playwright.config.ts. Default on a 4-vCPU runner is ~2 workers, which the user observed as "two tests at a time". Bumping to 5 lets five spec files run in parallel. fullyParallel stays false, so within a file tests still run serially. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
settings.spec.ts:176 created and deleted a temporary path pair to
verify the Server Directory field becomes disabled. With workers > 2
that test ran in parallel with path-pairs.spec.ts, whose beforeEach
and afterEach delete every existing pair to start from a clean slate.
The result was a 404 on cleanup ("Failed to delete temp pair … 404")
and intermittently the assertion itself, depending on which spec's
hook fired first.
Move the test into path-pairs.spec.ts so it shares the same per-file
serialized cleanup. No other spec creates pairs from outside its own
describe block, which removes the cross-file pair race entirely.
integrations.spec.ts has the same nuclear-cleanup pattern but no other
spec creates integrations, so it isn't affected today — flagging it
as a pattern to watch as we add more cross-cutting tests.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
nitrobass24
force-pushed
the
ci/cache-warm-and-bump-workers
branch
from
c9f5c35 to
99e5f49
Compare
ubuntu-latest has 4 vCPUs. With workers: 10 we ran 10 chromium processes plus the SeedSync container on those 4 cores, ~2.5x CPU oversubscription. The suite ran slower overall — context-switching overhead and contention on the shared backend dominated any parallelism gain. With fullyParallel: false the per-file serialization also caps useful parallelism at the spec count, and the longest spec (settings) sets the floor regardless of worker count. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Leave one of the runner's 4 vCPUs free for the SeedSync Docker container and runner overhead instead of saturating all four with chromium worker processes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
430-443: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
publishdoesn't gate onbuild-test— E2E failures won't block a develop publish.Now that
build-testruns on develop pushes, a regression that breaks the E2E suite would still allowpublishto proceed and push adevelop-tagged image. Addingbuild-testtopublish'sneedsand condition closes this gap. Using|| needs.build-test.result == 'skipped'handles the tag-push andworkflow_dispatchpaths wherebuild-testis intentionally skipped.🛡️ Proposed addition to the
publishjobpublish: name: Publish if: >- always() && !cancelled() && (startsWith(github.ref, 'refs/tags/v') || (github.ref == 'refs/heads/develop' && github.event_name == 'push') || github.event_name == 'workflow_dispatch') && needs.unit-test.result == 'success' && needs.angular-lint.result == 'success' && needs.python-lint.result == 'success' && needs.python-typecheck.result == 'success' && needs.python-test.result == 'success' && needs.python-integration-test.result == 'success' + && (needs.build-test.result == 'success' || needs.build-test.result == 'skipped') && needs.build-amd64.result == 'success' && needs.build-arm64.result == 'success' - needs: [unit-test, angular-lint, python-lint, python-typecheck, python-test, python-integration-test, build-amd64, build-arm64] + needs: [unit-test, angular-lint, python-lint, python-typecheck, python-test, python-integration-test, build-test, build-amd64, build-arm64]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 430 - 443, The publish job currently does not depend on build-test so E2E/regression failures won’t block publishing for develop; update the publish job (job name "publish") to include build-test in its needs array and add a check for needs.build-test.result in the job's if condition (allowing "skipped" via || needs.build-test.result == 'skipped' to support tag pushes and workflow_dispatch), ensuring the publish job only proceeds when build-test succeeds or is intentionally skipped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 430-443: The publish job currently does not depend on build-test
so E2E/regression failures won’t block publishing for develop; update the
publish job (job name "publish") to include build-test in its needs array and
add a check for needs.build-test.result in the job's if condition (allowing
"skipped" via || needs.build-test.result == 'skipped' to support tag pushes and
workflow_dispatch), ensuring the publish job only proceeds when build-test
succeeds or is intentionally skipped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e8fb929-bf0d-497c-b433-60ff4b0d5e7a
📒 Files selected for processing (4)
.github/workflows/ci.ymlsrc/e2e-playwright/playwright.config.tssrc/e2e-playwright/tests/path-pairs.spec.tssrc/e2e-playwright/tests/settings.spec.ts
💤 Files with no reviewable changes (1)
- src/e2e-playwright/tests/settings.spec.ts
Replace the host-installed Playwright pipeline with a single docker run of mcr.microsoft.com/playwright:v<lockfile-version>-noble. That image ships chromium plus all system deps prebaked, so we eliminate: - Set up Node.js (cache: npm) - Install Playwright dependencies (npm ci on host) - Cache Playwright browsers (actions/cache) - Install Playwright browsers (npx playwright install) - Install Playwright system deps (apt install fonts/xfonts) The version pin is read from package-lock.json with jq so a future @playwright/test bump auto-bumps the image tag — the chromium binary in the image must match the @playwright/test API expectations. Container-to-SUT networking uses --network host so localhost:8800 still resolves to the seedsync test-container started in the previous step. Also drop workers from 3 to 2: with the Playwright image now running as another process group on the same 4-vCPU runner, the previous "leave one core for runner overhead" math becomes "leave two cores for runner + Playwright container coordination." Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Trying 4 (one per vCPU) now that the system-deps install is gone and chromium runs inside the official Playwright image instead of on the host. If we see contention or test races we can drop again. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Build and Test (amd64)so it actually runs on merges to developworkers: 5inplaywright.config.ts(was defaulting to ~2 on the 4-vCPU runner)Why — cache warming
The current
if:skips Build and Test onpushtodevelop. Combined withactions/cache@v4's default branch-scoped storage, this means develop never has a populated Playwright browser cache:GitHub Actions caches inherit from the parent branch, not from sibling PRs. So as long as develop never populates, every fresh PR pays the install cost (~2 min for browsers + ~1 min apt deps when the network cooperates, or 11+ minutes when an Ubuntu mirror flakes).
Letting the job run once on each develop push gives every subsequent PR a cache hit. With cache warm the job runs in ~2-3 min total — a small price for shared cache benefit.
Why — workers
With
fullyParallel: falseand no explicitworkerssetting, Playwright defaults to roughly half the available CPUs in CI — 2 on a 4-vCPU runner. Bumping to 5 lets up to 5 spec files run in parallel. Within each file tests still run serially (sincefullyParallel: falseis preserved).Risks / things to watch
Test plan
Cache hit for ...on the Cache Playwright browsers step and skips browser installRun E2E testsreports up to 5 workers active (visible in the test list output)🤖 Generated with Claude Code
Summary by CodeRabbit