NP-790: Harden bundle smoke test and document commitOpts coupling

illiaizotov-dev · illiaizotov-dev · commit 7740e05361f1 · 2026-04-23T12:41:24.000-06:00
Addresses ts-node-reviewer findings on the previous commit (4de84b0): * `runChildAndCapture` now listens for `error` on the child so a failed spawn (e.g. ENOENT on the node binary) folds the error into the captured stderr and resolves immediately instead of hanging until the SIGKILL timer fires. A `settled` guard protects against the close/error race. * The mock HTTP server's `listen` promise now attaches a one-shot `error` handler so a failed bind (EADDRINUSE on a constrained CI host, etc.) rejects with a useful message instead of letting the test silently wedge until Vitest's test timeout. * `src/action.ts` gains an explicit comment next to the preset pre- resolution explaining that upstream `load-changelog-config.js` never merges a plugin-supplied `commits` field, so `commitOpts` always comes from angular when no `preset` / `config` is passed. Harmless today (angular and conventionalcommits both default to `{ ignore: undefined, merges: false }`) but worth flagging for future upstream drift. No behavioural change to the shipping bundle — `esbuild` strips comments and the smoke test's runtime changes only affect error paths that previously caused hangs, not the happy path. All 37 tests across 5 files still pass; lint, prettier, and typecheck clean. Made-with: Cursor
diff --git a/src/action.ts b/src/action.ts
@@ -216,6 +216,14 @@ export default async function main() {
   // `preset: 'conventionalcommits'` would trigger a dynamic
   // `import-from-esm` lookup that fails on the Actions runner
   // where no `node_modules` directory exists.
+  //
+  // Note: upstream `load-changelog-config.js` only reads
+  // `loadedConfig.commits` (never merging a plugin-supplied `commits`
+  // field), so when no `preset` / `config` is passed `commitOpts`
+  // always comes from the angular default preset. That is harmless
+  // today — angular and conventionalcommits both default to
+  // `{ ignore: undefined, merges: false }` — but is worth knowing if
+  // those defaults ever diverge upstream.
   const resolvedPreset = conventionalCommitsPreset({
     types: mergeWithDefaultChangelogRules(mappedReleaseRules),
   });
diff --git a/tests/bundle.smoke.test.ts b/tests/bundle.smoke.test.ts
@@ -54,7 +54,8 @@ interface ChildResult {
  * Drain stdout/stderr asynchronously while the child runs so large
  * bursts of startup output can't deadlock the parent. Always resolves
  * (never rejects) so the caller can assert on the captured streams even
- * if the child crashed, exited non-zero, or had to be SIGKILLed.
+ * if the child crashed, exited non-zero, had to be SIGKILLed, or failed
+ * to spawn in the first place.
  */
 function runChildAndCapture(
   child: ChildProcess,
@@ -63,21 +64,39 @@ function runChildAndCapture(
   return new Promise((resolve) => {
     const stdoutChunks: Buffer[] = [];
     const stderrChunks: Buffer[] = [];
+    let settled = false;
+
+    const finish = (overrides: Partial<ChildResult>): void => {
+      if (settled) return;
+      settled = true;
+      clearTimeout(timer);
+      resolve({
+        stdout: Buffer.concat(stdoutChunks).toString('utf8'),
+        stderr: Buffer.concat(stderrChunks).toString('utf8'),
+        status: null,
+        signal: null,
+        ...overrides,
+      });
+    };
+
     child.stdout?.on('data', (chunk: Buffer) => stdoutChunks.push(chunk));
     child.stderr?.on('data', (chunk: Buffer) => stderrChunks.push(chunk));
 
+    // If spawn itself fails (e.g. ENOENT for the node binary), Node emits
+    // `error` and never a `close`. Fold the error into stderr so the
+    // caller still sees a useful message instead of hanging until the
+    // SIGKILL timer fires.
+    child.on('error', (err) => {
+      stderrChunks.push(Buffer.from(`\n${String(err)}\n`));
+      finish({});
+    });
+
     const timer = setTimeout(() => {
       child.kill('SIGKILL');
     }, timeoutMs);
 
     child.on('close', (status, signal) => {
-      clearTimeout(timer);
-      resolve({
-        stdout: Buffer.concat(stdoutChunks).toString('utf8'),
-        stderr: Buffer.concat(stderrChunks).toString('utf8'),
-        status,
-        signal,
-      });
+      finish({ status, signal });
     });
   });
 }
@@ -209,8 +228,18 @@ describe('bundled action artefact', () => {
     };
 
     const server = createServer(handleRequest);
-    await new Promise<void>((resolve) => {
-      server.listen(0, '127.0.0.1', resolve);
+    await new Promise<void>((resolve, reject) => {
+      // Without an `error` listener, a failed `listen` (e.g. EADDRINUSE
+      // on a constrained CI host) would never reject and the test would
+      // silently hang until vitest's test timeout.
+      const onError = (err: Error): void => {
+        reject(err);
+      };
+      server.once('error', onError);
+      server.listen(0, '127.0.0.1', () => {
+        server.off('error', onError);
+        resolve();
+      });
     });
 
     try {
