From 9becb5f790c4b37e3b7ba558eebd8723ad6bb5c0 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Fri, 13 Dec 2019 16:16:46 -0800 Subject: [PATCH 1/3] Have one place manage the config dir location --- context/config_file.go | 9 +++------ context/context.go | 11 ++++++++--- main.go | 8 +++----- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/context/config_file.go b/context/config_file.go index 5236930a1..49b0bf559 100644 --- a/context/config_file.go +++ b/context/config_file.go @@ -8,7 +8,6 @@ import ( "os" "path" - "github.com/mitchellh/go-homedir" "gopkg.in/yaml.v3" ) @@ -36,11 +35,9 @@ func parseConfigFile(fn string) (*configEntry, error) { return parseConfig(f) } -// ParseDefaultConfig reads the configuration from ~/.config/gh +// ParseDefaultConfig reads the configuration file func ParseDefaultConfig() (*configEntry, error) { - // FIXME: this duplicates fsContext.configFile - fn, _ := homedir.Expand("~/.config/gh") - return parseConfigFile(fn) + return parseConfigFile(configFile()) } func parseConfig(r io.Reader) (*configEntry, error) { @@ -75,7 +72,7 @@ func parseConfig(r io.Reader) (*configEntry, error) { // If ~/.config/gh is a file, convert it to a directory and place the file // into ~/.config/gh/config.yml func migrateConfigFile() { - p, _ := homedir.Expand("~/.config/gh") + p := ConfigDir() fi, err := os.Stat(p) if err != nil { // This means the file doesn't exist, and that is fine. return diff --git a/context/context.go b/context/context.go index 20a921ffc..e5d74634c 100644 --- a/context/context.go +++ b/context/context.go @@ -1,6 +1,7 @@ package context import ( + "path" "strings" "github.com/github/gh-cli/git" @@ -39,14 +40,18 @@ type fsContext struct { authToken string } -func (c *fsContext) configFile() string { - dir, _ := homedir.Expand("~/.config/gh/config.yml") +func ConfigDir() string { + dir, _ := homedir.Expand("~/.config/gh") return dir } +func configFile() string { + return path.Join(ConfigDir(), "config.yml") +} + func (c *fsContext) getConfig() (*configEntry, error) { if c.config == nil { - entry, err := parseOrSetupConfigFile(c.configFile()) + entry, err := parseOrSetupConfigFile(configFile()) if err != nil { return nil, err } diff --git a/main.go b/main.go index f09edb18d..726b50784 100644 --- a/main.go +++ b/main.go @@ -3,14 +3,15 @@ package main import ( "fmt" "os" + "path" "strings" "github.com/github/gh-cli/command" + "github.com/github/gh-cli/context" "github.com/github/gh-cli/update" "github.com/github/gh-cli/utils" "github.com/mattn/go-isatty" "github.com/mgutz/ansi" - "github.com/mitchellh/go-homedir" ) var updaterEnabled = "" @@ -61,9 +62,6 @@ func checkForUpdate(currentVersion string) (*update.ReleaseInfo, error) { } repo := updaterEnabled - stateFilePath, err := homedir.Expand("~/.config/gh/state.yml") - if err != nil { - return nil, err - } + stateFilePath := path.Join(context.ConfigDir(), "state.yml") return update.CheckForUpdate(client, stateFilePath, repo, currentVersion) } From d1905f5824d7914f0e912a5f7e8993d6644a4ca0 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Fri, 13 Dec 2019 16:17:28 -0800 Subject: [PATCH 2/3] update comment --- context/config_file.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/context/config_file.go b/context/config_file.go index 49b0bf559..6c053286d 100644 --- a/context/config_file.go +++ b/context/config_file.go @@ -69,8 +69,8 @@ func parseConfig(r io.Reader) (*configEntry, error) { // This is a temporary function that will migrate the config file. It can be removed // in January. // -// If ~/.config/gh is a file, convert it to a directory and place the file -// into ~/.config/gh/config.yml +// If the config dir is a file, convert it to a directory and place the file +// into a file named config.yml func migrateConfigFile() { p := ConfigDir() fi, err := os.Stat(p) From 48aeff1ca7c739b057d8fb594ba11c1b382b060b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 16 Dec 2019 15:46:42 +0100 Subject: [PATCH 3/3] Assert stdout separarely from stderr in command tests This stubs stderr separately from stdout in command tests (before those streams were combined) and improves test assertions around output. Additionally, no longer use the `cmd.Print*()` family of Cobra functions because their name sounds like the text will go to stdout, but they write to stderr instead. Use the more explicit `cmd.ErrOrStderr()` as output destination instead. --- command/completion_test.go | 12 ++++---- command/issue.go | 2 +- command/issue_test.go | 20 +++++++------- command/pr.go | 2 +- command/pr_checkout_test.go | 18 ++++++------ command/pr_create.go | 4 +-- command/pr_create_test.go | 12 ++++---- command/pr_test.go | 53 ++++++++++++++++++++++-------------- command/title_body_survey.go | 2 +- 9 files changed, 67 insertions(+), 58 deletions(-) diff --git a/command/completion_test.go b/command/completion_test.go index 5ea8e8ed8..e66a00124 100644 --- a/command/completion_test.go +++ b/command/completion_test.go @@ -6,24 +6,24 @@ import ( ) func TestCompletion_bash(t *testing.T) { - outStr, err := RunCommand(completionCmd, `completion`) + output, err := RunCommand(completionCmd, `completion`) if err != nil { t.Fatal(err) } - if !strings.Contains(outStr, "complete -o default -F __start_gh gh") { - t.Errorf("problem in bash completion:\n%s", outStr) + if !strings.Contains(output.String(), "complete -o default -F __start_gh gh") { + t.Errorf("problem in bash completion:\n%s", output) } } func TestCompletion_zsh(t *testing.T) { - outStr, err := RunCommand(completionCmd, `completion -s zsh`) + output, err := RunCommand(completionCmd, `completion -s zsh`) if err != nil { t.Fatal(err) } - if !strings.Contains(outStr, "#compdef _gh gh") { - t.Errorf("problem in zsh completion:\n%s", outStr) + if !strings.Contains(output.String(), "#compdef _gh gh") { + t.Errorf("problem in zsh completion:\n%s", output) } } diff --git a/command/issue.go b/command/issue.go index ec4f7b188..2a24e790b 100644 --- a/command/issue.go +++ b/command/issue.go @@ -214,7 +214,7 @@ func issueView(cmd *cobra.Command, args []string) error { } openURL := issue.URL - cmd.Printf("Opening %s in your browser.\n", openURL) + fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", openURL) return utils.OpenInBrowser(openURL) } diff --git a/command/issue_test.go b/command/issue_test.go index b5f8f0272..04a1cf7e1 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -33,7 +33,7 @@ func TestIssueStatus(t *testing.T) { } for _, r := range expectedIssues { - if !r.MatchString(output) { + if !r.MatchString(output.String()) { t.Errorf("output did not match regexp /%s/\n> output\n%s\n", r, output) return } @@ -60,7 +60,7 @@ func TestIssueList(t *testing.T) { } for _, r := range expectedIssues { - if !r.MatchString(output) { + if !r.MatchString(output.String()) { t.Errorf("output did not match regexp /%s/\n> output\n%s\n", r, output) return } @@ -72,11 +72,14 @@ func TestIssueList_withFlags(t *testing.T) { http.StubResponse(200, bytes.NewBufferString(`{"data": {}}`)) // Since we are testing that the flags are passed, we don't care about the response - _, err := RunCommand(issueListCmd, "issue list -a probablyCher -l web,bug -s open") + output, err := RunCommand(issueListCmd, "issue list -a probablyCher -l web,bug -s open") if err != nil { t.Errorf("error running command `issue list`: %v", err) } + eq(t, output.String(), "") + eq(t, output.Stderr(), "No issues match your search\n") + bodyBytes, _ := ioutil.ReadAll(http.Requests[0].Body) reqBody := struct { Variables struct { @@ -115,9 +118,8 @@ func TestIssueView(t *testing.T) { t.Errorf("error running command `issue view`: %v", err) } - if output == "" { - t.Errorf("command output expected got an empty string") - } + eq(t, output.String(), "") + eq(t, output.Stderr(), "Opening https://github.com/OWNER/REPO/issues/123 in your browser.\n") if seenCmd == nil { t.Fatal("expected a command to run") @@ -176,9 +178,7 @@ func TestIssueView_urlArg(t *testing.T) { t.Errorf("error running command `issue view`: %v", err) } - if output == "" { - t.Errorf("command output expected got an empty string") - } + eq(t, output.String(), "") if seenCmd == nil { t.Fatal("expected a command to run") @@ -223,5 +223,5 @@ func TestIssueCreate(t *testing.T) { eq(t, reqBody.Variables.Input.Title, "hello") eq(t, reqBody.Variables.Input.Body, "cash rules everything around me") - eq(t, output, "https://github.com/OWNER/REPO/issues/12\n") + eq(t, output.String(), "https://github.com/OWNER/REPO/issues/12\n") } diff --git a/command/pr.go b/command/pr.go index 584fd3a8f..d4fa4cc3a 100644 --- a/command/pr.go +++ b/command/pr.go @@ -280,7 +280,7 @@ func prView(cmd *cobra.Command, args []string) error { } } - cmd.Printf("Opening %s in your browser.\n", openURL) + fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", openURL) return utils.OpenInBrowser(openURL) } diff --git a/command/pr_checkout_test.go b/command/pr_checkout_test.go index b3b991c39..93eaf81bb 100644 --- a/command/pr_checkout_test.go +++ b/command/pr_checkout_test.go @@ -53,7 +53,7 @@ func TestPRCheckout_sameRepo(t *testing.T) { output, err := RunCommand(prCheckoutCmd, `pr checkout 123`) eq(t, err, nil) - eq(t, output, "") + eq(t, output.String(), "") eq(t, len(ranCommands), 4) eq(t, strings.Join(ranCommands[0], " "), "git fetch origin +refs/heads/feature:refs/remotes/origin/feature") @@ -105,7 +105,7 @@ func TestPRCheckout_urlArg(t *testing.T) { output, err := RunCommand(prCheckoutCmd, `pr checkout https://github.com/OWNER/REPO/pull/123/files`) eq(t, err, nil) - eq(t, output, "") + eq(t, output.String(), "") eq(t, len(ranCommands), 4) eq(t, strings.Join(ranCommands[1], " "), "git checkout -b feature --no-track origin/feature") @@ -154,7 +154,7 @@ func TestPRCheckout_branchArg(t *testing.T) { output, err := RunCommand(prCheckoutCmd, `pr checkout hubot:feature`) eq(t, err, nil) - eq(t, output, "") + eq(t, output.String(), "") eq(t, len(ranCommands), 5) eq(t, strings.Join(ranCommands[1], " "), "git fetch origin refs/pull/123/head:feature") @@ -203,7 +203,7 @@ func TestPRCheckout_existingBranch(t *testing.T) { output, err := RunCommand(prCheckoutCmd, `pr checkout 123`) eq(t, err, nil) - eq(t, output, "") + eq(t, output.String(), "") eq(t, len(ranCommands), 3) eq(t, strings.Join(ranCommands[0], " "), "git fetch origin +refs/heads/feature:refs/remotes/origin/feature") @@ -255,7 +255,7 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { output, err := RunCommand(prCheckoutCmd, `pr checkout 123`) eq(t, err, nil) - eq(t, output, "") + eq(t, output.String(), "") eq(t, len(ranCommands), 4) eq(t, strings.Join(ranCommands[0], " "), "git fetch robot-fork +refs/heads/feature:refs/remotes/robot-fork/feature") @@ -307,7 +307,7 @@ func TestPRCheckout_differentRepo(t *testing.T) { output, err := RunCommand(prCheckoutCmd, `pr checkout 123`) eq(t, err, nil) - eq(t, output, "") + eq(t, output.String(), "") eq(t, len(ranCommands), 4) eq(t, strings.Join(ranCommands[0], " "), "git fetch origin refs/pull/123/head:feature") @@ -359,7 +359,7 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) { output, err := RunCommand(prCheckoutCmd, `pr checkout 123`) eq(t, err, nil) - eq(t, output, "") + eq(t, output.String(), "") eq(t, len(ranCommands), 2) eq(t, strings.Join(ranCommands[0], " "), "git fetch origin refs/pull/123/head:feature") @@ -409,7 +409,7 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { output, err := RunCommand(prCheckoutCmd, `pr checkout 123`) eq(t, err, nil) - eq(t, output, "") + eq(t, output.String(), "") eq(t, len(ranCommands), 2) eq(t, strings.Join(ranCommands[0], " "), "git fetch origin refs/pull/123/head") @@ -459,7 +459,7 @@ func TestPRCheckout_maintainerCanModify(t *testing.T) { output, err := RunCommand(prCheckoutCmd, `pr checkout 123`) eq(t, err, nil) - eq(t, output, "") + eq(t, output.String(), "") eq(t, len(ranCommands), 4) eq(t, strings.Join(ranCommands[0], " "), "git fetch origin refs/pull/123/head:feature") diff --git a/command/pr_create.go b/command/pr_create.go index abff4e5be..529925941 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -27,7 +27,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { noun = noun + "s" } - cmd.Printf("Warning: %d uncommitted %s\n", ucc, noun) + fmt.Fprintf(cmd.ErrOrStderr(), "Warning: %d uncommitted %s\n", ucc, noun) } repo, err := ctx.BaseRepo() @@ -55,7 +55,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { } if isWeb { openURL := fmt.Sprintf(`https://github.com/%s/%s/pull/%s`, repo.RepoOwner(), repo.RepoName(), head) - cmd.Printf("Opening %s in your browser.\n", openURL) + fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", openURL) return utils.OpenInBrowser(openURL) } diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 50e280d69..badfe7204 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -91,7 +91,7 @@ func TestPRCreate(t *testing.T) { eq(t, reqBody.Variables.Input.BaseRefName, "master") eq(t, reqBody.Variables.Input.HeadRefName, "feature") - eq(t, output, "https://github.com/OWNER/REPO/pull/12\n") + eq(t, output.String(), "https://github.com/OWNER/REPO/pull/12\n") } func TestPRCreate_web(t *testing.T) { @@ -115,9 +115,8 @@ func TestPRCreate_web(t *testing.T) { output, err := RunCommand(prCreateCmd, `pr create --web`) eq(t, err, nil) - if output == "" { - t.Fatal("expected output") - } + eq(t, output.String(), "") + eq(t, output.Stderr(), "Opening https://github.com/OWNER/REPO/pull/feature in your browser.\n") eq(t, len(ranCommands), 3) eq(t, strings.Join(ranCommands[1], " "), "git push --set-upstream origin HEAD:feature") @@ -155,7 +154,6 @@ func TestPRCreate_ReportsUncommittedChanges(t *testing.T) { output, err := RunCommand(prCreateCmd, `pr create -t "my title" -b "my body"`) eq(t, err, nil) - eq(t, output, `Warning: 1 uncommitted change -https://github.com/OWNER/REPO/pull/12 -`) + eq(t, output.String(), "https://github.com/OWNER/REPO/pull/12\n") + eq(t, output.Stderr(), "Warning: 1 uncommitted change\n") } diff --git a/command/pr_test.go b/command/pr_test.go index dadeeada0..6c7377216 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -24,16 +24,30 @@ func eq(t *testing.T, got interface{}, expected interface{}) { } } -func RunCommand(cmd *cobra.Command, args string) (string, error) { +type cmdOut struct { + outBuf, errBuf *bytes.Buffer +} + +func (c cmdOut) String() string { + return c.outBuf.String() +} + +func (c cmdOut) Stderr() string { + return c.errBuf.String() +} + +func RunCommand(cmd *cobra.Command, args string) (*cmdOut, error) { rootCmd := cmd.Root() argv, err := shlex.Split(args) if err != nil { - return "", err + return nil, err } rootCmd.SetArgs(argv) outBuf := bytes.Buffer{} cmd.SetOut(&outBuf) + errBuf := bytes.Buffer{} + cmd.SetErr(&errBuf) // Reset flag values so they don't leak between tests cmd.Flags().VisitAll(func(f *pflag.Flag) { @@ -48,7 +62,10 @@ func RunCommand(cmd *cobra.Command, args string) (string, error) { }) _, err = rootCmd.ExecuteC() - return outBuf.String(), err + cmd.SetOut(nil) + cmd.SetErr(nil) + + return &cmdOut{&outBuf, &errBuf}, err } func TestPRStatus(t *testing.T) { @@ -72,7 +89,7 @@ func TestPRStatus(t *testing.T) { } for _, r := range expectedPrs { - if !r.MatchString(output) { + if !r.MatchString(output.String()) { t.Errorf("output did not match regexp /%s/", r) } } @@ -91,7 +108,7 @@ func TestPRList(t *testing.T) { t.Fatal(err) } - eq(t, output, `32 New feature feature + eq(t, output.String(), `32 New feature feature 29 Fixed bad bug hubot:bug-fix 28 Improve documentation docs `) @@ -104,11 +121,14 @@ func TestPRList_filtering(t *testing.T) { respBody := bytes.NewBufferString(`{ "data": {} }`) http.StubResponse(200, respBody) - _, err := RunCommand(prListCmd, `pr list -s all -l one,two -l three`) + output, err := RunCommand(prListCmd, `pr list -s all -l one,two -l three`) if err != nil { t.Fatal(err) } + eq(t, output.String(), "") + eq(t, output.Stderr(), "No pull requests match your search\n") + bodyBytes, _ := ioutil.ReadAll(http.Requests[0].Body) reqBody := struct { Variables struct { @@ -183,9 +203,8 @@ func TestPRView_currentBranch(t *testing.T) { t.Errorf("error running command `pr view`: %v", err) } - if output == "" { - t.Errorf("command output expected got an empty string") - } + eq(t, output.String(), "") + eq(t, output.Stderr(), "Opening https://github.com/OWNER/REPO/pull/10 in your browser.\n") if seenCmd == nil { t.Fatal("expected a command to run") @@ -248,9 +267,7 @@ func TestPRView_numberArg(t *testing.T) { t.Errorf("error running command `pr view`: %v", err) } - if output == "" { - t.Errorf("command output expected got an empty string") - } + eq(t, output.String(), "") if seenCmd == nil { t.Fatal("expected a command to run") @@ -281,9 +298,7 @@ func TestPRView_urlArg(t *testing.T) { t.Errorf("error running command `pr view`: %v", err) } - if output == "" { - t.Errorf("command output expected got an empty string") - } + eq(t, output.String(), "") if seenCmd == nil { t.Fatal("expected a command to run") @@ -316,9 +331,7 @@ func TestPRView_branchArg(t *testing.T) { t.Errorf("error running command `pr view`: %v", err) } - if output == "" { - t.Errorf("command output expected got an empty string") - } + eq(t, output.String(), "") if seenCmd == nil { t.Fatal("expected a command to run") @@ -352,9 +365,7 @@ func TestPRView_branchWithOwnerArg(t *testing.T) { t.Errorf("error running command `pr view`: %v", err) } - if output == "" { - t.Errorf("command output expected got an empty string") - } + eq(t, output.String(), "") if seenCmd == nil { t.Fatal("expected a command to run") diff --git a/command/title_body_survey.go b/command/title_body_survey.go index af4dc34e1..450742d7a 100644 --- a/command/title_body_survey.go +++ b/command/title_body_survey.go @@ -92,7 +92,7 @@ func titleBodySurvey(cmd *cobra.Command, providedTitle string, providedBody stri case _unconfirmed: continue case _cancel: - cmd.Println("Discarding.") + fmt.Fprintln(cmd.ErrOrStderr(), "Discarding.") return nil, nil default: panic("reached unreachable case")