tests: raise sdk-node coverage from 62% to 93%#1
Merged
Conversation
- add vitest.config.ts that scopes v8 coverage to src/ and adds a json-summary reporter - add test:coverage npm script and devDependency on @vitest/coverage-v8 - tests/client_extra.test.ts: cover sendMessage, sendFile, publishEvent, subscribeEvent, hostname resolution, library singleton helpers, and the ack-read fallback paths - tests/ffi_extra.test.ts: cover parseJSON / checkErr / unwrapHandleErr edge cases plus PILOT_LIB_PATH override and runtimeLibraryPath fallback - tests/runtime_extra.test.ts: cover isDaemonLive async UNIX-socket probe, daemon-skip behaviour, version fallback, stale-lock recovery, and ensureDirWritable error - tests/cli.test.ts: cover all four bin shims via a hoisted spawnSync mock; verify success, non-zero, default-1, and launch-error paths - tests/index.test.ts: smoke-check the public re-export surface Coverage (v8): statements 62.03% -> 92.93%, lines same, branches 81.6% -> 85.8%, functions 88.3% -> 86.0% (denominator widened as loadLibrary wrappers now register). 173 tests, 7 files, all green under both 'npm test' and 'npm run test:coverage'.
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.
Summary
Adds five new test files and a
vitest.config.tsthat scopes v8 coverage tosrc/. Raises measured coverage on the published SDK surface from 62.03% to 92.93% (v8 statements/lines) with 173 tests across 7 files, all green.cli.tsclient.tsffi.tsruntime.tsindex.tsSource:
@vitest/coverage-v8@^3.0.0(matches the pin already used in.github/workflows/ci.yml).What's new
vitest.config.ts— scopes coverage tosrc/**/*.ts, excludes the re-export shim, addsjson-summaryreporter.package.json— newtest:coveragescript; coverage-v8 promoted to a dev-dep (CI already installs it inline, this just keeps local installs consistent).tests/client_extra.test.ts(20 tests) — coverssendMessage,sendFile,publishEvent,subscribeEvent, hostname resolution, library-singleton helpers, and the ACK-read fallback branches insendMessage/sendFile. Uses an in-memory FFI fake so the wire frame layout ([type][len][payload], event frames, file-transfer name header) is exercised end-to-end.tests/ffi_extra.test.ts(12 tests) —parseJSON/checkErr/unwrapHandleErredge cases plusfindLibrary'sPILOT_LIB_PATHoverride and theruntimeLibraryPathfallback that exercises the seeder.tests/runtime_extra.test.ts(10 tests) —isDaemonLiveagainst a real ephemeral UNIX-socket server (live + missing + dead-file + malformed-config), seeder daemon-skip behaviour, version fallback topackage.json, stale-lock reclamation,ensureDirWritableerror message.tests/cli.test.ts(16 tests) — covers all fourbin-stubsshims using a hoistedspawnSyncmock; verifies success / non-zero / default-1 / launch-error paths.process.exitis replaced with a thrower so the test runner survives.tests/index.test.ts(4 tests) — smoke-checks the public re-export surface.What's hard to test without a live daemon
loadLibrary()'s koffi-loaded function bodies (the fourPilot*wrappers that allocate buffers viakoffi.decode) — these need the actuallibpilot.{so|dylib}on disk. Currently 86.8% covered becausefindLibrarydiscovers a bundled lib when one is present in~/.pilot/bin/from a previous SDK install; on a fresh CI runner without a bundled binary the loader path will be skipped and coverage will drop to ~40%. Recommend an integration test that builds and links libpilot in CI, or an FFI replay/stub harness if you want to stay unit-test-only.cli.tsactually executing the seeded binaries — current tests mockspawnSync, so we verify the contract but not the real process launch.probeDaemonLiveSync— usesnc -z -Usynchronously. The probe is environment-dependent (see "Bugs noticed" below), so the test asserts a tolerant invariant ("either upgrade or daemon-skip") rather than a single fixed outcome.Bugs noticed (NOT fixed in this PR)
probeDaemonLiveSyncis unreliable — BSDnc -z -U <sock>on macOS returns status 1 even when the socket is connectable, so the seeder may overwritepilot-daemonwhile it is actually running. The asyncisDaemonLive(which uses a realnet.Socket.connect) is correct; the sync probe should use the same primitive viaspawnSync(node, ['-e', '...'])or expose an async seeder API.probeDaemonLiveSyncrequire('node:child_process')fails in pure ESM — In an ESM module, therequirecall insideprobeDaemonLiveSyncthrows, falling through to the conservativeexistsSync(sockPath)path. Result: ANY socket file at the configured path is treated as a live daemon andpilot-daemonis silently skipped. This is the same root cause flagged by iter-3 as a HIGH bug.PILOT_LIB_PATHallows arbitrary.so/.dylibload with no signature / checksum / origin check. Documented for posterity; expected behaviour for an FFI escape hatch, but worth a warning in the README.Test plan
npm test— 173 tests pass, no warningsnpm run test:coverage— 92.93% statements / 92.93% lines / 85.80% branches / 85.96% functions onsrc/npm testfor flakiness; all stableffi.tscoverage if no bundled libpilot — see note above)