Repository navigation
test: strengthen rewritten contribution coverage - #5786
Conversation
Test methodology and referencesThis PR combines several established testing techniques rather than treating line coverage or a single testing style as the objective. The practical goal is: a test should fail when a behavioral contract changes, remain stable when only implementation structure changes, and provide a small counterexample when it fails. 1. Contract-first, behavior-oriented testsThe primary oracle is an observable contract: returned result, persisted state, emitted projection, ordering invariant, or explicit rejection. Tests avoid asserting private helper call order unless that order is itself part of the protocol. Examples in this PR:
This is also why the PR is intentionally test-only: production behavior is the fixed reference while the test implementation is replaced. 2. Equivalence partitions and boundary matricesInstead of accumulating examples opportunistically, inputs are partitioned by the behavior they should produce, then boundaries are tested explicitly. For example:
The point is not simply more cases. It is to make the classification table visible and reviewable, so adding a new enum member or parser branch produces an obvious test decision. 3. Property-based and model/state-machine testing ideasThe underlying idea comes from QuickCheck: state properties and generators separately, then search a larger input space and shrink failures to small counterexamples. References:
Current application in this PR is deliberately deterministic:
This PR does not claim to introduce a full property-based framework. It codifies the properties and state model first, using deterministic examples that are reproducible in the existing runner. A logical follow-up would be generated command sequences with seed persistence and shrinking, using a tool such as 4. Metamorphic relationsMetamorphic testing checks relationships between executions when a complete expected output is inconvenient or brittle. Instead of asking only “is this one output exactly X?”, it asks whether a controlled input transformation preserves or predictably changes an invariant. References:
Metamorphic-style relations used here include:
These are currently explicit deterministic relations, not a separate metamorphic-testing harness. 5. Fail-safe defaults and negative-space testingFor the Eval egress and Host authority paths, malformed or incomplete input must not become an implicit allow. This follows the fail-safe-default principle described by Saltzer and Schroeder: access or authority should be granted by an affirmative validated condition, not by failing to match a denial case. Reference:
Concrete assertions in this PR cover missing CONNECT targets, malformed authorities and ports, unclassifiable requests, raw TCP, malformed durable replay records, stale Host Epochs, and fail-stop transitions both before and after waiting for admission. The second fail-stop check is important because a condition validated before an 6. Mutation testing as an oracle-quality checkLine coverage answers whether code executed. Mutation testing asks whether the assertions distinguish the implementation from small plausible faults. The tools used here insert changes such as boundary/operator replacements, constant changes, removed conditions, and altered returns, then rerun the focused tests. References:
Results for the focused rewritten surfaces:
A surviving mutant is reviewed, not blindly converted into another assertion. The four surviving TypeScript mutants are behaviorally equivalent under the current contracts: changing them cannot produce a different externally observable result. Treating mutation score as a target without equivalent-mutant review would encourage implementation-coupled tests. Mutation testing is therefore a diagnostic tool, not a proposed universal per-commit CI gate. Focused mutation runs on changed policy/state-machine surfaces, plus periodic broader runs, give a better cost/signal tradeoff. 7. Mutation-guided ablation and test reductionThe reduction pass removed fixtures, snapshots, and assertions one at a time while preserving the ability to kill meaningful mutants and detect the intended regression. This is related to the broader idea behind delta debugging: minimize a failure-inducing configuration while preserving the failure. Reference:
This PR uses a manual, mutation-guided adaptation; it is not an implementation of the formal 8. Layering by cost and fidelityThe suite does not attempt to prove everything through end-to-end tests. Small policy tests provide speed and precise failures; medium projection/adapter tests verify boundaries between Runtime, Host, transport, and UI; the existing full workspace suite remains the regression/fidelity gate. References:
For this PR the intended balance is:
Review criterionThe main review question is not “does this increase the number of tests?” It is:
That criterion explains both sides of this PR: significantly less duplicated test scaffolding, but stronger boundary, concurrency, and mutation sensitivity on the high-risk contracts. |
hqhq1025
left a comment
There was a problem hiding this comment.
I found two P2 coverage regressions at exact head 6e5a3b3e12d9567139b8f28255cd1a9775ba8ac0; this test-only rewrite is not ready to merge.
- The Agent Graph boundary suite no longer exercises deferred reads or stop requests across Session and epoch changes.
- The queue-mutation integration coverage no longer verifies each handler's complete payload identity on conflicting replays.
Both gaps survive concrete mutations of production safety guards, which conflicts with the PR's mutation-guided objective. The rewritten positive-path tests themselves pass.
Verification: clean Node 24.18.1 install and build:test; all changed TypeScript suites and 32/32 changed Python tests; full typecheck, lint, format, ASF-header, and diff checks; hosted test green. A synthetic merge with current main (de4fc5ff95b1f8034ca00b448984b31ca711dce1) built successfully and its focused TypeScript/Python suites passed. The full local workspace run had one unchanged, environment-dependent Runtime Host Bash sandbox-boundary failure that also failed alone; Desktop passed 2821/2821 standalone. The merge tree is conflict-free.
Not verified: native Windows/macOS, packaged Electron, or the author's mutation-tool command itself. The findings below are based on direct manual mutation probes.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| readonly #snapshots = new Map<string, Snapshot>(); | ||
| readonly #current = new Map<string, string>(); | ||
| readonly #listeners = new Set<Listener>(); | ||
| #readsFail = false; |
There was a problem hiding this comment.
P2: This replacement fixture can only fail reads synchronously; it can no longer hold an epoch/snapshot read or stop request across a Session or epoch change. I removed the production scheduler freshness checks at agent-graph-panel.tsx:117,133 and the stop request identity checks at :213-221, rebuilt, and this entire six-test suite still passed. Those guards prevent a disposed Session's late snapshot/error/loading state or a prior stop result from overwriting the current panel. Please restore deferred read/stop controls and regressions for Session switch, epoch rollover/history selection, and late stop settlement.
| assert.deepEqual( | ||
| projection.steering.map((entry) => [entry.entryId, entry.content.text, entry.placement]), | ||
| [['id-1', 'steer me', 'current_turn']], | ||
| await queueMutation(fixture, 'queue.entry.retract', { |
There was a problem hiding this comment.
P2: Every replay in this combined trace repeats the original payload, so it no longer checks that each coordinator handler includes the full request identity when wiring QueuedMutationExecutor. As a mutation probe I removed entryId from completedPayloadIdentity('retract_entry', ...); the full message-coordinator plus queued-mutation-executor suites still passed 84/84, which would let the same retractId replay a previous success for a different entry. The old per-operation tests covered conflicting retries. Please add different-payload replays for retract/update/promote/reorder through the real handlers.
Replace broad implementation-coupled coverage with focused contract, state-machine, failure-boundary, and projection tests. Add mutation-guided coverage for queued host mutations, provider retry policy, and Eval egress enforcement. Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
6e5a3b3 to
079fe59
Compare
Remaining attributed tests rewrittenPushed Attribution auditI reran whitespace- and move-aware blame over every tracked test/e2e path against the full candidate commit set.
The rewrite replaces shared mutable fixture state where practical with explicit inputs, pure projections, immutable records, table-driven partitions, and small state-transition helpers. Effectful filesystem/process/IPC boundaries remain imperative, while their setup and expected-state derivation are separated. Verification
The full Runtime and Storage package builds are still blocked by the existing main-branch Agent Graph supervisor-wake contract mismatch ( |
hqhq1025
left a comment
There was a problem hiding this comment.
At exact head 7e6210638d19f2b93a374d306405312ed694db32, this test-only rewrite is not ready to merge: I found one P1 gate failure and two P2 coverage regressions.
The PR rewrites 38 test files (+2537/-2627) around Desktop, Runtime Host, Storage, Core, Eval, and CLI contracts. The two mutation-surviving gaps from the prior head remain, and the new head also changes a working Desktop E2E helper to an unsupported Playwright API.
Verification on Node 22.22.1: clean npm ci, full build:test, Agent Graph panel 6/6, Runtime Host queue suites 84/84, Storage project-catalog 24/24, and git diff --check passed. The two production-safety mutations described below still left their focused suites green. The exact-head hosted CI passed lint, format, build, typecheck, workspace tests, Runtime Host, and the preceding policy gates, then failed Desktop E2E 1/34; later Storybook and CLI stages were skipped.
Fresh main is ab012de9f24cdd2bda9afd7a4e75ab7f383e8512. The PR is 3 commits ahead and 1 behind, and the merge tree currently conflicts in packages/ui/src/__tests__/composer-send-toggle.test.tsx. The visible human approvals bind the previous commit 079fe59e, not this head.
Not verified: native Windows/macOS, packaged Electron, the skipped downstream CI stages, or the author's mutation-tool command.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| ): Promise<void> { | ||
| await composer.fill(text); | ||
| await composer.press("ControlOrMeta+A"); | ||
| await composer.insertText(text); |
There was a problem hiding this comment.
[P1] Use a supported Playwright text-entry API here. Locator has no insertText method: the exact-head hosted test run fails this spec at this line with TypeError: composer.insertText is not a function (33 passed, 1 failed), which skips all later Storybook and CLI gates. Keep fill, use pressSequentially, or focus the locator and call page.keyboard.insertText, then rerun Desktop E2E.
| return { | ||
| listEpochs: async (sessionId: string) => directory(sessionId), | ||
| listCurrentEpochs: async (sessionId: string) => directory(sessionId), | ||
| getSnapshot: async (sessionId: string, options?: { graphId?: string }) => { |
There was a problem hiding this comment.
[P2] Preserve a deferred read/stop boundary in this fixture. getSnapshot and stop now settle synchronously, so these six tests cannot exercise an old Session/epoch response arriving after rerender or an old stop result arriving after rollover. I temporarily removed the production scheduler.isCurrent(fence) read checks and the stop rootSessionId/requestId result checks; all 6/6 tests still passed. Add controllable deferred reads/stops and assert stale success, error, loading, and stop results cannot mutate the current panel.
| expectedQueueRevision: initialRevision + 1, | ||
| entryIds: ['id-3', 'id-2', 'id-1'], | ||
| }), | ||
| queueMutation(fixture, 'queue.entry.retract', { |
There was a problem hiding this comment.
[P2] Replay each operation ID with a different payload, not only the same payload. This trace repeats trace-retract with the same entryId, so it misses the handler's payload-identity contract. I temporarily removed entryId from completedPayloadIdentity('retract_entry'); message-coordinator plus queued-mutation-executor still passed 84/84, allowing the same retractId for another entry to replay the old success. Add a conflicting replay through the real handler and assert operation_conflict.
hqhq1025
left a comment
There was a problem hiding this comment.
At exact head aae1690bf6cb04bf3d6db58de2e8791f13a7c5cd, this PR is not ready to merge. The branch rewrites tests but also changes production UI, Host recovery, protocol, storage, and Eval code. I found two concrete regressions: the Desktop E2E path still calls nonexistent Locator.insertText() (P1, preventing its CI lane from completing), and the relative-time component stops scheduling after its first refresh (P2). Two previously reported coverage gaps also remain in the rewritten tests: the Agent Graph fixture at apps/desktop/src/main/__tests__/agent-graph-panel.test.ts:198-213 never delays a read or stop to test stale session/epoch replies, and the queue replay trace at packages/runtime-host/src/__tests__/message-coordinator.test.ts:1879-1900 only retries identical payloads, not the same retractId with a different entryId.
Node 24 clean install and build:test pass; focused Core/Agent Graph/Host tests pass 86/86. A mounted React timer probe shows just now -> 1 minute ago after the first timer and zero remaining timers. The current change plan enables Desktop E2E, but this head has no completed hosted checks; I did not rerun packaged Electron or native Windows/macOS. git merge-tree against current main 2f322055 reports a conflict in packages/ui/src/__tests__/composer-send-toggle.test.tsx; the PR is also marked dirty. Existing human approvals target an older commit, not this head.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| ): Promise<void> { | ||
| await composer.fill(text); | ||
| await composer.press("ControlOrMeta+A"); | ||
| await composer.insertText(text); |
There was a problem hiding this comment.
P1: Locator has no insertText() method in this Playwright version; that method belongs to page.keyboard. The Desktop E2E CI lane is selected for this PR, and this helper throws TypeError when the steering scenario reaches it. Use a supported locator input method and rerun the E2E lane.
| const id = setTimeout(refresh, delay); | ||
| return () => clearTimeout(id); | ||
| }); | ||
| }, [props.ts, props.variant]); |
There was a problem hiding this comment.
P2: This effect now depends only on ts and variant, so refresh() rerenders once but does not rerun the effect or schedule the next timer. A mounted timer probe advances just now to 1 minute ago and then finds zero timers; the label remains stale for the rest of the component's lifetime unless a parent changes props. Keep the refresh cycle dependent on its own tick (or use an interval with correct bucket scheduling).
Mechanical rewrite sweep completedPushed I reran the whitespace- and move-aware
This pass mechanically rewrote the remaining candidate surfaces with domain-oriented names and equivalent local structure, including queue mutation handlers, Turn/session projection types, egress hook locals, inventory scripts, protocol exports, and related workflow/docs wording. The protocol-only edits are covered by Verification on the pushed head:
The branch is clean and the PR head is now |
# Conflicts: # packages/ui/src/__tests__/composer-send-toggle.test.tsx
Summary
This is a test-only change. It does not change production behavior.
Test strategy
The suite now follows six layers:
Mutation scores after the rewrite:
Verification
python3 -m unittest test_egress_filter.py test_egress_audit_journal.py: 24 passed.npm --workspace @maka/runtime run build: passed.npm --workspace @maka/runtime-host run build: passed.npm --workspace @maka/runtime-host run test:dist: 2,188 tests; 2,176 passed, 12 skipped, 0 failed.npm run test:dist: all other workspaces passed. The parallel aggregate observed one transient Runtime Host nonzero exit without a retained failing test; the immediate complete standalone rerun was green.git diff --check: passed.AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex rewrote and simplified the tests, designed mutation-guided boundary cases, and ran the verification described above.
Checklist
Does this PR entail a change in behavior?