diff --git a/packages/cli/src/bin/main.ts b/packages/cli/src/bin/main.ts index a880950d..c9cbbc27 100644 --- a/packages/cli/src/bin/main.ts +++ b/packages/cli/src/bin/main.ts @@ -4,10 +4,16 @@ import { config } from 'dotenv' // not overwrite vars that are already set, so loading .env.local first means // its values win over .env for the same keys. Users can still set anything in // the real environment to override both. -config({ path: '.env.local' }) -config({ path: '.env.development.local' }) -config({ path: '.env.development' }) -config({ path: '.env' }) +// +// `quiet: true` suppresses dotenv v17's `injected env (N) from …` banner, +// which it now prints to stdout on every `config()` call. Without it the CLI +// emits four noisy, non-deterministic banner lines (with rotating tips) ahead +// of its own output on every invocation — restoring the silent behaviour of +// dotenv v16. +config({ path: '.env.local', quiet: true }) +config({ path: '.env.development.local', quiet: true }) +config({ path: '.env.development', quiet: true }) +config({ path: '.env', quiet: true }) import { readFileSync } from 'node:fs' import { dirname, join } from 'node:path' diff --git a/packages/cli/tests/e2e/runner-aware-help.e2e.test.ts b/packages/cli/tests/e2e/runner-aware-help.e2e.test.ts index a2a636d8..786f9cae 100644 --- a/packages/cli/tests/e2e/runner-aware-help.e2e.test.ts +++ b/packages/cli/tests/e2e/runner-aware-help.e2e.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import { render } from '../helpers/pty.js' +import { run } from '../helpers/run.js' /** * E2E coverage for the runner-aware help rendering. The smoke suite @@ -29,9 +29,8 @@ describe('--help — runner-aware Usage + Examples', () => { ua, label, }) => { - const r = render(['--help'], { env: { npm_config_user_agent: ua } }) - const { exitCode } = await r.exit - expect(exitCode).toBe(0) + const r = await run(['--help'], { env: { npm_config_user_agent: ua } }) + expect(r.exitCode).toBe(0) // Usage line must use the right runner. The leader is stable // (`messages.cli.usagePrefix === 'Usage: '`) so we assert on the // suffix the renderer composes at runtime. @@ -50,9 +49,8 @@ describe('auth — runner-aware Usage + Examples', () => { label, }) => { // `auth` with no subcommand prints the auth HELP and exits 0. - const r = render(['auth'], { env: { npm_config_user_agent: ua } }) - const { exitCode } = await r.exit - expect(exitCode).toBe(0) + const r = await run(['auth'], { env: { npm_config_user_agent: ua } }) + expect(r.exitCode).toBe(0) expect(r.output).toContain(`Usage: ${label} stash auth`) expect(r.output).toContain(`${label} stash auth login`) }) diff --git a/packages/cli/tests/e2e/smoke.e2e.test.ts b/packages/cli/tests/e2e/smoke.e2e.test.ts index 55f6373e..777f4e5a 100644 --- a/packages/cli/tests/e2e/smoke.e2e.test.ts +++ b/packages/cli/tests/e2e/smoke.e2e.test.ts @@ -1,9 +1,11 @@ -import { readFileSync } from 'node:fs' -import { dirname, resolve } from 'node:path' +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { dirname, join, resolve } from 'node:path' import { fileURLToPath } from 'node:url' import { describe, expect, it } from 'vitest' import { messages } from '../../src/messages.js' import { render } from '../helpers/pty.js' +import { run } from '../helpers/run.js' const __dirname = dirname(fileURLToPath(import.meta.url)) const pkg = JSON.parse( @@ -21,6 +23,28 @@ describe('stash CLI — non-interactive smoke', () => { // copy strings, so they stay inline. expect(r.output).toContain('init') expect(r.output).toContain('db install') + // The dotenv "injected env" banner regression guard lives in the + // dedicated test below — this cwd has no .env file, so a bare + // `not.toContain('injected env')` here would pass vacuously. + }) + + it('suppresses dotenv v17\'s "injected env" banner when a .env file exists in cwd', async () => { + // dotenv v17 prints an `injected env (N) from …` banner to stdout on + // every `config()` call that actually injects a variable, unless + // `quiet: true` is passed. The repo has no `.env` anywhere, so exercising + // this requires a real .env file in the spawned process's cwd — without + // it, `config()` never finds anything to inject and the banner can never + // appear regardless of whether `quiet: true` is present in the CLI. + const tmpDir = mkdtempSync(join(tmpdir(), 'stash-dotenv-quiet-')) + try { + writeFileSync(join(tmpDir, '.env'), 'STASH_DOTENV_QUIET_TEST=1\n') + const r = await run(['--version'], { cwd: tmpDir }) + expect(r.exitCode).toBe(0) + expect(r.output).toContain(pkg.version) + expect(r.output).not.toContain('injected env') + } finally { + rmSync(tmpDir, { recursive: true, force: true }) + } }) it('--version prints the package version', async () => { @@ -73,4 +97,30 @@ describe('stash CLI — non-interactive smoke', () => { // suffix is the stable assertion target. expect(r.output).toContain('stash db migrate" is not yet implemented.') }) + + // The two cases below exercise `run()` directly rather than `render()`. + // Every existing `run()` consumer only asserted `exitCode === 0` + // (runner-aware-help.e2e.test.ts) and every exit-1 case above still went + // through `render()`, so `run()`'s actual purpose — correctly propagating + // a non-zero/null exit code instead of masking it — was untested by + // anything that used `run()`. + it('run(): surfaces a non-zero exit code + error output for an unknown command', async () => { + const r = await run(['definitely-not-a-command']) + expect(r.exitCode).toBe(1) + expect(r.output).toContain(messages.cli.unknownCommand) + expect(r.output).toContain('definitely-not-a-command') + }) + + it('run(): splits stdout/stderr into independent channels on a failure path', async () => { + // The unknown-command error is written via `console.error` (stderr); the + // HELP banner that follows it is written via `console.log` (stdout). + // Assert on the dedicated `stdout`/`stderr` fields — not just the + // combined `output` — so a regression that wired both `data` handlers + // to the same buffer would be caught. + const r = await run(['definitely-not-a-command']) + expect(r.stderr).toContain(messages.cli.unknownCommand) + expect(r.stderr).not.toContain(messages.cli.usagePrefix) + expect(r.stdout).toContain(messages.cli.usagePrefix) + expect(r.stdout).not.toContain(messages.cli.unknownCommand) + }) }) diff --git a/packages/cli/tests/helpers/run.test.ts b/packages/cli/tests/helpers/run.test.ts new file mode 100644 index 00000000..a4a024f4 --- /dev/null +++ b/packages/cli/tests/helpers/run.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, it } from 'vitest' +import { buildRunResult } from './run.js' + +// Unit coverage for the pure `close`-event → RunResult mapping extracted +// from `run()`. `run()` itself always spawns the built CLI binary and +// exposes no handle to signal the child, which makes the signal-terminated +// path (the headline fix in aedfd9d) impractical to exercise deterministically +// through an E2E test. Testing the extracted pure function directly avoids +// needing a timeout/SIGKILL escape hatch or other API surface change. +describe('buildRunResult', () => { + it('reports a clean exit', () => { + const r = buildRunResult(0, null, 'hello\n', '') + expect(r.exitCode).toBe(0) + expect(r.signal).toBeNull() + expect(r.stdout).toBe('hello\n') + expect(r.output).toBe('hello\n') + }) + + it('reports a non-zero exit code', () => { + const r = buildRunResult(1, null, '', 'boom\n') + expect(r.exitCode).toBe(1) + expect(r.signal).toBeNull() + expect(r.stderr).toBe('boom\n') + }) + + it('keeps exitCode null (never masked to 0) when the child is signal-terminated', () => { + // Node guarantees exactly one of code/signal is non-null on 'close'. A + // naive `code ?? 0` would collapse this to a "successful" exit 0, hiding + // a crash/OOM/SIGTERM kill from callers. + const r = buildRunResult(null, 'SIGTERM', '', '') + expect(r.exitCode).toBeNull() + expect(r.signal).toBe('SIGTERM') + }) + + it('strips ANSI codes independently from stdout/stderr while combining into output/raw', () => { + // Build the ESC control byte at runtime (rather than embedding it + // literally in the source) to keep this file free of non-printable + // characters. + const ESC = String.fromCharCode(27) + const green = `${ESC}[32mok${ESC}[0m` + const red = `${ESC}[31merr${ESC}[0m` + const r = buildRunResult(0, null, green, red) + expect(r.stdout).toBe('ok') + expect(r.stderr).toBe('err') + expect(r.output).toBe('okerr') + expect(r.raw).toBe(green + red) + }) +}) diff --git a/packages/cli/tests/helpers/run.ts b/packages/cli/tests/helpers/run.ts new file mode 100644 index 00000000..91254296 --- /dev/null +++ b/packages/cli/tests/helpers/run.ts @@ -0,0 +1,102 @@ +import { spawn } from 'node:child_process' +import stripAnsi from 'strip-ansi' +import { STASH_BIN } from './pty.js' + +export interface RunOptions { + cwd?: string + env?: Record +} + +export interface RunResult { + /** + * The process's numeric exit code, or `null` when it was terminated by a + * signal (see {@link RunResult.signal}). Never coerced to 0, so a + * signal-killed child (crash, SIGKILL) is distinguishable from a clean exit. + */ + exitCode: number | null + signal: NodeJS.Signals | null + /** ANSI-stripped combined stdout + stderr. */ + output: string + /** Raw combined stdout + stderr including ANSI escapes. */ + raw: string + stdout: string + stderr: string +} + +/** + * Run the built CLI to completion over plain pipes and capture all output. + * + * Prefer this over {@link render} for NON-INTERACTIVE commands (`--help`, + * `--version`, usage/error paths). A pipe buffers the child's output in the + * Node process and delivers every byte before `close`, so large bursts are + * never truncated. + * + * A PTY, by contrast, loses data on Linux: when the child writes a big burst + * (the ~5KB `--help` text is a single `console.log`) and exits immediately, + * the kernel discards whatever is still unread in the pty buffer when the + * slave closes. On a loaded CI runner node-pty can't drain fast enough, so the + * tail of the help — the Examples section — goes missing and assertions on it + * flake. (macOS ptys keep buffered data on close, which is why it only bites + * in CI.) Post-exit "drain" waits can't help because the bytes are gone before + * `onData` ever fires. Only interactive tests that must send keystrokes or + * exercise TTY-specific rendering need {@link render}. + */ +export function run(args: string[], opts: RunOptions = {}): Promise { + const env: Record = { + ...(process.env as Record), + // Mirror render()'s env so both helpers hit the same CLI code paths. + NO_COLOR: '1', + FORCE_COLOR: '0', + CI: 'true', + ...(opts.env ?? {}), + } + + const child = spawn(process.execPath, [STASH_BIN, ...args], { + cwd: opts.cwd ?? process.cwd(), + env, + stdio: ['ignore', 'pipe', 'pipe'], + }) + + let stdout = '' + let stderr = '' + child.stdout.setEncoding('utf8') + child.stderr.setEncoding('utf8') + child.stdout.on('data', (d: string) => { + stdout += d + }) + child.stderr.on('data', (d: string) => { + stderr += d + }) + + return new Promise((res, rej) => { + child.on('error', rej) + child.on('close', (code, signal) => { + res(buildRunResult(code, signal, stdout, stderr)) + }) + }) +} + +/** + * Pure `close`-event → {@link RunResult} mapping, factored out of `run()` so + * the exit-code/signal handling can be unit-tested directly without spawning + * a real child process (see `run.test.ts`). Node guarantees exactly one of + * `code`/`signal` is non-null on `'close'` — this must never coerce a null + * `code` to `0`, or a signal-terminated child (crash, SIGKILL, OOM) would be + * misreported as a clean exit. + */ +export function buildRunResult( + code: number | null, + signal: NodeJS.Signals | null, + stdout: string, + stderr: string, +): RunResult { + const raw = stdout + stderr + return { + exitCode: code, + signal, + output: stripAnsi(raw), + raw, + stdout: stripAnsi(stdout), + stderr: stripAnsi(stderr), + } +}