fix(test): restore full test suite on Windows#4
Closed
SahilRakhaiya05 wants to merge 42 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.
ruili-testsprite
requested changes
Jun 22, 2026
Contributor
There was a problem hiding this comment.
Thanks for this, @SahilRakhaiya05 — this is a clean, well-scoped fix and the PR description is excellent. We'd like to merge it. Two required items and one optional nit before we can:
1. format:check will fail CI (required)
Our CI runs npm run format:check (Prettier), and two new files are missing a trailing newline:
test/helpers/execNpm.ts.gitattributes
Please run npm run format and commit the result — that should make the check green. (You ran npm run lint, which is ESLint; Prettier is a separate step here.)
2. Please rebase onto latest main (required)
We've cut a couple of releases since you opened this, so the branch now conflicts. The only conflict is in test/cli.subprocess.test.ts:
git fetch origin
git rebase origin/main
# resolve test/cli.subprocess.test.ts, then:
git push --force-with-lease3. Optional nit — dead branch in stripTrailingSeparators (src/lib/bundle.ts)
This line is unreachable and can be dropped:
if (end === 1 && rawPath[0] === '/') break;The while (end > 1 ...) guard means end is never 1 at that check, and the rawPath.length <= 1 early return already covers a bare /. Totally fine to leave it if you'd rather not touch it — just flagging.
The drive-root handling (C:\) and the multi-separator stripping all look correct. Thanks again for improving the Windows story here! 🙏
SahilRakhaiya05
force-pushed
the
fix/windows-cross-platform-tests
branch
2 times, most recently
from
e3e582b to
18fc2b9
Compare
Contributor
Author
|
Thanks for the review and the kind feedback! • Fixed trailing newlines in test/helpers/execNpm.ts and .gitattributes (npm run format) |
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.
This was referenced Jun 25, 2026
ruili-testsprite
approved these changes
Jun 25, 2026
ruili-testsprite
requested changes
Jun 25, 2026
ruili-testsprite
left a comment
ruili-testsprite
left a comment
Contributor
There was a problem hiding this comment.
There are lint issues found in the pr, could you fix that please?
…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>
SahilRakhaiya05
force-pushed
the
fix/windows-cross-platform-tests
branch
from
48bac34 to
b508bd6
Compare
12 tasks
…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").
…prite#34) * fix(test): reject empty code get --out and strip code-file BOM When test code get --out receives an empty inline body, closeOutputFile left a zero-byte file with exit 0 — scripts and agents treated that as a successful download. Abort the sink, unlink any artifact, and surface VALIDATION_ERROR instead. Plan/steps JSON already strip a leading UTF-8 BOM from PowerShell 5.1 files; apply the same strip to --code-file reads so uploaded test source is not corrupted by an invisible U+FEFF prefix. * fix(test): rebase onto v0.2.0 and harden empty-code --out cleanup - Resolve rebase conflicts: keep atomic temp-file --out writes from main while rejecting empty inline code with VALIDATION_ERROR (exit 5) - abortOutputFile: wait for stream close before unlinking tmpPath - BOM test: use .py code-file (assertPythonCodeFile from v0.2.0) - CI: build before test + fileParallelism false (dist/ race flake)
Co-authored-by: cmdr-chara <249489759+cmdr-chara@users.noreply.github.com>
…run and run --all (TestSprite#128) test run, test create, create-batch, plan put, code put, update, and delete all print the auto-minted idempotency key to stderr under --output json (as well as --verbose / --debug) so JSON-mode automation can capture the key and replay a retry safely. test rerun and test run --all minted a key but only echoed it under --debug / --verbose, so CI flows using --output json silently lost it. Align both paths with the shared guard used by every other minting site, and cover the JSON-mode emission (and the text-mode silence) with regression tests. Co-authored-by: ahndohun <19940813+ahndohun@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…alls (TestSprite#129) The codex managed-section branch already runs inspectTargetPath during --dry-run ([P2]) so a planted symlink is refused the same way the real install refuses it. The own-file targets (claude, cursor, cline, antigravity) skipped that guard in dry-run and reported the write as successful, so 'agent install --dry-run' could claim success for an install that would actually exit 5. Run the same guard in the own-file dry-run branch and cover both the symlinked-target and symlinked-parent cases with regression tests. Co-authored-by: ahndohun <19940813+ahndohun@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…ared deadline (TestSprite#130) runTestRunAll computes each member's poll budget as Math.max(1, ceil(batchDeadlineMs - now)), so a run whose turn arrives after the shared --timeout deadline still gets a fresh >=1s poll. With --max-concurrency bounding the fan-out, a batch could overshoot the documented shared deadline and report a late 'passed' for a member that should have been reported as 'timeout' (exit 7). Guard the poll helper the same way the create-batch --run --wait path already does: if the shared deadline is exhausted before a member's poll starts, return the existing timeout-shaped member result without calling pollRunUntilTerminal. Regression test drives the clock past the deadline during the first member's poll and asserts the second member is never polled and reports 'timeout'. Co-authored-by: ahndohun <19940813+ahndohun@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…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).
…TestSprite#17) `parseRequestTimeoutFlag` was copy-pasted byte-for-byte into five command files (auth, project, usage, init, test). Every copy silently returned `undefined` for an invalid value, so an explicit `--request-timeout 30s` (a natural "30 seconds" typo) resolved to undefined and the command ran with the default 120s deadline — the operator believed they had set a timeout but had not, with no signal. Hoist a single definition into client-factory.ts (next to resolveRequestTimeoutMs and the REQUEST_TIMEOUT_* constants) and make the flag strict: a non-numeric, zero, or negative value now throws a typed VALIDATION_ERROR (exit 5), consistent with every other validated flag (--page-size, --output, --type). Positive out-of-range values are still accepted and clamped by resolveRequestTimeoutMs, and the TESTSPRITE_REQUEST_TIMEOUT_MS env-var path stays lenient by design (a stray global env var should not hard-fail every command). Adds unit coverage for parseRequestTimeoutFlag and a subprocess regression that `--request-timeout 30s` exits 5 instead of falling back to 120s.
- Strip both slash and backslash trailing separators in resolveBundleDir - Add cross-platform npm helper for subprocess/snapshot builds - Set USERPROFILE alongside HOME in subprocess tests - Replace Unix-only rm with unlinkSync for credential cleanup - Skip symlink and chmod assertions when platform cannot honor them - Normalize CRLF in frontmatter description parsing - Enforce LF line endings via .gitattributes
Rebased onto v0.2.0 main introduced skill-nudge tests that assumed Unix-style path prefixes. Use path.join/resolve and TARGETS landing paths so probes match on Windows.
SahilRakhaiya05
force-pushed
the
fix/windows-cross-platform-tests
branch
from
0c47752 to
ef279db
Compare
Contributor
|
@ruili-testsprite the author pushed follow-up commits on 2026-07-03 addressing your review, and CI is fully green — mind taking a re-look when you get a chance? This is the oldest open PR in the queue. |
Contributor
|
Hi @SahilRakhaiya05 — 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 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
12 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.
Fixes 10 Windows-only test failures so the Vitest suite runs cleanly on Windows dev machines. Linux/macOS CI behavior is unchanged.
Windows result:
What changed
resolveBundleDirto strip both/and\trailing separators while preserving roots likeC:\and/.HOMEandUSERPROFILEso credentials stay inside the test temp directory on Windows.rmusage withunlinkSync().test/helpers/execNpm.tssonpmsubprocess calls work on Windows.cmdshims.\r..gitattributesto normalize line endings to LF.0600chmod assertions on Windows because NTFS does not enforce POSIX file modes.path.resolve(),path.join(), andhomedir()based checks.Test Plan
npm test— 1416 passed, 3 skipped on Windows onlynpm run typechecknpm run lintSkipped on Windows by design:
0600test due to NTFS behaviorSummary by CodeRabbit
Summary by CodeRabbit
Solving Issue #108