test(ci): stabilize shared-runner budgets and cron interactive checks - #10648
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
|
|
Thanks for the PR! Template looks good ✓ Problem: observed, well-evidenced. The Ubuntu job in run 33392035277 failed with Direction: aligned. Fixing the flake by switching the metric — while keeping the one-second budget and the ReDoS/algorithmic-regression guard intact — is the right call for CI reliability. Size: touches Approach: minimal and right-sized. The budget and every classification assertion are preserved; only the measurement changes from wall-clock to process CPU time. The alternatives (loosening the budget, dropping the assertion, retrying on failure) would all weaken the very guard this test exists to enforce, and nothing in the diff goes beyond that. Risk: no elevated risk signals — test-only change, no high-risk paths. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到,证据充分。run 33392035277 的 Ubuntu 任务以 方向:对齐。通过切换计时口径来修复抖动,同时保留一秒预算和 ReDoS/算法回归防线,对 CI 可靠性是正确方向。 规模:触及 方案:最小且恰当。预算和全部分类断言均保留,只是测量方式从墙钟时间改为进程 CPU 时间。替代方案(放宽预算、删除断言、失败重试)都会削弱该测试本来的防线,且 diff 没有任何超出此范围的内容。 风险:无升级风险信号——仅测试改动,未命中高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
Code review — clean. Measuring process CPU time instead of wall-clock is exactly the right instrument here: a ReDoS or algorithmic regression burns CPU regardless of how busy the shared host is, so the guard keeps its teeth, while time the process spends descheduled waiting for the scheduler no longer counts against it.
No blockers. Testing — unattended CI run: this review builds and executes no PR code; the evidence below is the PR's own CI on the reviewed commit, fetched via the API. Note that the macOS and Windows unit jobs are event-gated (merge queue / nightly / manual dispatch only) and are expected to be skipped on PR CI — the Ubuntu unit job is the PR CI's unit gate, and it is the same job where this test flaked. It was still running at review time; the table is updated automatically once CI settles, and no result here is taken from the author's self-report. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 中文说明代码审查——干净。用进程 CPU 时间代替墙钟时间正是这里该用的度量:ReDoS 或算法回归无论共享主机多忙都会烧 CPU,所以防线依然有效;而进程被调度器挂起等待的时间不再计入预算。
无阻塞问题。 测试——无人值守 CI 运行:本审查不构建或执行任何 PR 代码;以下证据来自 API 拉取的该提交自身 CI。注意 macOS/Windows 单测任务按事件门控(仅合并队列/每日/手动触发),在 PR CI 中跳过属预期——Ubuntu 单测任务是 PR CI 的单测关口,也正是该测试发生抖动的任务。审查时它仍在运行;CI 结束后下表自动更新,此处不采用作者自报结果。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — the right fix for a real, observed shared-runner flake, and as small as it can be while keeping the regression guard intact. Stepping back: the problem is an actual CI failure with a run link and a tracking issue, not a hypothesis. The metric switch is the minimal change that removes scheduler noise without weakening what the test exists to catch — loosening or deleting the assertion would have been simpler and worse, and I don't see a simpler path that still detects ReDoS-class regressions. The diff carries nothing beyond the two assertions and a named constant for the unchanged budget; there is no scope creep to speak of, and production code is untouched. Six months from now this reads as an obvious, boring fix — exactly what a flake fix should be. The one open item is evidence rather than code: the Ubuntu unit job — the very job where this test flaked — was still running on the reviewed commit when this pass finished, so approval is deferred until CI lands green on 中文说明置信度:5/5——针对真实观测到的共享 runner 抖动的正确修复,并且在保留回归防线的前提下做到了最小化。 退一步看:问题是带运行链接和跟踪 issue 的真实 CI 失败,不是假设。计时口径切换是去掉调度噪声、同时不削弱测试本意的最小改动——放宽或删除断言会更简单但更糟,我也想不出更简单且仍能捕获 ReDoS 类回归的路径。diff 除两处断言和一个为不变预算命名的常量外别无其他,没有范围蔓延,生产代码未动。半年后再看,这是一个显而易见、平淡无奇的修复——抖动修复就该是这样。 唯一未决项是证据而非代码:审查结束时,Ubuntu 单测任务(正是该测试发生抖动的任务)在被审提交上仍在运行,因此批准推迟到 CI 在 — Qwen Code · qwen3.8-max Reviewed at |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
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
left a comment
There was a problem hiding this comment.
Reviewed exact head fcf2c82. No blocking issues found. The process-CPU timing preserves the one-second computation guard without charging shared-runner descheduling, and requiring a second marker occurrence distinguishes the model response from the echoed prompt while decoupling the test from dynamic composer suggestions. Validation: exact-head build succeeded; the focused core suite passed 548 of 548 tests; Prettier and ESLint passed; build, test, and security checks are green.
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed exact head fcf2c82; no blocking issues.
- The shell safety guards preserve the 1-second budget and every classification assertion, while measuring user+system CPU time in the correct microseconds-to-milliseconds conversion. Vitest 3.2.7 uses isolated fork workers, so unrelated parallel test files are not counted.
- The cron assertion now distinguishes the echoed user prompt from the model response by requiring the marker twice, matching the captured failing screen. Removing the fixed composer placeholder check correctly allows generated follow-up suggestions.
- Verified against both original failure logs and all downstream test-harness semantics. Focused shell suite: 548/548 passed. Both changed files pass Prettier and ESLint. PR CI and security checks are green.
What this PR does
This PR fixes two test-only CI failures without changing production behavior:
Type your messageplaceholder, because a generated follow-up suggestion can legitimately replace it.Why it's needed
Shared-runner CPU contention
The Ubuntu test job in run 33392035277 failed with
expected 1445.8520749999998 to be less than 1000. The previous wall-clock assertion counted time spent descheduled on the shared ECS host, producing a false performance failure even when classification consumed well under one second of CPU.After this change, CPU-intensive algorithmic or ReDoS regressions still exceed the unchanged budget, while scheduler wait does not.
Release cron interactive check
The no-sandbox integration job in release run 33401719747 failed after the model successfully returned
USERPRIORITY77. The composer displayed the valid follow-up suggestionCancel the cron job, which replaced the default placeholder and tripped an unrelated assertion.The previous wait could also match
USERPRIORITY77inside the user's own prompt. After this change, the test requires a second occurrence from the model response and stops coupling cron behavior to dynamic composer placeholder text.Reviewer Test Plan
How to verify
Evidence
Tested on
Risk & Scope
Linked Issues
Fixes #10637
Fixes #10646
Related to #10490
中文说明
本 PR 做了什么
这个 PR 合并修复两类仅影响测试的 CI 假失败,不修改生产行为:
Type your message占位符的依赖,因为 follow-up suggestion 合法地会替换该占位符。为什么需要
run 33392035277 中,墙钟计时在共享 ECS runner 竞争时超过一秒并误报。改用 CPU 时间后,算法退化和 ReDoS 仍会消耗 CPU 并触发同一预算,而调度等待不会。
release run 33401719747 中,模型已经成功返回
USERPRIORITY77,但输入框显示了合法的 follow-up suggestionCancel the cron job,固定 placeholder 断言因此失败。旧等待条件也可能直接匹配用户输入中的同名标记。新条件要求标记出现两次,从而确认模型确实响应,并解除 cron 测试与动态输入框文案的耦合。验证
关联问题
Fixes #10637
Fixes #10646
Related to #10490