-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(ci): stop shared-pool hosts exiting all-green E2E runs red (#10325) #10329
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 |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| /** | ||
| * @license | ||
| * Copyright 2026 Qwen Team | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; | ||
| import { tmpdir } from 'node:os'; | ||
| import { join } from 'node:path'; | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| // The env keys globalSetup's setup() writes, saved so a case can restore the | ||
| // suite-wide values after re-importing the module and running its lifecycle. | ||
| const SETUP_ENV_KEYS = [ | ||
| 'INTEGRATION_TEST_FILE_DIR', | ||
| 'QWEN_CODE_INTEGRATION_TEST', | ||
| 'TELEMETRY_LOG_FILE', | ||
| 'E2E_TEST_FILE_DIR', | ||
| 'TEST_CLI_PATH', | ||
| 'VERBOSE', | ||
| 'KEEP_OUTPUT', | ||
| ] as const; | ||
|
|
||
| describe('globalSetup memory-file save/restore', () => { | ||
| let qwenHome: string; | ||
| let savedEnv: Map<string, string | undefined>; | ||
|
|
||
| beforeEach(async () => { | ||
| qwenHome = await mkdtemp(join(tmpdir(), 'qwen-globalsetup-test-')); | ||
| savedEnv = new Map( | ||
| [...SETUP_ENV_KEYS, 'QWEN_HOME'].map((key) => [key, process.env[key]]), | ||
| ); | ||
| process.env['QWEN_HOME'] = qwenHome; | ||
| // Let teardown remove the run directories this case creates. | ||
| process.env['KEEP_OUTPUT'] = 'false'; | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| for (const [key, value] of savedEnv) { | ||
| if (value === undefined) { | ||
| delete process.env[key]; | ||
| } else { | ||
| process.env[key] = value; | ||
| } | ||
| } | ||
| vi.resetModules(); | ||
| await rm(qwenHome, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| // memoryFilePath is captured at module import time, so point QWEN_HOME at | ||
| // the scratch dir BEFORE a fresh import of the module. | ||
| async function loadGlobalSetup() { | ||
| vi.resetModules(); | ||
| return import('./globalSetup.js'); | ||
| } | ||
|
|
||
| it('restores the saved memory file after the run', async () => { | ||
| await writeFile(join(qwenHome, 'QWEN.md'), 'original content', 'utf-8'); | ||
| const { setup, teardown } = await loadGlobalSetup(); | ||
| await setup(); | ||
| await writeFile(join(qwenHome, 'QWEN.md'), 'mutated by tests', 'utf-8'); | ||
|
|
||
| await expect(teardown()).resolves.toBeUndefined(); | ||
|
|
||
| await expect(readFile(join(qwenHome, 'QWEN.md'), 'utf-8')).resolves.toBe( | ||
| 'original content', | ||
| ); | ||
| }); | ||
|
|
||
| it('does not exit an all-green run red when the restore cannot write', async () => { | ||
| // The persistent pool runners can carry a readable-but-unwritable | ||
| // QWEN.md left behind by a privileged job; before #10325 the teardown | ||
| // restore threw on it and exited every all-green E2E run on that host | ||
| // red with no failing test. Swap the file for a directory after setup() | ||
| // read it — the write then fails regardless of privilege, since root | ||
| // bypasses permission bits. | ||
| await writeFile(join(qwenHome, 'QWEN.md'), 'original content', 'utf-8'); | ||
| const { setup, teardown } = await loadGlobalSetup(); | ||
| await setup(); | ||
| await rm(join(qwenHome, 'QWEN.md'), { force: true }); | ||
| await mkdir(join(qwenHome, 'QWEN.md')); | ||
|
|
||
| await expect(teardown()).resolves.toBeUndefined(); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -119,8 +119,17 @@ export async function teardown() { | |
| } | ||
|
|
||
| if (originalMemoryContent !== null) { | ||
| await mkdir(dirname(memoryFilePath), { recursive: true }); | ||
| await writeFile(memoryFilePath, originalMemoryContent, 'utf-8'); | ||
| try { | ||
| await mkdir(dirname(memoryFilePath), { recursive: true }); | ||
| await writeFile(memoryFilePath, originalMemoryContent, 'utf-8'); | ||
| } catch (e) { | ||
|
Comment on lines
+122
to
+125
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] This best-effort restore guard and its only witness test ( npx vitest run --root ./integration-tests globalSetup.test.tsor have a maintainer dispatch e2e.yml on this branch before merging (it supports Fix witness: 中文说明这个尽力而为的还原守卫及其唯一见证测试( 修复见证: — qwen3.8-max via Qwen Code /review (v0.22.2) |
||
| // Best-effort restore: on the persistent pool runners a privileged job | ||
| // can leave a readable-but-unwritable QWEN.md behind, and the throw | ||
| // turned every all-green E2E run on that host red with no failing test | ||
| // ('Startup Error: EACCES'; #10325). Keep the warning visible so the | ||
| // poisoned host is still diagnosable. | ||
| console.error(`Warning: could not restore ${memoryFilePath}:`, e); | ||
| } | ||
| } else { | ||
| try { | ||
| await unlink(memoryFilePath); | ||
|
|
||
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] The failure-path witness test pins only that
teardown()resolves — it never asserts the warning this diff's own comment deliberately keeps ("Keep the warning visible so the poisoned host is still diagnosable"). I mutation-tested this at the reviewed commit: deleting theconsole.errorline inglobalSetup.tsleavesTests 2 passed (2)— the mutation survives. If a later cleanup removes or debug-levels that warning, every test stays green and a poisoned pool host becomes undiagnosable: the next #10325-flavoured incident has no warning in the log and a green suite, reproducing the exact "red run with no signal" diagnosis problem this PR set out to keep visible. Spy on the warning and pin it in the failure-path case:Fix witness: with this assertion in place, deleting the
console.errorline fromintegration-tests/globalSetup.tsturns the failure-path case red — verified at the reviewed commit: mutant + assertion →1 failed(AssertionError: expected "error" to be called with arguments [ StringContaining, Anything ]); un-mutated code + assertion →Tests 2 passed (2).中文说明
失败路径的见证测试只钉住了
teardown()正常结束,却没有断言本 diff 注释刻意保留的警告("Keep the warning visible so the poisoned host is still diagnosable")。已在评审提交上做了变异测试:删除globalSetup.ts中的console.error行后仍是Tests 2 passed (2)—— 变异存活。若后续清理移除或降级该警告,所有测试依旧绿色,中毒的池宿主机将变得无法诊断:下一次 #10325 式事故在日志中没有任何警告且套件全绿,重新造成本 PR 着力保留可见性的"红色运行却无信号"诊断难题。建议在失败路径用例中监听并钉住该警告(见上方 suggestion 代码块)。修复见证:加上该断言后,删除
integration-tests/globalSetup.ts中的console.error行会使失败路径用例变红 —— 已在评审提交上验证:变异体 + 断言 →1 failed(AssertionError: expected "error" to be called with arguments [ StringContaining, Anything ]);未变异代码 + 断言 →Tests 2 passed (2)。— qwen3.8-max via Qwen Code /review (v0.22.2)