fix(cli): stop review test-efficacy tests depending on ambient tmpdir vitest - #8537
Conversation
… vitest Two tests failed on hosts where vitest resolves up-tree from os.tmpdir() (observed on self-hosted CI where a node_modules above TMPDIR provides one): findVitestBin's "cannot be resolved" case never threw, and runControlMutant's "cannot run" case executed the probe for real instead of throwing. Make the failure conditions host-deterministic while keeping every assertion: findVitestBin accepts an injected resolver (default unchanged) so the MODULE_NOT_FOUND case is forced directly, and the runControlMutant test plants a shadow vitest whose exports hide package.json, which wins resolution from any ancestor install and makes the run fail deterministically.
E2E Report — deflake review test-efficacy testsFlaky mechanism
Both assumed Which fix was applied and whyAllowed-fix class 4 (isolate/serialize interference): remove the dependence on ambient filesystem state, preserving every assertion verbatim.
No assertion was weakened, skipped, or deleted; the production call path is byte-for-byte the default. Repeated-run evidenceA/B reproduction harness on macOS:
中文说明失败机制
修复方式断言逐字保留,消除对环境文件系统状态的依赖:
未弱化、跳过或删除任何断言;生产调用路径与默认值完全一致。 重复运行证据macOS 本地 A/B 复现:以 |
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. Verified from the CI logs: run 30909672420's Direction: aligned — this is CI stability for the review command's own test-efficacy harness, exactly the class of environment-dependent flake this repo actively works to eliminate. Size: not core ( Approach: the scope feels right. Every assertion is preserved verbatim; only the way each failure condition is produced changes, from "ambient filesystem happens to cooperate" to host-deterministic constructions — an injected resolver for the not-found path, and a planted shadow package whose Risk: no elevated risk signals — no high-risk paths touched. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到,不是理论问题。已从 CI 日志核实:run 30909672420 的 方向:对齐——这是 review 命令自身 test-efficacy 测试框架的 CI 稳定性修复,正是本仓库一直在消除的那类环境依赖型 flaky。 规模:非核心路径( 方案:范围合理。断言全部逐字保留,只把失败条件的构造方式从"环境文件系统恰好配合"改为主机确定的构造——not-found 路径注入解析器,control-mutant 路径种植 风险:无升级风险信号——未触及高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewI formed my independent expectation before reading the diff — for two tests that assume nothing up-tree of What I verified against the surrounding code:
No blockers, no convention violations. Test evidence — this PR's own CIThe problem itself is proven from CI logs, not the author's word: run 30909672420's Bot orchestration checks (triage, review-pr) excluded from the table. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 The decisive signal is still running: 中文说明代码审查:先独立推演再对照 diff——两个假设 测试证据:问题本身由 CI 日志证实(非作者自述)——run 30909672420 的 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — verified problem, minimal fix, production path untouched; the only thing deferred is waiting for the decisive CI job to land green. Reflection: I went back to my independent proposal — inject the failure for the not-found test, plant a deterministic decoy for the control-mutant test — and the diff is exactly that, with nothing extra. The failure is proven from real CI logs (run 30909672420), not from the author's word; both assertions survive verbatim; the single production call site is byte-for-byte unchanged; and every edit in the diff earns its place. The author has a high volume of open PRs, but this one stands entirely on its own evidence, and I'm approving it because it is genuinely good — an observed flake killed at the root, not papered over. Approval is deferred only because the Approval will land once CI is green on 中文说明置信度:5/5 —— 问题已证实、修法最小、生产路径未动;唯一推迟的事项是等待决定性 CI 任务变绿。 复盘:回到我的独立方案——给 not-found 测试注入失败、给 control-mutant 测试种植确定性影子包——diff 正是如此,没有多余内容。失败由真实 CI 日志证实(run 30909672420),而非作者自述;两个断言逐字保留;唯一生产调用点逐字节未变;diff 中每一处改动都必要。作者有较多开放 PR,但本 PR 完全凭自身证据成立,我批准它是因为它确实好——从根上消除了观测到的 flaky,而不是掩盖它。推迟批准仅因为用于在此前失败 runner 类别上证明修复的 待 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
Code Review — #8537Verdict: LGTM, approve with two minor suggestions. I reproduced the CI condition and verified every claim in the PR body independently. What it doesTwo tests in Verification I ranBuilt the fake-TMPDIR harness from the test plan (
Mutation check — the reworked test still has teeth. I broke the production invariant it guards, moving the restore in const __v = perFile.some((r) => r.verdict === 'gated');
writeFileSync(abs, original, 'utf8');
return __v;
} finally { /* no longer survives the throw path */ }→ Production path is untouched. Convention fit. The optional-parameter seam matches what this file already does — Side benefit worth noting: the reworked test 2 now throws before any spawn, dropping from ~604ms (it was actually executing a real vitest run under the ambient-vitest condition) to ~10ms. Suggestions1. Pin test 2's assertion to the error the comment promises ( The comment states the point precisely — "findVitestBin surfaces the - expect(() => runControlMutant(dir, 'a.test.ts')).toThrow();
+ expect(() => runControlMutant(dir, 'a.test.ts')).toThrow(
+ /not defined by "exports"/,
+ );I applied this and it passes — the throw genuinely is that one. It also matches the sibling 2. With the resolver injected, - const worktree = mkdtempSync(join(tmpdir(), 'no-vitest-'));
+ // Never touched on disk — the injected resolver decides the outcome and
+ // the path only has to appear in the message.
+ const worktree = join(tmpdir(), 'no-vitest-unused');(Alternatively keep Note, not a defectAfter this change nothing verifies the link "a genuine Risk / security / performanceNo production behavior change, no new dependency, no I/O outside per-test temp dirs, and the planted shadow package lives inside a |
|
@qwen-code /takeover |
doudouOUC
left a comment
There was a problem hiding this comment.
LGTM. The default findVitestBin behavior remains unchanged, and the tests now exercise the intended error paths without relying on ambient TMPDIR node_modules layout.
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the 中文说明🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。本 PR 来自 fork,首轮处理将由下一次定时扫描执行(通常几分钟内)。移除 |
|
Released in v0.21.6. |
What this PR does
Stabilizes two environment-dependent tests in
packages/cli/src/commands/review/test-efficacy.test.tsthat fail on hosts where vitest resolves up-tree fromos.tmpdir(). Both tests keep every assertion verbatim; only the way the failure condition is produced becomes host-deterministic. ThefindVitestBinhelper gains an optional resolver parameter whose default is exactly the previous inline behavior, and one test plants a local shadow package instead of relying on a bare tmpdir having no vitest anywhere above it.Why it's needed
CI's ubuntu Test job fails with
AssertionError: expected [Function] to throw an errorin two cases:findVitestBin > names the search root when vitest cannot be resolvedandrunControlMutant > leaves the probe file byte-identical when it cannot run(PR run 30909672420; the same signature appears on the latest main run 30893612230). Both tests assumed nothing up-tree ofos.tmpdir()provides avitest/package.json— true on macOS dev machines, false on the project's self-hosted runner where anode_modulesabove TMPDIR supplies one. In that environment the first test never throws and the second actually executes the probe suite instead of throwing.Reviewer Test Plan
How to verify
mkdir -p /tmp/qwen-fake-tmp/node_modules && ln -s <repo>/node_modules/vitest /tmp/qwen-fake-tmp/node_modules/vitest, then runcd packages/cli && TMPDIR=/tmp/qwen-fake-tmp npx vitest run src/commands/review/test-efficacy.test.ts. With the old code both tests fail exactly as on CI; with this PR the full file passes (128/128).findVitestBinis called without the new parameter at its only production call site, so the default resolver (anchoredcreateRequire) is identical to before.toThrow(...)messages and the same byte-identical probe file restore.cd packages/cli && npm run typecheckplus ESLint/Prettier on the two touched files — all clean locally.Evidence (Before & After)
Before: both tests fail on the self-hosted ubuntu runner (and locally under the fake TMPDIR harness above). After: full file passes 128/128 in the repro environment and in 4 consecutive runs in the normal environment. Full evidence is in the e2e report comment below.
Tested on
Environment (optional)
macOS local runs plus a fake TMPDIR harness that simulates the Linux self-hosted runner's ambient vitest; the real ubuntu runner validates via this PR's own CI run.
Risk & Scope
findVitestBingains an optional parameter used only by tests; its default is the previous inlinecreateRequireanchor, so production behavior is unchanged.Linked Issues
None.
中文说明
本 PR 做了什么
稳定
packages/cli/src/commands/review/test-efficacy.test.ts中两个依赖环境的测试——它们在能从os.tmpdir()向上解析到 vitest 的主机上会失败。两个测试的断言全部逐字保留,只把失败条件的构造方式改为在任何宿主上都确定。findVitestBin增加一个可选解析器参数,其默认值与之前的内联行为完全一致;另一个测试改为种植本地 shadow 包,不再依赖裸 tmpdir 上方恰好没有 vitest。为什么需要
CI 的 ubuntu Test 任务以
AssertionError: expected [Function] to throw an error失败于两个用例:findVitestBin > names the search root when vitest cannot be resolved和runControlMutant > leaves the probe file byte-identical when it cannot run(PR run 30909672420;最新 main run 30893612230 同签名)。两个测试都假设os.tmpdir()向上没有任何vitest/package.json——在 macOS 开发机上成立,在项目自托管 runner 上不成立(TMPDIR 之上的 node_modules 提供了 vitest)。该环境下第一个测试从不抛错,第二个则真实执行了探针套件而非抛错。Reviewer Test Plan
如何验证
mkdir -p /tmp/qwen-fake-tmp/node_modules && ln -s <repo>/node_modules/vitest /tmp/qwen-fake-tmp/node_modules/vitest,然后cd packages/cli && TMPDIR=/tmp/qwen-fake-tmp npx vitest run src/commands/review/test-efficacy.test.ts。旧代码下两个测试与 CI 完全一致地失败;本 PR 下整文件通过(128/128)。findVitestBin在唯一生产调用点不传新参数,默认解析器(锚定 createRequire)与之前完全相同。toThrow(...)消息与相同的探针文件字节恢复。cd packages/cli && npm run typecheck加两个改动文件的 ESLint/Prettier——本地全部通过。Evidence (Before & After)
改动前:两个测试在自托管 ubuntu runner 上失败(本地用上述 fake TMPDIR 装置同样复现)。改动后:整文件在复现环境与常规环境 4 轮连续通过(128/128)。完整证据见下方 e2e report 评论。
Tested on
Environment (optional)
macOS 本地运行,外加模拟 Linux 自托管 runner 环境 vitest 的 fake TMPDIR 装置;真实 ubuntu runner 由本 PR 自身的 CI 运行验证。
Risk & Scope
findVitestBin新增仅测试使用的可选参数;默认值即原先内联的 createRequire 锚定,生产行为不变。Linked Issues
无。