-
Notifications
You must be signed in to change notification settings - Fork 78
feat(test): add "test open <test-id>" to jump from the terminal to the dashboard #182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Andy00L
wants to merge
3
commits into
TestSprite:main
Choose a base branch
from
Andy00L:feat/test-open
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | ||
|
|
||
| // Mock the real spawner so the default `exec` path (detached spawn + unref) | ||
| // is exercised without launching a real process. | ||
| const spawnMock = vi.fn(); | ||
| vi.mock('node:child_process', () => ({ | ||
| spawn: (command: string, args: readonly string[], opts: unknown) => | ||
| spawnMock(command, args, opts), | ||
| })); | ||
|
|
||
| import { openInBrowser } from './browser.js'; | ||
| import { ApiError } from './errors.js'; | ||
|
|
||
| describe('openInBrowser', () => { | ||
| const url = 'https://portal.example.com/tests/t_123'; | ||
|
|
||
| it('uses `open <url>` on darwin', () => { | ||
| const calls: Array<{ command: string; args: readonly string[] }> = []; | ||
| openInBrowser(url, { | ||
| platform: 'darwin', | ||
| exec: (command, args) => calls.push({ command, args }), | ||
| }); | ||
| expect(calls).toEqual([{ command: 'open', args: [url] }]); | ||
| }); | ||
|
|
||
| it('uses rundll32 FileProtocolHandler on win32', () => { | ||
| const calls: Array<{ command: string; args: readonly string[] }> = []; | ||
| openInBrowser(url, { | ||
| platform: 'win32', | ||
| exec: (command, args) => calls.push({ command, args }), | ||
| }); | ||
| expect(calls).toEqual([{ command: 'rundll32', args: ['url.dll,FileProtocolHandler', url] }]); | ||
| }); | ||
|
|
||
| it('uses xdg-open on other platforms', () => { | ||
| const calls: Array<{ command: string; args: readonly string[] }> = []; | ||
| openInBrowser(url, { | ||
| platform: 'linux', | ||
| exec: (command, args) => calls.push({ command, args }), | ||
| }); | ||
| expect(calls).toEqual([{ command: 'xdg-open', args: [url] }]); | ||
| }); | ||
|
|
||
| it('refuses a non-http(s) URL with exit 5 before spawning', () => { | ||
| const exec = vi.fn(); | ||
| let error: unknown; | ||
| try { | ||
| openInBrowser('file:///etc/passwd', { platform: 'linux', exec }); | ||
| } catch (err) { | ||
| error = err; | ||
| } | ||
| expect(error).toBeInstanceOf(ApiError); | ||
| expect((error as ApiError).exitCode).toBe(5); | ||
| expect(exec).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('throws on a malformed URL', () => { | ||
| const exec = vi.fn(); | ||
| expect(() => openInBrowser('not a url', { exec })).toThrow(); | ||
| expect(exec).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| describe('default spawner', () => { | ||
| beforeEach(() => { | ||
| spawnMock.mockReset(); | ||
| }); | ||
|
|
||
| it('spawns detached, ignores stdio, and unrefs the child', () => { | ||
| const unref = vi.fn(); | ||
| spawnMock.mockReturnValue({ unref, on: vi.fn() }); | ||
| openInBrowser(url, { platform: 'darwin' }); | ||
| expect(spawnMock).toHaveBeenCalledWith('open', [url], { detached: true, stdio: 'ignore' }); | ||
| expect(unref).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it("handles the child's async 'error' (missing binary) with a stderr hint, not a crash", () => { | ||
| // spawn() reports ENOENT asynchronously on the child; an unhandled | ||
| // 'error' event would crash the CLI. The default spawner must register | ||
| // a listener that degrades to the manual-open hint. | ||
| const listeners = new Map<string, (err: Error) => void>(); | ||
| spawnMock.mockReturnValue({ | ||
| unref: vi.fn(), | ||
| on: (event: string, listener: (err: Error) => void) => { | ||
| listeners.set(event, listener); | ||
| }, | ||
| }); | ||
| const stderrSpy = vi.spyOn(process.stderr, 'write').mockReturnValue(true); | ||
| try { | ||
| openInBrowser(url, { platform: 'linux' }); | ||
| const onError = listeners.get('error'); | ||
| expect(onError).toBeDefined(); | ||
| // Firing the listener must not throw and must print the hint. | ||
| expect(() => onError!(new Error('spawn xdg-open ENOENT'))).not.toThrow(); | ||
| expect(String(stderrSpy.mock.calls.at(-1)?.[0])).toContain('could not launch a browser'); | ||
| } finally { | ||
| stderrSpy.mockRestore(); | ||
| } | ||
| }); | ||
| }); | ||
| }); |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| /** | ||
| * Cross-platform "open this URL in the default browser" helper for | ||
| * `test open` (issue #121). Spawns the platform opener with an argv array | ||
| * (never a shell string) so a URL can never be shell-injected, and refuses | ||
| * anything that is not http(s) before any process is spawned. | ||
| * | ||
| * Platform openers: | ||
| * darwin open <url> | ||
| * win32 rundll32 url.dll,FileProtocolHandler <url> (avoids `cmd /c start`, | ||
| * whose re-parsing would mangle `&` and other metachars in the URL) | ||
| * other xdg-open <url> | ||
| * | ||
| * The child is detached and unref'd so the CLI exits immediately; failures | ||
| * are the caller's to surface (it already printed the URL as the fallback). | ||
| */ | ||
| import { spawn } from 'node:child_process'; | ||
| import { localValidationError } from './errors.js'; | ||
|
|
||
| export interface OpenInBrowserDeps { | ||
| platform?: NodeJS.Platform; | ||
| /** Process spawner taking an argv array. Defaults to a detached spawn. */ | ||
| exec?: (command: string, args: readonly string[]) => void; | ||
| } | ||
|
|
||
| export function openInBrowser(url: string, deps: OpenInBrowserDeps = {}): void { | ||
| const parsed = new URL(url); | ||
| if (parsed.protocol !== 'https:' && parsed.protocol !== 'http:') { | ||
| // User-input error, not an internal failure: classify as VALIDATION_ERROR | ||
| // so it maps to exit 5 (like every other bad-argument path), not exit 1. | ||
| throw localValidationError( | ||
| 'url', | ||
| `must be an http(s) URL (got ${parsed.protocol})`, | ||
| undefined, | ||
| 'field', | ||
| ); | ||
| } | ||
| const platform = deps.platform ?? process.platform; | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| const exec = | ||
| deps.exec ?? | ||
| ((command: string, args: readonly string[]) => { | ||
| const child = spawn(command, [...args], { detached: true, stdio: 'ignore' }); | ||
| // spawn() reports a missing binary (ENOENT) ASYNCHRONOUSLY on the child, | ||
| // so the caller's try/catch cannot see it — and an unhandled 'error' | ||
| // event would crash the whole CLI. The URL is already on stdout, so | ||
| // degrade to the same manual-open hint the sync failure path prints. | ||
| child.on('error', () => { | ||
| process.stderr.write( | ||
| 'could not launch a browser; open the URL above manually (or use --no-browser)\n', | ||
| ); | ||
| }); | ||
| child.unref(); | ||
| }); | ||
|
|
||
| if (platform === 'darwin') { | ||
| exec('open', [url]); | ||
| return; | ||
| } | ||
| if (platform === 'win32') { | ||
| exec('rundll32', ['url.dll,FileProtocolHandler', url]); | ||
| return; | ||
| } | ||
| exec('xdg-open', [url]); | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ 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-runstays offline." Instead, this hardcodes a brand-newdashboardUrltemplate (https://www.testsprite.com/dashboard/tests/p_dryrun_2026/test/...) directly inrunOpen, entirely bypassingresolvePortalUrland theclient. This duplicates the URL-format logic that already lives inresolvePortalUrl(src/lib/facade.ts), so the two can silently drift (e.g., ifresolvePortalUrl's path shape changes, this literal won't follow). It also doesn'tencodeURIComponenttestIdthe 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-runshould "exercise the full code path offline with canned data" — i.e., dry-run should still flow throughclient.get(backed by a cannedCliTestfixture fromsamples.ts) andresolvePortalUrl, not shortcut around them with a separately-maintained literal.There's also no dry-run test case in
src/commands/test.test.ts'sdescribe('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
Source: Path instructions