Pass web browser to each individual command

This removes sensitivity to the BROWSER environment variable in tests
and makes it easier to verify the URL that the browser was invoked with
without having to stub sub-processes.

Author: mislav

go.mod

@@ -7,6 +7,7 @@ require (
 	github.com/MakeNowJust/heredoc v1.0.0
 	github.com/briandowns/spinner v1.11.1
 	github.com/charmbracelet/glamour v0.2.1-0.20200724174618-1246d13c1684
+	github.com/cli/browser v1.1.0
 	github.com/cli/oauth v0.8.0
 	github.com/cli/safeexec v1.0.0
 	github.com/cpuguy83/go-md2man/v2 v2.0.0

go.sum

@@ -10,7 +10,7 @@ import (
 
 	"github.com/cli/cli/api"
 	"github.com/cli/cli/internal/ghinstance"
-	"github.com/cli/cli/pkg/browser"
+	"github.com/cli/cli/pkg/cmdutil"
 	"github.com/cli/cli/pkg/iostreams"
 	"github.com/cli/oauth"
 )
@@ -93,11 +93,9 @@ func authFlow(oauthHost string, IO *iostreams.IOStreams, notice string, addition
 			fmt.Fprintf(w, "- %s to open %s in your browser... ", cs.Bold("Press Enter"), oauthHost)
 			_ = waitForEnter(IO.In)
 
-			browseCmd, err := browser.Command(url)
-			if err != nil {
-				return err
-			}
-			if err := browseCmd.Run(); err != nil {
+			// FIXME: read the browser from cmd Factory rather than recreating it
+			browser := cmdutil.NewBrowser(os.Getenv("BROWSER"), IO.Out, IO.ErrOut)
+			if err := browser.Browse(url); err != nil {
 				fmt.Fprintf(w, "%s Failed opening a web browser at %s\n", cs.Red("!"), url)
 				fmt.Fprintf(w, "  %s\n", err)
 				fmt.Fprint(w, "  Please try entering the URL in your browser manually\n")

internal/authflow/flow.go

@@ -76,5 +76,6 @@ func New(appVersion string) *cmdutil.Factory {
 			return currentBranch, nil
 		},
 		Executable: ghExecutable,
+		Browser:    cmdutil.NewBrowser(os.Getenv("BROWSER"), io.Out, io.ErrOut),
 	}
 }

pkg/browser/browser.go

@@ -23,6 +23,10 @@ import (
 	"github.com/spf13/cobra"
 )
 
+type browser interface {
+	Browse(string) error
+}
+
 type CreateOptions struct {
 	IO *iostreams.IOStreams
 
@@ -33,12 +37,14 @@ type CreateOptions struct {
 	WebMode          bool
 
 	HttpClient func() (*http.Client, error)
+	Browser    browser
 }
 
 func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Command {
 	opts := CreateOptions{
 		IO:         f.IOStreams,
 		HttpClient: f.HttpClient,
+		Browser:    f.Browser,
 	}
 
 	cmd := &cobra.Command{
@@ -144,7 +150,7 @@ func createRun(opts *CreateOptions) error {
 	if opts.WebMode {
 		fmt.Fprintf(opts.IO.Out, "Opening %s in your browser.\n", utils.DisplayURL(gist.HTMLURL))
 
-		return utils.OpenInBrowser(gist.HTMLURL)
+		return opts.Browser.Browse(gist.HTMLURL)
 	}
 
 	fmt.Fprintln(opts.IO.Out, gist.HTMLURL)

pkg/browser/browser_test.go

@@ -171,6 +171,7 @@ func Test_createRun(t *testing.T) {
 		wantStderr string
 		wantParams map[string]interface{}
 		wantErr    bool
+		wantBrowse string
 	}{
 		{
 			name: "public",
@@ -264,6 +265,7 @@ func Test_createRun(t *testing.T) {
 			wantOut:    "Opening gist.github.com/aa5a315d61ae9438b18d in your browser.\n",
 			wantStderr: "- Creating gist fixture.txt\n✓ Created gist fixture.txt\n",
 			wantErr:    false,
+			wantBrowse: "https://gist.github.com/aa5a315d61ae9438b18d",
 			wantParams: map[string]interface{}{
 				"description": "",
 				"updated_at":  "0001-01-01T00:00:00Z",
@@ -291,11 +293,11 @@ func Test_createRun(t *testing.T) {
 		io, stdin, stdout, stderr := iostreams.Test()
 		tt.opts.IO = io
 
-		cs, teardown := run.Stub()
+		browser := &cmdutil.TestBrowser{}
+		tt.opts.Browser = browser
+
+		_, teardown := run.Stub()
 		defer teardown(t)
-		if tt.opts.WebMode {
-			cs.Register(`https://gist\.github\.com/aa5a315d61ae9438b18d$`, 0, "")
-		}
 
 		t.Run(tt.name, func(t *testing.T) {
 			stdin.WriteString(tt.stdin)
@@ -313,6 +315,7 @@ func Test_createRun(t *testing.T) {
 			assert.Equal(t, tt.wantStderr, stderr.String())
 			assert.Equal(t, tt.wantParams, reqBody)
 			reg.Verify(t)
+			browser.Verify(t, tt.wantBrowse)
 		})
 	}
 }

pkg/cmd/factory/default.go

@@ -19,9 +19,14 @@ import (
 	"github.com/spf13/cobra"
 )
 
+type browser interface {
+	Browse(string) error
+}
+
 type ViewOptions struct {
 	IO         *iostreams.IOStreams
 	HttpClient func() (*http.Client, error)
+	Browser    browser
 
 	Selector  string
 	Filename  string
@@ -34,6 +39,7 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman
 	opts := &ViewOptions{
 		IO:         f.IOStreams,
 		HttpClient: f.HttpClient,
+		Browser:    f.Browser,
 	}
 
 	cmd := &cobra.Command{
@@ -94,7 +100,7 @@ func viewRun(opts *ViewOptions) error {
 		if opts.IO.IsStderrTTY() {
 			fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", utils.DisplayURL(gistURL))
 		}
-		return utils.OpenInBrowser(gistURL)
+		return opts.Browser.Browse(gistURL)
 	}
 
 	if strings.Contains(gistID, "/") {

pkg/cmd/gist/create/create.go

@@ -9,7 +9,6 @@ import (
 	issueShared "github.com/cli/cli/pkg/cmd/issue/shared"
 	prShared "github.com/cli/cli/pkg/cmd/pr/shared"
 	"github.com/cli/cli/pkg/cmdutil"
-	"github.com/cli/cli/utils"
 	"github.com/spf13/cobra"
 )
 
@@ -20,7 +19,7 @@ func NewCmdComment(f *cmdutil.Factory, runF func(*prShared.CommentableOptions) e
 		EditSurvey:            prShared.CommentableEditSurvey(f.Config, f.IOStreams),
 		InteractiveEditSurvey: prShared.CommentableInteractiveEditSurvey(f.Config, f.IOStreams),
 		ConfirmSubmitSurvey:   prShared.CommentableConfirmSubmitSurvey,
-		OpenInBrowser:         utils.OpenInBrowser,
+		OpenInBrowser:         f.Browser.Browse,
 	}
 
 	var bodyFile string

pkg/cmd/gist/create/create_test.go

@@ -152,6 +152,7 @@ func TestNewCmdComment(t *testing.T) {
 
 			f := &cmdutil.Factory{
 				IOStreams: io,
+				Browser:   &cmdutil.TestBrowser{},
 			}
 
 			argv, err := shlex.Split(tt.input)

pkg/cmd/gist/view/view.go

@@ -17,11 +17,16 @@ import (
 	"github.com/spf13/cobra"
 )
 
+type browser interface {
+	Browse(string) error
+}
+
 type CreateOptions struct {
 	HttpClient func() (*http.Client, error)
 	Config     func() (config.Config, error)
 	IO         *iostreams.IOStreams
 	BaseRepo   func() (ghrepo.Interface, error)
+	Browser    browser
 
 	RootDirOverride string
 
@@ -44,6 +49,7 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co
 		IO:         f.IOStreams,
 		HttpClient: f.HttpClient,
 		Config:     f.Config,
+		Browser:    f.Browser,
 	}
 
 	var bodyFile string
@@ -171,7 +177,7 @@ func createRun(opts *CreateOptions) (err error) {
 		if isTerminal {
 			fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", utils.DisplayURL(openURL))
 		}
-		return utils.OpenInBrowser(openURL)
+		return opts.Browser.Browse(openURL)
 	}
 
 	if isTerminal {
@@ -286,7 +292,7 @@ func createRun(opts *CreateOptions) (err error) {
 		if isTerminal {
 			fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", utils.DisplayURL(openURL))
 		}
-		return utils.OpenInBrowser(openURL)
+		return opts.Browser.Browse(openURL)
 	} else if action == prShared.SubmitAction {
 		params := map[string]interface{}{
 			"title": tb.Title,