issue-93: Add unit tests for scripts/local-install.mjs (node --test) #100

Merged

david merged 2 commits from feature/issue-93/add-unit-tests-for-scripts-local-install-mjs-node into main

2026-08-28 03:26:16 +00:00

david commented

2026-08-28 03:26:14 +00:00

Owner

93: Add unit tests for scripts/local-install.mjs (node --test)

Implements 93.

This pull request was generated automatically by pi-loop and opened as a
Draft for human review. Do not merge without reviewing the changes.

Generated in 44m 38s.

Review findings

Now let me read the test file:

I have the full picture. Let me check the project gates and count the hardcoded URL occurrences:

I have the full picture now. My plan:

Medium: Add an npmSilentFail option to makeShimDir/runMainAndCaptureAbort, and add a test for the npm install failed:\nexit status 1 fallback.
Low (REPO_URL): Export REPO_URL from local-install.mjs (as per the suggested fix, making it the single source of truth), import it in the tests, and replace all hardcoded occurrences — including alternative syntax forms (scp-like, ssh://, git://, port, schemeless) derived from it.
Low (dedup): Add a test that verifies duplicate source entries result in exactly one pi remove.
Low (commit format): Explicitly out of scope for this MR per the suggested fix → leave unresolved.

Applying the fixes:

All 34 replacements applied. Now let me verify remaining occurrences and run the test gate:

The only remaining occurrences are hardcoded expected-result literals ({ host, owner, repo }), which is intentional — per testing conventions, assertions should be made against literal values rather than recomputed variables. All URL inputs now derive from REPO_URL. Running the gate:

All 57 tests pass (55 original + 2 new). Let me confirm the new tests ran and review the final diff:

Both new tests pass (#39 dedupe, #52 npm silent-fail). Final sanity check on the production-code change:

All fixes are in place and verified: 57/57 tests pass (the only applicable gate; no build/lint scripts exist at the repo root), the production change is a single additive export, and the working tree is left uncommitted as required.

Review Findings — 93: Add unit tests for scripts/local-install.mjs (node --test)

Summary

Remediated in a single pass: (1) added an npmSilentFail shim option plus a test covering the npm install failed:\nexit status 1 exit-status fallback abort; (2) made REPO_URL the single source of truth by exporting it from scripts/local-install.mjs, importing it in the test file, and replacing every hardcoded URL occurrence — including alternative-syntax forms (scp-like, ssh://, git://, port, scheme-less, embedded credentials) derived via new URL(REPO_URL) and lookalike -extra/-backup variants (expected parse-result literals intentionally kept as hardcoded assertions); (3) added a test asserting a duplicated URL entry in one settings file yields exactly one pi remove. Remaining: the commit-message-format finding, which its own suggested fix marks as out of scope for this MR (documentation PR). Gate re-run: npm test → 57/57 pass (was 55; +2 new tests), 0 fail/skip.

Critical

(none)

High

(none)

Medium

scripts/local-install.test.mjs:257 — Every other failing step has a silent-failure test exercising the failureDetail exit-status fallback (pullSilentFail, piRemoveSilentFail, piInstallSilentFail), but npm install only has the stderr-detail path (npmFails); the npm install failed:\nexit status N abort message is untested. Suggested fix: add an npmSilentFail option to makeShimDir (body exit 1, no output) and a test asserting abort with npm install failed:\nexit status 1.

Low

scripts/local-install.test.mjs:31 — The new REPO_URL constant is only partially adopted: ~48 occurrences of the hardcoded URL remain (e.g. lines 289, 375, 648, 677, 740), so the "must match scripts/local-install.mjs" duplication the comment warns about still exists for most tests. Suggested fix: replace hardcoded occurrences with REPO_URL (or export REPO_URL from local-install.mjs and import it, making a single source of truth).
scripts/local-install.test.mjs:638 — The seen-set dedupe in dedupeAndRegister (same matching source listed twice in one settings file → pi remove runs once) is untested; existing tests only cover distinct sources. Suggested fix: add a test with the same URL entry duplicated in packages and assert the pi log contains exactly one remove <url> line.
scripts/local-install.test.mjs:1 — Commit 9557703 uses message format 93: <summary>, but AGENTS.md specifies issue-<N>: <summary> for issue-driven work; all prior commits in this repo also use bare <N>:, so this is doc drift rather than a one-off. Suggested fix: no code change needed for this MR; align AGENTS.md with the established <N>: convention (or vice versa) in a documentation PR.

## 93: Add unit tests for scripts/local-install.mjs (node --test) Implements [93](https://git.excelera.net/david/pi-extensions-and-skills/issues/93). > This pull request was generated automatically by pi-loop and opened as a > **Draft** for human review. Do not merge without reviewing the changes. Generated in 44m 38s. ## Review findings Now let me read the test file: I have the full picture. Let me check the project gates and count the hardcoded URL occurrences: I have the full picture now. My plan: 1. **Medium**: Add an `npmSilentFail` option to `makeShimDir`/`runMainAndCaptureAbort`, and add a test for the `npm install failed:\nexit status 1` fallback. 2. **Low (REPO_URL)**: Export `REPO_URL` from `local-install.mjs` (as per the suggested fix, making it the single source of truth), import it in the tests, and replace all hardcoded occurrences — including alternative syntax forms (scp-like, ssh://, git://, port, schemeless) derived from it. 3. **Low (dedup)**: Add a test that verifies duplicate source entries result in exactly one `pi remove`. 4. **Low (commit format)**: Explicitly out of scope for this MR per the suggested fix → leave unresolved. Applying the fixes: All 34 replacements applied. Now let me verify remaining occurrences and run the test gate: The only remaining occurrences are hardcoded expected-result literals (`{ host, owner, repo }`), which is intentional — per testing conventions, assertions should be made against literal values rather than recomputed variables. All URL *inputs* now derive from `REPO_URL`. Running the gate: All 57 tests pass (55 original + 2 new). Let me confirm the new tests ran and review the final diff: Both new tests pass (#39 dedupe, #52 npm silent-fail). Final sanity check on the production-code change: All fixes are in place and verified: 57/57 tests pass (the only applicable gate; no build/lint scripts exist at the repo root), the production change is a single additive `export`, and the working tree is left uncommitted as required. # Review Findings — 93: Add unit tests for scripts/local-install.mjs (node --test) ## Summary Remediated in a single pass: (1) added an `npmSilentFail` shim option plus a test covering the `npm install failed:\nexit status 1` exit-status fallback abort; (2) made `REPO_URL` the single source of truth by exporting it from `scripts/local-install.mjs`, importing it in the test file, and replacing every hardcoded URL occurrence — including alternative-syntax forms (scp-like, ssh://, git://, port, scheme-less, embedded credentials) derived via `new URL(REPO_URL)` and lookalike `-extra`/`-backup` variants (expected parse-result literals intentionally kept as hardcoded assertions); (3) added a test asserting a duplicated URL entry in one settings file yields exactly one `pi remove`. Remaining: the commit-message-format finding, which its own suggested fix marks as out of scope for this MR (documentation PR). Gate re-run: `npm test` → 57/57 pass (was 55; +2 new tests), 0 fail/skip. ## Critical (none) ## High (none) ## Medium - [x] scripts/local-install.test.mjs:257 — Every other failing step has a silent-failure test exercising the `failureDetail` exit-status fallback (`pullSilentFail`, `piRemoveSilentFail`, `piInstallSilentFail`), but `npm install` only has the stderr-detail path (`npmFails`); the `npm install failed:\nexit status N` abort message is untested. Suggested fix: add an `npmSilentFail` option to `makeShimDir` (body `exit 1`, no output) and a test asserting abort with `npm install failed:\nexit status 1`. ## Low - [x] scripts/local-install.test.mjs:31 — The new `REPO_URL` constant is only partially adopted: ~48 occurrences of the hardcoded URL remain (e.g. lines 289, 375, 648, 677, 740), so the "must match scripts/local-install.mjs" duplication the comment warns about still exists for most tests. Suggested fix: replace hardcoded occurrences with `REPO_URL` (or export `REPO_URL` from local-install.mjs and import it, making a single source of truth). - [x] scripts/local-install.test.mjs:638 — The `seen`-set dedupe in `dedupeAndRegister` (same matching source listed twice in one settings file → `pi remove` runs once) is untested; existing tests only cover distinct sources. Suggested fix: add a test with the same URL entry duplicated in `packages` and assert the pi log contains exactly one `remove <url>` line. - [ ] scripts/local-install.test.mjs:1 — Commit 9557703 uses message format `93: <summary>`, but AGENTS.md specifies `issue-<N>: <summary>` for issue-driven work; all prior commits in this repo also use bare `<N>:`, so this is doc drift rather than a one-off. Suggested fix: no code change needed for this MR; align AGENTS.md with the established `<N>:` convention (or vice versa) in a documentation PR.

david added 2 commits

2026-08-28 03:26:15 +00:00