feat(test): add "test open <test-id>" to jump from the terminal to the dashboard#182
feat(test): add "test open <test-id>" to jump from the terminal to the dashboard#182Andy00L 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 ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds a cross-platform browser-opening helper and a new ChangesTest Open Command
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant runOpen
participant openInBrowser
User->>CLI: test open <test-id> [--no-browser]
CLI->>runOpen: runOpen(opts, deps, opener)
runOpen->>runOpen: fetch test and resolve dashboardUrl
runOpen->>User: print dashboardUrl
alt --no-browser not set
runOpen->>openInBrowser: opener(dashboardUrl)
openInBrowser-->>runOpen: success or error
runOpen->>User: stderr warning on opener failure
else --no-browser set
runOpen->>User: skip opener
end
Related issues: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (3)
src/lib/browser.test.ts (1)
57-69: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd coverage for the async spawn-failure path.
The
default spawnersuite mocksspawnto return{ unref }and never exercises an'error'event, so the crash scenario noted insrc/lib/browser.ts(missing opener binary) has no regression test. Once anon('error', ...)handler is added there, extend the mock to emit'error'and assert the process doesn't throw.🤖 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/browser.test.ts` around lines 57 - 69, Add test coverage for the default spawner error path in browser.test.ts by extending the mocked spawn return value used by openInBrowser so it can trigger an on('error', ...) callback. Update the default spawner case around openInBrowser/spawnMock/unref to simulate a missing opener binary and assert the call does not throw when the error event is emitted, covering the new async failure handling in src/lib/browser.ts.src/commands/test.ts (1)
7266-7283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider documenting exit codes for
test openin--help, matching sibling commands.
test run,test wait,test rerun, andtest delete-batchall enumerate their exit codes directly in.description()(0/1/4/5/etc.).test openhas multiple distinct outcomes (0 success, 4 test-not-found via the underlyingclient.get, 1 for the no-portal-mappingCLIError) but omits this from its help text, breaking the established discoverability convention for scripting/CI consumers.🤖 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/commands/test.ts` around lines 7266 - 7283, Update the `test.command('open <test-id>')` help text so it explicitly documents the possible exit codes, matching the sibling `test run`, `test wait`, `test rerun`, and `test delete-batch` commands. Keep the change in the `.description()` for the `open` command and list the distinct outcomes from `runOpen`/its underlying `client.get` and `CLIError` paths (success, test-not-found, and no-portal-mapping).Source: Path instructions
src/commands/test.test.ts (1)
2271-2358: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage of the print/spawn/
--no-browser/error-hint paths; missing a dry-run case.No test exercises
runOpenwithdryRun: true. Given the dry-run parity concern raised insrc/commands/test.ts(Lines 3657-3664), a dry-run test would also help lock in the fix once the hardcoded sample is replaced with the canned-fixture path.🤖 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/commands/test.test.ts` around lines 2271 - 2358, Add a runOpen test that covers the dryRun: true path, since the current suite only verifies print/spawn/no-browser/error-hint behavior. Update the runOpen describe block to invoke runOpen with dryRun enabled using the existing makeCreds, makeFetch, and TEST_ROW setup, then assert it returns/prints the expected dashboard URL and does not spawn the opener. This will lock in the dry-run parity behavior alongside the existing runOpen cases.Source: Path instructions
🤖 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/commands/test.ts`:
- Around line 3657-3664: The dry-run branch in runOpen is bypassing the
canned-sample flow and duplicating portal URL construction. Update the
opts.dryRun path to use the offline client.get-backed fixture from
src/lib/dry-run/samples.ts and let resolvePortalUrl in src/lib/facade.ts build
the dashboard URL, rather than hardcoding a template and returning a custom
sample. Also add a dry-run case to the runOpen tests in
src/commands/test.test.ts to verify the canned sample contract and keep the
offline path aligned with production behavior.
In `@src/lib/browser.ts`:
- Around line 24-46: The default opener in openInBrowser currently uses spawn()
without handling its asynchronous ENOENT error, so a missing platform opener can
crash the CLI instead of letting runOpen fall back. Update the openInBrowser
default exec path to attach an 'error' listener to the spawned child and handle
ENOENT there, and make sure runOpen can detect that failure and continue to the
stderr hint. Keep the fix localized around openInBrowser and the spawn-based
exec callback so the browser launch still works on darwin, win32, and xdg-open
platforms.
---
Nitpick comments:
In `@src/commands/test.test.ts`:
- Around line 2271-2358: Add a runOpen test that covers the dryRun: true path,
since the current suite only verifies print/spawn/no-browser/error-hint
behavior. Update the runOpen describe block to invoke runOpen with dryRun
enabled using the existing makeCreds, makeFetch, and TEST_ROW setup, then assert
it returns/prints the expected dashboard URL and does not spawn the opener. This
will lock in the dry-run parity behavior alongside the existing runOpen cases.
In `@src/commands/test.ts`:
- Around line 7266-7283: Update the `test.command('open <test-id>')` help text
so it explicitly documents the possible exit codes, matching the sibling `test
run`, `test wait`, `test rerun`, and `test delete-batch` commands. Keep the
change in the `.description()` for the `open` command and list the distinct
outcomes from `runOpen`/its underlying `client.get` and `CLIError` paths
(success, test-not-found, and no-portal-mapping).
In `@src/lib/browser.test.ts`:
- Around line 57-69: Add test coverage for the default spawner error path in
browser.test.ts by extending the mocked spawn return value used by openInBrowser
so it can trigger an on('error', ...) callback. Update the default spawner case
around openInBrowser/spawnMock/unref to simulate a missing opener binary and
assert the call does not throw when the error event is emitted, covering the new
async failure handling in src/lib/browser.ts.
🪄 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: c3e707f0-ffb6-46f2-8a34-c036c20324bd
⛔ Files ignored due to path filters (1)
test/__snapshots__/help.snapshot.test.ts.snapis excluded by!**/*.snap,!**/*.snap
📒 Files selected for processing (4)
src/commands/test.test.tssrc/commands/test.tssrc/lib/browser.test.tssrc/lib/browser.ts
| if (opts.dryRun) { | ||
| emitDryRunBanner(stderrFn); | ||
| const sample = { | ||
| dashboardUrl: `https://www.testsprite.com/dashboard/tests/p_dryrun_2026/test/${opts.testId}`, | ||
| }; | ||
| out.print(sample, data => (data as { dashboardUrl: string }).dashboardUrl); | ||
| return sample; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Dry-run branch bypasses the canned-sample contract and duplicates URL-building logic.
Per path instructions, a new endpoint needs "a matching canned sample in src/lib/dry-run/samples.ts (and its test) so --dry-run stays offline." Instead, this hardcodes a brand-new dashboardUrl template (https://www.testsprite.com/dashboard/tests/p_dryrun_2026/test/...) directly in runOpen, entirely bypassing resolvePortalUrl and the client. This duplicates the URL-format logic that already lives in resolvePortalUrl (src/lib/facade.ts), so the two can silently drift (e.g., if resolvePortalUrl's path shape changes, this literal won't follow). It also doesn't encodeURIComponent testId the way the real path does, so dry-run output can differ in shape from production output for the same input.
The project's own stated design principle is that --dry-run should "exercise the full code path offline with canned data" — i.e., dry-run should still flow through client.get (backed by a canned CliTest fixture from samples.ts) and resolvePortalUrl, not shortcut around them with a separately-maintained literal.
There's also no dry-run test case in src/commands/test.test.ts's describe('runOpen', ...) block, compounding the gap.
♻️ Suggested direction
if (opts.dryRun) {
emitDryRunBanner(stderrFn);
- const sample = {
- dashboardUrl: `https://www.testsprite.com/dashboard/tests/p_dryrun_2026/test/${opts.testId}`,
- };
+ const test = DRY_RUN_SAMPLES.test; // canned CliTest fixture from src/lib/dry-run/samples.ts
+ const dashboardUrl =
+ resolvePortalUrl(resolveApiUrl(opts, deps), test.projectId, opts.testId) ??
+ `https://www.testsprite.com/dashboard/tests/${test.projectId}/test/${encodeURIComponent(opts.testId)}`;
+ const sample = { dashboardUrl };
out.print(sample, data => (data as { dashboardUrl: string }).dashboardUrl);
return sample;
}🤖 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/commands/test.ts` around lines 3657 - 3664, The dry-run branch in runOpen
is bypassing the canned-sample flow and duplicating portal URL construction.
Update the opts.dryRun path to use the offline client.get-backed fixture from
src/lib/dry-run/samples.ts and let resolvePortalUrl in src/lib/facade.ts build
the dashboard URL, rather than hardcoding a template and returning a custom
sample. Also add a dry-run case to the runOpen tests in
src/commands/test.test.ts to verify the canned sample contract and keep the
offline path aligned with production behavior.
Source: Path instructions
|
Fixed both review findings: (1) the dry-run sample URL is now derived through the same resolvePortalUrl resolver as the live path (canonical prod endpoint + dry-run project id), so the two can never drift; (2) the default spawner now registers an 'error' listener on the spawned child — a missing open/xdg-open binary surfaces as an ASYNC ENOENT that the caller's try/catch cannot see, and an unhandled 'error' event would have crashed the CLI; it now degrades to the same manual-open stderr hint (new test). |
|
Approved — no changes requested on substance. This went CONFLICTING purely because today's merge wave landed a large batch into the same files first. Rebase onto current |
|
One more rebase, sorry — your own stack collided with itself: #132 and #176 merged first this round, moving the same |
|
Rebased onto current main. Ready to merge. |
2 participants
What does this PR do?
Adds
testsprite test open <test-id>— the CLI already computes the Portaldeep-link and prints it as text on several commands; this closes the last inch
(the
gh browse/cypress openhop). The URL is ALWAYS printed to stdoutfirst (so headless use and piping compose), then the OS default browser is
spawned unless
--no-browser. The opener uses an argv-array spawn (never ashell string) so a URL can never be shell-injected, refuses non-http(s)
schemes before any process is spawned, and is detached/unref'd so the CLI
exits immediately. A missing opener (containers, SSH) downgrades to a stderr
hint, never a failure; an endpoint with no known portal mapping is a hard
error pointing at TESTSPRITE_PORTAL_URL.
Related issue
Fixes #121
Type of change
Checklist
mainbranch.npm run lintandnpm run format:checkpass.npm run typecheckpasses.npm testpasses and coverage stays at or above the 80% gate.Notes for reviewers
Platform openers:
open(darwin),rundll32 url.dll,FileProtocolHandler(win32 — avoids
cmd /c start, whose re-parsing mangles&in URLs),xdg-open(elsewhere). The opener is injectable (runOpen's third param), sotests never spawn a real process;
src/lib/browser.tsis at 100% coverage.Summary by CodeRabbit
New Features
test open <test-id>command that resolves a test’s dashboard URL, prints it to stdout, and opens it in your browser.--no-browserto print the URL without launching (dry-run also prints a sample URL).Bug Fixes
Tests
test openURL/output and opener-failure scenarios.