Add tests and a little polish

internal/config/config_type.go

@@ -8,8 +8,8 @@ import (
 
 // This interface describes interacting with some persistent configuration for gh.
 type Config interface {
-	Get(hostname string, key string) (string, error)
-	GetWithSource(hostname string, key string) (string, string, error)
+	Get(string, string) (string, error)
+	GetWithSource(string, string) (string, string, error)
 	Set(string, string, string) error
 	UnsetHost(string)
 	Hosts() ([]string, error)
@@ -57,7 +57,7 @@ var configOptions = []ConfigOption{
 	},
 	{
 		Key:          "browser",
-		Description:  "the path to the browser to use when opening URLs",
+		Description:  "the web browser to use for opening URLs",
 		DefaultValue: "",
 	},
 }
@@ -198,6 +198,15 @@ func NewBlankRoot() *yaml.Node {
 						Kind:  yaml.ScalarNode,
 						Value: "",
 					},
+					{
+						HeadComment: "What web browser gh should use when opening URLs. If blank, will refer to environment.",
+						Kind:        yaml.ScalarNode,
+						Value:       "browser",
+					},
+					{
+						Kind:  yaml.ScalarNode,
+						Value: "",
+					},
 				},
 			},
 		},

internal/config/config_type_test.go

@@ -52,6 +52,8 @@ func Test_defaultConfig(t *testing.T) {
 		    co: pr checkout
 		# The path to a unix socket through which send HTTP connections. If blank, HTTP traffic will be handled by net/http.DefaultTransport.
 		http_unix_socket:
+		# What web browser gh should use when opening URLs. If blank, will refer to environment.
+		browser:
 	`)
 	assert.Equal(t, expected, mainBuf.String())
 	assert.Equal(t, "", hostsBuf.String())
@@ -69,6 +71,10 @@ func Test_defaultConfig(t *testing.T) {
 	assert.Equal(t, len(aliases.All()), 1)
 	expansion, _ := aliases.Get("co")
 	assert.Equal(t, expansion, "pr checkout")
+
+	browser, err := cfg.Get("", "browser")
+	assert.NoError(t, err)
+	assert.Equal(t, "", browser)
 }
 
 func Test_ValidateValue(t *testing.T) {
@@ -106,4 +112,7 @@ func Test_ValidateKey(t *testing.T) {
 
 	err = ValidateKey("http_unix_socket")
 	assert.NoError(t, err)
+
+	err = ValidateKey("browser")
+	assert.NoError(t, err)
 }

pkg/cmd/factory/default.go

@@ -29,7 +29,7 @@ func New(appVersion string) *cmdutil.Factory {
 	f.HttpClient = httpClientFunc(f, appVersion) // Depends on Config, IOStreams, and appVersion
 	f.Remotes = remotesFunc(f)                   // Depends on Config
 	f.BaseRepo = BaseRepoFunc(f)                 // Depends on Remotes
-	f.Browser = browser(f)                       // Depends on IOStreams
+	f.Browser = browser(f)                       // Depends on Config, and IOStreams
 
 	return f
 }
@@ -89,22 +89,22 @@ func httpClientFunc(f *cmdutil.Factory, appVersion string) func() (*http.Client,
 	}
 }
 
-// browser returns the path to the browser. Attempts to read a path from Config,
-// and falls back to the $BROWSER env var if Config value is blank.
 func browser(f *cmdutil.Factory) cmdutil.Browser {
 	io := f.IOStreams
+	return cmdutil.NewBrowser(browserLauncher(f), io.Out, io.ErrOut)
+}
 
+// Browser precedence
+// 1. browser from config
+// 2. BROWSER
+func browserLauncher(f *cmdutil.Factory) string {
 	cfg, err := f.Config()
-	if err != nil {
-		fmt.Fprintf(io.ErrOut, "problem loading config %s", err)
-	}
-
-	fromConfig, err := cfg.Get("", "browser")
-	if err == nil && fromConfig != "" {
-		return cmdutil.NewBrowser(fromConfig, io.Out, io.ErrOut)
+	if err == nil {
+		if browser, _ := cfg.Get("", "browser"); browser != "" {
+			return browser
+		}
 	}
-
-	return cmdutil.NewBrowser(os.Getenv("BROWSER"), io.Out, io.ErrOut)
+	return os.Getenv("BROWSER")
 }
 
 func executable() string {

pkg/cmd/factory/default_test.go

@@ -378,6 +378,57 @@ func Test_ioStreams_prompt(t *testing.T) {
 	}
 }
 
+func Test_browserLauncher(t *testing.T) {
+	tests := []struct {
+		name        string
+		env         map[string]string
+		config      config.Config
+		wantBrowser string
+	}{
+		{
+			name: "BROWSER set",
+			env: map[string]string{
+				"BROWSER": "BROWSER",
+			},
+			wantBrowser: "BROWSER",
+		},
+		{
+			name:        "config browser set",
+			config:      browserConfig(),
+			wantBrowser: "CONFIG_BROWSER",
+		},
+		{
+			name: "config browser and BROWSER set",
+			env: map[string]string{
+				"BROWSER": "BROWSER",
+			},
+			config:      browserConfig(),
+			wantBrowser: "CONFIG_BROWSER",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.env != nil {
+				for k, v := range tt.env {
+					old := os.Getenv(k)
+					os.Setenv(k, v)
+					defer os.Setenv(k, old)
+				}
+			}
+			f := New("1")
+			f.Config = func() (config.Config, error) {
+				if tt.config == nil {
+					return config.NewBlankConfig(), nil
+				} else {
+					return tt.config, nil
+				}
+			}
+			browser := browserLauncher(f)
+			assert.Equal(t, tt.wantBrowser, browser)
+		})
+	}
+}
+
 func defaultConfig() config.Config {
 	return config.InheritEnv(config.NewFromString(heredoc.Doc(`
     hosts:
@@ -393,3 +444,7 @@ func pagerConfig() config.Config {
 func disablePromptConfig() config.Config {
 	return config.NewFromString("prompt: disabled")
 }
+
+func browserConfig() config.Config {
+	return config.NewFromString("browser: CONFIG_BROWSER")
+}