fix: run the whole test suite in CI, and repair the break it was hiding - #335
Merged
Conversation
19 of 42 test files were present in the repo and referenced nowhere in .github/workflows/shell.yml — nearly half the suite, run only when someone remembered to run it by hand. That is not theoretical. #334 changed how the Kimaki lock port reaches a systemd unit, updated tests/kimaki-multi-instance.sh to match, merged with 20/20 checks green, and left that test failing on main, because it was one of the 19. CI reported green over red. The only reason it surfaced was an unrelated full-suite run afterwards. The break itself: #334 removed the `--lock-port` argument that bridge_render_systemd used to append to ExecStart, in favour of an Environment= line. But the renderer only interpolates the env block its caller hands it, so the guarantee moved from something the RENDERER enforced unconditionally to something every CALLER has to remember. A caller that renders with an env block lacking the line silently drops the port, and the instance falls back to the default — two Kimaki instances then contend for one lock, which is the exact failure multi-instance support exists to prevent. The renderer now re-asserts the line when KIMAKI_LOCK_PORT is set and the block does not already carry it, at the same layer that always owned it. The suites are wired in as a matrix rather than 21 more hand-written jobs, so adding a test is a one-line change. They run through `bash` because several of these files do not carry the executable bit — invoking them as ./tests/x.sh reports a Permission denied that reads exactly like a test failure. tests/ci-coverage.sh is the part that matters. A workflow that lists jobs by hand fails silently and permanently: the person adding a test is the same person who must remember to wire it, and nothing complains when they do not. The guard fails loudly instead, and has an EXCLUDED list so that deliberately skipping a test is a visible decision rather than an omission. It checks for the test name anywhere in the workflow rather than one exact invocation, so a matrix, a composite job, or a future restructure all still satisfy it — the claim is "CI knows about this file", not "CI runs it this particular way". Local suite is green apart from three environment-only failures that also fail on origin/main: this host has a real /usr/bin/kimaki, which two tests resolve instead of their fixtures, and no studio CLI for a third.
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
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.
The problem
19 of 42 test files were never run by CI. Present in the repo, referenced nowhere in
.github/workflows/shell.yml, executed only when someone remembered to by hand.This is how #334 shipped 20/20 green while leaving
mainred:tests/kimaki-multi-instance.shto matchmain.CI reported green over red. It only surfaced because I happened to run the full suite afterwards for an unrelated reason.
The break
#334 removed the
--lock-portargument thatbridge_render_systemdappended toExecStart, moving it to anEnvironment=line. But the renderer only interpolates the env block its caller hands it.So a guarantee the renderer enforced unconditionally became something every caller has to remember. A caller rendering with an env block that lacks the line silently drops the port, and the instance falls back to the default — two Kimaki instances then contend for one lock, which is precisely what multi-instance support exists to prevent.
Fixed at the layer that always owned it: the renderer re-asserts the line when
KIMAKI_LOCK_PORTis set and the block doesn't already carry it.The structural fix
tests/ci-coverage.sh— this is the part that matters.A workflow that lists jobs by hand fails this way silently and permanently: the person adding a test is the same person who must remember to wire it, and nothing complains when they don't. The guard fails loudly instead, and carries an
EXCLUDEDlist so that deliberately skipping a test is a visible decision in review rather than an omission.It checks for the test name anywhere in the workflow rather than one exact invocation, so a matrix, a composite job, or a future restructure all still satisfy it. The claim is "CI knows about this file", not "CI runs it this particular way".
The 21 suites are wired as a matrix rather than 21 more hand-written jobs, so adding a test stays a one-line change. They run via
bashbecause several of these files lack the executable bit — invoking./tests/x.shproduces aPermission deniedthat reads exactly like a test failure. (That cost me a false "4 regressions" reading earlier today.)Expect some of the newly-wired jobs to be informative
These have never run in CI. If any fail, that is the point — they are reporting real state that was previously invisible. Locally, everything passes except three environment-only failures that also fail on
origin/main: this host has a real/usr/bin/kimakithat two tests resolve instead of their fixtures, and nostudioCLI for a third.