fix(test): raise timeout for cold-import suites to stop CI flake - #5880
Merged
Conversation
Two packages/core suites cold-import a large module graph inside the 5s per-test budget and intermittently time out under CI runner load: - config-session-env.test.ts: afterEach -> vi.resetModules() makes every test re-import config.js's full transitive graph cold (a sibling already runs ~4.3s, right at the edge). - skill-activation.test.ts: the integration test await-imports coreToolScheduler.js just to reach one pure helper, pulling the whole scheduler graph cold. Neither touches the network/LLM; it's transform+evaluate cost, not a hang. Give both suites a 30s testTimeout so a slow-but-correct import stops being reported as a failure while a genuine hang still trips.
Collaborator
|
Thanks for the PR, @yiliang114! Template looks good ✓ — all required sections present, bilingual description included. On direction: clearly aligned. CI flake from cold-import timeouts is a real productivity drain — the linked run shows it kicking PRs out of the merge queue, which is exactly the kind of infrastructure problem worth fixing. No product-direction concerns. On approach: the scope is tight and appropriate. Two files, one line each ( Moving on to code review and test verification. 🔍 中文说明感谢贡献，@yiliang114！ 模板完整 ✓ — 所有必要章节齐全，包含双语说明。 方向：完全对齐。冷加载超时导致的 CI 偶发失败是真实的生产力损耗——链接的 CI run 显示它把无关 PR 踢出了 merge queue，这正是值得修复的基础设施问题。无产品方向顾虑。 方案：范围紧凑且恰当。两个文件，各加一行 进入代码审查和测试验证。🔍 — Qwen Code · qwen3.7-max |
Collaborator
Code ReviewIndependent proposal: Before reading the diff, my approach would have been identical — add file-level Diff comparison: The PR matches this baseline exactly. The changes are:
No correctness issues, no security concerns, no regressions. The comments are substantive — they explain the mechanism (cold import graph, load-bearing Reuse check: N/A — this is a test configuration change, not new logic. Test VerificationRan both affected suites on this runner (Linux, self-hosted). Results reproduce the exact problem the PR describes and confirm the fix. Before (main branch, no timeout fix)After (PR diff applied)The two previously-failing tests ran at 6.6s and 7.1s — both well above the old 5s default, confirming the timeout was the actual failure mode (not a hang). All 21 previously-passing tests still pass. Note on tmux testing: This PR only changes test runner configuration — no CLI behavior, no user-visible output. The vitest run above is the correct verification surface. 中文说明代码审查独立方案： 在读 diff 之前，我的做法完全一致——在两个受影响的测试套件中添加文件级 Diff 对比： PR 完全符合这个基线。改动如下：
无正确性问题，无安全隐患，无回归。注释有实质内容——解释了机制（冷加载模块图、load-bearing 复用检查： 不适用——这是测试配置变更，不是新逻辑。 测试验证在本 runner（Linux，self-hosted）上运行了两个受影响的测试套件。结果精确复现了 PR 描述的问题，并确认修复有效。 修复前（main 分支，无超时修复）修复后（应用 PR diff）两个之前失败的测试分别跑了 6.6s 和 7.1s——都明显超过了旧的 5s 默认值，确认超时是真正的失败模式（而非卡死）。所有 21 个之前通过的测试仍然通过。 关于 tmux 测试： 此 PR 仅修改测试运行器配置——无 CLI 行为变化，无用户可见输出。上述 vitest 运行结果是正确的验证方式。 — Qwen Code · qwen3.7-max |
Collaborator
|
This is a clean, well-justified CI reliability fix. The before/after test run on this runner tells the whole story: the same two tests that flake in CI timed out at exactly 5011ms and 5014ms here, then passed at 6.6s and 7.1s with the fix — proving the failure mode is a tight timing margin, not a hang. The implementation is minimal in the right way: one config line per file, comments that explain the mechanism rather than the obvious, and no scope creep. My independent proposal was identical, and I can't find a simpler path. The PR also correctly defers the root-cause work (shrinking the module graph) to a separate effort. This is the kind of fix that makes everyone's day slightly better — no more merge-queue ejections from a cold-import race against a 5s clock. Approving. 中文说明这是一个干净、充分论证的 CI 可靠性修复。在本 runner 上的前后对比测试说明了一切：在 CI 中偶发失败的两个测试在这里精确地在 5011ms 和 5014ms 超时，而应用修复后分别在 6.6s 和 7.1s 通过——证明失败模式是紧张的时间窗口，而非卡死。 实现方式恰到好处地精简：每个文件一行配置，注释解释的是机制而非显而易见的内容，没有范围蔓延。我的独立方案与此完全一致，找不到更简单的路径。PR 还正确地把根因工作（缩减模块图）推迟到了单独的优化中。 这种修复让每个人的日子都好过一点——不会再因为冷加载与 5s 时钟的赛跑而被 merge queue 踢出。批准。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jun 26, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
Two optional observations for human review:
- Both
vi.setConfigcalls omithookTimeout; all 5 other call sites inpackages/coreset both. Low functional risk (vi.resetModules()is fast), but a one-line consistency fix. skill-activation.test.tsapplies the 30s timeout file-wide, but only 2 of 20 tests do the cold import. Scoping to the integrationdescribewould be cleaner.
— qwen3.7-max via Qwen Code /review
chiga0
approved these changes
Jun 26, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Overview
Final Verdict: Approve — Clean, minimal CI flake fix. Two test suites cold-import a large module graph and intermittently cross the 5s default timeout under CI load. Raising to 30s gives headroom without masking genuine hangs.
Findings Summary
- Critical/Major: 0 items
- Minor: 0 items
- Nit: 0 items
Additional Audit Coverage
- Comment quality: Both
vi.setConfigadditions have clear, accurate comments explaining why the timeout needs raising (cold module graph,vi.resetModules()in afterEach,await import()dragging the scheduler graph). Future maintainers will understand the load-bearing nature of these patterns. - Scope: Only touches the two flaky suites. No test logic, assertions, or mocks changed — pure timeout configuration.
- Genuine hang detection: 30s is still a reasonable upper bound. A real hang would trip at 30s vs 5s, which is acceptable tradeoff for CI stability.
hookTimeoutomission (noted by CI bot):beforeEach/afterEachhooks in these suites run simple setup (env vars,vi.resetModules()) — unlikely to need more than the default 5s hook timeout. Non-blocking observation.
This review was generated by QoderWork AI
doudouOUC
reviewed
Jun 26, 2026
| // just to reach one pure helper, but that drags in the whole scheduler module | ||
| // graph cold. The first such import runs a few seconds and, under a contended | ||
| // CI runner, crosses the 5s default — a flaky timeout, not a hang. | ||
| vi.setConfig({ testTimeout: 30_000 }); |
Collaborator
There was a problem hiding this comment.
[Suggestion] vi.setConfig({ testTimeout: 30_000 }) at module scope applies 30s to all 21 tests in this file, but only the 2 integration tests below (await import('../core/coreToolScheduler.js')) need the extra headroom. The other 19 are pure synchronous unit tests that should fail fast.
All 5 other vi.setConfig call sites in packages/core place it inside a describe block, not at module scope. Moving it into the integration describe block would match the established convention and correctly scope the timeout:
Suggested change
| vi.setConfig({ testTimeout: 30_000 }); | |
| describe('extractToolFilePaths \u2192 SkillActivationRegistry integration', () => { | |
| vi.setConfig({ testTimeout: 30_000 }); |
— qwen3.7-max via Qwen Code /review
| // flag resets). That cold transform+evaluate runs several seconds and, under | ||
| // a contended CI runner, crosses the 5s default — a flaky timeout, not a hang. | ||
| // The reset is load-bearing for what these tests check, so give them headroom. | ||
| vi.setConfig({ testTimeout: 30_000 }); |
Collaborator
There was a problem hiding this comment.
[Suggestion] All 5 existing vi.setConfig call sites in packages/core set both testTimeout and hookTimeout. This file has non-trivial hooks (vi.resetModules() + env restoration in afterEach) that could theoretically exceed the default 10s hookTimeout under extreme CI contention. Consider adding hookTimeout for consistency:
Suggested change
| vi.setConfig({ testTimeout: 30_000 }); | |
| vi.setConfig({ testTimeout: 30_000, hookTimeout: 30_000 }); |
— qwen3.7-max via Qwen Code /review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Raises the per-test timeout to 30s for two
packages/coretest suites whose tests cold-import a large module graph, so a slow-but-correct import under CI load stops being reported as a 5s timeout. No test logic changes.Why it's needed
The CI / merge-queue Test job intermittently fails with two
Test timed out in 5000mserrors, and because it runs in the merge queue it kicks unrelated PRs out (e.g. #5878 was dropped twice by exactly these two failures).Neither test touches the network or an LLM — they mock or avoid it. The cost is the cold transform + evaluate of a big dependency graph measured against the 5s per-test budget:
config-session-env.test.ts:afterEachcallsvi.resetModules()(load-bearing — these tests check a module-levelsessionEnvClaimedflag), so every test re-importsconfig.js's full transitive graph cold. A passing sibling already runs ~4.3s, right at the 5s edge.skill-activation.test.ts: the integration test doesawait import('../core/coreToolScheduler.js')just to reach one pure helper, dragging in the whole scheduler graph cold on first import.On a contended self-hosted runner these cross 5000ms → flaky timeout, not a hang.
Reviewer Test Plan
How to verify
npx vitest run packages/core/src/config/config-session-env.test.ts packages/core/src/skills/skill-activation.test.ts→ both green (23 tests).vi.setConfig({ testTimeout: 30_000 })to each file (plus aviimport in the skills file). No assertions or mocks changed.Before: intermittent
Test timed out in 5000mson the two named tests under CI load.After: the import-heavy suites have headroom; a genuine hang still trips at 30s.
Evidence (Before & After)
N/A (CI reliability, not user-visible). Example failing run: https://github.com/QwenLM/qwen-code/actions/runs/28213047069/job/83578267928
Tested on
Risk & Scope
config.js/coreToolScheduler.jsgraph size). Shrinking that is a separate refactor, not a flake fix.Linked Issues
None. Referencing the failing run above rather than a closing keyword.
中文说明
把
packages/core两个测试套件的单测超时从默认 5s 提到 30s。这俩测试在 5s 预算内冷加载了一个巨大的模块图,在 CI runner 负载高时偶发超时——不是真卡死,也没有任何 LLM/网络调用(都被 mock 或绕开了)。config-session-env.test.ts:afterEach里的vi.resetModules()是必需的(测的就是模块级sessionEnvClaimed标志),导致每个用例都要把config.js的完整依赖图冷加载一遍;同文件一个通过的兄弟用例已经跑到 ~4.3s,贴着 5s 边缘。skill-activation.test.ts:集成用例await import('../core/coreToolScheduler.js')只是为了拿一个纯函数,却把整个 scheduler 模块图冷加载进来。它会在 merge queue 里把无辜的 PR 踢出去(#5878 就被这两个失败踢了两次)。验证:
npx vitest run跑这两个文件即可,23 个用例全绿。改动只是给每个文件加一行vi.setConfig({ testTimeout: 30_000 }),不动任何测试逻辑;真卡死仍会在 30s 失败。