Skip to content

test: make npm failure diagnostics deterministic - #138

Merged
mohanagy merged 1 commit into
developmentfrom
fix/137-windows-package-contract
Jul 21, 2026
Merged

test: make npm failure diagnostics deterministic#138
mohanagy merged 1 commit into
developmentfrom
fix/137-windows-package-contract

Conversation

@mohanagy

@mohanagy mohanagy commented Jul 21, 2026

Copy link
Copy Markdown
Owner

Resolves #137.

The Windows Node 22 failure was the captured-output formatter test itself: it launched a real npm exec process while Vitest applied its five-second per-test limit. The production command has its own 25-second lifecycle; the test was not a valid latency assertion.

This keeps real package behavior covered by the existing pack, check:pack, tarball-install, CLI, and smoke contracts. It changes only the formatter unit test to use the existing injectable child-process seam, explicitly writing stdout/stderr and closing with status 1.

No production behavior, timeout, coverage, shell setting, package validation, or process containment changed.

Local validation:

  • focused regression: passed
  • full tests/package-contract.test.ts: 17 passed
  • npm run typecheck: passed
  • git diff --check: passed

CI must validate the exact head on Linux, macOS, and all Windows Node versions before merge.

Summary by CodeRabbit

  • Tests
    • Improved coverage for failed command handling by simulating process failures deterministically.
    • Verified that error messages include captured standard output and error details.

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 9924de9d-5177-485d-b09e-cd5a1a19c5dc

📥 Commits

Reviewing files that changed from the base of the PR and between d65f3f1 and 0d73aef.

📒 Files selected for processing (1)
  • tests/package-contract.test.ts

📝 Walkthrough

Walkthrough

The package contract test replaces a real npm child process with a mocked process that emits deterministic stdout and stderr before closing with exit status 1, then verifies the rejection includes both captured outputs.

Changes

npm failure handling test

Layer / File(s) Summary
Mocked process validation
tests/package-contract.test.ts
The failure test uses TermIgnoringNpmProcess, emits fixed stdout and stderr content, triggers a failed close event, and checks both outputs in the rejection message.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Poem

A rabbit taps, “No shell today,”

Mocked streams hop along the way.
Though npm fails with status one,
Its captured words are safely spun.
Tests now breathe a steadier air.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the change: making the npm failure diagnostics test deterministic.
Description check ✅ Passed The description explains the change and validation, but the Security impact section and template-style validation checklist are incomplete.
Linked Issues check ✅ Passed The test isolation fix addresses [#137] by preserving CLI behavior and coverage while removing the flaky real-process dependency.
Out of Scope Changes check ✅ Passed Only the targeted formatter test changed, with no unrelated production or packaging edits introduced.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/137-windows-package-contract

Comment @coderabbitai help to get the list of available commands.

@mohanagy

Copy link
Copy Markdown
Owner Author

Maintainer self-review complete. The diff is limited to the existing runNpm formatter contract: it replaces a real npm exec child with the existing injected child-process seam, emits the same stdout/stderr and non-zero close result, and leaves production execution, timeout policy, package validation, shell settings, and containment unchanged. I found no unresolved human review comments. Current exact-head CI is green across Linux, macOS, and Windows Node 20/22/24; CodeRabbit is still quota-blocked and remains a required gate before merge.

@mohanagy

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@mohanagy
mohanagy merged commit 8ace2c4 into development Jul 21, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ci: diagnose intermittent Windows Node 22 CLI compatibility failure

1 participant