ci: split playwright tests, preempt more CPUs#1706
ci: split playwright tests, preempt more CPUs#1706PupilTong merged 3 commits intolynx-family:mainfrom
Conversation
|
|
Warning Rate limit exceeded@PupilTong has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughReplaces Playwright CI matrix with multi-axis (thread, render, shard) and per-axis env exports; adds workflow input for web-report naming; adjusts Playwright/rspack configs (workerLimit, watcher, .nyc_output setup); and gates several test suites under ENABLE_SSR / ENABLE_MULTI_THREAD. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ Finishing Touches
🧪 Generate unit tests
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web-platform/web-tests/playwright.config.ts (1)
51-57: Fix ReferenceError:testFPOnlyis not defined.This will crash config evaluation on CI. Gate FP-only ignores via
TEST_MODEinstead.-const testIgnore: string[] = (() => { +const testIgnore: string[] = (() => { const ignore = ['**vitest**']; - if (isCI && !testFPOnly) { + if (isCI && TEST_MODE !== 'fp_only') { ignore.push('**/fp-only.spec.ts'); // fp-only tests has its own test steps } return ignore; })();
🧹 Nitpick comments (4)
packages/web-platform/web-tests/playwright.config.ts (3)
11-15: TypeTEST_MODEto allow absence.At runtime
process.env.TEST_MODEmay be undefined; make that explicit to avoid misleading types.-const TEST_MODE = process.env.TEST_MODE as - | 'react' - | 'fp_only' - | 'web_ele' - | 'mts-apis'; +type TestMode = 'react' | 'fp_only' | 'web_ele' | 'mts-apis'; +const TEST_MODE = process.env.TEST_MODE as TestMode | undefined;
17-31: Harden worker computation (min bound, NaN-safe).Ensure at least 1 worker and guard against odd host/container reports.
-const workerLimit = Math.floor(((cpuCount, envCPULimit) => { +const workerLimit = Math.max(1, Math.floor(((cpuCount, envCPULimit) => { if (isCI) { if (envCPULimit) { return envCPULimit / 2; } else { if (cpuCount <= 32) { return 8; } else { return 8 + (cpuCount - 32) / 6; } } } return cpuCount / 2; -})(os.cpus().length, parseFloat(process.env['cpu_limit'] ?? '0'))); +})(os.cpus()?.length ?? 1, Number(process.env['cpu_limit']) || 0)));
78-78: Minor: simplify boolean.
!!isCIcan beisCI.-forbidOnly: !!isCI, +forbidOnly: isCI,.github/workflows/test.yml (1)
123-126: Propagate container CPU quota to Playwright.Set
cpu_limitfromnprocso the config’s worker calculator uses the real container CPUs.- export NODE_OPTIONS="--max-old-space-size=32768" ${{ join(matrix.config.env, ' ') }} + export NODE_OPTIONS="--max-old-space-size=32768" ${{ join(matrix.config.env, ' ') }} + export cpu_limit="$(nproc || echo 0)"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/test.yml(1 hunks)packages/web-platform/web-tests/playwright.config.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/web-platform/web-tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Place Playwright E2E tests in the web platform’s web-tests suite
Files:
packages/web-platform/web-tests/playwright.config.ts
🧠 Learnings (1)
📚 Learning: 2025-09-09T12:38:10.439Z
Learnt from: CR
PR: lynx-family/lynx-stack#0
File: AGENTS.md:0-0
Timestamp: 2025-09-09T12:38:10.439Z
Learning: Applies to packages/web-platform/web-tests/** : Place Playwright E2E tests in the web platform’s web-tests suite
Applied to files:
.github/workflows/test.ymlpackages/web-platform/web-tests/playwright.config.ts
🧬 Code graph analysis (1)
packages/web-platform/web-tests/playwright.config.ts (1)
packages/web-platform/web-tests/rspack.config.js (2)
port(17-17)isCI(16-16)
🔇 Additional comments (2)
packages/web-platform/web-tests/playwright.config.ts (1)
32-47: Confirm glob patterns match actual test files.Before merging, verify that each
TEST_MODEmaps to existing tests to avoid empty shards.#!/bin/bash # List matched files for each TEST_MODE for mode in fp_only react web_ele mts-apis default; do case "$mode" in fp_only) pat='**/fp-only.spec.ts' ;; react) pat='**/react.{test,spec}.ts' ;; web_ele) pat='**/web-elements.{test,spec}.ts' ;; mts-apis) pat='**/main-thread-apis.{test,spec}.ts' ;; default) pat='**/{performance,rpc,web-core}.{test,spec}.ts' ;; esac echo "=== $mode => $pat" rg -n --glob "$pat" -S '' packages/web-platform/web-tests/tests || true done.github/workflows/test.yml (1)
90-117: Matrix/env structure LGTM.The new TEST_MODE-driven split looks coherent and aligns with the config mappings.
PupilTong
force-pushed
the
p/hw/split-ci-for-parallel
branch
from
d16ee56 to
198f9b3
Compare
React Example#5053 Bundle Size — 238.2KiB (0%).62a1a52(current) vs 67ee3e6 main#5022(baseline) Bundle metrics
|
| Current #5053 |
Baseline #5022 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
163 |
163 |
| Duplicate Modules |
67 |
67 |
| Duplicate Code |
46.88% |
46.88% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #5053 |
Baseline #5022 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
92.45KiB |
92.45KiB |
Bundle analysis report Branch PupilTong:p/hw/split-ci-for-para... Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#5046 Bundle Size — 366.97KiB (0%).62a1a52(current) vs 67ee3e6 main#5015(baseline) Bundle metrics
|
| Current #5046 |
Baseline #5015 |
|
|---|---|---|
| Initial JS |
144.4KiB |
144.4KiB |
| Initial CSS |
31.84KiB |
31.84KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
8 |
8 |
| Assets |
8 |
8 |
| Modules |
218 |
218 |
| Duplicate Modules |
16 |
16 |
| Duplicate Code |
3.32% |
3.32% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #5046 |
Baseline #5015 |
|
|---|---|---|
| JS |
236.07KiB |
236.07KiB |
| Other |
99.06KiB |
99.06KiB |
| CSS |
31.84KiB |
31.84KiB |
Bundle analysis report Branch PupilTong:p/hw/split-ci-for-para... Project dashboard
Generated by RelativeCI Documentation Report issue
CodSpeed Performance ReportMerging #1706 will improve performances by 10.69%Comparing 🎉 Hooray!
|
| Benchmark | BASE |
HEAD |
Change | |
|---|---|---|---|---|
| ⚡ | basic-performance-small-css |
7.4 ms | 6.7 ms | +10.69% |
colinaaa
left a comment
colinaaa
left a comment
There was a problem hiding this comment.
Instead of splitting manually, can we use sharding instead? https://playwright.dev/docs/test-sharding
We could do sharding for react tests (~800tests) in future. These tests has different settings we can naturally split them for now. The sharding needs one more task to join all test results, for now doing it manually is more efficient. |
PupilTong
force-pushed
the
p/hw/split-ci-for-parallel
branch
from
198f9b3 to
98587b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/web-platform/web-tests/tests/rpc.spec.ts (1)
200-209: Assertions not executed: missing parentheses on toBeTruthy.Both expectations are properties, not function calls; the assertions never run.
- ).toBeTruthy; + ).toBeTruthy(); @@ - ).toBeTruthy; + ).toBeTruthy();packages/web-platform/web-tests/tests/performance.test.ts (1)
175-175: Bug: ‘??’ prevents retries config from ever running.
isCI ?? expralways returnsisCI(boolean is never null/undefined). Use&&to apply retries in CI.-isCI ?? test.describe.configure({ retries: 8 }); +isCI && test.describe.configure({ retries: 8 });
🧹 Nitpick comments (10)
packages/web-platform/web-tests/tests/rpc.spec.ts (1)
12-12: Nit: clearer skip reason.“no difference for different mode” → “no difference across modes.”
-test.skip(isSSR || ENABLE_MULTI_THREAD, 'no difference for different mode'); +test.skip(isSSR || ENABLE_MULTI_THREAD, 'no difference across modes');packages/web-platform/web-tests/tests/web-core.test.ts (1)
66-66: Nit: wording.“not support ssr” → “does not support SSR”.
-test.skip(isSSR, 'not support ssr'); +test.skip(isSSR, 'does not support SSR');packages/web-platform/web-tests/tests/main-thread-apis.test.ts (1)
19-19: Nit: wording.“mts api tests not support ssr” → “MTS API tests do not support SSR”.
-test.skip(isSSR, 'mts api tests not support ssr'); +test.skip(isSSR, 'MTS API tests do not support SSR');packages/web-platform/web-tests/tests/fp-only.spec.ts (1)
48-48: Nit: wording.“no difference for different mode” → “no difference across modes.”
-test.skip(isSSR || ENABLE_MULTI_THREAD, 'no difference for different mode'); +test.skip(isSSR || ENABLE_MULTI_THREAD, 'no difference across modes');packages/web-platform/web-tests/tests/performance.test.ts (1)
68-68: Nit: wording.“no difference for different mode” → “no difference across modes.”
-test.skip(isSSR || ENABLE_MULTI_THREAD, 'no difference for different mode'); +test.skip(isSSR || ENABLE_MULTI_THREAD, 'no difference across modes');packages/web-platform/web-tests/tests/web-elements.spec.ts (1)
44-44: Nit: wording.“no difference for different mode” → “no difference across modes.”
-test.skip(isSSR || ENABLE_MULTI_THREAD, 'no difference for different mode'); +test.skip(isSSR || ENABLE_MULTI_THREAD, 'no difference across modes');packages/web-platform/web-tests/rspack.config.js (1)
206-209: Broaden watcher to include ESM files.Some deps publish .mjs. Watching only “.js” may miss hot updates locally.
- : ['./node_modules/@lynx-js/**/*.js'], + : [ + './node_modules/@lynx-js/**/*.js', + './node_modules/@lynx-js/**/*.mjs', + ],.github/workflows/test.yml (1)
85-85: Shard label mismatch ("of 4" vs actual 3 shards).Matrix defines 3 shards and CLI uses /3, but the job name says "of 4".
- name: Playwright (${{ matrix.thread }}/${{ matrix.render }} shard ${{ matrix.shard }} of 4) + name: Playwright (${{ matrix.thread }}/${{ matrix.render }} shard ${{ matrix.shard }} of 3)packages/web-platform/web-tests/playwright.config.ts (2)
12-25: Clamp workerLimit to a minimum of 1.On non-CI with a single logical CPU, Math.floor(cpuCount/2) could be 0. Guard against 0 workers.
-const workerLimit = Math.floor(((cpuCount, envCPULimit) => { +const workerLimit = Math.max(1, Math.floor(((cpuCount, envCPULimit) => { if (isCI) { if (envCPULimit) { return envCPULimit / 2; } else { if (cpuCount <= 32) { return 8; } else { return 8 + (cpuCount - 32) / 6; } } } return cpuCount / 2; -})(os.cpus().length, parseFloat(process.env['cpu_limit'] ?? '0'))); +})(os.cpus().length, parseFloat(process.env['cpu_limit'] ?? '0'))));
45-46: Remove dead comment or wire testMatch back if needed.Keeping stale commented config invites drift.
- // testMatch,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/test.yml(1 hunks).github/workflows/workflow-test.yml(2 hunks)packages/web-platform/web-tests/playwright.config.ts(2 hunks)packages/web-platform/web-tests/rspack.config.js(2 hunks)packages/web-platform/web-tests/tests/fp-only.spec.ts(2 hunks)packages/web-platform/web-tests/tests/main-thread-apis.test.ts(2 hunks)packages/web-platform/web-tests/tests/performance.test.ts(2 hunks)packages/web-platform/web-tests/tests/rpc.spec.ts(1 hunks)packages/web-platform/web-tests/tests/web-core.test.ts(2 hunks)packages/web-platform/web-tests/tests/web-elements.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/web-platform/web-tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Place Playwright E2E tests in the web platform’s web-tests suite
Files:
packages/web-platform/web-tests/tests/rpc.spec.tspackages/web-platform/web-tests/tests/main-thread-apis.test.tspackages/web-platform/web-tests/tests/fp-only.spec.tspackages/web-platform/web-tests/rspack.config.jspackages/web-platform/web-tests/tests/web-core.test.tspackages/web-platform/web-tests/tests/web-elements.spec.tspackages/web-platform/web-tests/tests/performance.test.tspackages/web-platform/web-tests/playwright.config.ts
🧠 Learnings (4)
📚 Learning: 2025-07-22T09:26:16.722Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:26:16.722Z
Learning: In the lynx-family/lynx-stack repository, CI checks require changesets when files matching the pattern "src/**" are modified (as configured in .changeset/config.json). For internal changes that don't need meaningful changesets, an empty changeset file is used to satisfy the CI requirement while not generating any release notes.
Applied to files:
packages/web-platform/web-tests/rspack.config.js
📚 Learning: 2025-08-06T13:28:57.182Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1453
File: vitest.config.ts:49-61
Timestamp: 2025-08-06T13:28:57.182Z
Learning: In the lynx-family/lynx-stack repository, the file `packages/react/testing-library/src/vitest.config.js` is source code for the testing library that gets exported for users, not a test configuration that should be included in the main vitest projects array.
Applied to files:
packages/web-platform/web-tests/rspack.config.js
📚 Learning: 2025-08-06T13:28:57.182Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1453
File: vitest.config.ts:49-61
Timestamp: 2025-08-06T13:28:57.182Z
Learning: In the lynx-family/lynx-stack repository, the file `packages/rspeedy/create-rspeedy/template-react-vitest-rltl-js/vitest.config.js` is a template file for scaffolding new Rspeedy projects, not a test configuration that should be included in the main vitest projects array.
Applied to files:
packages/web-platform/web-tests/rspack.config.js
📚 Learning: 2025-09-09T12:38:10.439Z
Learnt from: CR
PR: lynx-family/lynx-stack#0
File: AGENTS.md:0-0
Timestamp: 2025-09-09T12:38:10.439Z
Learning: Applies to packages/web-platform/web-tests/** : Place Playwright E2E tests in the web platform’s web-tests suite
Applied to files:
packages/web-platform/web-tests/playwright.config.ts
🧬 Code graph analysis (4)
packages/web-platform/web-tests/tests/main-thread-apis.test.ts (1)
packages/web-platform/web-tests/tests/coverage-fixture.ts (1)
test(11-61)
packages/web-platform/web-tests/tests/fp-only.spec.ts (1)
packages/webpack/template-webpack-plugin/test/cases/assets/production/rspack.config.js (1)
process(34-34)
packages/web-platform/web-tests/tests/web-core.test.ts (1)
packages/web-platform/web-tests/tests/coverage-fixture.ts (1)
test(11-61)
packages/web-platform/web-tests/tests/performance.test.ts (1)
packages/webpack/template-webpack-plugin/test/cases/assets/production/rspack.config.js (1)
process(34-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build / Build (Windows)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: test-rust / Test (Ubuntu)
- GitHub Check: test-rust / clippy
🔇 Additional comments (9)
packages/web-platform/web-tests/rspack.config.js (1)
239-241: CI watch suppression LGTM.
watchOptions.ignoredin CI prevents unnecessary file watching and reduces CPU..github/workflows/workflow-test.yml (2)
20-23: Config input LGTM.
web-report-nameinput is a clean way to name Playwright artifacts per run.
95-95: Verify callers set unique artifact names per matrix shard.If multiple shards default to the same
web-report-name, artifacts can overwrite. Ensure producers pass a unique value (e.g., include matrix keys).Would you like me to scan
.github/workflows/test.ymland related callers to confirm each matrix job sets a uniqueweb-report-name? I can provide a script..github/workflows/test.yml (3)
101-106: Env exports align with per-test skip guards. LGTM.Setting ENABLE_MULTI_THREAD/ENABLE_SSR per-axis cleanly drives the new test-level gating.
98-99: Per-run report naming and reporters look consistent.web-report-name uniqueness + blob/junit reporters should integrate with your upload step. Just ensure the upload step consumes the blob-report for each shard.
Also applies to: 108-110
91-95: Relying on partial matrix.exclude matching is safe
GitHub Actions supports partial-match excludes, so the existingthread: mt+render: ssrrule will omit that combination across all shards. You may optionally list eachshardexplicitly for clarity, but no change is required.packages/web-platform/web-tests/playwright.config.ts (3)
27-31: Config defaults and CI heuristics look solid.Static testIgnore, CI-aware retries/forbidOnly/maxFailures, and trace policy are reasonable.
Also applies to: 41-75
5-11: Projects and webServer config look fine.Device presets and chromium launch args are consistent with visual stability goals.
Also applies to: 76-125
1-125: No remaining TEST_MODE/TEST_FP_ONLY consumers found.
PupilTong
force-pushed
the
p/hw/split-ci-for-parallel
branch
from
98587b5 to
dc52195
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/web-platform/web-tests/tests/main-thread-apis.test.ts (1)
9-10: Parse env booleans correctly (treat "false" as false).Using
!!process.env...makes"false"truthy. Parse explicit truthy tokens for both flags.-const ENABLE_MULTI_THREAD = !!process.env.ENABLE_MULTI_THREAD; -const isSSR = !!process.env['ENABLE_SSR']; +const ENABLE_MULTI_THREAD = /^(1|true|yes|on)$/i.test(process.env.ENABLE_MULTI_THREAD ?? ''); +const isSSR = /^(1|true|yes|on)$/i.test(process.env.ENABLE_SSR ?? '');
🧹 Nitpick comments (2)
packages/web-platform/web-tests/tests/main-thread-apis.test.ts (1)
19-19: Polish skip message.Clearer, grammatically correct message.
- test.skip(isSSR, 'mts api tests not support ssr'); + test.skip(isSSR, 'main-thread API tests do not support SSR');.github/workflows/test.yml (1)
85-85: Parameterize shard count to avoid “/4” drift.Keep shard total in one place; use it in the job name, report name, and CLI.
- name: Playwright (${{ matrix.thread }}/${{ matrix.render }} shard ${{ matrix.shard }} of 4) + name: Playwright (${{ matrix.thread }}/${{ matrix.render }} shard ${{ matrix.shard }} of ${{ matrix.total_shards }}) matrix: thread: [ui, mt] render: [csr, ssr] shard: [1, 2, 3, 4] + total_shards: [4] exclude: - thread: mt render: ssr @@ - web-report-name: "playwright-${{ matrix.thread }}-${{ matrix.render }}-shard${{ matrix.shard }}" + web-report-name: "playwright-${{ matrix.thread }}-${{ matrix.render }}-shard${{ matrix.shard }}-of-${{ matrix.total_shards }}" @@ - pnpm --filter @lynx-js/web-tests run test --reporter='github,dot,junit,html,blob' --shard=${{ matrix.shard }}/4 + pnpm --filter @lynx-js/web-tests run test --reporter='github,dot,junit,html,blob' --shard=${{ matrix.shard }}/${{ matrix.total_shards }}Also applies to: 91-91, 98-99, 109-109
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/test.yml(1 hunks).github/workflows/workflow-test.yml(2 hunks)packages/web-platform/web-tests/playwright.config.ts(2 hunks)packages/web-platform/web-tests/rspack.config.js(2 hunks)packages/web-platform/web-tests/tests/fp-only.spec.ts(2 hunks)packages/web-platform/web-tests/tests/main-thread-apis.test.ts(2 hunks)packages/web-platform/web-tests/tests/performance.test.ts(2 hunks)packages/web-platform/web-tests/tests/rpc.spec.ts(1 hunks)packages/web-platform/web-tests/tests/web-core.test.ts(2 hunks)packages/web-platform/web-tests/tests/web-elements.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/web-platform/web-tests/tests/rpc.spec.ts
- packages/web-platform/web-tests/tests/web-core.test.ts
- packages/web-platform/web-tests/tests/fp-only.spec.ts
- .github/workflows/workflow-test.yml
- packages/web-platform/web-tests/tests/performance.test.ts
- packages/web-platform/web-tests/rspack.config.js
- packages/web-platform/web-tests/tests/web-elements.spec.ts
- packages/web-platform/web-tests/playwright.config.ts
🧰 Additional context used
📓 Path-based instructions (1)
packages/web-platform/web-tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Place Playwright E2E tests in the web platform’s web-tests suite
Files:
packages/web-platform/web-tests/tests/main-thread-apis.test.ts
🧬 Code graph analysis (1)
packages/web-platform/web-tests/tests/main-thread-apis.test.ts (1)
packages/web-platform/web-tests/tests/coverage-fixture.ts (1)
test(11-61)
🔇 Additional comments (1)
.github/workflows/test.yml (1)
85-95: LGTM on matrix refactor and mt/ssr exclusion.The axis split and exclusion look correct and align with the PR goal.
PupilTong
force-pushed
the
p/hw/split-ci-for-parallel
branch
from
dc52195 to
ed568b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/web-platform/web-tests/tests/rpc.spec.ts (1)
8-9: Parse env flags robustly (avoid treating "false"/"0" as true).
!!process.env.*will skip when the value is the string "false"/"0". Parse explicit truthy values.Apply:
-const ENABLE_MULTI_THREAD = !!process.env['ENABLE_MULTI_THREAD']; -const isSSR = !!process.env['ENABLE_SSR']; +const ENABLE_MULTI_THREAD = /^(1|true|yes|on)$/i.test(process.env.ENABLE_MULTI_THREAD ?? ''); +const isSSR = /^(1|true|yes|on)$/i.test(process.env.ENABLE_SSR ?? '');Optionally centralize in a shared helper (tests/env.ts) to keep flags consistent across suites.
Run to ensure CI/workflows use the same variable names:
#!/bin/bash # Verify env names are consistent across workflows and tests rg -nI --no-ignore -C2 -g ".github/workflows/**" -g "packages/web-platform/web-tests/**" \ -e '\bENABLE_(MULTI_THREAD|SSR)\b'
🧹 Nitpick comments (1)
packages/web-platform/web-tests/tests/rpc.spec.ts (1)
12-12: Clarify skip reason string (minor).Tweak wording for readability; logic is fine.
-test.skip(isSSR || ENABLE_MULTI_THREAD, 'no difference for different mode'); +test.skip(isSSR || ENABLE_MULTI_THREAD, 'Skipped in SSR/multi-thread shards: no coverage gain across modes');
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/test.yml(1 hunks).github/workflows/workflow-test.yml(2 hunks)packages/web-platform/web-tests/playwright.config.ts(2 hunks)packages/web-platform/web-tests/rspack.config.js(2 hunks)packages/web-platform/web-tests/tests/fp-only.spec.ts(2 hunks)packages/web-platform/web-tests/tests/main-thread-apis.test.ts(2 hunks)packages/web-platform/web-tests/tests/performance.test.ts(2 hunks)packages/web-platform/web-tests/tests/rpc.spec.ts(1 hunks)packages/web-platform/web-tests/tests/web-core.test.ts(2 hunks)packages/web-platform/web-tests/tests/web-elements.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/web-platform/web-tests/tests/fp-only.spec.ts
- packages/web-platform/web-tests/tests/web-core.test.ts
- .github/workflows/workflow-test.yml
- .github/workflows/test.yml
- packages/web-platform/web-tests/tests/main-thread-apis.test.ts
- packages/web-platform/web-tests/tests/performance.test.ts
- packages/web-platform/web-tests/tests/web-elements.spec.ts
- packages/web-platform/web-tests/rspack.config.js
- packages/web-platform/web-tests/playwright.config.ts
🧰 Additional context used
📓 Path-based instructions (1)
packages/web-platform/web-tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Place Playwright E2E tests in the web platform’s web-tests suite
Files:
packages/web-platform/web-tests/tests/rpc.spec.ts
🧬 Code graph analysis (1)
packages/web-platform/web-tests/tests/rpc.spec.ts (1)
packages/webpack/template-webpack-plugin/test/cases/assets/production/rspack.config.js (1)
process(34-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: build / Build (Windows)
- GitHub Check: test-rust / Test (Ubuntu)
PupilTong
force-pushed
the
p/hw/split-ci-for-parallel
branch
2 times, most recently
from
14ea776 to
47894f6
Compare
To mitigate CPU timing limitation for each container task in lynx's CI. we create more runner to preempt more CPU resources.
PupilTong
force-pushed
the
p/hw/split-ci-for-parallel
branch
from
47894f6 to
1c2673b
Compare
Co-authored-by: Qingyu Wang <40660121+colinaaa@users.noreply.github.com> Signed-off-by: Haoyang Wang <12288479+PupilTong@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/web-platform/web-tests/tests/main-thread-apis.test.ts (1)
10-10: Fix boolean parsing of ENABLE_SSR (current code misinterprets "false"/"0" as true).Use an explicit truthy check so CI doesn't skip/run suites incorrectly when ENABLE_SSR is set to "false" or "0".
-const isSSR = !!process.env['ENABLE_SSR']; +const isSSR = /^(1|true|yes|on)$/i.test(process.env.ENABLE_SSR ?? '');
🧹 Nitpick comments (4)
packages/web-platform/web-tests/tests/main-thread-apis.test.ts (1)
19-19: Clarify skip reason (grammar/clarity).- test.skip(isSSR, 'mts api tests not support ssr'); + test.skip(isSSR, 'Main-thread API tests do not support SSR');packages/web-platform/web-tests/tests/coverage-fixture.ts (2)
9-9: Fix ESM __dirname derivation (currently a file path, not a directory).
fileURLToPath(import.meta.url)returns the file path, but it’s assigned to__dirname. Use__filename+path.dirnamefor clarity and consistency with other files.-import { fileURLToPath } from 'node:url'; +import { fileURLToPath } from 'node:url'; -const __dirname = fileURLToPath(import.meta.url); +const __filename = fileURLToPath(import.meta.url); +const __dirname = path.dirname(__filename);
32-58: Avoid potential name collisions and usemapinstead offlatMap.
flatMapis unnecessary here;mapis clearer for producing an array of Promises.- File name based only on
testInfo.titlecan collide across specs. Include the spec file to make names unique.- await Promise.all( - Array.from(pages.values()).flatMap(async (page, index) => { + await Promise.all( + Array.from(pages.values()).map(async (page, index) => { const coverage = await page.coverage.stopJSCoverage(); … - return fs.writeFile( - path.join( - dir, - `playwright_output_${ - testInfo.title.replaceAll('/', '_') - }_${index}.json`, - ), + const spec = path.basename(testInfo.file ?? 'unknown.spec.ts').replaceAll('/', '_'); + const name = `playwright_output_${spec}__${testInfo.title.replaceAll('/', '_')}__${index}.json`; + return fs.writeFile( + path.join(dir, name), JSON.stringify(coverageMapData), { flag: 'w' }, ); }), );.github/workflows/test.yml (1)
85-85: Shard label mismatch: update “/4” to “/3”.Matrix uses 3 shards (
[1, 2, 3]) and CLI passes--shard=…/3, but the job name shows/4. Fix the label for clarity.- name: Playwright (${{ matrix.thread }}-${{ matrix.render }}) ${{ matrix.shard }}/4 + name: Playwright (${{ matrix.thread }}-${{ matrix.render }}) ${{ matrix.shard }}/3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/test.yml(1 hunks).github/workflows/workflow-test.yml(2 hunks)packages/web-platform/web-tests/playwright.config.ts(2 hunks)packages/web-platform/web-tests/rspack.config.js(2 hunks)packages/web-platform/web-tests/tests/coverage-fixture.ts(1 hunks)packages/web-platform/web-tests/tests/fp-only.spec.ts(2 hunks)packages/web-platform/web-tests/tests/main-thread-apis.test.ts(2 hunks)packages/web-platform/web-tests/tests/performance.test.ts(2 hunks)packages/web-platform/web-tests/tests/rpc.spec.ts(1 hunks)packages/web-platform/web-tests/tests/web-core.test.ts(2 hunks)packages/web-platform/web-tests/tests/web-elements.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/web-platform/web-tests/tests/rpc.spec.ts
- packages/web-platform/web-tests/tests/web-core.test.ts
- packages/web-platform/web-tests/tests/web-elements.spec.ts
- packages/web-platform/web-tests/tests/fp-only.spec.ts
- packages/web-platform/web-tests/tests/performance.test.ts
- .github/workflows/workflow-test.yml
- packages/web-platform/web-tests/rspack.config.js
🧰 Additional context used
📓 Path-based instructions (1)
packages/web-platform/web-tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Place Playwright E2E tests in the web platform’s web-tests suite
Files:
packages/web-platform/web-tests/tests/coverage-fixture.tspackages/web-platform/web-tests/tests/main-thread-apis.test.tspackages/web-platform/web-tests/playwright.config.ts
🧠 Learnings (1)
📚 Learning: 2025-09-09T12:38:10.439Z
Learnt from: CR
PR: lynx-family/lynx-stack#0
File: AGENTS.md:0-0
Timestamp: 2025-09-09T12:38:10.439Z
Learning: Applies to packages/web-platform/web-tests/** : Place Playwright E2E tests in the web platform’s web-tests suite
Applied to files:
packages/web-platform/web-tests/tests/coverage-fixture.tspackages/web-platform/web-tests/playwright.config.ts
🧬 Code graph analysis (3)
packages/web-platform/web-tests/tests/coverage-fixture.ts (1)
packages/web-platform/web-tests/rspack.config.js (1)
__dirname(14-14)
packages/web-platform/web-tests/tests/main-thread-apis.test.ts (1)
packages/web-platform/web-tests/tests/coverage-fixture.ts (1)
test(11-60)
packages/web-platform/web-tests/playwright.config.ts (1)
packages/web-platform/web-tests/rspack.config.js (1)
isCI(16-16)
🔇 Additional comments (6)
packages/web-platform/web-tests/tests/coverage-fixture.ts (1)
13-14: Create NYC dir earlier: good placement for sharded runs.Initializing the coverage output directory before the Chromium guard avoids race/missing-dir issues across shards. LGTM.
.github/workflows/test.yml (4)
89-94: Matrix split and exclusion look correct.Thread/render axes and the
MULTI_THREAD+SSRexclusion align with the stated CI strategy.
98-98: Per-axis web report naming: good for artifact disambiguation.Distinct
web-report-namevalues per shard/thread/render will keep reports easy to trace.
101-107: Env export per axis is appropriate.Conditionally exporting
ENABLE_MULTI_THREAD/ENABLE_SSRmatches test gating behavior.
109-109: Confirm JUnit reporter wiring.You set
PLAYWRIGHT_JUNIT_OUTPUT_NAME, and also pass--reporter='github,dot,junit,html'. Ensure the reusable workflow consumesPLAYWRIGHT_JUNIT_OUTPUT_NAMEto pick the file up; otherwise specifyjunit’soutputFiledirectly in the CLI or config.Would you like me to scan the repo for references to
PLAYWRIGHT_JUNIT_OUTPUT_NAMEand suggest a consistent wiring if missing?packages/web-platform/web-tests/playwright.config.ts (1)
49-49: Workers assignment uses computed limit: good.With the clamp above, this will scale safely on CI.
PupilTong
force-pushed
the
p/hw/split-ci-for-parallel
branch
2 times, most recently
from
6068016 to
a4f3fcd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/web-platform/web-tests/playwright.config.ts (1)
12-25: Clamp workers to ≥1 and honor cgroup CPU limits (fix potential workers=0).If cpu_limit < 2 (e.g., 0.5, 1),
envCPULimit / 2floors to 0; Playwrightworkers: 0is invalid and will break runs. Also preferos.availableParallelism()to respect container quotas. Tighten the env check to avoid NaN/0 truthiness pitfalls.Apply:
-const workerLimit = Math.floor(((cpuCount, envCPULimit) => { +const workerLimit = Math.max(1, Math.floor(((cpuCount, envCPULimit) => { if (isCI) { - if (envCPULimit) { + if (Number.isFinite(envCPULimit) && envCPULimit > 0) { return envCPULimit / 2; } else { if (cpuCount <= 32) { return 6; } else { return 6 + (cpuCount - 32) / 6; } } } return cpuCount / 2; -})(os.cpus().length, parseFloat(process.env['cpu_limit'] ?? '0'))); +})( + typeof os.availableParallelism === 'function' ? os.availableParallelism() : os.cpus().length, + Number.parseFloat(process.env['cpu_limit'] ?? '') || 0 +)));.github/workflows/test.yml (1)
85-85: Job name clarity LGTM.Adopts the suggested shard notation
(${{ matrix.shard }}/4)—clear and readable.
🧹 Nitpick comments (2)
packages/web-platform/web-tests/playwright.config.ts (1)
45-45: Remove dead// testMatchstub or rewire intentionally.If selection has fully moved to in-test gating + sharding, drop the commented line to avoid confusion. If not, wire it back explicitly.
- // testMatch, + // Intentionally selecting via in-test env gates and --shard; no testMatch..github/workflows/test.yml (1)
109-109: Sharding now in use—update PR description for accuracy.The workflow runs
--shard=${{ matrix.shard }}/4; consider updating the PR description that previously argued against sharding to reflect the current approach.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/test.yml(1 hunks)packages/web-platform/web-tests/playwright.config.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/web-platform/web-tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Place Playwright E2E tests in the web platform’s web-tests suite
Files:
packages/web-platform/web-tests/playwright.config.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: lynx-family/lynx-stack#0
File: AGENTS.md:0-0
Timestamp: 2025-09-09T12:38:10.439Z
Learning: Applies to packages/web-platform/web-tests/** : Place Playwright E2E tests in the web platform’s web-tests suite
📚 Learning: 2025-09-09T12:38:10.439Z
Learnt from: CR
PR: lynx-family/lynx-stack#0
File: AGENTS.md:0-0
Timestamp: 2025-09-09T12:38:10.439Z
Learning: Applies to packages/web-platform/web-tests/** : Place Playwright E2E tests in the web platform’s web-tests suite
Applied to files:
packages/web-platform/web-tests/playwright.config.ts
🧬 Code graph analysis (1)
packages/web-platform/web-tests/playwright.config.ts (1)
packages/web-platform/web-tests/rspack.config.js (1)
isCI(16-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: build / Build (Windows)
🔇 Additional comments (1)
.github/workflows/test.yml (1)
101-109: Verified mode gating for tests
All web-platform/web-tests cases referencing ENABLE_MULTI_THREAD or ENABLE_SSR include appropriatetest.skip/test.fixmeguards; no unguarded mode-specific tests were found.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
.github/workflows/test.yml (2)
89-91: Matrix values don't match past review suggestions.Based on past review comments, the expected values for the matrix should be:
thread: [UI, MultiThread]instead of[ALL_ON_UI, MULTI_THREAD]render: [CSR, SSR](this is correct)
92-94: Exclusion values don't match past review suggestions.The exclusion should use
MultiThreadandSSRto match the suggested matrix values from past reviews.packages/web-platform/web-tests/playwright.config.ts (1)
12-25: Workers can become 0 when cpu_limit < 2 (breaks runs). Clamp to ≥1.If
cpu_limitis fractional (e.g.,0.5,1.5),envCPULimit / 2floors to0. Playwrightworkers: 0is invalid and will cause test runs to fail.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/test.yml(1 hunks)packages/web-platform/web-tests/playwright.config.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/web-platform/web-tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Place Playwright E2E tests in the web platform’s web-tests suite
Files:
packages/web-platform/web-tests/playwright.config.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: lynx-family/lynx-stack#0
File: AGENTS.md:0-0
Timestamp: 2025-09-09T12:38:10.439Z
Learning: Applies to packages/web-platform/web-tests/** : Place Playwright E2E tests in the web platform’s web-tests suite
📚 Learning: 2025-09-09T12:38:10.439Z
Learnt from: CR
PR: lynx-family/lynx-stack#0
File: AGENTS.md:0-0
Timestamp: 2025-09-09T12:38:10.439Z
Learning: Applies to packages/web-platform/web-tests/** : Place Playwright E2E tests in the web platform’s web-tests suite
Applied to files:
packages/web-platform/web-tests/playwright.config.ts
🧬 Code graph analysis (1)
packages/web-platform/web-tests/playwright.config.ts (1)
packages/web-platform/web-tests/rspack.config.js (1)
isCI(16-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build / Build (Windows)
🔇 Additional comments (5)
.github/workflows/test.yml (2)
98-98: Web report naming looks good.The per-axis web report naming aligns well with the sharding strategy and will help distinguish test results across different matrix combinations.
101-107: Environment variable setup looks correct.The conditional environment variable exports properly map matrix values to the expected environment flags that the tests use for gating. This aligns with the removal of environment-driven test matching from the Playwright config.
packages/web-platform/web-tests/playwright.config.ts (3)
17-21: Updated worker limits look reasonable.The change from
8workers to6workers for systems with ≤32 CPUs, and the adjusted scaling formula6 + (cpuCount - 32) / 6should help better manage resource allocation in the CI environment.
27-30: Simplified testIgnore is cleaner.Removing the CI-specific conditional logic and keeping only
['**vitest**']simplifies the configuration since test selection is now handled by sharding and environment-based test gating.
45-45: No TEST_MODE env var usage detected
Search confirms zero occurrences ofTEST_MODEorprocess.env.TEST_MODEinpackages/web-platform/web-tests/tests/; the onlyfp-onlymatches are filenames and URLs, not conditional environment checks.
PupilTong
force-pushed
the
p/hw/split-ci-for-parallel
branch
from
a4f3fcd to
b0776ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
89-92: Shard count increased to 10—watch for imbalance.Playwright sharding is file-based; a few very large specs can skew shard runtimes. Consider splitting oversized specs or grouping heavy cases under multiple files for better balance.
If helpful, I can generate a quick report of test counts per file to identify hot spots.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/test.yml(1 hunks)packages/web-platform/web-tests/playwright.config.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web-platform/web-tests/playwright.config.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: lynx-family/lynx-stack#0
File: AGENTS.md:0-0
Timestamp: 2025-09-09T12:38:10.439Z
Learning: Applies to packages/web-platform/web-tests/** : Place Playwright E2E tests in the web platform’s web-tests suite
🔇 Additional comments (4)
.github/workflows/test.yml (4)
85-85: Job name placement and content look correct.Name is inside the job, parameterized by matrix axes and shard count. Good.
101-107: Per-axis env export is clear and safe.Conditional exports for
ENABLE_MULTI_THREADandENABLE_SSRavoid leakage. No change needed.
98-98:web-report-nameinput consumption verified. The reusable workflow (.github/workflows/workflow-test.yml) declares theweb-report-nameinput (line 20) and uses it as the artifactnamein the Upload Test Result step (line 95), preventing shard collisions.
89-94: Exclude list is complete. Verified through test guards that only the MULTI_THREAD × SSR combination is fully unsupported—no other thread×render pairs are entirely skipped.
PupilTong
force-pushed
the
p/hw/split-ci-for-parallel
branch
from
1b759b0 to
d9f6ab9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/test.yml (2)
85-85: Fix job name placement - it should be inside the workflow job definition.The
namefield is currently at the wrong indentation level. In GitHub Actions reusable workflows, the job name cannot be defined here.Remove this line as job names cannot be customized when calling reusable workflows:
secrets: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} - name: Playwright ${{ matrix.thread }}-${{ matrix.render }} (${{ matrix.shard }}/4) strategy:
101-109: Verify JUnit XML upload and consider test distribution balance.The test execution looks correct, but there are two concerns:
- The JUnit XML file needs to be uploaded alongside the HTML report
- Test distribution might be unbalanced due to file-based sharding
#!/bin/bash # Check if the reusable workflow uploads the JUnit XML echo "=== Checking workflow-test.yml for artifact uploads ===" grep -A 10 -B 2 "upload.*artifact\|Upload.*Result" .github/workflows/workflow-test.yml # Check test file distribution echo -e "\n=== Test file sizes for sharding analysis ===" for file in packages/web-platform/web-tests/tests/*.spec.ts packages/web-platform/web-tests/tests/*.test.ts; do if [ -f "$file" ]; then testcount=$(grep -c "test\s*(" "$file" 2>/dev/null || echo 0) printf "%4d tests: %s\n" "$testcount" "$(basename "$file")" fi done | sort -rnpackages/web-platform/web-tests/playwright.config.ts (1)
18-31: Add minimum worker limit to prevent invalid configuration.When
cpu_limitis less than 2 (e.g., 0.5, 1.5), the calculation can result in 0 workers, which is invalid for Playwright.Apply this fix to ensure at least 1 worker:
-const workerLimit = Math.floor(((cpuCount, envCPULimit) => { +const workerLimit = Math.max(1, Math.floor(((cpuCount, envCPULimit) => { if (isCI) { if (envCPULimit) { return envCPULimit / 2; } else { if (cpuCount <= 32) { return 4; } else { return 4 + (cpuCount - 32) / 6; } } } return cpuCount / 2; -})(os.cpus().length, parseFloat(process.env['cpu_limit'] ?? '0'))); +})(os.cpus().length, parseFloat(process.env['cpu_limit'] ?? '0'))));
🧹 Nitpick comments (1)
packages/web-platform/web-tests/playwright.config.ts (1)
51-51: Clean up commented code or document why it's preserved.The commented
testMatchsuggests this was intentionally removed. Either remove the comment entirely or add a note explaining why it's kept for reference.Either remove the comment or document it:
- // testMatch, + // testMatch removed - test selection now handled via environment variables in individual test files
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/test.yml(1 hunks)packages/web-platform/web-tests/playwright.config.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/web-platform/web-tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Place Playwright E2E tests in the web platform’s web-tests suite
Files:
packages/web-platform/web-tests/playwright.config.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: lynx-family/lynx-stack#0
File: AGENTS.md:0-0
Timestamp: 2025-09-09T12:38:10.439Z
Learning: Applies to packages/web-platform/web-tests/** : Place Playwright E2E tests in the web platform’s web-tests suite
📚 Learning: 2025-09-09T12:38:10.439Z
Learnt from: CR
PR: lynx-family/lynx-stack#0
File: AGENTS.md:0-0
Timestamp: 2025-09-09T12:38:10.439Z
Learning: Applies to packages/web-platform/web-tests/** : Place Playwright E2E tests in the web platform’s web-tests suite
Applied to files:
packages/web-platform/web-tests/playwright.config.ts
🧬 Code graph analysis (1)
packages/web-platform/web-tests/playwright.config.ts (2)
packages/web-platform/web-tests/rspack.config.js (3)
__dirname(14-14)isCI(16-16)port(17-17)packages/web-platform/web-tests/server.js (1)
__dirname(6-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build / Build (Windows)
- GitHub Check: build / Build (Ubuntu)
- GitHub Check: test-rust / Test (Ubuntu)
🔇 Additional comments (2)
.github/workflows/test.yml (2)
98-98: LGTM! Good naming convention for test artifacts.The dynamic web-report-name clearly identifies each shard's results, making debugging easier when tests fail.
89-94: Retain only the MULTI_THREAD×SSR exclusion. Other flag combinations are supported (tests merely skip irrelevant cases), so no additional matrix exclusions are needed.
PupilTong
force-pushed
the
p/hw/split-ci-for-parallel
branch
2 times, most recently
from
e6e6ce2 to
5b99ff8
Compare
PupilTong
force-pushed
the
p/hw/split-ci-for-parallel
branch
from
5b99ff8 to
62a1a52
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web-platform/web-tests/playwright.config.ts (1)
62-63: Define reporters here and wire JUnit output to env var.This lets CI set PLAYWRIGHT_JUNIT_OUTPUT_NAME and drop CLI reporters.
Apply:
- reporter: 'html', + reporter: [ + ['github'], + ['dot'], + ['junit', { outputFile: process.env['PLAYWRIGHT_JUNIT_OUTPUT_NAME'] ?? 'test-report.junit.xml' }], + ['html'], + ],
♻️ Duplicate comments (2)
packages/web-platform/web-tests/playwright.config.ts (2)
11-13: Fix __dirname resolution and await directory creation.Use the directory of the module, and await mkdir to avoid race conditions.
Apply:
-const __dirname = fileURLToPath(import.meta.url); -const dir = path.join(__dirname, '..', '.nyc_output'); -fs.mkdir(dir, { recursive: true }); +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const dir = path.join(__dirname, '..', '.nyc_output'); +await fs.mkdir(dir, { recursive: true }).catch(() => {});
18-31: Clamp workers to ≥1 (cpu_limit < 2 can yield 0).Playwright rejects workers=0; clamp result.
Apply:
-const workerLimit = Math.floor(((cpuCount, envCPULimit) => { +const workerLimit = Math.max(1, Math.floor(((cpuCount, envCPULimit) => { if (isCI) { if (envCPULimit) { return envCPULimit / 2; } else { if (cpuCount <= 32) { return 8; } else { return 8 + (cpuCount - 32) / 6; } } } return cpuCount / 2; -})(os.cpus().length, parseFloat(process.env['cpu_limit'] ?? '0'))); +})(os.cpus().length, parseFloat(process.env['cpu_limit'] ?? '0') || 0)));Consider supporting Kubernetes-style millicpu (e.g., "1500m"):
const raw = process.env['cpu_limit'] ?? '0'; const limit = raw.endsWith('m') ? parseFloat(raw) / 1000 : parseFloat(raw);
🧹 Nitpick comments (2)
.github/workflows/test.yml (1)
108-109: Make JUnit filenames unique per shard and define reporters in config (avoid CLI override).Unique XML names avoid collisions; letting config own reporters enables setting JUnit outputFile.
Apply:
- export PLAYWRIGHT_JUNIT_OUTPUT_NAME=test-report.junit.xml - pnpm --filter @lynx-js/web-tests run test --reporter='github,dot,junit,html' --shard=${{ matrix.shard }}/4 + export PLAYWRIGHT_JUNIT_OUTPUT_NAME="junit-${{ matrix.thread }}-${{ matrix.render }}-shard${{ matrix.shard }}.xml" + pnpm --filter @lynx-js/web-tests run test --shard=${{ matrix.shard }}/4Confirm the reusable workflow uploads the XML file path that config will emit (see my Playwright config comment). If not, add it to the artifact upload step.
packages/web-platform/web-tests/playwright.config.ts (1)
51-51: Remove dead comment or restore testMatch intentionally.Leaving a commented placeholder can confuse future maintainers.
Apply:
- // testMatch, + // Intentionally using default testMatch; tests gated by env.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/test.yml(1 hunks)packages/web-platform/web-tests/playwright.config.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
packages/web-platform/web-tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Place Playwright E2E tests in the web platform’s web-tests suite
Files:
packages/web-platform/web-tests/playwright.config.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: lynx-family/lynx-stack#0
File: AGENTS.md:0-0
Timestamp: 2025-09-09T12:38:10.439Z
Learning: Applies to packages/web-platform/web-tests/** : Place Playwright E2E tests in the web platform’s web-tests suite
📚 Learning: 2025-09-09T12:38:10.439Z
Learnt from: CR
PR: lynx-family/lynx-stack#0
File: AGENTS.md:0-0
Timestamp: 2025-09-09T12:38:10.439Z
Learning: Applies to packages/web-platform/web-tests/** : Place Playwright E2E tests in the web platform’s web-tests suite
Applied to files:
packages/web-platform/web-tests/playwright.config.ts
🧬 Code graph analysis (1)
packages/web-platform/web-tests/playwright.config.ts (3)
packages/web-platform/web-tests/rspack.config.js (3)
__dirname(14-14)isCI(16-16)port(17-17)packages/web-platform/web-tests/server.js (1)
__dirname(6-6)packages/webpack/template-webpack-plugin/test/cases/assets/production/rspack.config.js (1)
process(34-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build / Build (Windows)
- GitHub Check: test-rust / clippy
- GitHub Check: test-rust / Test (Ubuntu)
🔇 Additional comments (5)
.github/workflows/test.yml (5)
85-85: Job name placement is correct.Name is now inside the job block and reflects matrix axes/shard. LGTM.
98-98: Per-run web report naming is good.Distinct HTML artifact names per axis/shard will simplify triage.
101-107: Env gating by axis is clean.Explicitly exporting ENABLE_MULTI_THREAD/ENABLE_SSR only when needed avoids bleed-over between shards.
89-94: Double-check intent: this uses Playwright sharding (not manual split).PR description mentions manual split to avoid result aggregation, but the job runs with --shard. Verify this is the intended direction.
Also applies to: 109-109
89-94: Only MULTI_THREAD×SSR must be excluded. Verified all other THREAD×RENDER combinations run at least one test.
colinaaa
left a comment
colinaaa
left a comment
There was a problem hiding this comment.
LGTM with the change. Try to make CodeRabbit happy :)
2 participants
To mitigate CPU timing limitation for each container task in lynx's CI. we create more runner to preempt more CPU resources.
Summary by CodeRabbit
Checklist