-
Notifications
You must be signed in to change notification settings - Fork 3k
ci: run Linux E2E with Vitest forks #11290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -127,13 +127,10 @@ jobs: | |
| sandbox: | ||
| - 'sandbox:none' | ||
| - 'sandbox:docker' | ||
| # The suite is ~16min of wall clock on one runner, dominated by a long | ||
| # tail of sdk-typescript files. vitest assigns files to shards by path | ||
| # hash, so those spread out instead of clustering in one shard. | ||
| # Keep three-way test concurrency inside one runner so each sandbox | ||
| # shares setup work instead of occupying three pool runners. | ||
| shard: | ||
| - '1/3' | ||
| - '2/3' | ||
| - '3/3' | ||
| - '1/1' | ||
| node-version: | ||
| - '22.x' | ||
| steps: | ||
|
|
@@ -362,10 +359,10 @@ jobs: | |
| # test:integration:sandbox:docker: that script would rebuild the image | ||
| # the step above just built. | ||
| if [[ "${{ matrix.sandbox }}" == "sandbox:docker" ]]; then | ||
| npx cross-env QWEN_E2E_RENDERER=ink QWEN_SANDBOX=docker vitest run --root ./integration-tests --exclude '**/interactive/cron-interactive.test.ts' --exclude '**/channel-plugin.test.ts' --exclude '**/chat-transcript-document.test.ts' --shard='${{ matrix.shard }}' 9>&- | ||
| npx cross-env QWEN_E2E_RENDERER=ink QWEN_SANDBOX=docker vitest run --root ./integration-tests --exclude '**/interactive/cron-interactive.test.ts' --exclude '**/channel-plugin.test.ts' --exclude '**/chat-transcript-document.test.ts' --poolOptions.forks.maxForks=3 --shard='${{ matrix.shard }}' 9>&- | ||
| else | ||
| run_shard() { | ||
| QWEN_E2E_RENDERER=ink npm run test:integration:sandbox:none -- --exclude '**/interactive/cron-interactive.test.ts' --exclude '**/channel-plugin.test.ts' --exclude '**/chat-transcript-document.test.ts' --shard='${{ matrix.shard }}' | ||
| QWEN_E2E_RENDERER=ink npm run test:integration:sandbox:none -- --exclude '**/interactive/cron-interactive.test.ts' --exclude '**/channel-plugin.test.ts' --exclude '**/chat-transcript-document.test.ts' --poolOptions.forks.maxForks=3 --shard='${{ matrix.shard }}' | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] R1-6: [fails-closed] [regression] Three Vitest forks now share one scratch root, and two files in the suite ask for that root by the same name — so one fork
This is not a pre-existing hazard the diff merely exposes. At the merge base the self-hosted branch of Witness: Make the rig directory fork-unique in this same change rather than after it — in // integration-tests/test-helper.ts:228
this.testDir = join(env['INTEGRATION_TEST_FILE_DIR']!, `${sanitizedName}-${process.pid}`);Renaming the The reset itself has to stay: 中文说明三个 Vitest fork 现在共用同一个临时根目录,而测试集中有两个文件用同一个名字去申请该目录 —— 于是一个 fork 会在测试进行中途
这并不是本 diff 仅仅「暴露」出来的既有隐患。在 merge base 上, 修复方式:在同一次改动里就让 rig 目录具备 fork 唯一性,而不是事后再补 —— 在 重置逻辑本身必须保留: — qwen3.8-max via Qwen Code /review (v0.23.0) |
||
| } | ||
| # One bounded retry: pool runners' sandbox:none shards die under | ||
| # shared-host pressure with every test green and no vitest FAIL | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,18 @@ describe('e2e workflow', () => { | |
| expect(group).toContain('github.head_ref || github.ref_name'); | ||
| }); | ||
|
|
||
| it('runs three Vitest forks on one Linux runner per sandbox', () => { | ||
| const linuxJob = yml.jobs['e2e-test-linux']; | ||
| const runStep = linuxJob.steps.find( | ||
| (step) => step.name === 'Run E2E tests', | ||
| ); | ||
|
|
||
| expect(linuxJob.strategy.matrix.shard).toEqual(['1/1']); | ||
| expect(runStep.run.match(/--poolOptions\.forks\.maxForks=3/g)).toHaveLength( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R1-7: This count does not pin the docker leg — the flag can leave the docker command and the whole suite stays green.
So delete Worth knowing because it corrects a claim already on the PR: the reported "5/5 mutants killed, including dropping the flag from only the docker leg (the Witness: Pin each leg's command rather than counting the string — extend the existing descriptor-close assertion at expect(runStep.run).toContain(
"--poolOptions.forks.maxForks=3 --shard='${{ matrix.shard }}' 9>&-",
);and keep Keep the 中文说明这个计数并没有钉住 docker 一侧 —— 该 flag 可以从 docker 命令中消失,而整个测试套件仍然是绿的。
于是:把 有一点值得说明,因为它更正了 PR 上已有的一处结论:已报告的「5/5 变异体全部被杀,包括只从 docker 一侧删掉该 flag(正是 修复方式:钉住每一侧的命令,而不是数字符串出现次数 —— 把 扩展后的钉住必须保留 — qwen3.8-max via Qwen Code /review (v0.23.0) |
||
| 2, | ||
| ); | ||
| }); | ||
|
|
||
| describe('sandbox image preparation', () => { | ||
| const steps = yml.jobs['e2e-test-linux'].steps; | ||
| const setupStep = steps.find((step) => step.name === 'Set up Docker'); | ||
|
|
@@ -193,7 +205,7 @@ describe('e2e workflow', () => { | |
| // shard and exclude coverage lives only in this argument list. The | ||
| // excludes are shared verbatim with the docker leg above. | ||
| expect(runStep.run).toContain( | ||
| "npm run test:integration:sandbox:none -- --exclude '**/interactive/cron-interactive.test.ts' --exclude '**/channel-plugin.test.ts' --exclude '**/chat-transcript-document.test.ts' --shard='${{ matrix.shard }}'", | ||
| "npm run test:integration:sandbox:none -- --exclude '**/interactive/cron-interactive.test.ts' --exclude '**/channel-plugin.test.ts' --exclude '**/chat-transcript-document.test.ts' --poolOptions.forks.maxForks=3 --shard='${{ matrix.shard }}'", | ||
| ); | ||
| }); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] R1-3: Two comments in this same job now state the opposite of what the file does, and neither is the wording the maintainer verification already asked you to change.
Collapsing the matrix to one shard retires the shard as the unit of work, but two present-tense statements inside this job still describe the old shape:
:252-254— "Mapped forintegration-tests/vitest.config.ts, which caps each shared-pool shard at one fork and exempts pressure-flake unhandled errors" — sits in the very step this diff edits, two lines above the two flags that raise that cap to three. The second half of the sentence is still true; the first half is not.:305-306— "Per-commit coordinator: one shard per host builds the image, its siblings wait here and then find it present" — withshard: ['1/1']there is exactly onesandbox:dockerjob per run, so it has no siblings. The only remaining contenders are other runs of the sameGITHUB_SHA, because the concurrency group at:22-24is keyed on workflow / event name / ref, so aworkflow_dispatchorschedulerun of the same SHA lands in a different group and can overlap the in-flightpushrun.The cost is a mis-diagnosis rather than a failure: a maintainer who finds the docker leg parked up to 30 minutes in
flock --wait 1800 8goes looking for the intra-run sibling the comment promises, finds none, and reads a cross-run collision as a stuck intra-run handoff. Nothing surfaces it —scripts/tests/e2e-workflow.test.js:131-135pins the same sibling-shard narrative against run 33637097713 while asserting only lock ordering, which is still valid, so that test stays green.I am deliberately not re-raising the mechanism attribution in the comment this diff adds at
:130-131("shares setup work"): the maintainer verification already in this thread measured it (setup is 27s per job, 0.9% of the runner-time saving; the durable reason is dynamic load balancing) and asked for that sentence, so you already have it.Witness:
Reword both to the post-collapse shape in this same edit: at
:252-254say what the mapping is still for (the pressure-flake unhandled-error exemption, and the fork floor) and note that the ceiling is set on the two test commands below; at:305-306say the coordinator's remaining contention is cross-run (sameGITHUB_SHA, different runs or attempts on one host) rather than between sibling shards.One constraint on the config-comment pointer the maintainer verification asked for: the consumer inventory it would be written from does not hold, and writing it as instructed would put a second false sentence exactly where this finding says the first one must be corrected. E2E is not the only self-hosted consumer of
integration-tests/vitest.config.ts—release.yml'sintegration_none(poolruns-on,RUNNER_ENVIRONMENTmapped at:767and:773, runningtest:integration:cli:sandbox:noneat:769and:interactiveat:775) andintegration_docker(mapped at:819, runningvitest run --root ./integration-testsat:871-872) pass no override and genuinely resolve{minForks:1, maxForks:1}. Andci.ymlnever mapsRUNNER_ENVIRONMENTat all, so its pool legs take the non-self-hosted branch —test:integration:no-akcarries an explicit--poolOptions.forks.maxForks 2andtest:integration:cli(ci.yml:2216) carries none, resolving{minForks:2, maxForks:4}— not one fork. Keeping the override here ine2e.ymlis still the right call; only the sentence describing everyone else needs to be accurate.中文说明
同一个 job 里有两处注释现在与文件的实际行为相反,而且都不是维护者验证评论已经请你修改的那一处措辞。
把 matrix 收成单个 shard,意味着 shard 不再是工作单元,但这个 job 内仍有两处现在时的描述在讲旧形态:
:252-254—— "Mapped forintegration-tests/vitest.config.ts, which caps each shared-pool shard at one fork and exempts pressure-flake unhandled errors" —— 就在本 diff 修改的这个 step 里,位于把该上限提到三的两处 flag 上方两行。这句话的后半仍然成立,前半已不成立。:305-306—— "Per-commit coordinator: one shard per host builds the image, its siblings wait here and then find it present" —— 在shard: ['1/1']下每次 run 只有一个sandbox:dockerjob,因此不存在「兄弟 shard」。剩下的竞争者只可能是同一GITHUB_SHA的其他 run::22-24的 concurrency group 以 workflow / event name / ref 为键,所以同一 SHA 的workflow_dispatch或schedulerun 属于不同 group,可以与进行中的pushrun 重叠。代价是误诊而非失败:当维护者发现 docker leg 在
flock --wait 1800 8上停留长达 30 分钟时,会按注释去找那个并不存在的同 run 兄弟 shard,找不到之后就把一次跨 run 的锁竞争误判为同 run 内的交接卡死。而且没有任何机制会把这一点暴露出来 ——scripts/tests/e2e-workflow.test.js:131-135针对 run 33637097713 钉住了同样的兄弟 shard 叙事,但它断言的只是锁的顺序,而顺序仍然有效,所以该测试保持绿色。我特意不重复提本 diff 在
:130-131新增注释里的机制归因("shares setup work"):线程中已有的维护者验证评论已经实测过(setup 每个 job 27 秒,占 runner 时间节省的 0.9%;真正持久的原因是动态负载均衡)并请作者修改那句话,因此作者已经掌握这一点。修复方式:在同一次编辑中把两处都改写为收拢后的形态 ——
:252-254说明该映射如今还为什么存在(pressure-flake 未处理错误豁免,以及 fork 下限),并注明上限是由下方的两条测试命令设定的;:305-306说明该协调锁剩余的竞争是跨 run 的(同一GITHUB_SHA、同一宿主上的不同 run 或 attempt),而不是兄弟 shard 之间的。关于维护者建议的「在配置注释里加一行指引」,有一个前提约束:它所依据的消费者清单并不成立,若照原样书写,就会在本条发现指出需要修正的地方再放一句假话。E2E 不是
integration-tests/vitest.config.ts唯一的 self-hosted 消费者 ——release.yml的integration_none(poolruns-on,:767与:773映射RUNNER_ENVIRONMENT,:769运行test:integration:cli:sandbox:none、:775运行:interactive)与integration_docker(:819映射,:871-872运行vitest run --root ./integration-tests)都没有传覆盖参数,确实解析为{minForks:1, maxForks:1}。而ci.yml根本没有映射RUNNER_ENVIRONMENT,因此它在 pool 上的 leg 走的是非 self-hosted 分支 ——test:integration:no-ak显式带--poolOptions.forks.maxForks 2,test:integration:cli(ci.yml:2216)不带任何覆盖,解析为{minForks:2, maxForks:4}—— 并非单 fork。把覆盖保留在e2e.yml里仍然是正确选择;只是描述「其他所有人」的那句话需要写准。— qwen3.8-max via Qwen Code /review (v0.23.0)