fix(cli): stabilize flaky UI tests - #6622
Conversation
50a052e to
3a2883d
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。 |
|
Thanks for the PR! Template looks good ✓ Problem: This is an observed CI failure — cold module import cost counted against Vitest timeouts on busy Node 22 runners, causing the first test to time out and cascade into subsequent assertion failures. The author provides specific before/after timing evidence ( Direction: Fixing flaky tests is squarely within scope. The root cause analysis (dynamic import cost hitting timeout on busy runners) is plausible and well-explained. Size: Not applicable — all changes are in Approach: The scope feels right — two test files, converting dynamic Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:这是一个已观测到的 CI 失败——繁忙 Node 22 runner 上冷启动模块导入耗时被计入 Vitest timeout,导致第一个测试超时并级联影响后续断言。作者提供了具体的 before/after 时间数据( 方向:修复 flaky 测试完全在项目范围内。根因分析(动态导入在繁忙 runner 上触发 timeout)合理且有充分说明。 规模:不适用——所有改动都在 方案:范围合理——两个测试文件,将动态 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
|
Code Review (2a): Clean diff. The conversion from dynamic Real-Scenario Testing (2b): Ran both affected test files with reduced timeouts ( voice-keyterms-race.test.tsBefore (installed build, main branch)After (this PR)useStatusLine.test.tsBefore (main branch)After (this PR)Summary: The flakiness reproduces reliably on the main branch with reduced timeouts — first test/hook times out due to cold — Qwen Code · qwen3.7-max |
|
This is a clean fix. The problem is real — I reproduced it on this runner: the first What I like about this PR: it's minimal (net -8 lines), focused (two test files, one change pattern), and the author clearly understood the root cause rather than just throwing higher timeouts at it. The before/after timing (3100ms → 6ms for voice-keyterms, hook-timeout → 146ms for useStatusLine) is compelling evidence. No concerns. Ships the fix, makes CI more reliable, and removes a misconception about needing dynamic imports after mock setup. LGTM, looks ready to ship. ✅ 中文说明这是一个干净的修复。问题是真实存在的——我在这台 runner 上复现了:每个测试文件中第一个 这个 PR 的优点:改动最小(净减 8 行)、聚焦(两个测试文件,一种修改模式),作者显然理解了根因而非简单地加大 timeout。before/after 时间对比(voice-keyterms 3100ms → 6ms,useStatusLine hook-timeout → 146ms)是有力的证据。 没有顾虑。修复有效,让 CI 更可靠,同时消除了"mock 设置后必须动态导入"的误解。 可以合并 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
The dynamic-to-static import conversion is correct — vi.mock() hoisting ensures mocks are in place before module evaluation, and the old dynamic imports were providing no isolation benefit (no vi.resetModules() calls). Build passes, all 74 tests pass (71 useStatusLine + 3 voice-keyterms-race).
— qwen3.7-max via Qwen Code /review
doudouOUC
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Clean test stabilization — converting unnecessary await import() to static import is correct since Vitest hoists vi.mock() above all imports. Build passes, 74/74 tests pass.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
Please drop
|
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Clean fix — converting dynamic await import() to static imports eliminates cold module load timeouts that caused CI flakiness. The vi.mock() hoisting guarantees mocks are in place before static imports resolve. All 74 tests pass.
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
The conversion from dynamic await import() to static top-level imports is correct in both files. vi.mock() is hoisted by Vitest above all imports, so mocks are in place before the static imports resolve. Neither file uses vi.resetModules(), so the old dynamic import was already returning the same cached module — just with unnecessary async scheduling overhead that plausibly caused the timeout cascades.
— qwen3.7-max via Qwen Code /review
What this PR does
This PR removes per-test dynamic imports from two CLI UI test files so the expensive module load happens during Vitest collection instead of inside the first test or hook. The tested behavior is unchanged; the tests now execute synchronously once the modules are loaded.
Why it's needed
The CI failure was caused by cold module import cost being counted against Vitest test and hook timeouts under Node 22 on a busy runner. When the first voice keyterms race test timed out, its unfinished async work could continue while later tests mutated shared mock state, which explains the follow-on assertion reading the later EvilTerm fixture. Moving the imports out of the per-test path removes that timeout cascade.
Reviewer Test Plan
How to verify
Run the focused CLI Vitest files and confirm both pass quickly. To stress the original failure mode, also run the same files with reduced timeout settings and confirm the actual test bodies complete in milliseconds rather than spending seconds in the first test or beforeEach.
Evidence (Before & After)
Before: under Node 22,
voice-keyterms-race.test.ts --testTimeout=2500timed out on the first test, anduseStatusLine.test.ts --hookTimeout=2500timed out in the first beforeEach. After:voice-keyterms-race.test.ts --testTimeout=2500passed 3 tests in 6ms test time, anduseStatusLine.test.ts --hookTimeout=2500passed 71 tests in 73ms test time.Tested on
Environment (optional)
Local Codex workspace on macOS. Focused test verification used Node 22.23.1 via
npx -p node@22 node ../../node_modules/vitest/vitest.mjsfrompackages/cli.Risk & Scope
packages/clitest suite locally, because the local sandbox blocks loopback listener tests and macOS pasteboard native calls.Linked Issues
N/A
中文说明
What this PR does
这个 PR 移除了两个 CLI UI 测试文件里的逐测试动态导入,让较重的模块加载发生在 Vitest 收集阶段,而不是第一条测试或 hook 内部。被测行为没有变化;模块加载完成后测试会同步执行。
Why it's needed
这次 CI 失败的原因是 Node 22 的繁忙 runner 上,冷启动模块导入耗时被计入 Vitest 的 test 和 hook timeout。第一个 voice keyterms race 测试超时后,未完成的 async work 仍可能继续执行,同时后续测试已经修改共享 mock state,这解释了后续断言读到 EvilTerm fixture 的现象。把导入移出逐测试路径后,可以消除这类 timeout 级联。
Reviewer Test Plan
How to verify
运行聚焦的 CLI Vitest 文件,确认两者都快速通过。为了压测原始失败模式,也可以用更低 timeout 运行相同文件,确认实际 test body 是毫秒级完成,而不是在第一条测试或 beforeEach 中耗费数秒。
Evidence (Before & After)
Before:Node 22 下,
voice-keyterms-race.test.ts --testTimeout=2500会在第一条测试超时,useStatusLine.test.ts --hookTimeout=2500会在第一个 beforeEach 超时。After:voice-keyterms-race.test.ts --testTimeout=2500通过 3 条测试,test time 为 6ms;useStatusLine.test.ts --hookTimeout=2500通过 71 条测试,test time 为 73ms。Tested on
Environment (optional)
macOS 本地 Codex workspace。聚焦测试使用 Node 22.23.1,在
packages/cli下通过npx -p node@22 node ../../node_modules/vitest/vitest.mjs验证。Risk & Scope
packages/cli测试套件,因为本地沙箱会阻止 loopback listener 测试,并且 macOS pasteboard native 调用会崩溃。Linked Issues
N/A