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.
|
Closing as superseded, with thanks to @Yazan-O — as your own 07-14 commit message noted, v0.3.0 independently landed most of this branch's Windows work, and the one remaining gap you identified (execFileSync('npm') ENOENT on Windows) has since been closed on main too, via a test/helpers/execNpm.ts helper solving the identical problem. One caution that also weighed in: this branch's version of the two symlink-guard tests in agent.test.ts is unconditional, while main's is runtime-probed (canCreateSymlinks()) — merging as-is would re-break Windows CI on runners without Developer-Mode elevation. Your #207 diagnosis of the credentials-clobber hazard was genuinely valuable — it shaped the fix that shipped. |
3 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