perf(cli): let tests resolve core modules individually - #10917
perf(cli): let tests resolve core modules individually#10917yiliang114 wants to merge 3 commits into
Conversation
…files importing the whole package Importing from the core package root pulls in its entire export graph — a bit over six hundred modules — however little of it a file actually uses. In a release run the cli workspace spent 2223s collecting modules against 1372s running tests, and a file that imports the package root costs about 11.5s before its first assertion where one importing a single module costs about 2s. cli's tsconfig already maps a wildcard subpath onto core's sources, so esbuild resolves per-module imports when it bundles. Vitest does not read tsconfig paths, and the alias list that stands in for them named only four subpaths, so those imports did not resolve under test at all. This adds the wildcard there. Expressing the alias list as an ordered array is what allows a pattern entry. The package root has to become an exact match in the process: as a string it would also match everything beneath it and rewrite each subpath into a path under index.ts. Two files move to per-module imports as a first check that the mapping holds end to end. Both were picked because nothing that depends on them replaces the core package with a mock factory — where a test does that, the mock stops intercepting once the code under test imports the module directly, so those call sites need their mocks moved in the same change and are left alone here.
|
Thanks for the PR — the motivation here is real and well measured. Template ✓ — all sections present, bilingual body complete. Problem: observed, not theoretical. #10908 documents concrete numbers (release run 33713579913: cli spends 2223s collecting vs 1372s running; a package-root import costs ~11.5s before the first assertion vs ~2s for a per-module import). Quantified and reproducible from CI data. Direction: aligned. CI collect-time is an active concern in this repo (#10908, and #10870 / #10869 landed in the same area this week). esbuild already resolves per-module imports through tsconfig paths while Vitest cannot — that gap is genuine, and closing it is the right move. Size: ~6 production source lines (two import swaps in Approach: the described change — ordered alias array, a wildcard mirroring the
Risk: no elevated-risk-path signals — none of the three changed files match the revert-correlated paths. Flagging the scope mismatch before diving deeper — moving on to code review. 🔍 中文说明感谢贡献！这个 PR 的动机是真实且有数据支撑的。 模板 ✓ —— 各节齐全，中英双语完整。 问题： 已观测到，而非理论问题。#10908 记录了具体数据（release run 33713579913：cli 模块收集 2223s 对比执行 1372s；包根导入到达第一条断言前约 11.5s，按模块导入约 2s）。量化且可从 CI 数据复现。 方向： 对齐。CI 收集耗时是仓库当前关注点（#10908，本周 #10870 / #10869 也落在同一领域）。esbuild 已通过 tsconfig paths 解析按模块导入，而 Vitest 不能——这个缺口真实存在，补上它是正确的。 规模： 约 6 行生产源码改动（两个文件的导入替换）；其余约 340 行改动在 方案： 描述中的改动——有序 alias 数组、对齐
风险： 无高风险路径信号——三个改动文件均不命中与 revert 相关的路径。 先提出范围不一致的问题，再深入——进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent baseline first: for "let tests resolve core modules individually", the minimal change is one wildcard alias mirroring the Blockers
Minor
TestingEvidence carried: the PR's own CI, read via the API at review time (unattended run — no PR code executed here). The unit suite is still in flight on this commit (~30-minute suite; not polling — the finalize job updates the table below once CI settles). Static read of Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Once the diff is reduced to the intended change, the perf claim's oracle is CI's own timing — the collect-vs-run split from #10908 — rather than a 中文说明代码审查先说独立基线：要让测试按模块解析 core，最小改动是加一条对齐 tsconfig 阻塞项
次要
测试证据说明：本节引用的是 PR 自身 CI 在审查时刻经 API 读取的真实状态（无人值守运行——此处未执行任何 PR 代码）。该提交的单测套件仍在运行（约 30 分钟；不轮询——finalize 任务会在 CI 结束后更新下表）。对 上表由 finalize 任务在 CI 结束后原地更新。待 diff 缩减为预期改动后，性能结论的判据是 CI 自身的时间数据（#10908 中"收集 vs 执行"的拆分），而非 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 2/5 — the idea is right and the alias-array half is well done, but the diff as committed regresses several deliberate, recently-landed settings and breaks resolution in two places; it can't merge as-is. Stepping back: the motivation is genuine — #10908's numbers are concrete, and the gap between esbuild (reads tsconfig paths) and Vitest (doesn't) is real. My independent proposal for this problem was almost exactly the additive half of this PR: one wildcard alias mirroring the tsconfig rule, the root kept exact, existing named aliases untouched, two files migrated as proof. That part here is clean and even comes with the right ordering rationale. What I can't sign is the rest. This commit's The fix is small: rebuild the commit on current — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
@yiliang114 Needs some rethinking before this can merge — full notes in the review comment above. The short version: the alias-array change itself is good, but this commit also drops settings that are live on main (ECS timeouts/maxWorkers, node-default environment, the #9149 globalSetup guard, coverage gating, the unhandled-error exemption — several pinned by scripts/tests/unit-vitest-configs.test.ts), removes three core aliases that still have import sites, and the new path-style imports don't resolve under npm run dev (no core exports entries). Rebuilding the commit on current main with only the alias restructuring + the two migrations should get it home. 🙏
中文说明
合并前需要重新考虑——完整说明见上方审查评论。简版：alias 数组改动本身没问题，但本提交同时删掉了 main 上生效的配置（ECS 超时/maxWorkers、node 默认环境、#9149 的 globalSetup 守卫、覆盖率门控、未处理错误豁免——其中数项被 scripts/tests/unit-vitest-configs.test.ts 钉住），移除了三个仍有导入点的 core alias，且新的路径风格导入在 npm run dev 下无法解析（core exports 没有对应条目）。基于当前 main 重建提交，只保留 alias 重构和两个文件的迁移，应该就能过。🙏
The previous commit was assembled from a working copy that predated main by several weeks, so it silently reverted this file to that older state. Four named core subpaths added since — envVarResolver, noFollowOpen, subSessionConstants and toolWriteOrigin — disappeared with it, and the new wildcard then claimed those specifiers and pointed them at files that do not exist. 257 test files failed to load as a result. All eight named subpaths are restored and kept ahead of the wildcard, with a comment saying why that order matters and what a contributor adding a ninth has to do. None of the eight can be derived from its specifier, so none of them can be folded into the pattern. The two migrated source files are rebuilt on their current contents for the same reason; one of them had also been reverted by a line.
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterFull-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
|
Cross-linking the plan: #10909 is the document this PR is phase ① of, and it names this exact step. Its §6.1 (「迁移写法（已确定）」) records the resolution chain and then says the wildcard alias has to come first — "vitest alias：目前只有 4 个具名 subpath …… 没有通配。深路径导入今天在测试里会解析失败，必须先补一条通配 alias，这是 phase ① 的第一步". This PR is that step. It also confirms the §6.1 table under vitest rather than only from the tsconfig rule: I checked this PR against §6.3, the plan's largest correctness risk, and it is clean — but the reason is worth writing down. §6.3's failure mode is that a test which mocks the core barrel silently stops intercepting once the code under test imports deeply: the suite stays green while testing something else. On this branch 138 cli test files carry
It is safe, and not by luck of ordering: line 42 replaces the whole The general rule that follows, for whoever writes the codemod: a barrel mock only breaks when the migrated module is the one the mock was meant to intercept through. A module that is itself stubbed is inert regardless of how it imports. That distinction is what makes the 138 tractable — the scope is not "138 files to audit" but "the subset whose barrel mock is intended to reach into a migrated module", and it should be computed per migration batch rather than up front. Why the payoff is worth pushing on. #10909's
中文说明互相链接一下计划侧：#10909 就是本 PR 所属的那份文档的 phase ①，而它点名了这一步。 其 §6.1（「迁移写法（已确定）」）记录了解析链，然后说明通配 alias 必须先做 —— 「vitest alias：目前只有 4 个具名 subpath …… 没有通配。深路径导入今天在测试里会解析失败，必须先补一条通配 alias，这是 phase ① 的第一步」。本 PR 就是这一步。它同时在 vitest 下印证了 §6.1 的表格，而不只是从 tsconfig 规则推导： 我拿本 PR 对着 §6.3（该计划最大的正确性风险）核对过，结论是干净的 —— 但原因值得写下来。 §6.3 的失效模式是：一旦被测代码改成深路径导入，mock 了 core barrel 的测试就会静默失去拦截，套件仍然是绿的，但测的东西变了。在本分支上，有 138 个 cli 测试文件带
它是安全的，而且不是靠顺序上的巧合：第 42 行把 由此得出的一般规则，供写 codemod 的人参考：barrel mock 只有在「被迁移的模块正是该 mock 意图经由其进行拦截的那个模块」时才会失效。 一个自身已被整体 stub 的模块，无论怎么导入都是惰性的。正是这个区分让 138 这个数字变得可处理 —— 范围不是「138 个文件要审」，而是「其 barrel mock 意图伸进某个被迁移模块的那个子集」，并且应当按每一批迁移分别计算，而不是一次性预估。 为什么这个收益值得推进。 #10909 的
|
3 participants
What this PR does
Teaches the cli test runner to resolve individual core modules, and moves two files off the core package root as a first end-to-end check that the mapping works.
The runner's alias list is expressed as an ordered array so it can carry a pattern entry, mirroring the wildcard subpath rule cli's tsconfig already has. The package root becomes an exact match in the process — spelled as a string it would also match everything beneath it and rewrite each subpath into a path under index.ts.
Why it's needed
Importing from the core package root pulls in its entire export graph, a bit over six hundred modules, however little of it a file actually uses. In release run 33713579913 the cli workspace reported 2223s collecting modules against 1372s running tests; core reported 546s against 251s. A file that imports the package root costs roughly 11.5s before it reaches its first assertion, where the same file importing a single module costs about 2s — and the suites that already replace the package with a mock factory, and so never evaluate it, have always run at about 1.9s.
esbuild reads tsconfig paths, so the bundle already resolves per-module imports. The test runner does not read them, and the alias list standing in for them named only four subpaths, so per-module imports did not resolve under test at all. That gap is what this PR closes; the two migrated files exist to prove it end to end before anything larger moves.
Background and measurements are in #10908.
Reviewer Test Plan
How to verify
The two migrated files should behave identically — the change is which module the same symbols come from. Their own suites cover them, and the rest of the cli suite exercises the alias change, since every test in the package now resolves the package root through a pattern entry rather than a string one. A resolution mistake here fails loudly at import time rather than subtly, so a green cli run is the signal.
Worth a reviewer's eye: the ordering in the alias array. The four named subpaths must stay ahead of the pattern entry because their targets are not derivable from their names, and the package root must remain an exact match.
Evidence (Before & After)
N/A — no user-visible behavior changes.
Tested on
Not run locally; relying on CI across the three platforms.
Risk & Scope
Linked Issues
Refs #10908
中文说明
这个 PR 做了什么
让 cli 的测试运行器能够解析 core 的单个模块，并把两个文件从包根导入改成按模块导入，作为端到端的第一次验证。
alias 列表改成有序数组形式，以便携带一条通配规则，对齐 cli tsconfig 里已有的通配 subpath 映射。包根在此过程中必须改成精确匹配——写成字符串时它同样会匹配其下所有子路径，把每个子路径重写成 index.ts 下的路径。
为什么需要
从 core 包根导入会拉进它的整个导出图，六百多个模块，无论调用方实际只用了多少。在 release run 33713579913 中，cli 的模块收集耗时 2223s、跑测试 1372s；core 是 546s 对 251s。一个从包根导入的文件在到达第一条断言之前要花约 11.5s，而同一文件改成按模块导入只要约 2s——那些本来就用 mock 工厂替换整个包、因而从不求值它的用例，一直是约 1.9s。
esbuild 会读 tsconfig 的 paths，所以打包时已经能解析按模块导入。测试运行器不读 paths，而代替它的 alias 列表只列了四个具名 subpath，因此按模块导入在测试里根本解析不了。本 PR 补的就是这个缺口；两个迁移文件的作用是在更大范围改动之前把链路走通。
背景和测量数据见 #10908。
审查者验证计划
如何验证
两个迁移文件的行为应完全不变——变的只是同一批符号来自哪个模块。它们各自的用例覆盖了自身，而整个 cli 套件则检验了 alias 改动，因为现在包中每个测试都通过通配规则而非字符串规则解析包根。这里若有解析错误会在导入期直接报错而不是悄悄改变行为，所以 cli 跑绿就是信号。
值得审查者留意的是数组里的顺序：四个具名 subpath 必须排在通配规则之前（它们的目标路径无法从名字推导），包根必须保持精确匹配。
证据（前后对比）
N/A —— 无用户可见行为变化。
风险与范围
关联 Issue
Refs #10908