From 5be8ecd5474859101fc7a3db0dfcb63bafb1335a Mon Sep 17 00:00:00 2001 From: Yazan-O Date: Tue, 7 Jul 2026 02:03:21 -0500 Subject: [PATCH] test: make Windows CLI tests portable --- CANDIDATES.md | 53 +++++++++++++++++++++++++++++++++++ src/lib/agent-targets.test.ts | 3 +- src/lib/bundle.test.ts | 5 ++-- src/lib/credentials.test.ts | 15 ++++++---- src/lib/skill-nudge.test.ts | 20 +++++++++---- test/cli.subprocess.test.ts | 41 ++++++++++++++++++--------- test/help.snapshot.test.ts | 3 +- test/helpers/npm.ts | 13 +++++++++ 8 files changed, 123 insertions(+), 30 deletions(-) create mode 100644 CANDIDATES.md create mode 100644 test/helpers/npm.ts diff --git a/CANDIDATES.md b/CANDIDATES.md new file mode 100644 index 0000000..15d214d --- /dev/null +++ b/CANDIDATES.md @@ -0,0 +1,53 @@ +# Candidate Improvements + +## Candidates + +1. Picked: make the test suite portable on Windows. + Evidence: `npm test -- src/lib/credentials.test.ts` failed on Windows before the patch because POSIX mode assertions saw `0o666` instead of `0o600`, and the default credentials path assertion assumed `/` separators. The full suite also failed where subprocess tests used `HOME` but not `USERPROFILE`, tried to unset inherited env vars with `undefined`, spawned `npm` directly, and compared POSIX path strings. Fixed in `src/lib/credentials.test.ts:22`, `test/cli.subprocess.test.ts:379`, `test/helpers/npm.ts:3`, `src/lib/skill-nudge.test.ts:11`, `src/lib/agent-targets.test.ts:36`, and `src/lib/bundle.test.ts:594`. + +2. `doctor` bypasses the shared `--output` validator. + Evidence: `src/commands/doctor.ts:260` assigns `globals.output ?? 'text'` directly, while the shared validator at `src/lib/output.ts:32` rejects invalid modes with a typed `VALIDATION_ERROR`. + +3. Empty `TESTSPRITE_PROFILE` is not normalized like the other env vars. + Evidence: `src/lib/config.ts:20` normalizes empty env values, but `src/lib/config.ts:42` reads `env.TESTSPRITE_PROFILE` raw before `readProfile`; malformed profile names are rejected at `src/lib/credentials.ts:38`. + +4. `--password-file` strips leading and trailing spaces from passwords. + Evidence: project create/update both use `readFileSync(...).trim()` at `src/commands/project.ts:205` and `src/commands/project.ts:326`, which changes a password whose value intentionally starts or ends with whitespace. + +5. `resolveBundleDir` trims only POSIX trailing slashes. + Evidence: `src/lib/bundle.ts:328` checks only `rawPath.endsWith('/')`; `src/lib/junit-report.ts:201` shows the safer local pattern of accepting both `/` and `\\`. + +## Picked Rationale + +I picked the Windows test portability issue because it was directly reproducible, blocked the documented `npm test` contributor loop on this workspace, and could be fixed entirely in tests without changing CLI behavior. + +## Diff Summary + +- Added `test/helpers/npm.ts` so subprocess-style tests run `npm run build` through the current npm entrypoint, with a Windows `.cmd` fallback. +- Isolated subprocess tests from the real Windows user profile by setting both `HOME` and `USERPROFILE`, and by removing inherited TestSprite API env vars case-insensitively. +- Made permission-bit assertions POSIX-only where Node/Windows cannot represent `0o600` reliably. +- Made path and CRLF-sensitive test assertions separator/line-ending neutral. + +## Validation + +- `npx -y -p node@22 -p npm@10 npm test` - passed, 50 files / 1846 tests. +- `npx -y -p node@22 -p npm@10 npm run lint:fix` - passed. +- `npx -y -p node@22 -p npm@10 npm run typecheck` - passed. +- `npx -y -p node@22 -p npm@10 npm run build` - passed. + +## PR Title + +test: make Windows CLI test harness portable + +## PR Body + +Summary: +- make path and CRLF-sensitive tests portable across Windows and POSIX +- isolate subprocess tests from the real Windows user profile and inherited TestSprite env vars +- run subprocess build setup through the active npm entrypoint instead of assuming `npm` is directly spawnable + +Tests: +- `npm test` +- `npm run lint:fix` +- `npm run typecheck` +- `npm run build` diff --git a/src/lib/agent-targets.test.ts b/src/lib/agent-targets.test.ts index 51bf9b5..27b7749 100644 --- a/src/lib/agent-targets.test.ts +++ b/src/lib/agent-targets.test.ts @@ -36,7 +36,8 @@ import { function parseFrontmatterDescription(content: string): string | undefined { const lines = content.split('\n'); let inFrontmatter = false; - for (const line of lines) { + for (const rawLine of lines) { + const line = rawLine.replace(/\r$/, ''); if (line.trim() === '---') { if (!inFrontmatter) { inFrontmatter = true; diff --git a/src/lib/bundle.test.ts b/src/lib/bundle.test.ts index aec451a..c3cdcec 100644 --- a/src/lib/bundle.test.ts +++ b/src/lib/bundle.test.ts @@ -10,7 +10,7 @@ import { existsSync, mkdirSync, mkdtempSync, readFileSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { join, resolve } from 'node:path'; import { describe, expect, it } from 'vitest'; import { applyFailedOnly, @@ -593,8 +593,7 @@ describe('resolveBundleDir', () => { it('resolves a relative path against cwd', () => { const out = resolveBundleDir('./tmp/x'); - expect(out.endsWith('/tmp/x')).toBe(true); - expect(out.startsWith('/')).toBe(true); + expect(out).toBe(resolve('./tmp/x')); }); it('strips a trailing slash', () => { diff --git a/src/lib/credentials.test.ts b/src/lib/credentials.test.ts index 896d057..2174b2b 100644 --- a/src/lib/credentials.test.ts +++ b/src/lib/credentials.test.ts @@ -1,5 +1,5 @@ import { mkdtempSync, statSync, readFileSync, writeFileSync, existsSync, mkdirSync } from 'node:fs'; -import { tmpdir } from 'node:os'; +import { homedir, tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import { @@ -19,6 +19,11 @@ import { ApiError } from './errors.js'; let tmpRoot: string; let credentialsPath: string; +function expectPosixMode(path: string, expected: number): void { + if (process.platform === 'win32') return; + expect(statSync(path).mode & 0o777).toBe(expected); +} + beforeEach(() => { tmpRoot = mkdtempSync(join(tmpdir(), 'testsprite-creds-')); credentialsPath = join(tmpRoot, 'credentials'); @@ -139,8 +144,7 @@ describe('writeProfile', () => { it('creates the file with mode 0600 and writes the profile', () => { writeProfile(DEFAULT_PROFILE, { apiKey: 'sk-new' }, { path: credentialsPath }); expect(existsSync(credentialsPath)).toBe(true); - const mode = statSync(credentialsPath).mode & 0o777; - expect(mode).toBe(0o600); + expectPosixMode(credentialsPath, 0o600); expect(readProfile(DEFAULT_PROFILE, { path: credentialsPath })).toEqual({ apiKey: 'sk-new' }); }); @@ -192,14 +196,13 @@ describe('ensureRestrictiveMode', () => { mkdirSync(tmpRoot, { recursive: true }); writeFileSync(credentialsPath, 'data', { mode: 0o644 }); ensureRestrictiveMode(credentialsPath); - const mode = statSync(credentialsPath).mode & 0o777; - expect(mode).toBe(0o600); + expectPosixMode(credentialsPath, 0o600); }); }); describe('defaultCredentialsPath', () => { it('points at ~/.testsprite/credentials', () => { - expect(defaultCredentialsPath().endsWith('/.testsprite/credentials')).toBe(true); + expect(defaultCredentialsPath()).toBe(join(homedir(), '.testsprite', 'credentials')); }); }); diff --git a/src/lib/skill-nudge.test.ts b/src/lib/skill-nudge.test.ts index c15b09d..a4c6f32 100644 --- a/src/lib/skill-nudge.test.ts +++ b/src/lib/skill-nudge.test.ts @@ -9,28 +9,36 @@ import { type SkillNudgeContext, } from './skill-nudge.js'; +function posixPath(path: string): string { + return path.replace(/\\/g, '/'); +} + // --------------------------------------------------------------------------- // isVerifySkillInstalled // --------------------------------------------------------------------------- describe('isVerifySkillInstalled', () => { it('true when the claude own-file SKILL.md exists', () => { - const existsSync = (p: string) => p.endsWith('.claude/skills/testsprite-verify/SKILL.md'); + const existsSync = (p: string) => + posixPath(p).endsWith('.claude/skills/testsprite-verify/SKILL.md'); expect(isVerifySkillInstalled('/proj', { existsSync })).toBe(true); }); it('true for the cursor .mdc landing file', () => { - const existsSync = (p: string) => p.endsWith('.cursor/rules/testsprite-verify.mdc'); + const existsSync = (p: string) => + posixPath(p).endsWith('.cursor/rules/testsprite-verify.mdc'); expect(isVerifySkillInstalled('/proj', { existsSync })).toBe(true); }); it('true for the cline landing file', () => { - const existsSync = (p: string) => p.endsWith('.clinerules/testsprite-verify.md'); + const existsSync = (p: string) => + posixPath(p).endsWith('.clinerules/testsprite-verify.md'); expect(isVerifySkillInstalled('/proj', { existsSync })).toBe(true); }); it('true for the antigravity landing file', () => { - const existsSync = (p: string) => p.endsWith('.agents/skills/testsprite-verify/SKILL.md'); + const existsSync = (p: string) => + posixPath(p).endsWith('.agents/skills/testsprite-verify/SKILL.md'); expect(isVerifySkillInstalled('/proj', { existsSync })).toBe(true); }); @@ -73,7 +81,7 @@ describe('isVerifySkillInstalled', () => { return false; }, }); - expect(seen.every(p => p.startsWith('/some/proj'))).toBe(true); + expect(seen.every(p => posixPath(p).startsWith('/some/proj'))).toBe(true); // One probe per target landing path. expect(seen).toHaveLength(Object.keys(TARGETS).length); }); @@ -198,6 +206,6 @@ describe('maybeEmitSkillNudge', () => { }); maybeEmitSkillNudge(ctx); expect(probed.length).toBeGreaterThan(0); - expect(probed.every(p => p.startsWith('/work/here'))).toBe(true); + expect(probed.every(p => posixPath(p).startsWith('/work/here'))).toBe(true); }); }); diff --git a/test/cli.subprocess.test.ts b/test/cli.subprocess.test.ts index b0537b1..a8f77e1 100644 --- a/test/cli.subprocess.test.ts +++ b/test/cli.subprocess.test.ts @@ -7,14 +7,15 @@ * and runs `auth whoami` against the mock." */ -import { execFileSync, spawn } from 'node:child_process'; -import { existsSync, mkdtempSync, statSync } from 'node:fs'; +import { spawn } from 'node:child_process'; +import { existsSync, mkdtempSync, renameSync, statSync } from 'node:fs'; import type { IncomingMessage, Server, ServerResponse } from 'node:http'; import { createServer } from 'node:http'; import { tmpdir } from 'node:os'; import { dirname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { runNpmScript } from './helpers/npm.js'; const __dirname = dirname(fileURLToPath(import.meta.url)); const REPO_ROOT = resolve(__dirname, '..'); @@ -31,13 +32,18 @@ let server: Server; let baseUrl: string; let tmpHome: string; +function expectPosixMode(path: string, expected: number): void { + if (process.platform === 'win32') return; + expect(statSync(path).mode & 0o777).toBe(expected); +} + beforeAll(async () => { // Always rebuild — `npm run build` is fast and a stale `dist/index.js` // would silently mask ESM/import regressions in this suite. The // existsSync skip we used to do here let `dist` rot under // refactors and gave false-green on `project list` once // already. - execFileSync('npm', ['run', 'build'], { cwd: REPO_ROOT, stdio: 'pipe' }); + runNpmScript('build', REPO_ROOT); server = createServer((req: IncomingMessage, res: ServerResponse) => { const url = req.url ?? '/'; if (url.startsWith('/api/cli/v1/projects/')) { @@ -349,7 +355,7 @@ beforeAll(async () => { }, 60_000); afterAll(async () => { - await new Promise(resolveClose => server.close(() => resolveClose())); + if (server) await new Promise(resolveClose => server.close(() => resolveClose())); }); interface SpawnResult { @@ -362,13 +368,7 @@ function runCli(args: string[], envOverrides: Record = {}): Prom return new Promise((resolveResult, rejectResult) => { const child = spawn('node', [BIN_PATH, ...args], { cwd: REPO_ROOT, - env: { - ...process.env, - HOME: tmpHome, - TESTSPRITE_API_KEY: undefined, - TESTSPRITE_API_URL: undefined, - ...envOverrides, - } as NodeJS.ProcessEnv, + env: childEnv(envOverrides), }); let stdout = ''; let stderr = ''; @@ -379,6 +379,21 @@ function runCli(args: string[], envOverrides: Record = {}): Prom }); } +function childEnv(envOverrides: Record): NodeJS.ProcessEnv { + const env = { ...process.env }; + for (const key of Object.keys(env)) { + if (key.toUpperCase() === 'TESTSPRITE_API_KEY' || key.toUpperCase() === 'TESTSPRITE_API_URL') { + delete env[key]; + } + } + return { + ...env, + HOME: tmpHome, + USERPROFILE: tmpHome, + ...envOverrides, + }; +} + describe('auth status subprocess (+ deprecated whoami alias)', () => { it('prints JSON me and exits 0 against the local server', async () => { const result = await runCli(['auth', 'status', '--output', 'json'], { @@ -897,7 +912,7 @@ describe('setup --from-env subprocess', () => { expect(result.exitCode).toBe(0); const credentialsPath = join(tmpHome, '.testsprite', 'credentials'); expect(existsSync(credentialsPath)).toBe(true); - expect(statSync(credentialsPath).mode & 0o777).toBe(0o600); + expectPosixMode(credentialsPath, 0o600); }, 30_000); it('exits 5 with VALIDATION_ERROR when --from-env is set without TESTSPRITE_API_KEY', async () => { @@ -1044,7 +1059,7 @@ describe('--dry-run subprocess smoke', () => { // skipped the prompt. const credPath = join(tmpHome, '.testsprite', 'credentials'); // Make sure any previous test didn't leave one behind. - if (existsSync(credPath)) execFileSync('rm', [credPath]); + if (existsSync(credPath)) renameSync(credPath, `${credPath}.bak-${process.pid}-${Date.now()}`); const result = await runCli(['setup', '--dry-run', '--no-agent', '--output', 'json']); expect(result.exitCode).toBe(0); expect(existsSync(credPath)).toBe(false); diff --git a/test/help.snapshot.test.ts b/test/help.snapshot.test.ts index ee89b3b..79b150b 100644 --- a/test/help.snapshot.test.ts +++ b/test/help.snapshot.test.ts @@ -13,6 +13,7 @@ import { execFileSync } from 'node:child_process'; import { dirname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; import { beforeAll, describe, expect, it } from 'vitest'; +import { runNpmScript } from './helpers/npm.js'; const __dirname = dirname(fileURLToPath(import.meta.url)); const REPO_ROOT = resolve(__dirname, '..'); @@ -47,7 +48,7 @@ const cases: Array<[string, string[]]> = [ describe('--help snapshots', () => { beforeAll(() => { - execFileSync('npm', ['run', 'build'], { cwd: REPO_ROOT, stdio: 'pipe' }); + runNpmScript('build', REPO_ROOT); }); for (const [name, args] of cases) { diff --git a/test/helpers/npm.ts b/test/helpers/npm.ts new file mode 100644 index 0000000..747405a --- /dev/null +++ b/test/helpers/npm.ts @@ -0,0 +1,13 @@ +import { execFileSync } from 'node:child_process'; + +export function runNpmScript(script: string, cwd: string): void { + const npmExecPath = process.env.npm_execpath; + if (npmExecPath) { + execFileSync(process.execPath, [npmExecPath, 'run', script], { cwd, stdio: 'pipe' }); + return; + } + execFileSync(process.platform === 'win32' ? 'npm.cmd' : 'npm', ['run', script], { + cwd, + stdio: 'pipe', + }); +}