refactor: production code no longer sniffs for a test mock - #214
Merged
Conversation
`streamSubprocess` inspected `runSubprocess` for vitest's `.mock` property and, when it found one, ran a completely different implementation that synthesised stream events from a buffered result. Its comment claimed this "still exercised the streaming code path in production". It exercised it nowhere: every dispatcher suite mocked `runSubprocess`, so every dispatcher suite ran the adapter, and the real streaming implementation had no coverage from any test that drove a dispatcher. The adapter moves to tests/support/buffered-stream.ts, where the six suites that want it opt in by mocking `streamSubprocess` — the function the dispatchers actually call — and pointing it at their existing buffered mock. Every assertion still reads argv, env and stdin off that same mock, so the suites' bodies are unchanged. Sabotage: dropping the stdout emit from the real streamer fails 12 tests across stream-subprocess.test.ts and generic-cli-real-subprocess.test.ts, confirming the real path is covered by the two suites that spawn processes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
streamSubprocessinspectedrunSubprocessfor vitest's.mockproperty and, when it found one, ran a different implementation that synthesised stream events from a buffered result.Its comment claimed this "still exercised the streaming code path in production". It exercised it nowhere: every dispatcher suite mocked
runSubprocess, so every dispatcher suite ran the adapter, and the real streaming implementation had no coverage from any test that drove a dispatcher.What changed
tests/support/buffered-stream.ts.streamSubprocess— the function the dispatchers actually call — and point it at their existing buffered mock.src/no longer contains any reference to a mock shape.Sabotage check
Dropping the stdout emit from the real streamer fails 12 tests across
stream-subprocess.test.tsandgeneric-cli-real-subprocess.test.ts— the two suites that spawn real processes. That is what confirms the real path is covered now that nothing reroutes around it.What this does not do
The six rewired suites still drive a stub, so they still do not exercise real piping — a stub yielding one stdout chunk cannot show a partial line surviving across two reads. That is deliberate and now stated in each file: the real-subprocess suites cover it, and these cover the ordering contract per harness protocol.
npm run check: 1219 passed, 8 skipped, exit 0.