refactor: collapse duplicate test infrastructure - #4323
Conversation
53e170c to
c12694b
Compare
Every workspace carried a `test` script that was `clean && build && test:dist`, but nothing consumed it: the root `npm test` runs `build:test` and then drives each workspace's `test:dist` through run-workspace-tests-parallel, and CI takes the same path. The nine copies had already begun to drift — @maka/mcp inlined `node --test` instead of delegating, and @maka/desktop's `test` covered a strictly smaller set of files than its own `test:dist`. @maka/cli's `pretest` was not a test hook at all: release.yml calls it to build the CLI's production workspace closure, which is why it survives here under the name it earned, `build:workspace-deps`, matching the @maka/desktop script that does the same job. @maka/desktop's `pretest` rebuilt what `build:test` had already built and has no other caller, so it goes. @maka/eval listed its seven Python suites by hand and chained them with `&&`, so a failure in the first one hid the other six and every new suite needed a package.json edit to run at all. `unittest discover` is the standard-library equivalent, reports all failures, and picks up new suites on its own. test_egress_filter_live.py stays excluded in practice through its existing `skipUnless` on MAKA_EVAL_EGRESS_PROXY_TEST, which `test:egress-proxy:live` still sets. Test execution now has one entry point per workspace. The documented single-workspace command becomes `npm --workspace <name> run test:dist`, which requires an explicit build first — the cost the deleted `test` scripts were hiding behind a full clean rebuild on every invocation. Generated-by: Claude Code
Seven of the shim's ten methods forwarded straight to node:assert with no
added semantics — toBe was assert.strictEqual, toEqual was
assert.deepStrictEqual, and so on — so they bought an extra API concept
and a stack frame in exchange for nothing. This rewrites those 1667 call
sites across 31 files onto the assertions they already were.
@maka/core used only forwarders, so its copy of the shim is deleted here.
@maka/runtime keeps its copy for now: 110 call sites still use toContain
and toMatchObject, whose shim semantics differ from the Jest methods they
are named after and need judgement rather than a rewrite rule.
The rewrite is mechanical and expected to be behaviour-preserving.
@maka/core reports the same 731 passing tests as before, @maka/runtime the
same 3089 passing and 13 skipped.
Three toHaveLength sites needed a hand edit: the shim cast its subject to
`{ length: number }`, which hid an optional chain from tsc. They now read
`backend?.sendInputs?.length`, so a missing backend fails the assertion
instead of throwing a TypeError out of the shim. Both outcomes are a
failing test; the new one names the reason.
Generated-by: Claude Code
…shim
The shim's last three methods borrowed Jest names without Jest semantics.
toContain ran `String(actual).includes(expected)`, so the 13 call sites
whose subject was an array — `events.map((event) => event.type)`,
`await manager.recoverInterruptedSessions()`, which returns string[] —
were matching against a comma-joined string rather than testing element
membership. `['run_created_extra']` would have satisfied
toContain('run_created'). toMatchObject compared only the top-level keys
of the expectation with deepStrictEqual, so a nested object had to match
exactly where Jest would have taken a subset.
toContain becomes a direct `includes` on the real subject, which is the
form the neighbouring assertions in these same files already use
(`assert.strictEqual(events.includes('run_failed'), false)`). One negated
site keeps substring matching deliberately: assistantTexts holds message
bodies, so leaked child output could appear inside a longer message, and
`!texts.some((text) => text.includes(...))` is what the old String()
coercion happened to provide. The other two negated sites compare
identifiers, where element equality is the stronger reading.
toMatchObject becomes assert.partialDeepStrictEqual only where the
expectation is entirely primitive, which is the 75 cases where the two
semantics coincide. partialDeepStrictEqual matches arrays as an ordered
subsequence — `[]` satisfies any array — and recurses partially into
nested objects, so for the 16 expectations containing an array or a
nested object it is strictly weaker than the shim was. Those keep the
old exactness with a deepStrictEqual per complex key. Without that split
this commit would have silently voided a sandbox-containment assertion in
builtin-tools and made `{ matches: [] }` in workspace-executor
unfalsifiable.
The change is therefore a trade, not a uniform strengthening: stronger on
array membership and on absent-versus-undefined, unchanged elsewhere.
No test changed colour. With no call sites left,
packages/runtime/src/test-helpers.ts is deleted and the repository has one
assertion API.
Generated-by: Claude Code
Ninety-six copies of three async primitives across five workspaces, none
of them carrying product semantics: a test reaching for `deferred` is
arranging timing, not asserting behaviour. `deferred` alone had 66
definitions in 42 distinct implementations — some generic, some fixed to
`Promise<void>`, 28 exposing `reject` and 38 dropping it — so which
rejection paths a suite could exercise depended on which copy its file
happened to grow.
They now live in @maka/core under a test-only subpath, alongside the
existing test-only exports in @maka/mcp, @maka/runtime and
@maka/runtime-host. `workspaceReleaseManifest` strips that prefix by
directory name rather than by an enumerated list, and electron-builder
already excludes `**/test-only/**`, so the new module inherits both
boundaries; release-cli-file-policy asserts the exact set of stripped
subpaths and gains the new one.
`deferred<T = void>` keeps the argument-free `resolve()` that the
void-typed copies used, and always exposes `reject`. The shared
`withTimeout` clears its timer on every outcome — three copies only
unref'd theirs, leaving a settled promise's timer to expire on its own.
The execution-host suite fixture exported a withTimeout identical to the
shared one; its five importers now take the shared one directly rather
than through a harness that has no reason to forward a general primitive.
Four definitions stay where they are, each with a signature the shared one
cannot express:
- `withTimeout(promise, message)` in three suites takes its deadline from
a file-local constant rather than an argument.
- `withTimeout(promise, timeoutMs, createTimeoutError)` in the ACP harness
builds a typed StartupTimeoutError and accepts a bare value as well as a
promise.
Two more in @maka/storage keep their local copies: they wrap the caller's
label into the message ("bundle export timed out"), so adopting the shared
version would mean rewording twenty call sites to keep the same failure
text. That is a separate judgement from removing duplicates.
@maka/runtime reports 3090 passing and 13 skipped, @maka/runtime-host 1443
passing and 12 skipped, both unchanged by this commit. The full build:test
typechecks every workspace touched here.
Generated-by: Claude Code
c12694b to
6de7700
Compare
`SERIAL_WORKSPACE_DIRS` has been empty since apache#2132 took runtime and runtime-host back out of it, so `partitionWorkspaces` has been splitting every workspace into the parallel batch and handing `runSerial` an empty array on every run since. The eight-line comment above that call explains why both batches' errors are collected rather than short-circuited — a failure mode that cannot occur while one of the two batches is always empty. The other entry point, `--serial`, was reachable only through `test:dist:serial`, which nothing calls: not CI, not another script, not the documentation. `--concurrency=1` already expresses the same thing and is what windows-baseline.yml uses when it wants one workspace at a time. `runWorkspaceTests` is now `runParallel` over every workspace. Scheduling has one knob, and the runner reports what passed through one path. Generated-by: Claude Code
Thirty-five suites each wrote their own poll-until-true loop under one of five names — `waitFor`, `waitUntil`, `waitForCondition`, `waitForAsync`, `waitForUpTo`. They agreed on the shape and disagreed on everything inside it: whether a spent budget throws an `Error` or an `AssertionError`, and whether the predicate is read once more after the budget runs out or the last read before it decides the result. Five of them could not fail at all on the final read, because they ended in `assert.ok(predicate())` after a loop that had already returned on the same condition. Those two decisions are now made once, in `@maka/core`'s test-only `waitFor`, in the more forgiving direction: the predicate is always read once more before the throw, so a wait that would have passed under any of the thirty-five still passes. What is not shared is the budget. The copies expressed it two ways — a wall-clock deadline or a poll count — and neither derives from the other, because under load `100 attempts × setImmediate` and `1_000ms` move in opposite directions. `waitFor` takes both, plus the poll delay and the failure message, so each call site passes exactly what it had. The 818 call sites are untouched. `tui-terminal-mock`'s `waitFor` keeps its own loop. Its budget is 250ms locally (apache#2221 records a docs-only PR failing this suite at 262ms), and at that scale the extra microtask of `await`-ing across a module boundary is observable: delegating made a `/model` picker test read the screen one frame early and then type into a TUI that had not settled. A wait that tight is measuring its own timing, which is the opposite of a primitive. Line count is a wash — a delegation with its options is about as long as the loop it replaces. What shrinks is the number of implementations of "poll until true, then fail": thirty-five to one. Generated-by: Claude Code
…licate-test-infrastructure # Conflicts: # packages/runtime/src/__tests__/session-manager.test.ts
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Reviewed the latest head 64d662221. No P0 or P1 — approving. Nothing at P2 or P3 either, so no inline comments. All 21 checks green.
This is 160 files, so I targeted the two places a consolidation like this would actually hide damage: whether the assertion migration silently weakened anything, and whether removing the serial scheduler dropped a capability.
The assertion migration strengthens rather than weakens, which is the key question. toBe maps to assert.strictEqual and toEqual maps to assert.deepStrictEqual. That second one matters: Jest's toEqual ignores undefined properties and does not compare prototypes, so deepStrictEqual is stricter than the semantics being replaced. That is what "correct the shim's borrowed Jest semantics" means in practice, and the whole suite passing under the tighter comparison is real evidence that the looser behaviour was not masking mismatches. The migration is also complete — no expect-shim import survives anywhere, and no bare expect( remains in the test sources.
The serial-scheduling removal is dead code, and I checked each claim rather than taking the commit message's word for it. SERIAL_WORKSPACE_DIRS really is [] on main (line 48), so partitionWorkspaces genuinely handed runSerial an empty array on every run. test:dist:serial really has no callers — nothing in CI, scripts, or docs outside an archived file. And --concurrency=1 really does preserve ordered serial execution: workerCount = Math.min(dirs.length, concurrency) yields a single worker consuming dirs in order, so the capability survives through the parameter that remains rather than being lost. if (!(concurrency > 0)) throw also rejects zero, negative and NaN, so a malformed --concurrency fails loudly instead of silently defaulting.
Removing a duplicate mechanism while keeping the capability in the surviving one is the right shape for this, and the reachability argument in the commit message held up on every point I tested.
One thing far below P3, mentioned only so you can dismiss it knowingly: docs/archive/runtime-kernel.md:279 still shows npm --workspace @maka/storage run test, which no longer exists — both workspaces keep test:dist. It is under docs/archive/, so being a historical record rather than executable guidance is presumably the point.
Scope. I drove the assertion-semantics question, the shim removal's completeness, and the scheduler change. I did not read all 160 files; on a change this size "approved" means the consolidation is sound where I probed it, not that every migrated assertion was individually re-derived.
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 64d662221cbed729a3ae2ad59b05369d7eec4ad5. The change consolidates workspace execution on test:dist, removes the unused serial-runner path, centralizes test-only async primitives, and replaces the local Jest-style assertion shim with Node assertions. I found no P0 or P1 issues. One non-blocking P2 is noted inline: current contributor-facing verification instructions still invoke workspace test scripts that this change removes.
Validation included a clean install, build:test, full typecheck, lint, format, ASF-header audit, git diff --check, current-main merge-tree, direct reproduction of both stale commands, and exact-head hosted checks (all required jobs successful). The local Node 22 full workspace run also exposed three environment/baseline failures outside the changed behavior: the warning-sensitive Storage child-process test passes with warnings suppressed, the unchanged Desktop OAuth deadline test cancels under this local runtime, and the managed-sandbox Host integration cannot run in this container.
Review notice: This review was prepared by an automated review agent operated by hqhq1025 and is published at the direction of AstroHan, who has read these findings and is the human accountable for them.
The commit that removed the workspace `test` scripts updated the top-level guides and missed seven live instructions further in: the Computer Use README, the Desktop smoke checklist, and six test-file headers that name the command for the suite they sit in. Following any of them on that head fails with `Missing script: "test"`. They now match CONTRIBUTING's shape — build first, then `test:dist` — because `test:dist` runs against `dist/` and the deleted scripts were hiding a full clean rebuild inside every invocation. Generated-by: Claude Code
zhiiw
left a comment
zhiiw
left a comment
There was a problem hiding this comment.
Reviewed at exact head e13abb62 (verified unchanged at review time; the full CI matrix is green on this head, including test 25m20s and package 23m47s).
Test-infrastructure consolidation, verified rather than assumed:
- The semantic commit is the one I read line by line, and the mapping is correct: string-subject
toContainkeeps substring semantics, array-subject sites become real membership tests (.includes), andtoMatchObjectmaps topartialDeepStrictEqualonly for the all-primitive expectations while complex keys keep per-keydeepStrictEqual. The old shim genuinely ranString(actual).includes(...)— the comma-joined-array false-positive it cites is real (['run_created_extra']would have satisfiedtoContain('run_created')). - The premises check out on the tree, not on the description: the base ledger's
SERIAL_WORKSPACE_DIRSwas empty and the mechanism is now gone rather than empty;release.ymlcallsbuild:workspace-deps;--serial's only consumer path was already covered by--concurrency=1(windows-baseline.yml uses it); the new@maka/core./test-only/*export is stripped from the shipped release manifest by the existingworkspaceReleaseManifestprojection (verified by executing the projection — the dropped set is exactly["./test-only/async-primitives"]); the sharedwaitForkeeps both budget shapes and reads the predicate once more before throwing, so no copy that passed before can fail now; and the TUI suite keeps its own loop for the documented one-frame reason. - Six separable commits: the mechanical rewrite is behaviour-preserving, and only the semantic commit can change a test's colour — which is the one whose callsites I re-read.
Executed on a real Windows machine at this head: clean build:test, then @maka/core 739/739, plus @maka/mcp 174/179, maka-agent 623/665, @maka/desktop 1729/1752, and the codemod-touched runtime files 220/239. Every local failure is a pre-existing platform class on this machine (symlink EPERM without Developer Mode, /bin/echo-style POSIX fixtures, LaunchAgent fsync EPERM, process-timing races) — and each one either sits in a file the PR never touched, or fails in fixture setup before the codemod's assertion lines are reached. The full hosted matrix runs the same suites green on this head. The complete @maka/runtime and @maka/runtime-host local runs do not finish within an hour on this machine (pre-existing slowness in the process-heavy suites); the hosted test job is the authority for them here.
Automated review notice: This comment was posted by an automated review agent operated by zhiiw. It is not an independent human review and does not replace one.
简体中文
测试基建归并，全部核实而非假设：语义修正提交逐行读过——字符串 toContain 保持子串语义、数组主体改真成员判定、toMatchObject 仅对全原始值期望映射 partialDeepStrictEqual（复杂键保留逐键 deepStrictEqual）；旧 shim 确实是 String(actual).includes 的逗号拼接误判。前提逐项在树上验证：SERIAL_WORKSPACE_DIRS 连机制删除而非留空；release.yml 改调 build:workspace-deps；--serial 唯一消费者已被 --concurrency=1 覆盖；core 新 test-only 导出被 workspaceReleaseManifest 投影精确剥除（实测）；共享 waitFor 保留两种预算且抛前再读一次谓词（任何旧副本能过的等待在新实现下仍过）；TUI 套件保留自有循环有文档化理由。六个提交可分离，只有语义提交可能改变测试颜色。本机真 Windows：build:test 干净，core 739/739，mcp/cli/desktop 与 codemod 触及的 runtime 文件的失败全部落在既有平台类（symlink EPERM、POSIX 夹具、LaunchAgent fsync、进程时序），且要么在 PR 未触碰的文件、要么挂在夹具建立阶段（在 codemod 改的断言行之前）；hosted 全矩阵在同 head 全绿。本机 runtime/runtime-host 全量一小时内跑不完（既有慢），由 hosted test 权威覆盖——如实声明。
…on (apache#3349) Main's apache#4323 collapsed the duplicate expect shim into node:assert and removed packages/{core,runtime}/src/test-helpers.ts. The series' new assertions were still written against the shim, so after the rebase they referenced an import that no longer exists. Translate them onto the same mapping apache#4323 used — toBe/toEqual become strictEqual/deepStrictEqual — keeping every subject and expectation byte-identical. Generated-by: ZCode (Z.ai GLM)
…on (apache#3349) Main's apache#4323 collapsed the duplicate expect shim into node:assert and removed packages/{core,runtime}/src/test-helpers.ts. The series' new assertions were still written against the shim, so after the rebase they referenced an import that no longer exists. Translate them onto the same mapping apache#4323 used — toBe/toEqual become strictEqual/deepStrictEqual — keeping every subject and expectation byte-identical. Generated-by: ZCode (Z.ai GLM)
…on (apache#3349) Main's apache#4323 collapsed the duplicate expect shim into node:assert and removed packages/{core,runtime}/src/test-helpers.ts. The series' new assertions were still written against the shim, so after the rebase they referenced an import that no longer exists. Translate them onto the same mapping apache#4323 used — toBe/toEqual become strictEqual/deepStrictEqual — keeping every subject and expectation byte-identical. Generated-by: ZCode (Z.ai GLM)
5 participants
Summary
Five facts in the test infrastructure that were each represented more than once.
Test execution entry points. Every workspace had a
testscript (clean && build && test:dist) that nothing consumed — the rootnpm testand CI both drivetest:distdirectly. The nine copies had drifted:@maka/mcpinlinednode --test,@maka/desktop's covered fewer files than its owntest:dist.@maka/cli'spretestwas not a test hook — release.yml builds the CLI's production closure with it — so it survives asbuild:workspace-deps.@maka/evallisted its seven Python suites by hand and&&-chained them, hiding six failures behind the first;unittest discoverreplaces that.The
expectshim. Two copies (core,runtime), 31 files, 1777 call sites. Seven of ten methods forwarded straight tonode:assertwith no added semantics. One assertion API now.The shim's borrowed Jest names.
toContainranString(actual).includes(expected), so 13 array-subject call sites matched a comma-joined string instead of testing membership —['run_created_extra']satisfiedtoContain('run_created').toMatchObjectcompared only top-level keys withdeepStrictEqual.Async primitives. 96 copies of
deferred,nextId,withTimeoutacross five workspaces;deferredalone had 66 definitions in 42 implementations, 28 exposingrejectand 38 not. They move to@maka/core's test-only subpath, whichworkspaceReleaseManifestand electron-builder already strip. Six stay local, each with a signature or failure text the shared one cannot express.Predicate polling. 35 hand-written poll-until-true loops under five names, disagreeing on whether a spent budget throws
ErrororAssertionErrorand whether the predicate is read once more before failing. The sharedwaitFordecides both, in the forgiving direction. Budgets are not shared — the copies expressed them as a deadline or as a poll count, and neither derives from the other — so each call site passes what it had. All 818 call sites are untouched.Plus:
SERIAL_WORKSPACE_DIRShas been empty since #2132, so the runner's serial batch has received an empty array on every run since;--serialwas reachable only throughtest:dist:serial, which nothing calls, and--concurrency=1already says it.Review focus
The semantic correction is a trade, not a uniform strengthening.
toContaingets stronger.toMatchObjectmaps ontoassert.partialDeepStrictEqualonly for the 75 expectations that are entirely primitive, where the semantics coincide;partialDeepStrictEqualmatches arrays as an ordered subsequence ([]satisfies any array), so the 16 expectations containing an array or nested object keep the old exactness through adeepStrictEqualper complex key. Without that split this would have voided a sandbox-containment assertion inbuiltin-toolsand made{ matches: [] }inworkspace-executorunfalsifiable.tui-terminal-mock'swaitFordeliberately keeps its own loop. Its budget is 250ms locally (#2221 records a docs-only PR failing that suite at 262ms), and at that scale the extra microtask ofawait-ing across a module boundary is observable — delegating made a/modelpicker test read the screen one frame early.Verification
npm run build:testnode scripts/run-workspace-tests-parallel.mjs --concurrency=1 --workspaces=packages/core,packages/runtime,packages/runtime-host,packages/mcp,packages/cli,apps/desktop— all six passpython3 -m unittest discover --start-directory harbor --pattern 'test_*.py'— 81 tests / 12 skippednpm run lint,npm run format:checkNot run locally: desktop e2e, Storybook,
@maka/ui/@maka/storage/@maka/eval. CI runs all of them.AI use
Select exactly one:
Tool(s) and scope: Claude Code found these duplicates, wrote and ran the AST codemods behind the rewrite, and drafted the commit messages and this description. The affected commits carry
Generated-by: Claude Codetrailers. A human contributor reviews the final diff and owns the merge decision.Checklist
Does this PR entail a change in behavior?