fix(cli): stop npm test from opening real browser tabs - #5
Merged
Merged
Conversation
The two auth-login tests spawn the real binary, and `auth login` calls openBrowser(), which shells out to `open <url>` on macOS. Every `npm test` run therefore opened two browser tabs pointing at throwaway loopback servers that close seconds later, leaving dead tabs on the developer's machine. Three runs during one session left six. BUTTERSTACK_NO_BROWSER makes openBrowser() print the URL rather than launch anything, and the test harness sets it for every invocation in buildEnv(). The flag is generally useful beyond tests: any non-interactive driver of `auth login` (CI, a container, a remote shell) wants the URL, not a spawn attempt against a browser that isn't there. No credentials were ever at risk - HOME is a per-test temp directory and the flow completes against a local listener, never a real account - but the tabs are noise a test suite has no business creating. Adds a regression test asserting the binary honors the flag and still prints the URL, so the printing half (which the harness's readAuthUrl depends on) cannot be removed with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…output
Follow-up within the same PR. Running the flag path by hand showed the output
was wrong in two ways:
Visit... no. It said:
Opening your browser for one-click authorization...
URL: http://127.0.0.1:57713/cli/auth?...
Please visit this URL to authenticate:
http://127.0.0.1:57713/cli/auth?...
The line claimed a browser was opening while BUTTERSTACK_NO_BROWSER was
deliberately suppressing exactly that, and the URL was printed twice - once by
authLogin and again by openBrowser's suppression branch. Now authLogin says
"Visit this URL to authorize:" when the flag is set, and openBrowser prints
nothing, because the caller has already printed the URL.
The first version of this change was only tested for what it did NOT do (no
browser spawned). That missed both defects, since neither is about spawning.
Three tests now cover the rendered output itself:
- the URL appears exactly once, and "Opening your browser" does not
- exactly one intro line and one URL line, and the URL line matches exactly
- the interactive path: "Opening your browser" IS printed, the no-browser
prompt is not, and the browser really receives the URL that was displayed
That last one runs the branch a real user hits. It puts a shim named `open`
(or `xdg-open`) first on PATH which records its argv, so the real branch
executes and nothing launches - the one way to cover it without reintroducing
the tab leak this PR exists to fix.
Two test-side details worth knowing: readAuthUrl matches with (\S+) against
coloured output, so the URL it returns carries a trailing ANSI reset code
(harmless for `new URL()`, fatal for string comparison - hence cleanUrl), and
openBrowser spawns without waiting, so the shim can still be writing after the
CLI has exited - hence the poll.
18 tests, 0 failures, stable across three consecutive runs.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
42 comment lines removed. The assertion messages already say what each check is for; the comments were restating them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
npm testopens real browser tabs on the developer's machine. Two per run.The two auth-login cases spawn the actual binary, and
auth logincallsopenBrowser(), which shells out toopen <url>on macOS. Those URLs point at throwaway loopback servers the test tears down seconds later, so what's left behind is a dead tab. Three test runs in one session left six of them — noticed because the URLs started getting pasted back at me:scope=ping read:projectsis the giveaway — that's the--scope "ping,read:projects"case attest/butter.test.js:292.Fix
BUTTERSTACK_NO_BROWSERmakesopenBrowser()print the URL instead of launching anything, andbuildEnv()sets it for every test invocation.The flag earns its place outside tests too: any non-interactive driver of
auth login— CI, a container, a remote shell — wants the URL printed, not a spawn attempt against a browser that isn't there. That path previously fell through to thechild.on("error")branch and printed "Could not automatically open browser", which is a worse way to arrive at the same place.Not a security issue
HOMEis a per-test temp directory and the flow completes against a local listener, so no real credential was ever involved and no real account was ever contacted. The tabs are noise, not exposure.Test
Asserts the binary honors the flag and still prints the URL — the harness's own
readAuthUrl()depends on that line, so this pins the printing half against being removed along with the launching half.15 tests, 0 failures. Verified the run opens zero browser processes.
🤖 Generated with Claude Code