test: layered test strategy — unit, command, container - #41
Merged
Conversation
The config directory was computed from the home directory at module load, so nothing could point the CLI elsewhere once the module had been imported. The existing test worked around it by setting HOME and then importing the module through a top-level await; that dance is gone now, and with it the reason command-level tests could not exercise `config profile add` or `select`. The directory-creation memo is keyed by path rather than being a single flag, because the path now follows HOME: caching "already created" would otherwise leave a later directory unmade while every write into it failed. It still deduplicates concurrent callers and still does not cache failures, so the saving that motivated it is unchanged. Two error messages named a function instead of the path they were reporting, and the active-profile read named the profiles file rather than its own.
The prompts had no coverage at all. Under vitest process.stdout.isTTY is undefined, so confirm() returned its non-interactive default and the prompt implementation was never even imported; `config profile add` and `select` were not exercised in any form, and cancellation was checked only by throwing a hand-made ExitPromptError rather than by cancelling anything. @inquirer/testing drives the real prompts through a shared screen, so these tests go through the same code a user does: the question sequence, the offered default, the validators as they are actually wired, and the profile the flow writes. Ctrl+C now goes through a genuine prompt and produces a genuine ExitPromptError. The existing non-interactive tests are kept rather than converted. The three confirm() call sites pass three deliberately different values for defaultWhenNonInteractive, and those decide what happens in CI, which is where most runs of this CLI happen. Reaching a prompt requires faking isTTY, since confirm() checks it before deciding to prompt at all. Whether that check reads a real terminal correctly is not something a test can answer by disabling it, so it is left to the container tests.
Four things no in-process test can reach: the exit status a pipeline reads, a real terminal, the spawn of a subcommand, and the artifact users actually install. The vitest suite has to fake the first two and cannot do the last two at all — abortExecution deliberately returns instead of exiting under NODE_ENV=test, which is what makes command-level tests possible in the first place and also why they can never check an exit code. The tests run against the shipped image, so the npm tarball, the bin entry, the shebang and the runtime package.json lookup are exercised on the way. A separate Dockerfile adds expect to drive the prompts through a pty; nothing in it reaches the image users install. They stay at the level of "did it exit correctly" on purpose. Assertions about content belong at the command level, where a failure points at a line rather than a terminal transcript. CONTRIBUTING now describes the three levels and which one new tests belong in.
CI built the smoke-test image with `FROM steadybit/cli:latest`, and the buildx container driver cannot read the local image store, so that tag resolved from Docker Hub instead of from the image built moments earlier in the same job. The suite was exercising the last release. It reported two failures, both real for 4.3.6 — the colour gating and the SIGINT exit status are new here — and after the next release it would have gone green again while still testing a stale artifact, which is worse than failing. There is no derived image now. The scripts are mounted into the image that was built, and the pty allocator they need is installed at run time, so nothing resolves a tag on the way. CI additionally tags its build by commit rather than `latest`: a tag that cannot exist in a registry fails loudly instead of quietly finding something else.
Four things, all found by reviewing what had just been written rather than by anything failing. The Ctrl+C helper held a literal 0x03 byte in its string. Editors and diffs render that as an empty string, so the next person to tidy it would have removed the only thing the test sends. It is an escape now, and nothing else in src or e2e carries an unprintable character. The memoisation followed the home directory for the directory it creates but not for the files it reads, so a changed home produced correct directories holding the previous home's contents. Reads, writes and the directory itself now go through one helper keyed the same way, which is also less code than the two mechanisms it replaces. Two container checks asserted only absences — no escape codes, no stack trace, no profile — and all of those hold when the CLI is missing entirely. Removing the binary used to leave them green; now every one of the eleven fails. The first attempt at a fix was still too weak, since `2>&1` catches the shell's own "not found" and made the output look present. The "an experiment is already running, run it in parallel?" recovery had no test at all. It was dead code until the response body it inspects stopped being consumed before it got there, and it has been reachable but unexercised since. It is now driven through the prompt, in both directions.
`docker run` pulls a name it cannot find locally, so steadybit/cli:latest would quietly fall back to the published release if this build ever stopped being loaded — someone switching to push, adding platforms, which disables load, or reordering the steps. That is the same wrong-artifact failure the smoke tests already hit once, arriving through a different door. The build action reports the id of what it just built, and an id is never looked up in a registry: a missing one is "No such image" rather than a pull. That removes the failure rather than making it loud, and the tag it replaces was only ever there to be run.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Establishes the three-level test structure discussed after #35, and fills the two gaps that had no coverage at all: the interactive prompts, and the process contract.
@inquirer/testinge2e/run.sh+ expectA test goes at the lowest level that can hold it.
CONTRIBUTING.mddocuments this.Why the prompts had no coverage
Under vitest
process.stdout.isTTYisundefined, soconfirm()returned its non-interactive default and never even imported the prompt.config profile addandselectwere untested in any form, and cancellation was checked by throwing a hand-madeExitPromptErrorrather than by cancelling anything.@inquirer/testingdrives the real prompts through a shared screen, so these tests go through the same code a user does — the question sequence, the offered default, the validators as wired, and the profile the flow writes.The existing non-interactive tests are kept, not converted. The three
confirm()call sites pass three deliberately different values fordefaultWhenNonInteractive, and those decide what happens in CI, where most runs of this CLI happen. Converting them would have swapped that coverage rather than added to it.Reaching a prompt requires faking
isTTY, sinceconfirm()checks it before deciding to prompt at all. Whether that check reads a real terminal correctly cannot be answered by a test that disables it, so it is left to the container level.Also now covered: the "an experiment is already running, run it in parallel?" recovery, driven through the prompt in both directions. That path was dead code until #35 stopped the response body being consumed before it got there, and has been reachable but unexercised since.
Why a container level as well
Four things no in-process test can reach: the exit status a pipeline reads, a real terminal, the spawn of a subcommand, and the artifact users install.
abortExecutiondeliberately returns instead of exiting underNODE_ENV=test— that is what makes command-level tests possible, and equally why they can never assert an exit code.Eleven checks, run against the built image so the npm tarball,
binentry, shebang and runtimepackage.jsonlookup are exercised on the way:They stay at "did it exit correctly" on purpose — assertions about content belong at the command level, where a failure points at a line rather than a terminal transcript.
Checked that the suite can fail: deleting the binary turns all eleven red. It did not start that way. Two of them asserted only absences — no escape codes, no stack trace, no profile — and every one of those also holds for a CLI that was never invoked. The first fix was still too weak, because
2>&1catches the shell's own "not found" and made the output look present; they now assert on text the CLI itself prints and on a marker proving the prompt was reached.The harness caught a real problem on its first CI run
The first push failed two checks that passed locally. They were genuine — for 4.3.6, which is what CI was testing.
docker/setup-buildx-actioncreates adocker-containerbuilder, which cannot read the local image store. SoFROM steadybit/cli:latestin the derived test image resolved from Docker Hub rather than from the image built seconds earlier in the same job. Building the harness against the published image reproduces CI's output exactly, same two checks in the same order — and both are new here, so the released CLI legitimately fails them.The red build was not the dangerous part. After the next release the tag would have matched again and the suite would have gone green while still exercising a stale artifact.
Fixed with two independent guards:
e2e/is mounted into the image that was built, so nothing resolves a tag and there is no opportunity to resolve the wrong one. Local and CI run an identical command, so they cannot drift.latest. A tag that cannot exist in a registry fails loudly instead of quietly finding something else.Known trade-off: expect is installed at test time
With no derived image, the pty allocator the interactive checks need is installed when the suite runs:
Three packages from
https://dl-cdn.alpinelinux.org— signature-verified by apk, but an unpinned third party fetched on every run. Cost is ~1.1s of a ~7.4s suite, so this is about the dependency, not the time.What it means in practice:
cannot install expect, which the interactive checks need) rather than silently skipping the interactive checks, which is the right failure mode but still a failure.--network nonefails at this step.spawn/expect/send, but it is an uncontrolled input.The alternative, if this becomes annoying: restore the derived image and build it with the default builder rather than buildx's container driver, which does read the local image store. Verified both halves:
So the commit-tag guard alone is enough to make the original bug loud, and expect would move back to build time — where the job already requires network to pull
node:24-alpineand runnpm ci, so it would add no new class of dependency. I went with mounting because one mechanism with no tag resolution at all is harder to get wrong, but that reasoning is worth revisiting rather than treating as settled.Prerequisite included
service.tsresolved the config directory at module load, so nothing could point the CLI at a different home once it was imported; the existing test worked around it with a top-level-await import. That dance is gone, which is what made command-level tests of the profile flows possible.Writing the test for it surfaced a second problem: the directory-creation memo was a single flag, so a changed HOME left the new directory unmade while every write into it failed with ENOENT. Reviewing that fix surfaced a third: the directory memo followed HOME but the read memo did not, so a changed home produced correct directories holding the previous home's contents. All three — directory, profiles, active profile — now go through one helper keyed on the config directory, which is less code than the two mechanisms it replaces.
Two error messages also named a function instead of the path they reported, and the active-profile read named the profiles file rather than its own.
Verification
106 tests, up from 88.
npm run cigreen at each of the five commits so the history bisects cleanly, and the container suite passes both piped and with-t, on arm64 and amd64.