-
Notifications
You must be signed in to change notification settings - Fork 3k
ci: quarantine platform-stalled E2E suites from push lanes (#10316) #10327
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
bb73864
335690c
a768b69
2ff45c4
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 | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -203,10 +203,20 @@ jobs: | |||||||||||||||||
| # The docker leg runs vitest directly instead of through | ||||||||||||||||||
| # test:integration:sandbox:docker: that script would rebuild the image | ||||||||||||||||||
| # the step above just built. | ||||||||||||||||||
| # external-context-mem0-write, external-context-auto-recall, | ||||||||||||||||||
| # context-compress-interactive and qwen-serve-channel-workers are | ||||||||||||||||||
| # quarantined to the nightly isolated matrix below as a precaution. | ||||||||||||||||||
| # Their #10272 startup stalls were the goal-runtime wait bug fixed | ||||||||||||||||||
| # by #10290, not a platform defect, and they have not stalled on | ||||||||||||||||||
| # any platform since. The nightly canaries run on | ||||||||||||||||||
| # ubuntu-latest — which never reproduced the stall — so they keep | ||||||||||||||||||
| # functional coverage on hosted Linux but give no signal about the | ||||||||||||||||||
| # former stall platforms; restoring these suites to the push lanes | ||||||||||||||||||
| # requires verifying them on those runners first. | ||||||||||||||||||
|
Comment on lines
+213
to
+215
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-1: (fix-induced) The rewrite added this round acknowledges the ubuntu-latest canary limitation exactly as this thread asked — but the new wording re-introduces the platform hypothesis that #10272's closing comment explicitly retracts. The same block two lines up says the stalls were "not a platform defect" and "they have not stalled on any platform since", yet these lines invoke "the former stall platforms" and require "verifying them on those runners first" — there are no special stall platforms to re-verify (#10272's closing comment: "Correction — this is deterministic, not platform-specific … 'ubuntu-hosted passes' reflects shard composition"), and the verification the condition demands already happened: at this PR's merge base (run 33130681501) all four suites are green on the macOS and ecs-qwen pool legs. A future maintainer following this lift condition will demand evidence of a platform difference the cited issue says does not exist, so the quarantine never lifts. The sentence start on the two lines above ("ubuntu-latest — which never reproduced the stall") carries the same retracted framing and should be adjusted with it.
Suggested change
中文说明(修复引发)本轮新增的改写确实按本线程的要求承认了 ubuntu-latest 金丝雀的局限 —— 但新措辞重新引入了 #10272 关闭评论明确纠正过的平台假设。上方两行刚说卡住"并非平台缺陷"且"此后未在任何平台上复现",这几行却又提到"原卡住平台"并要求"先在这些 runner 上验证"—— 并不存在需要重新验证的特殊卡住平台(#10272 关闭评论:"Correction — this is deterministic, not platform-specific … 'ubuntu-hosted passes' reflects shard composition"),而该条件所要求的验证其实已经发生:在本 PR 的合并基线上(run 33130681501),四个套件在 macOS 与 ecs-qwen 池分支上全绿。未来按此解除条件行事的维护者会去索要一份所引 issue 已声明不存在的平台差异证据,隔离因此永远无法解除。上方两行的句首("ubuntu-latest — which never reproduced the stall")带有同样的已被纠正的框架,应一并调整。 — qwen3.8-max via Qwen Code /review (v0.22.2) |
||||||||||||||||||
| if [[ "${{ matrix.sandbox }}" == "sandbox:docker" ]]; then | ||||||||||||||||||
| npx cross-env QWEN_SANDBOX=docker vitest run --root ./integration-tests --exclude '**/interactive/cron-interactive.test.ts' --exclude '**/channel-plugin.test.ts' --shard='${{ matrix.shard }}' | ||||||||||||||||||
| npx cross-env QWEN_SANDBOX=docker vitest run --root ./integration-tests --exclude '**/interactive/cron-interactive.test.ts' --exclude '**/channel-plugin.test.ts' --exclude '**/interactive/external-context-mem0-write.test.ts' --exclude '**/interactive/external-context-auto-recall.test.ts' --exclude '**/interactive/context-compress-interactive.test.ts' --exclude '**/cli/qwen-serve-channel-workers.test.ts' --shard='${{ matrix.shard }}' | ||||||||||||||||||
| else | ||||||||||||||||||
| npm run test:integration:sandbox:none -- --exclude '**/interactive/cron-interactive.test.ts' --exclude '**/channel-plugin.test.ts' --shard='${{ matrix.shard }}' | ||||||||||||||||||
| npm run test:integration:sandbox:none -- --exclude '**/interactive/cron-interactive.test.ts' --exclude '**/channel-plugin.test.ts' --exclude '**/interactive/external-context-mem0-write.test.ts' --exclude '**/interactive/external-context-auto-recall.test.ts' --exclude '**/interactive/context-compress-interactive.test.ts' --exclude '**/cli/qwen-serve-channel-workers.test.ts' --shard='${{ matrix.shard }}' | ||||||||||||||||||
| fi | ||||||||||||||||||
|
|
||||||||||||||||||
| # The sandbox build retags the same image name every run, so on a | ||||||||||||||||||
|
|
@@ -275,7 +285,7 @@ jobs: | |||||||||||||||||
| OPENAI_API_KEY: '${{ secrets.OPENAI_API_KEY }}' | ||||||||||||||||||
| OPENAI_BASE_URL: '${{ secrets.OPENAI_BASE_URL }}' | ||||||||||||||||||
| OPENAI_MODEL: '${{ secrets.OPENAI_MODEL }}' | ||||||||||||||||||
| run: 'npx cross-env VERBOSE=true KEEP_OUTPUT=true QWEN_SANDBOX=false vitest run --root ./integration-tests --exclude "**/interactive/cron-interactive.test.ts" --exclude "**/channel-plugin.test.ts" --shard="${{ matrix.shard }}"' | ||||||||||||||||||
| run: 'npx cross-env VERBOSE=true KEEP_OUTPUT=true QWEN_SANDBOX=false vitest run --root ./integration-tests --exclude "**/interactive/cron-interactive.test.ts" --exclude "**/channel-plugin.test.ts" --exclude "**/interactive/external-context-mem0-write.test.ts" --exclude "**/interactive/external-context-auto-recall.test.ts" --exclude "**/interactive/context-compress-interactive.test.ts" --exclude "**/cli/qwen-serve-channel-workers.test.ts" --shard="${{ matrix.shard }}"' | ||||||||||||||||||
|
|
||||||||||||||||||
| isolated-nightly: | ||||||||||||||||||
| name: '${{ matrix.label }} (nightly)' | ||||||||||||||||||
|
|
@@ -291,6 +301,16 @@ jobs: | |||||||||||||||||
| test_file: 'interactive/cron-interactive.test.ts' | ||||||||||||||||||
| - label: 'channel-plugin E2E' | ||||||||||||||||||
| test_file: 'channel-plugin.test.ts' | ||||||||||||||||||
| # Quarantined from the push lanes as a precaution; see the | ||||||||||||||||||
| # excludes and rationale in the push jobs above. | ||||||||||||||||||
| - label: 'external-context-mem0-write E2E' | ||||||||||||||||||
| test_file: 'interactive/external-context-mem0-write.test.ts' | ||||||||||||||||||
|
Comment on lines
+306
to
+307
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] The canary entries live in 中文说明这些金丝雀条目所在的 — qwen3.8-max via Qwen Code /review (v0.22.2) |
||||||||||||||||||
| - label: 'external-context-auto-recall E2E' | ||||||||||||||||||
| test_file: 'interactive/external-context-auto-recall.test.ts' | ||||||||||||||||||
| - label: 'context-compress-interactive E2E' | ||||||||||||||||||
| test_file: 'interactive/context-compress-interactive.test.ts' | ||||||||||||||||||
| - label: 'qwen-serve-channel-workers E2E' | ||||||||||||||||||
| test_file: 'cli/qwen-serve-channel-workers.test.ts' | ||||||||||||||||||
| steps: | ||||||||||||||||||
| - name: 'Checkout' | ||||||||||||||||||
| uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,7 +4,7 @@ | |||||||||||||||||||||||||||||||||
| * SPDX-License-Identifier: Apache-2.0 | ||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| import { readFileSync } from 'node:fs'; | ||||||||||||||||||||||||||||||||||
| import { existsSync, readFileSync } from 'node:fs'; | ||||||||||||||||||||||||||||||||||
| import { describe, expect, it } from 'vitest'; | ||||||||||||||||||||||||||||||||||
| import { parse } from 'yaml'; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
@@ -32,4 +32,70 @@ describe('e2e workflow', () => { | |||||||||||||||||||||||||||||||||
| expect(group).toContain('github.event_name'); | ||||||||||||||||||||||||||||||||||
| expect(group).toContain('github.head_ref || github.ref_name'); | ||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| it('quarantines the nightly-isolated suites from every push lane', () => { | ||||||||||||||||||||||||||||||||||
| // cron-interactive is timing-flaky, and external-context-mem0-write, | ||||||||||||||||||||||||||||||||||
| // external-context-auto-recall, context-compress-interactive and | ||||||||||||||||||||||||||||||||||
| // qwen-serve-channel-workers are quarantined to the nightly isolated | ||||||||||||||||||||||||||||||||||
| // matrix as a precaution after the #10272 startup stalls (since traced | ||||||||||||||||||||||||||||||||||
| // to the goal-runtime wait bug fixed by #10290). Every nightly canary | ||||||||||||||||||||||||||||||||||
| // must be excluded from every push lane, and every push-lane exclusion | ||||||||||||||||||||||||||||||||||
| // must keep its nightly canary — dropping either side silently loses | ||||||||||||||||||||||||||||||||||
| // coverage. | ||||||||||||||||||||||||||||||||||
| const nightlyFiles = yml.jobs['isolated-nightly'].strategy.matrix.include | ||||||||||||||||||||||||||||||||||
|
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] R3-1: The pin test derives its expected exclude set solely from e2e.yml's own // after the nightlyFiles computation — pin the quarantined membership
// absolutely, alongside the relational check:
for (const file of [
'interactive/external-context-mem0-write.test.ts',
'interactive/external-context-auto-recall.test.ts',
'interactive/context-compress-interactive.test.ts',
'cli/qwen-serve-channel-workers.test.ts',
]) {
expect(nightlyFiles, file).toContain(file);
}Fix witness: with the membership pin added, revert this PR's e2e.yml hunks (e.g. 中文说明钉桩测试的期望排除集完全取自 e2e.yml 自身的 修复见证:加入成员资格钉桩后,回退本 PR 的 e2e.yml 改动(例如 — qwen3.8-max via Qwen Code /review (v0.22.2) |
||||||||||||||||||||||||||||||||||
| .map((entry) => entry.test_file) | ||||||||||||||||||||||||||||||||||
| .sort(); | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+45
to
+47
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] The pin test's oracle is closed inside e2e.yml: the push-lane Anchor the pin to the filesystem — for example: import { existsSync } from 'node:fs';
// inside the test:
for (const file of nightlyFiles) {
expect(existsSync(`integration-tests/${file}`)).toBe(true);
}Fix witness: with that assertion in place, renaming any of the six quarantined suites without updating e2e.yml turns this test red — remove the fix and confirm the test fails. 中文说明钉桩测试的对照范围封闭在 e2e.yml 内部:push 通道的 建议把钉桩锚定到文件系统,例如: import { existsSync } from 'node:fs';
// 测试内:
for (const file of nightlyFiles) {
expect(existsSync(`integration-tests/${file}`)).toBe(true);
}修复见证:加入该断言后,重命名六个被隔离套件中的任意一个而不更新 e2e.yml,都会让本测试变红 —— 请移除该修复并确认测试失败。 — qwen3.8-max via Qwen Code /review (v0.22.2)
Comment on lines
+45
to
+47
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] The canary side is verified only as matrix const canaryStep = yml.jobs['isolated-nightly'].steps.find(
(s) => s.name === 'Run ${{ matrix.label }} tests',
);
expect(canaryStep.run).toContain('${{ matrix.test_file }}');Fix witness: deleting 中文说明金丝雀一侧只被验证为矩阵 const canaryStep = yml.jobs['isolated-nightly'].steps.find(
(s) => s.name === 'Run ${{ matrix.label }} tests',
);
expect(canaryStep.run).toContain('${{ matrix.test_file }}');修复见证:从 e2e.yml 的 — qwen3.8-max via Qwen Code /review (v0.22.2) |
||||||||||||||||||||||||||||||||||
| // Anchor the oracle to the filesystem: a renamed suite would otherwise | ||||||||||||||||||||||||||||||||||
| // match no exclude (vitest ignores a non-matching --exclude silently) | ||||||||||||||||||||||||||||||||||
| // and no canary file while this test compared e2e.yml strings only. | ||||||||||||||||||||||||||||||||||
| for (const file of nightlyFiles) { | ||||||||||||||||||||||||||||||||||
| expect(existsSync(`integration-tests/${file}`), file).toBe(true); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+51
to
+53
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-2: (fix-induced) The
Suggested change
Fix witness: with the collectability assertions in place, point one canary entry and its push-lane excludes at a real file under 中文说明(修复引发)本轮作为本线程修复而加入的 修复见证:加入可收集性断言后,把一个金丝雀条目及其 push 通道 exclude 指向 — qwen3.8-max via Qwen Code /review (v0.22.2) |
||||||||||||||||||||||||||||||||||
| const countExcludes = (run) => { | ||||||||||||||||||||||||||||||||||
| const counts = new Map(); | ||||||||||||||||||||||||||||||||||
| for (const match of run.matchAll(/--exclude ['"]([^'"]+)['"]/g)) { | ||||||||||||||||||||||||||||||||||
| const file = match[1].replace(/^\*\*\//, ''); | ||||||||||||||||||||||||||||||||||
| counts.set(file, (counts.get(file) ?? 0) + 1); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| return counts; | ||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||
| // Discover push lanes instead of listing job names: every job with a | ||||||||||||||||||||||||||||||||||
| // `Run E2E tests` step is one, so a future lane that forgets the | ||||||||||||||||||||||||||||||||||
| // excludes fails here instead of stalling on the quarantined suites. | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+62
to
+64
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-4: (fix-induced) The lane discovery added this round as this thread's fix is keyed on the step name // in the discovery loop, next to `lanes += 1;`:
laneJobs.push(jobName);
// next to `expect(lanes).toBeGreaterThan(0);`:
expect(laneJobs.sort()).toEqual(['e2e-test-linux', 'e2e-test-macos']);Fix witness: with the lane-identity assertion in place, rename one lane's 中文说明(修复引发)本轮作为本线程修复而加入的通道发现以步骤名 修复见证:加入通道身份断言后,重命名某个通道的 — qwen3.8-max via Qwen Code /review (v0.22.2) |
||||||||||||||||||||||||||||||||||
| let lanes = 0; | ||||||||||||||||||||||||||||||||||
| for (const [jobName, job] of Object.entries(yml.jobs)) { | ||||||||||||||||||||||||||||||||||
| const step = job.steps.find((s) => s.name === 'Run E2E tests'); | ||||||||||||||||||||||||||||||||||
| if (!step) continue; | ||||||||||||||||||||||||||||||||||
| lanes += 1; | ||||||||||||||||||||||||||||||||||
| // One vitest invocation per matrix leg; counting them separately | ||||||||||||||||||||||||||||||||||
| // catches an exclude moved between legs, which a whole-step count | ||||||||||||||||||||||||||||||||||
| // cancels out. | ||||||||||||||||||||||||||||||||||
| const invocations = step.run | ||||||||||||||||||||||||||||||||||
| .split('\n') | ||||||||||||||||||||||||||||||||||
| .filter((line) => !line.trim().startsWith('#')) | ||||||||||||||||||||||||||||||||||
| .filter((line) => /vitest run|test:integration/.test(line)); | ||||||||||||||||||||||||||||||||||
| expect(invocations.length, jobName).toBeGreaterThan(0); | ||||||||||||||||||||||||||||||||||
| for (const invocation of invocations) { | ||||||||||||||||||||||||||||||||||
| const counts = countExcludes(invocation); | ||||||||||||||||||||||||||||||||||
| expect([...counts.keys()].sort(), `${jobName}: ${invocation}`).toEqual( | ||||||||||||||||||||||||||||||||||
| nightlyFiles, | ||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||
| for (const file of nightlyFiles) { | ||||||||||||||||||||||||||||||||||
| expect(counts.get(file), `${jobName}: ${file}`).toBe(1); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| expect(lanes).toBeGreaterThan(0); | ||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| it('runs the nightly canary step on its matrix file', () => { | ||||||||||||||||||||||||||||||||||
| // The quarantine test above pins the canary matrix against the push | ||||||||||||||||||||||||||||||||||
| // lanes but never reads the canary run step: if that step stopped | ||||||||||||||||||||||||||||||||||
| // consuming matrix.test_file, every canary would silently run nothing | ||||||||||||||||||||||||||||||||||
| // while continue-on-error painted the jobs green. | ||||||||||||||||||||||||||||||||||
| const canaryStep = yml.jobs['isolated-nightly'].steps.find( | ||||||||||||||||||||||||||||||||||
| (step) => step.name === 'Run ${{ matrix.label }} tests', | ||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||
| expect(canaryStep.run).toContain('${{ matrix.test_file }}'); | ||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||
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.
[Critical] R2-1: The comment block no longer asserts the contradicted premise — that half of this blocker is fixed by this commit (verified at HEAD). But its replay core still stands and the
Fixes #10316closing claim is deliberately kept, so the blocker is re-posted under its original id.The PR still claims
Fixes #10316, but #10316's observed failure — main E2E red — completes unchanged after this change. #10316 is the per-commit tracker "Main CI failed: E2E Tests" (run 33091403027). The stall this PR's description narrates was fixed by #10290 before this PR existed: #10272 was closed COMPLETED 58 minutes before this PR opened, and its closing comment retracts the platform hypothesis ("Correction — this is deterministic, not platform-specific … Fixed in #10290"). #10290 is an ancestor of this PR's merge base, yet the PR description still says "the platform-specific stalls remain tracked in #10272. Until that fix lands".At this PR's exact merge base (7357136, re-verified this round), the only failing files are
sdk-typescript/tool-control.test.tsandsdk-typescript/permission-control.test.ts, while all four quarantined suites pass on every leg. This PR excludes neither sdk-typescript suite, so after merge the push E2E workflow still fails on them (post-merge-base runs carry the same signature; one run went fully green without any of these excludes), #10316's incident persists, and merging closes the tracker while removing push-lane coverage from four currently-green suites.witness:
Either drop the
Fixes #10316closing claim and reframe the description as a precautionary CI-hygiene change (the description, unlike git history, is editable), or close this PR and re-aim at thesdk-typescriptfailures that actually keep main red. Maintainer @wenshao has approved this head with the premise objection on record, so the decision is the maintainers' — but the closing claim should not survive this review unchallenged.中文说明
本 blocker 的注释块部分已由本次提交修复(已在 HEAD 核实):注释不再断言已被推翻的前提。但其重放核心依然成立,且
Fixes #10316的关闭声明被刻意保留,因此以原 id 重新发布。本 PR 仍声明
Fixes #10316,但 #10316 所记录的故障 —— main 分支 E2E 红色 —— 在本次改动后原样复现。#10316 是按提交开出的跟踪 issue:"Main CI failed: E2E Tests"(run 33091403027)。PR 描述所叙述的卡住在本 PR 出现之前就已被 #10290 修复:#10272 在本 PR 创建前 58 分钟以 COMPLETED 关闭,其关闭评论纠正了平台假设("Correction — this is deterministic, not platform-specific … Fixed in #10290")。#10290 是本 PR 合并基线的祖先,而 PR 描述仍写着 "the platform-specific stalls remain tracked in #10272. Until that fix lands"。在本 PR 的精确合并基线(7357136dd1,本轮重新核实)上,唯一失败的文件是
sdk-typescript/tool-control.test.ts与sdk-typescript/permission-control.test.ts,而四个被隔离的套件在每一个分支上都通过。本 PR 并未排除这两个 sdk-typescript 套件,因此合入后 push E2E 依然会在它们上面失败(合并基线之后的运行保持同样的失败特征;其中一次运行在没有任何这些 exclude 的情况下全绿),#10316 的事故持续存在,而合入会关闭该跟踪 issue,同时把四个当前全绿的套件移出 push 通道。或移除
Fixes #10316关闭声明、把描述改写为预防性的 CI 卫生改动(描述与 git 历史不同,是可以编辑的),或关闭本 PR、转而处理真正让 main 保持红色的sdk-typescript失败。维护者 @wenshao 已在前提异议在案的情况下批准了当前 head,决定权在维护者 —— 但该关闭声明不应未经质疑地通过本审查。— qwen3.8-max via Qwen Code /review (v0.22.2)