-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(ci): give the scripts test suite the shared-ECS timeout ceiling (#10853) #10858
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
3a8d309
8558d2a
bcda5d7
db92f1b
3941251
b097f06
9de9aa0
20d0a38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,8 @@ | |
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { readFileSync } from 'node:fs'; | ||
| import { readdirSync, readFileSync } from 'node:fs'; | ||
| import { join } from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
| import { describe, expect, it, vi } from 'vitest'; | ||
|
|
||
|
|
@@ -227,8 +228,9 @@ describe('scripts suite timeout', () => { | |
| // 30s was the quiet-host figure. Release run 33725742855 lost its Quality | ||
| // Checks (Scripts) job to two files at once — qwen-autofix-workflow, whose | ||
| // heaviest case measures ~14s idle, and acp-serve-boundary-guard — neither | ||
| // slow, both past 30s under contention. A per-file `vi.setConfig` cannot | ||
| // fix it: these cases register their timeout at collection. | ||
| // slow, both past 30s under contention. A per-file `vi.setConfig` does | ||
| // outrank this config, but not the per-test budget the cases that timed out | ||
| // carry: that is registered at collection and beats both. | ||
| for (const [stub, expected] of [ | ||
| [undefined, 90_000], | ||
| ['5000', 5_000], | ||
|
|
@@ -245,4 +247,30 @@ describe('scripts suite timeout', () => { | |
| vi.unstubAllEnvs(); | ||
| } | ||
| }); | ||
|
|
||
| it('leaves no file-level runtime override shadowing that ceiling', () => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R4-2: Still stands at the post-merge head — this guard test is named Confirmed by probe (the guard's own scan, executed): The shape is live at HEAD — 8 instances, including in the very file this diff de-shadows: Failure scenario: one of the spawn-heavy capped cases lands on a contended release host and needs more than its stale cap — it times out at 30s while the 90s ceiling was raised precisely to shelter it; Quality Checks (Scripts) goes red again, the exact incident class this PR fixes, while this guard stays green. Suggested fix: either extend the guard to per-test timeout arguments (a scan anchored to the test-call closer shape), or delete the residual sub-ceiling caps the same way the six Fix constraint: scanned files contain fixture strings reproducing the un-anchored closer form — Fix witness: after the change, planting 中文说明[Suggestion] R4-2:在合并后的 HEAD 上仍然成立 —— 该守卫测试名为 探针确认(执行的是守卫自己的扫描逻辑):在 90 秒上限下给 300ms 测试体加单测试 50ms 上限 → 该形态在当前 HEAD 真实存在 —— 共 8 处,包括本 diff 刚移除遮蔽的那个文件: 失败场景:某个拉子进程的重用例落在有争用的发布机器上、耗时超过其陈旧上限时,会在 90 秒上限本是为保护它而抬高的情况下于 30 秒超时 —— Quality Checks (Scripts) 再次变红,正是本 PR 修复的事故类别,而守卫保持绿色。 建议修复:或把守卫扩展到单测试超时参数(以测试调用收尾形状为锚的扫描),或像删除六个 修复约束:被扫描文件中存在复现未锚定收尾形式的夹具字符串 —— qwen-autofix-workflow.test.js:21676 与 upload-aliyun-oss-assets.test.js:96 的 修复见证:修改后,在任何被扫描文件植入 — qwen3.8-max via Qwen Code /review (v0.23.0)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Partly implemented, sweep deferred — leaving this thread open. Implemented: the coverage boundary is now explicit in the guard's comment instead of implied by its name — "Deliberate per-test budgets ( Premise reproduced first, on this suite's own config (vitest 3.2.7, 90s ceiling): a per-test cap of Not implemented — neither extending the guard nor deleting the caps:
Recorded in the deferred-findings queue so the residual survives merge instead of being lost. This round removed the two couplings that could be deleted outright (R4-1's argument shape, R4-3's extension list) rather than adding a third scanner beside them. 中文说明部分实现,清扫延后 —— 本线程保持开放。 已实现:覆盖边界现在在守卫注释中显式写明,而不再由测试名暗示 —— “Deliberate per-test budgets ( 先复现前提,用的是本套件自身的配置(vitest 3.2.7,90 秒上限):300ms 测试体加 未实现 —— 既未扩展守卫,也未删除这些上限:
已记入延后发现队列,使该残留在合并后仍然留存、不至于丢失。本轮移除的是可以整体删掉的两处耦合(R4-1 的参数形状、R4-3 的扩展名列表),而不是在它们旁边再加第三个扫描器。 |
||
| // The pin above re-imports the config module, so it cannot see an override | ||
| // a suite applies to itself: a runtime `setConfig` in a test file outranks | ||
| // the project config and holds that suite at its own number while this file | ||
| // reads green — install-script.test.js sat at 30s under the raised ceiling | ||
|
qwen-code-dev-bot marked this conversation as resolved.
|
||
| // that way. Every argument shape does that, so the call itself is the | ||
| // violation; matching `testTimeout` inside the arguments let a parenthesized | ||
| // value ahead of it slip past. The walk takes every file here, not the | ||
| // include glob's extension list, so widening that list cannot leave the scan | ||
| // behind. Deliberate per-test budgets (`it(name, fn, ms)`) are out of scope: | ||
| // they are visible at the test they clamp, unlike a file-wide override. | ||
| const here = fileURLToPath(new URL('.', import.meta.url)); | ||
| const shadowing = readdirSync(here, { | ||
| recursive: true, | ||
| withFileTypes: true, | ||
| }) | ||
| .filter((entry) => entry.isFile()) | ||
| .filter((entry) => | ||
| /vi\.setConfig\(/.test( | ||
| readFileSync(join(entry.parentPath, entry.name), 'utf8'), | ||
| ), | ||
| ) | ||
| .map((entry) => entry.name); | ||
| expect(shadowing).toEqual([]); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] R4-2: This guard test is named
leaves no file-level runtime override shadowing that ceiling, but it scans only forvi.setConfig(...testTimeout...)— while per-test timeout arguments (it(name, fn, timeout)) also outrank the config ceiling, and that is the exact override shape this PR removes seven instances of fromscripts/tests/qwen-autofix-workflow.test.js(the}, 30000);endings). A revert or merge-conflict resolution restoringit('...', fn, 30000)re-opens the #10853 timeout class onecs-qwen-*runners while this test reads green — the guard cannot catch a regression of the very hunks this PR ships. Either extend the scan to numeric per-test ceilings over the same surface (asserting none reaches the flat ceiling), or narrow the test's name/comment to state it detects onlyvi.setConfigoverrides.Witness:
Any extended scan must keep the pre-existing intentional per-test budgets passing:
}, 20000);atscripts/tests/qwen-autofix-workflow.test.js:1543,}, 10000);at :11694 and :21601,}, 60_000);atscripts/tests/brand-create-safety.test.js:256,}, 40000);atscripts/tests/check-tui-dep-direction.test.js:573. If the scan is extended, prove it by mutation: restore any one of the}, 30000);endings this PR removed fromscripts/tests/qwen-autofix-workflow.test.js— this test must fail naming that file.中文说明
R4-2:该守卫测试名为
leaves no file-level runtime override shadowing that ceiling,但它只扫描vi.setConfig(...testTimeout...)——而单测试超时参数(it(name, fn, timeout))同样优先于配置上限,且那正是本 PR 从scripts/tests/qwen-autofix-workflow.test.js中移除的七处覆盖形式(}, 30000);结尾)。一次回退或合并冲突解决若恢复it('...', fn, 30000),就会在ecs-qwen-*runner 上重新引发 #10853 的超时类失败,而该测试仍显示绿色——守卫无法捕获本 PR 所提交改动自身的回归。要么把扫描扩展到同一面上的数值型单测试上限(断言没有任何一个达到固定上限),要么收窄测试名/注释,声明它只检测vi.setConfig覆盖。证据(Witness)见英文部分代码块。
任何扩展扫描都必须让既有的、有意写下的单测试预算继续通过:
scripts/tests/qwen-autofix-workflow.test.js:1543的}, 20000);、:11694 与 :21601 的}, 10000);、scripts/tests/brand-create-safety.test.js:256的}, 60_000);、scripts/tests/check-tui-dep-direction.test.js:573的}, 40000);。如果扩展扫描,请用变异证明:恢复本 PR 从scripts/tests/qwen-autofix-workflow.test.js移除的任意一个}, 30000);结尾——该测试必须失败并指名该文件。— qwen3.8-max via Qwen Code /review (v0.22.3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not applied this round — same reason as R4-1: this guard test cannot survive a merge with
main.The finding is correct and my own probe agrees with the semantics you measured: a file-level
vi.setConfig({ testTimeout: 100 })beats a 3000ms suite ceiling (Test timed out in 100ms), and per-test third-argument timeouts outrank it too. So the name does promise more than avi.setConfig-only scan delivers.It is moot on this branch:
origin/mainalready replaced theRUNNER_NAMEternary this guard pins withNumber(process.env['QWEN_SCRIPTS_TEST_TIMEOUT_MS'] ?? 90_000), and this PR's pin assertingtoBe(60_000)fails against it. Neither extending the scan nor narrowing the name is worth doing on a test the merge deletes.For #10910, the substance of this finding is live on
mainin a form neither the guard nor #10910's described scope covers:scripts/tests/install-script.test.js:63still carries a file-levelvi.setConfig({ testTimeout: 30_000 });that provably shadows main's 90s ceiling, pinning that file at the old flat 30s on the sharedecs-qwen-*pool. This PR removes that line; #10910 is described as removing the six per-test}, 30000)ceilings inqwen-autofix-workflow.test.js, which is a different file and a different override shape. Suggest folding the one-line removal in before this PR closes.Thread left open.
本轮未应用 —— 与 R4-1 同理:该守卫测试无法在与
main的合并中存活。该发现正确,我自己的探测也与你测到的语义一致:文件级
vi.setConfig({ testTimeout: 100 })会压过 3000ms 的套件上限(Test timed out in 100ms),单测试第三参数超时同样优先。所以测试名的承诺确实超过只扫vi.setConfig所能兑现的范围。但在本分支上已无意义:
origin/main已经用Number(process.env['QWEN_SCRIPTS_TEST_TIMEOUT_MS'] ?? 90_000)替换了本守卫所钉的RUNNER_NAME三元表达式,而本 PR 断言toBe(60_000)的 pin 对它失败。无论是扩展扫描还是收窄测试名,都不值得花在一个合并会删掉的测试上。对 #10910 而言,这条发现的实质内容正以守卫和 #10910 描述范围都未覆盖的形式存活在
main上:scripts/tests/install-script.test.js:63仍带有文件级vi.setConfig({ testTimeout: 30_000 });,可证明地遮蔽了 main 的 90s 上限,把该文件在共享ecs-qwen-*机器池上钉回旧的固定 30s。本 PR 删除了这一行;而 #10910 被描述为移除qwen-autofix-workflow.test.js中六处单测试}, 30000)上限 —— 那是另一个文件、另一种覆盖形式。建议在本 PR 关闭前把这一行的删除并进去。Thread 保持 open。