test(cli): raise i18n ToolMessage test timeout to 15s to stop merge-queue flake#5858
Merged
Merged
Conversation
The two '<ToolMessage /> localized badge' locale tests await setLanguageAsync(), which loads locale resources lazily. Under the heavily parallelized macOS CI runner the call intermittently exceeds the 5s default and times out, repeatedly kicking PRs out of the merge queue (e.g. #5828) despite unrelated changes.
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. |
Collaborator
|
Thanks for the PR, @yiliang114! Template looks complete ✓ — all required headings present, including the bilingual 中文 section and a concrete Reviewer Test Plan with a linked failing merge-queue job as evidence. On direction: flaky CI that kicks unrelated PRs out of the merge queue is a real, recurring pain — #5828 getting bounced three times is a credible motivator. This sits squarely within the project's mission to keep the test suite reliable and the merge queue usable. On approach: the scope is about as tight as it gets — 3 net lines, scoped to exactly the two tests that await 中文说明感谢 @yiliang114 的贡献！ 模板完整 ✓ —— 所有必填章节齐全，包括双语的中文部分和具体的 Reviewer Test Plan，并附上了 merge queue 失败任务的链接作为证据。 方向：flaky CI 反复把无关 PR 踢出 merge queue 是真实存在的痛点——#5828 被踢三次就是可信的动机。这与项目让测试套件可靠、merge queue 可用的目标完全契合。 方案：范围几乎是最小化——净 3 行改动，精确限定在两个会 — Qwen Code · qwen3.7-max |
Collaborator
Code ReviewDiff is exactly three things: raise vitest's per-test timeout from the 5s default to 15s on the two Reuse check: vitest's built-in Real-Scenario TestingRan the exact command from the Reviewer Test Plan on Linux ( Before (main, 5s default timeout)After (this PR, 15s timeout on the two targeted tests)Both runs: 2 passed, 40 skipped, under 8s wall-clock. The change is a no-op on Linux but adds the headroom the macOS runner needs under contention. 中文说明代码审查Diff 精确地只做了三件事：把两个 复用检查：vitest 内置的 真实场景测试在 Linux 上执行了 Reviewer Test Plan 中的精确命令（ 前：1 passed / 2 passed / 7.72s 两边都是 2 passed、40 skipped、8s 以内。该改动在 Linux 上是 no-op，但在 macOS runner 高负载下提供了所需的余量。 — Qwen Code · qwen3.7-max |
Collaborator
|
Stepping back: this is a well-scoped surgical fix for a real, documented merge-queue pain. My independent proposal before reading the diff would have been roughly "raise the timeout on the tests that await Does it solve a real problem? Yes — #5828 bounced three times on an unrelated change, with an 830s collect-heavy CLI suite as context. Is the code straightforward? Yes — 3 net lines, no abstractions, no speculative work. Is every change necessary? Yes — two timeout bumps and one explanatory comment, nothing else. Do the results match the promise? Yes — the tests pass cleanly and faster than either budget; the fix is a no-op on fast runners but provides headroom on contended ones. Would I curse the author in six months? No — the comment explains the why, so a future maintainer can decide whether to remove it after i18n loading is optimized. Am I approving because it's genuinely good, or because I ran out of reasons to say no? Genuinely good — it's the right size for the right problem. Approving. ✅ 中文说明退一步看：这是一个范围精准、针对真实且被充分记录的 merge queue 痛点的手术式修复。我在读 diff 之前的独立方案大致也是"给会 是否解决真实问题？是——#5828 在无关改动上被踢了三次，背景是 collect 占大头的 830s CLI 套件。代码是否直白？是——净 3 行，无抽象，无投机性工作。每个改动都必要？是——两次超时调整 + 一条解释性注释，没有别的。结果是否匹配承诺？是——测试干净通过且远低于任一预算；在快的 runner 上是 no-op，在高负载 runner 上提供了余量。六个月后会不会骂作者？不会——注释解释了 为什么，未来维护者可以在 i18n 加载优化后决定是否移除。是因为真的好吗？是真的好——针对正确的问题，用了正确的规模。 批准合并。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jun 25, 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. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
| ); | ||
| expect(lastFrame() ?? '').toContain('ReadFile'); | ||
| }); | ||
| }, 15000); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The English-locale test may not need the 15s timeout bump. By the time this test runs, afterEach has already called setLanguageAsync('en') and populated translationCache['en'] — so setLanguageAsync('en') here is a cache hit and the dynamic import() (the slow path) never fires.
If the en test ever becomes slow for a non-i18n reason (rendering, test infra), the 15s ceiling would mask it. Consider reverting this test to the default 5s timeout and only keeping the bump on the zh test.
Suggested change
| }, 15000); | |
| }); |
— qwen3.7-max via Qwen Code /review
wenshao
reviewed
Jun 25, 2026
| expect(output).toContain('读取文件'); | ||
| expect(output).not.toContain('ReadFile'); | ||
| }); | ||
| // 15s timeout (not the 5s default): setLanguageAsync() loads locale |
Collaborator
There was a problem hiding this comment.
[Suggestion] The comment explaining the 15s timeout sits inside the zh test body (between assertions and }, 15000)), while the en test at line 1011 has }, 15000) with no explanation. A future maintainer grepping for 15000 will see one test with rationale and one without — inviting either removal of the "unnecessary" timeout or wasted investigation.
The en test does need 15s when run in isolation (e.g., vitest -t "keeps the English") because no prior afterEach has warmed translationCache['en']. But this rationale exists only in the PR description, not in the source.
Consider moving the comment above the it( call and adding one line covering both tests:
// 15s timeout (not the 5s default): setLanguageAsync() loads locale
// resources lazily and intermittently exceeds 5s on heavily parallelized
// macOS CI. The en test also cold-loads when run in isolation via -t.
it('shows the localized display name under the zh locale', async () => {
// ...
}, 15000);— qwen3.7-max via Qwen Code /review
wenshao
approved these changes
Jun 25, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
Jun 25, 2026
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
3 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 from the 5s default to 15s for the two
<ToolMessage /> localized badgelocale tests inToolMessage.test.tsx.Why it's needed
Both tests
await setLanguageAsync(), which loads locale resources lazily. On the heavily parallelized macOS CI runner that call intermittently takes longer than the 5s default and the test times out — even though nothing is actually broken. Because the failure only surfaces under merge-queue load (not at PR-check time, where the macOS job is skipped), it repeatedly kicks otherwise-passing PRs out of the merge queue. #5828 was bounced three times this way on an unrelated change; in the failing run the full CLI suite took 830s wall-clock, so a 5s budget for a lazy locale load is too tight under contention.Reviewer Test Plan
How to verify
Expected: both
zhandenlocale tests pass. The change only lifts the timeout ceiling; assertions are unchanged.Evidence (Before & After)
Before:
FAIL src/ui/components/messages/ToolMessage.test.tsx > <ToolMessage /> localized badge > shows the localized display name under the zh locale—Error: Test timed out in 5000ms.(failing merge-queue job)After (local macOS):
Tested on
Environment (optional)
Unit test only.
Risk & Scope
Linked Issues
Refs #5828 (the PR repeatedly evicted from the merge queue by this flake).
中文说明
这个 PR 做了什么
把
ToolMessage.test.tsx里两个<ToolMessage /> localized badge语言用例的单测超时从默认 5s 提到 15s。为什么需要
这两个用例都会
await setLanguageAsync()，而该调用是惰性加载语言资源的。在高度并行的 macOS CI runner 上，这个调用偶尔会超过 5s 默认超时导致用例超时——其实并没有真正的功能问题。由于该失败只在 merge queue 阶段（macOS job 在 PR check 阶段是 skip 的）才暴露，它会反复把本来能通过的 PR 踢出合并队列。#5828 就因此在一个无关改动上被踢了三次；那趟失败的 run 里整个 CLI 套件跑了 830s，所以在高负载下给惰性语言加载只留 5s 预算太紧了。如何验证
预期：
zh和en两个语言用例都通过。本改动只抬高超时上限，断言未变。风险与范围