Recovered: test: make the Windows CLI test harness portable (#207 by @Yazan-O)#250
Recovered: test: make the Windows CLI test harness portable (#207 by @Yazan-O)#250jangjos-128 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a shared, platform-aware npm script runner and replaces direct npm build invocations in the CLI subprocess and help snapshot tests. ChangesNpm test portability
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/agent-targets.test.ts`:
- Around line 39-40: Add a deterministic offline regression assertion in the
parser tests around the rawLine normalization loop, using frontmatter input with
explicit CRLF sequences such as "---\r\ndescription: ...\r\n---\r\n". Verify the
parsed result handles CRLF correctly, without relying on fixture files whose
line endings may vary.
In `@test/cli.subprocess.test.ts`:
- Line 371: Update childEnv used by the subprocess tests to remove all inherited
TESTSPRITE_* variables case-insensitively, including TESTSPRITE_PROFILE and any
future matching names. Apply explicit envOverrides afterward so test-provided
values are preserved, and keep the existing subprocess environment behavior
unchanged otherwise.
In `@test/helpers/npm.ts`:
- Around line 9-12: Update the Windows command selection in the execFileSync
invocation to run npm.cmd through cmd.exe with /d /s /c and preserve the
existing npm run script arguments; keep the non-Windows npm command path
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c87dc992-69cd-4ce7-b035-c6baa1e4b4a9
📒 Files selected for processing (8)
CANDIDATES.mdsrc/lib/agent-targets.test.tssrc/lib/bundle.test.tssrc/lib/credentials.test.tssrc/lib/skill-nudge.test.tstest/cli.subprocess.test.tstest/help.snapshot.test.tstest/helpers/npm.ts
| for (const rawLine of lines) { | ||
| const line = rawLine.replace(/\r$/, ''); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a deterministic CRLF regression case.
The parser change is correct, but the current fixture-based coverage may use LF files and never prove CRLF handling. Add an assertion using input such as ---\r\ndescription: ...\r\n---\r\n.
As per path instructions, tests must cover new behavior deterministically and offline.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/agent-targets.test.ts` around lines 39 - 40, Add a deterministic
offline regression assertion in the parser tests around the rawLine
normalization loop, using frontmatter input with explicit CRLF sequences such as
"---\r\ndescription: ...\r\n---\r\n". Verify the parsed result handles CRLF
correctly, without relying on fixture files whose line endings may vary.
Source: Path instructions
| TESTSPRITE_API_URL: undefined, | ||
| ...envOverrides, | ||
| } as NodeJS.ProcessEnv, | ||
| env: childEnv(envOverrides), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Scrub every inherited TESTSPRITE_* variable.
This loop removes only TESTSPRITE_API_KEY and TESTSPRITE_API_URL; an inherited TESTSPRITE_PROFILE can still alter profile resolution and make subprocess results depend on the host environment.
const env = { ...process.env };
for (const key of Object.keys(env)) {
- if (key.toUpperCase() === 'TESTSPRITE_API_KEY' || key.toUpperCase() === 'TESTSPRITE_API_URL') {
+ if (key.toUpperCase().startsWith('TESTSPRITE_')) {
delete env[key];
}
}As per path instructions, subprocess tests must remove inherited TESTSPRITE_* environment variables case-insensitively while preserving explicit test overrides.
Also applies to: 382-388
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/cli.subprocess.test.ts` at line 371, Update childEnv used by the
subprocess tests to remove all inherited TESTSPRITE_* variables
case-insensitively, including TESTSPRITE_PROFILE and any future matching names.
Apply explicit envOverrides afterward so test-provided values are preserved, and
keep the existing subprocess environment behavior unchanged otherwise.
Source: Path instructions
| execFileSync(process.platform === 'win32' ? 'npm.cmd' : 'npm', ['run', script], { | ||
| cwd, | ||
| stdio: 'pipe', | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
node <<'NODE'
const { execFileSync } = require('node:child_process');
if (process.platform === 'win32') {
execFileSync(process.env.ComSpec || 'cmd.exe', ['/d', '/s', '/c', 'npm.cmd', '--version'], {
stdio: 'inherit',
});
}
NODERepository: TestSprite/testsprite-cli
Length of output: 163
Use cmd.exe for the Windows fallback
execFileSync('npm.cmd', ...) won’t launch the batch shim directly on Windows. If npm_execpath is unset here, this branch fails; use cmd.exe /d /s /c npm.cmd run <script> instead.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/helpers/npm.ts` around lines 9 - 12, Update the Windows command
selection in the execFileSync invocation to run npm.cmd through cmd.exe with /d
/s /c and preserve the existing npm run script arguments; keep the non-Windows
npm command path unchanged.
Source: MCP tools
|
Hi @Yazan-O 👋 — this is the recovery of your PR #207 , which was auto-closed by an error in our 2026-07-09 release process. Your commits are intact — thank you for the contribution! It currently shows merge conflicts because Option A — merge (no force-push): git checkout BRANCH
git fetch upstream # your remote for TestSprite/testsprite-cli (may be "origin")
git merge upstream/main # resolve conflicts, commit
git pushOption B — rebase (cleaner history, needs force): git checkout BRANCH
git fetch upstream
git rebase upstream/main # resolve conflicts
git push --force-with-leaseOnce you push, the conflicts here resolve automatically and we'll take it into review. Sorry again for the extra step! |
v0.3.0 independently landed most of this branch's Windows work, so the
overlapping parts are dropped in favour of upstream's versions:
- credentials.test.ts / cli.subprocess.test.ts: upstream's inline
process.platform guards and HOME+USERPROFILE child env supersede the
expectPosixMode()/childEnv() helpers here.
- skill-nudge.test.ts: upstream's toPosix() supersedes posixPath().
- agent-targets.test.ts: upstream's split(/\r?\n/) already drops the CRLF,
which makes the per-line \r strip here a no-op.
- bundle.test.ts: upstream's resolveBundleDir assertion is stricter
(adds isAbsolute); taken as-is.
What is left is the one thing v0.3.0 does not fix, which still stops the
suite from running on Windows at all:
execFileSync('npm', ['run', 'build'], ...) throws "spawnSync npm ENOENT"
on Windows, because npm is npm.cmd and execFileSync does not go through
a shell. It fails the beforeAll() in both cli.subprocess.test.ts and
help.snapshot.test.ts, so neither file can run.
test/helpers/npm.ts routes the build through process.env.npm_execpath with
the current node binary, falling back to npm.cmd / npm.
CANDIDATES.md is dropped; it was a working note, not part of the fix.
Verified on Windows: 50 files / 1859 passed, 1 skipped; typecheck + lint clean.
2 participants
Summary
Running
npm teston a Windows workstation currently fails in several suites, which blocks the documented contributor loop (CONTRIBUTING.md) for Windows users. This PR makes the test harness portable without changing any CLI behavior — the diff is tests + one test helper only.0o600) POSIX-only — Node on Windows cannot represent them, so they asserted0o666and failed.HOMEandUSERPROFILE, and remove inheritedTESTSPRITE_*env vars case-insensitively..cmdfallback) instead of assumingnpmis directly spawnable.Why the isolation fix matters beyond CI
While validating this patch we hit the failure mode in the wild: running the unpatched suite on a Windows machine with a real TestSprite profile overwrote
~/.testsprite/credentialswith the test fixture (sk-keep-me@http://127.0.0.1:14979), silently breaking every subsequent real CLI call withUNAVAILABLE: fetch faileduntil the file was restored by hand. TheHOME/USERPROFILEisolation in this PR prevents that.Tests
npm test— 50 files / 1846 tests pass on Windows 11 (Node 23) and undernpx -p node@22.npm run lint:fix,npm run typecheck,npm run build— all pass.Summary by CodeRabbit