test(i18n): raise timeout for slow must-translate locale suites on Windows CI - #5024
Merged
Merged
Conversation
The must-translate locale coverage tests switch locales and build the full built-in command tree (loadCommands runs twice per strict-parity locale), which triggers dynamic locale imports plus command construction. On cold Windows CI runners this intermittently exceeds vitest's default 5s per-test budget and times out (zh-TW / zh-CN strict-parity cases), while ubuntu and macOS pass. The test logic is unchanged. Give the three locale-iterating it.each blocks an explicit 20000ms timeout, matching the convention already used by the sibling i18n suite (index.test.ts).
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Raises the per-test timeout for locale-iterating i18n tests to reduce CI flakiness on slow Windows runners.
Changes:
- Introduced a shared
SLOW_LOCALE_TEST_TIMEOUT_MSconstant (20s) with rationale in comments. - Applied the explicit timeout to three
it.each(...)locale-iterating blocks inmustTranslateKeys.test.ts.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
|
Thanks for the PR, @yiliang114! Template: headings differ from the repo template (e.g. "Summary" vs "What this PR does", "Validation" vs "Reviewer Test Plan"), but all the required content is present and well-organized — not going to block on naming. On direction: straightforward CI stability fix. Flaky Windows timeouts in i18n locale tests are a real pain point that wastes contributor time and blocks unrelated PRs. Clearly aligned with the project. On approach: minimal and well-justified. A single constant, applied to the two One small observation: the first Moving on to code review. 🔍 中文说明感谢贡献，@yiliang114！ 模板：标题和仓库模板不完全一致（例如"Summary" vs "What this PR does"），但所有必需内容都齐全且组织良好，不会因此阻断。 方向：直接的 CI 稳定性修复。i18n 本地化测试在 Windows 上超时是一个真实存在的痛点，浪费贡献者时间并阻断无关 PR。方向完全对齐。 方案：最小且充分。一个常量，应用到两个执行昂贵 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
Code ReviewThe diff is exactly what Stage 1 predicted: a No correctness issues. No security concerns. The constant name is descriptive, the value matches the sibling suite convention, and no logic or assertions were touched. One question for the author: the first Test ResultsRan the affected test suite on Linux: All 20 tests pass in 329ms — well under the new 20s ceiling. The Windows timeout fix can't be reproduced on Linux (it only manifests on cold Windows runners), but the CI job on this PR itself will serve as the real validation. 中文说明代码审查Diff 与 Stage 1 预期一致：一个 无正确性问题，无安全隐患。常量名具有描述性，值与兄弟测试套件惯例一致，未触及任何逻辑或断言。 一个小问题：第一个 测试结果在 Linux 上运行了受影响的测试套件，20 个测试全部通过（329ms），远低于新的 20s 上限。Windows 超时问题在 Linux 上无法复现，但此 PR 自身的 CI 将作为真正的验证。 — Qwen Code · qwen3.7-max |
Collaborator
|
This is a clean, minimal fix that does exactly what it says. 9 lines added, zero logic changes, following an established convention in the codebase. The motivation is backed by real CI failure evidence, and the test results confirm no regression on Linux. Going back to my independent proposal: I would have done the same thing — add a timeout constant to the expensive test blocks. The PR matches the approach exactly, and the author went further by adding a clear comment and matching the sibling suite's convention. The only thing I'd flag is that the first Ships it. ✅ 中文说明这是一个干净、最小化的修复，完全按描述工作。添加 9 行，零逻辑变更，遵循代码库中已有的惯例。动机有真实 CI 失败证据支持，测试结果确认在 Linux 上无回归。 回到我的独立方案：我会做同样的事情——给昂贵的测试块添加超时常量。PR 的方案完全一致，作者还进一步添加了清晰的注释并与兄弟测试套件的惯例保持一致。 唯一值得注意的是第一个 可以合并 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jun 12, 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. |
qqqys
approved these changes
Jun 12, 2026
qqqys
left a comment
qqqys
left a comment
Collaborator
There was a problem hiding this comment.
Critical-only review: no critical issues found. The change is test-only, raises timeouts for slow locale coverage cases, and CI is green across lint, CodeQL, and all OS test jobs.
doudouOUC
pushed a commit
that referenced
this pull request
Jun 15, 2026
The must-translate locale coverage tests switch locales and build the full built-in command tree (loadCommands runs twice per strict-parity locale), which triggers dynamic locale imports plus command construction. On cold Windows CI runners this intermittently exceeds vitest's default 5s per-test budget and times out (zh-TW / zh-CN strict-parity cases), while ubuntu and macOS pass. The test logic is unchanged. Give the three locale-iterating it.each blocks an explicit 20000ms timeout, matching the convention already used by the sibling i18n suite (index.test.ts).
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
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.
Summary
20000msfor all three locale-iterating must-translate checks in the affected i18n suite. This is test-only; assertions and runtime behavior are unchanged.5000msper-test budget, while the same work passed on ubuntu and macOS. A shared timeout matches the convention already used by the sibling i18n suite.20000msbudget, is the right scope. Passing runs are not delayed; the higher ceiling only matters for slow or hung locale cases.Validation
cd packages/cli npx vitest run src/i18n/mustTranslateKeys.test.tsTest (windows-latest, Node 22.x)job. The flaky failure is Windows-specific, so that job is the authoritative validation for the original timeout symptom.Test (windows-latest)failed.Scope / Risk
Testing Matrix
Testing matrix notes:
npx vitestand it passed.Linked Issues / Bugs
Pre-existing CI flakiness; the affected suite landed earlier with Vitest's default timeout, so this is not a regression from a single PR. No closing keyword.