Repository navigation
refactor(test): simplify Vitest launch and project ownership - #139377
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review in progressClawSweeper is reviewing this revision. This supersedes any previous blocked status. |
|
@clawsweeper review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Codex review: blocked before merge. Reviewed September 5, 2026, 4:47 PM ET / 20:47 UTC. ClawSweeper reviewWhat this changesSimplifies focused test launches, preserves Vitest option operands, centralizes plugin test routing, and restores isolated UI tests to root discovery. Merge readiness⛔ Blocked before merge - 3 items remain This remains useful: current main still contains the operand-rewriting defect and omits isolated UI tests from root discovery. No actionable introduced defect was found, and the maintainer-owned PR is not eligible for cleanup closure. Priority: P2 Review scores
Verification
How this fits togetherOpenClaw’s test launcher converts developer and CI test selections into Vitest projects and child processes. It owns worker preparation, cancellation, cleanup, and final test reports. flowchart TD
A[Developer or CI test request] --> B[Test launcher]
B --> C[Argument and project routing]
C --> D[Project execution owner]
D --> E[Compiled worker preparation]
D --> F[Native Vitest processes]
E --> F
F --> G[Cleanup and final reports]
Before merge
Agent review detailsSecurityNone. PR surfaceTests -42, Other -92. Total -134 across 15 files. View PR surface stats
Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep one project-execution owner and canonical argument routing while preserving native worker budgets, cancellation, reports, and complete UI discovery. Do we have a high-confidence way to reproduce the issue? Yes, from source: plugin-directory operands fall through to path rewriting, and root discovery omits the owner of explicitly excluded UI tests. No tests were executed during this read-only review. Is this the best way to solve the issue? The consolidation addresses the owning boundaries without another routing table or wrapper. Full native-operand compatibility remains independently unverified because Vitest dependency source could not be inspected. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against ea94f24ebb21. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
|
@clawsweeper re-review The PR body now includes the completed final-source controlled Linux comparison and direct pinned Vitest parser inspection. The pushed CI repair keeps compiler environment preparation in the invocation owner and routes report generation through the canonical launcher; full native worker/report suites and dependency audits pass locally. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Merged via squash.
|
…w#139377) * refactor(test): simplify Vitest launch and project ownership * fix(test): preserve compiler and report execution boundaries
Closes #139360
Related: #138264
What Problem This Solves
Focused Vitest runs started another Node/tsx orchestration process before reaching the existing project runner. Plugin selection duplicated the canonical routing chain and kept a stale option table that rewrote valid Vitest 5 operands from plugin directories—for example,
--tagsFilter slowbecame--tagsFilter browser/slow, and--max-workers 2became--max-workers browser/2.Root discovery also omitted the isolated UI project even though the shared UI owner excludes all 34 of its files. Native root discovery returned no result for an isolated UI test that its dedicated project correctly found.
Why This Change Was Made
Call the existing project execution owner directly with explicit arguments and environment. It continues to own native children, bounded generations, report completion, and cancellation; public entrypoints retain final outcome reporting. Invocation owners prepare thread-budget settings and pass them to the minimal compiler owner. Remove the obsolete child entry, delegation helper, duplicate plugin predicates, and stale option table; reuse the existing CLI classifier and plugin resolver. Report utilities now use the same Node launcher instead of requiring pnpm on PATH.
Register the existing isolated UI owner in the root aggregate and extend its ownership regression. The dependency patch, worker limits, ordering, isolation, and preparation/report contracts remain intact.
User Impact
Developers get faster focused launches, correct Vitest 5 option forwarding, and complete root UI discovery. Existing plugin assignments remain identical across all 149 registered roots. Production runtime code is unchanged.
Evidence
Final candidate:
b38bcbfda3fd15ef1e1901e6c327080fb554e6d7; frozen baseline:ae8f9b612aaa241beb26c76bf58706d0d6f29715.Controlled Linux proof: Node 24.19.0, pnpm 12.3.4, four fixed CPU affinities, two Vitest workers, one warmup and seven alternating measured pairs per workload.
All 32 invocations passed with identical native test/status inventories. All 38,136 tracked source entries, including modes, symlinks, and deletions, matched before/after measurement and final local verification. The committed tree matches the measured tree. Downloaded evidence archive SHA256:
bd440939c7037e2997f261956301ba2588c106a4431a36f9902bbbc33dcc8653. The lease is completed. The earlier shared-Mac timings were too noisy to support a speed claim and remain retained; no full-suite speedup is claimed..mjsand direct.mtsentrypoints, success/failure/startup/help/signals, final trailer ordering, and temporary/generation cleanup.dist/chunks/cac.D805sv8h.js(SHA256f37acc539be54a668448c9ae1faeacc4b37df68f81952a21001c377a775da55e), MRI operand handling at lines 13–108, CAC separator handling at 559–575, option declarations at 938/1161/1196/1225, andparseCLIat 2316–2338. Regressions additionally exercise the actual native parser.Initial CI exposed a new scheduling import outside the copied compiler closure on both Windows and Linux, plus the legacy raw Vitest binary in the dependency scan. The repair keeps environment preparation in the invocation owner and routes the report helper through the canonical launcher. Both failed contracts pass locally and in final exact-head CI. The final run needed one exact-job retry for an unchanged authorization-fixture early-exit assertion; all three local authorization cases and the retried full job passed. Authorization checks were not changed. Captured subprocess output should be added to that fixture's assertion diagnostics in a follow-up.
The broader audit identified a separate project-sequencing opportunity that needs shared-state and mixed-worker-limit proof. This PR preserves the existing scheduling policy.
ClawSweeper follow-through: the requested controlled Linux comparison and native identity checks are complete above. Maintainer inspection of the pinned dependency source and native parser tests addresses the reviewer's source-access limitation. A refreshed review was requested for the final head; the existing completed review is under 12 hours old and its stated proof items are resolved.