Repository navigation
fix(test): stop the systemd tests from deleting the real ~/.config/systemd/user - #30
Conversation
test/install/systemd.test.ts built a temp home but never pointed HOME at it, then called the real resolveTelePiInstallContext(). Every path it used was therefore the developer's own: the suite deleted ~/.config/systemd/user recursively (all units, not just telepi's, plus the *.target.wants symlinks), overwrote the live telepi.service with paths inside a temp dir that afterEach then removed, and ran a real 'systemctl --user daemon-reload/enable/restart'. - stub HOME to the per-test temp home (as test/install.test.ts already does) and restore process.env afterwards - do not delete a directory to prove auto-creation: point serviceUnitPath at a path under tempDir that does not exist yet, and assert it is under tempDir - mock node:child_process so reconcile/getStatus cannot drive the real service manager Fixes benedict2310#29
benedict2310
left a comment
There was a problem hiding this comment.
Reviewed the complete diff and added deterministic success-path and exact failure/status assertions in 77b830f. Independent local Codex review found no defects. Local build and coverage pass: 443 tests, 86.87% lines/statements, 81.01% branches, 90.27% functions. All 14 focused tests also pass in a network-isolated, non-root Podman container. The temporary HOME, non-destructive directory creation test, and child-process mock should stay together. Awaiting CI for the updated head before merge.
|
Hi - is there anything else needed before merge? Thank you. |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Fixes #29.
test/install/systemd.test.tscreates a temp home undertmpdir()but never pointsHOMEat it, then calls the realresolveTelePiInstallContext(). Since that resolver derives its paths fromprocess.env.HOME || homedir()(src/paths.ts), every path the suite touches belongs to whoever runs the tests. Runningnpm testtherefore:~/.config/systemd/userrecursively —rmSync(path.dirname(ctx.serviceUnitPath), { recursive: true, force: true }), which removes every unit file the user has there, not just telepi's, along withdefault.target.wants/andtimers.target.wants/;telepi.servicewithExecStart/StandardOutputpaths inside the temp dir thatafterEachthen deletes, leaving the unit pointing at files that no longer exist;systemctl --user daemon-reload / enable / restart telepi.servicefrom thereconciletest.Changes
HOMEto the per-test temp home inbeforeEach, and restoreprocess.envinafterEach— the same patterntest/install.test.tsalready uses.serviceUnitPathat a not-yet-created path undertempDirand asserts it is undertempDir, so the destructive shape cannot return if the env stub is ever lost.node:child_processfor this file soreconcile/getStatuscannot drive the real service manager.Verification
npx vitest run test/install/systemd.test.ts— 13 passed.npm test— 32 files, 441 passed / 1 skipped.HOME, checksumming~/.config/systemd/userbefore and after: unchanged, and telepi'sExecMainStartTimestampdid not move (no restart).Happy to split the mock into its own commit, or to leave the
rmSynctest shape alone and rely on theHOMEstub only, if you prefer a more minimal change.