Migrate tests to node:test; block dependency install scripts in CI - #9
Merged
Merged
Conversation
Two SonarCloud issues that surfaced on main after #8. S2187 (BLOCKER, test/logic.test.mjs): "Add some tests to this file or delete it." The file did contain 15 real assertions, but as a hand-rolled harness (bare blocks, a local ok() helper, a manual pass/fail tally), so Sonar's JavaScript analyzer saw no recognizable tests. Rather than dismiss it, migrate to the built-in node:test runner — no new dependencies, and it is a genuine upgrade: real per-test isolation, proper reporting, and a nonzero exit on failure that comes from the runner instead of a hand-rolled process.exit. 11 tests, all 15 original assertions preserved. Retention cases now get a fresh temp dir each via a setup() helper instead of sharing one directory, so they no longer depend on execution order. The multi-step state-machine sequences stay grouped in one test each, since their assertions are checkpoints in a single run and splitting them would mean replaying setup. Test script is `node --test test/*.test.mjs`, not `node --test test/`: since Node 22 positional args are glob patterns, and a bare directory matches nothing (verified failing on Node 24, passing on both with the glob). S6505 (.github/workflows/ci.yml): `npm ci` ran dependency lifecycle scripts, which is a supply-chain foothold — any transitive dep can execute arbitrary code at install time. Now `npm ci --ignore-scripts`. Deliberately no `npm rebuild` afterwards. Three deps ship install scripts (esbuild, ffmpeg-for-homebridge, protobufjs) and all three only fetch or build binaries this job never uses — it type-checks and runs hermetic tests, which touch neither ffmpeg nor tsx. Verified by running a clean `npm ci --ignore-scripts` into a scratch copy: build succeeds, 11/11 pass. Also verified the suite still exits 1 on a deliberately broken assertion, so this cannot silently green the build. A comment on the step records why nothing is rebuilt and what to do if a future test needs real ffmpeg.
|
🤖 Review skipped: Repository Owner rate limit exceeded. Free accounts are limited to 3 reviews per 4 hours across all repositories. Upgrade to a paid plan for unlimited reviews. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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.
Clears the two SonarCloud issues that surfaced on
mainafter #8.S2187(BLOCKER) —test/logic.test.mjsThe file did contain 15 real assertions, but as a hand-rolled harness (bare blocks, a local
ok()helper, a manual pass/fail tally), so Sonar's JavaScript analyzer saw nothing it recognized as a test. I opted to fix rather than dismiss, because the migration is a genuine upgrade and costs no dependencies: the built-innode:testrunner gives real per-test isolation, proper reporting, and a nonzero exit driven by the runner instead of a hand-rolledprocess.exit.11 tests, all 15 original assertions preserved. Two things changed beyond the mechanical translation:
setup()helper instead of sharing one directory across four assertions, so they no longer depend on execution order.watchCamerarun (false → true → true → finish → false → true), so splitting them would mean replaying setup for each and would obscure what the test is actually pinning down.Note the script is
node --test test/*.test.mjs, notnode --test test/— since Node 22, positional args are glob patterns and a bare directory matches nothing. Verified: the directory form fails on Node 24, the glob form passes on both it and CI's Node 22.S6505—.github/workflows/ci.ymlnpm ciran dependency lifecycle scripts, which is a supply-chain foothold: any transitive dep can execute arbitrary code at install time. Nownpm ci --ignore-scripts.Deliberately no
npm rebuildafterwards. Three deps ship install scripts —esbuild,ffmpeg-for-homebridge,protobufjs— and all three only fetch or build binaries this job never uses. CI type-checks and runs the hermetic tests, which touch neither ffmpeg nor tsx. Dropping the rebuild is both the stronger hardening and a faster, less network-dependent job (ffmpeg-for-homebridgewas downloading an ffmpeg binary on every run). The step carries a comment explaining this and what to do if a future test needs real ffmpeg.Verification
npm ci --ignore-scriptsinto a scratch copy → build succeeds, 11/11 pass.npm testexits 1, confirming this can't silently green the build.🤖 Generated with Claude Code