From d881a2e52ed3d2cb6a9b276ac9cf83f4252688fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 1 Nov 2019 22:16:23 +0100 Subject: [PATCH 1/5] Ensure git operations preserve their stderr in error output This also provides a SetPrepareCmd hook for tests to be able to define stubs for commands that are supposed to be run --- git/git.go | 39 ++++++++++------------------- utils/prepare_cmd.go | 59 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 26 deletions(-) create mode 100644 utils/prepare_cmd.go diff --git a/git/git.go b/git/git.go index b3b63388e..a2a385f7f 100644 --- a/git/git.go +++ b/git/git.go @@ -8,12 +8,13 @@ import ( "os/exec" "path/filepath" "strings" + + "github.com/github/gh-cli/utils" ) func Dir() (string, error) { dirCmd := exec.Command("git", "rev-parse", "-q", "--git-dir") - dirCmd.Stderr = nil - output, err := dirCmd.Output() + output, err := utils.PrepareCmd(dirCmd).Output() if err != nil { return "", fmt.Errorf("Not a git repository (or any of the parent directories): .git") } @@ -33,7 +34,7 @@ func Dir() (string, error) { func WorkdirName() (string, error) { toplevelCmd := exec.Command("git", "rev-parse", "--show-toplevel") toplevelCmd.Stderr = nil - output, err := toplevelCmd.Output() + output, err := utils.PrepareCmd(toplevelCmd).Output() dir := firstLine(output) if dir == "" { return "", fmt.Errorf("unable to determine git working directory") @@ -44,8 +45,7 @@ func WorkdirName() (string, error) { func HasFile(segments ...string) bool { // The blessed way to resolve paths within git dir since Git 2.5.0 pathCmd := exec.Command("git", "rev-parse", "-q", "--git-path", filepath.Join(segments...)) - pathCmd.Stderr = nil - if output, err := pathCmd.Output(); err == nil { + if output, err := utils.PrepareCmd(pathCmd).Output(); err == nil { if lines := outputLines(output); len(lines) == 1 { if _, err := os.Stat(lines[0]); err == nil { return true @@ -97,8 +97,7 @@ func BranchAtRef(paths ...string) (name string, err error) { func Editor() (string, error) { varCmd := exec.Command("git", "var", "GIT_EDITOR") - varCmd.Stderr = nil - output, err := varCmd.Output() + output, err := utils.PrepareCmd(varCmd).Output() if err != nil { return "", fmt.Errorf("Can't load git var: GIT_EDITOR") } @@ -112,8 +111,7 @@ func Head() (string, error) { func SymbolicFullName(name string) (string, error) { parseCmd := exec.Command("git", "rev-parse", "--symbolic-full-name", name) - parseCmd.Stderr = nil - output, err := parseCmd.Output() + output, err := utils.PrepareCmd(parseCmd).Output() if err != nil { return "", fmt.Errorf("Unknown revision or path not in the working tree: %s", name) } @@ -145,9 +143,7 @@ func CommentChar(text string) (string, error) { func Show(sha string) (string, error) { cmd := exec.Command("git", "-c", "log.showSignature=false", "show", "-s", "--format=%s%n%+b", sha) - cmd.Stderr = nil - - output, err := cmd.Output() + output, err := utils.PrepareCmd(cmd).Output() return strings.TrimSpace(string(output)), err } @@ -157,7 +153,7 @@ func Log(sha1, sha2 string) (string, error) { "-c", "log.showSignature=false", "log", "--no-color", "--format=%h (%aN, %ar)%n%w(78,3,3)%s%n%+b", "--cherry", shaRange) - outputs, err := cmd.Output() + outputs, err := utils.PrepareCmd(cmd).Output() if err != nil { return "", fmt.Errorf("Can't load git log %s..%s", sha1, sha2) } @@ -167,14 +163,13 @@ func Log(sha1, sha2 string) (string, error) { func listRemotes() ([]string, error) { remoteCmd := exec.Command("git", "remote", "-v") - remoteCmd.Stderr = nil - output, err := remoteCmd.Output() + output, err := utils.PrepareCmd(remoteCmd).Output() return outputLines(output), err } func Config(name string) (string, error) { configCmd := exec.Command("git", "config", name) - output, err := configCmd.Output() + output, err := utils.PrepareCmd(configCmd).Output() if err != nil { return "", fmt.Errorf("unknown config key: %s", name) } @@ -190,21 +185,16 @@ func ConfigAll(name string) ([]string, error) { } configCmd := exec.Command("git", "config", mode, name) - output, err := configCmd.Output() + output, err := utils.PrepareCmd(configCmd).Output() if err != nil { return nil, fmt.Errorf("Unknown config %s", name) } return outputLines(output), nil } -func Run(args ...string) error { - cmd := exec.Command("git", args...) - return cmd.Run() -} - func LocalBranches() ([]string, error) { branchesCmd := exec.Command("git", "branch", "--list") - output, err := branchesCmd.Output() + output, err := utils.PrepareCmd(branchesCmd).Output() if err != nil { return nil, err } @@ -218,9 +208,6 @@ func LocalBranches() ([]string, error) { func outputLines(output []byte) []string { lines := strings.TrimSuffix(string(output), "\n") - if lines == "" { - return []string{} - } return strings.Split(lines, "\n") } diff --git a/utils/prepare_cmd.go b/utils/prepare_cmd.go new file mode 100644 index 000000000..41cfeecd0 --- /dev/null +++ b/utils/prepare_cmd.go @@ -0,0 +1,59 @@ +package utils + +import ( + "bytes" + "fmt" + "os/exec" + "strings" +) + +// Runnable is typically an exec.Cmd or its stub in tests +type Runnable interface { + Output() ([]byte, error) + Run() error +} + +// PrepareCmd extends exec.Cmd with extra error reporting features and provides a +// hook to stub command execution in tests +var PrepareCmd = func(cmd *exec.Cmd) Runnable { + return &cmdWithStderr{cmd} +} + +// SetPrepareCmd overrides PrepareCmd and returns a func to revert it back +func SetPrepareCmd(fn func(*exec.Cmd) Runnable) func() { + origPrepare := PrepareCmd + PrepareCmd = fn + return func() { + PrepareCmd = origPrepare + } +} + +// cmdWithStderr augments Output() by adding stderr to the error message +type cmdWithStderr struct { + *exec.Cmd +} + +func (c cmdWithStderr) Output() ([]byte, error) { + errStream := &bytes.Buffer{} + c.Cmd.Stderr = errStream + out, err := c.Cmd.Output() + if err != nil { + err = &CmdError{errStream, c.Cmd.Args, err} + } + return out, err +} + +// CmdError provides more visibility into why an exec.Cmd had failed +type CmdError struct { + Stderr *bytes.Buffer + Args []string + Err error +} + +func (e CmdError) Error() string { + msg := e.Stderr.String() + if msg != "" && !strings.HasSuffix(msg, "\n") { + msg += "\n" + } + return fmt.Sprintf("%s%s: %s", msg, e.Args[0], e.Err) +} From f6fcdf114e8958c32f2850684dc6eb69a0cbcd46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 1 Nov 2019 22:18:12 +0100 Subject: [PATCH 2/5] Use SetPrepareCmd hook to spy on OpenInBrowser We are now able to assert that the browse command was called with the correct URL --- command/pr_test.go | 64 ++++++++++++++++++++++++--------------- test/fixtures/prView.json | 8 ++--- utils/utils.go | 5 +-- 3 files changed, 46 insertions(+), 31 deletions(-) diff --git a/command/pr_test.go b/command/pr_test.go index e4a5122d7..88eb6f2a8 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -2,6 +2,7 @@ package command import ( "os" + "os/exec" "regexp" "testing" @@ -63,8 +64,12 @@ func TestPRView(t *testing.T) { defer jsonFile.Close() http.StubResponse(200, jsonFile) - teardown, callCount := mockOpenInBrowser() - defer teardown() + var seenCmd *exec.Cmd + restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + seenCmd = cmd + return &outputStub{} + }) + defer restoreCmd() output, err := test.RunCommand(RootCmd, "pr view") if err != nil { @@ -75,9 +80,25 @@ func TestPRView(t *testing.T) { t.Errorf("command output expected got an empty string") } - if *callCount != 1 { - t.Errorf("OpenInBrowser should be called 1 time but was called %d time(s)", *callCount) + if seenCmd == nil { + t.Fatal("expected a command to run") } + url := seenCmd.Args[len(seenCmd.Args)-1] + if url != "https://github.com/OWNER/REPO/pull/10" { + t.Errorf("got: %q", url) + } +} + +type outputStub struct { + contents []byte +} + +func (s outputStub) Output() ([]byte, error) { + return s.contents, nil +} + +func (s outputStub) Run() error { + return nil } func TestPRView_NoActiveBranch(t *testing.T) { @@ -88,16 +109,20 @@ func TestPRView_NoActiveBranch(t *testing.T) { defer jsonFile.Close() http.StubResponse(200, jsonFile) - teardown, callCount := mockOpenInBrowser() - defer teardown() + var seenCmd *exec.Cmd + restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + seenCmd = cmd + return &outputStub{} + }) + defer restoreCmd() output, err := test.RunCommand(RootCmd, "pr view") if err == nil || err.Error() != "the 'master' branch has no open pull requests" { t.Errorf("error running command `pr view`: %v", err) } - if *callCount > 0 { - t.Errorf("OpenInBrowser should NOT be called but was called %d time(s)", *callCount) + if seenCmd != nil { + t.Fatalf("unexpected command: %v", seenCmd.Args) } // Now run again but provide a PR number @@ -110,22 +135,11 @@ func TestPRView_NoActiveBranch(t *testing.T) { t.Errorf("command output expected got an empty string") } - if *callCount != 1 { - t.Errorf("OpenInBrowser should be called once but was called %d time(s)", *callCount) + if seenCmd == nil { + t.Fatal("expected a command to run") + } + url := seenCmd.Args[len(seenCmd.Args)-1] + if url != "https://github.com/OWNER/REPO/pull/23" { + t.Errorf("got: %q", url) } } - -func mockOpenInBrowser() (func(), *int) { - callCount := 0 - originalOpenInBrowser := utils.OpenInBrowser - teardown := func() { - utils.OpenInBrowser = originalOpenInBrowser - } - - utils.OpenInBrowser = func(_ string) error { - callCount++ - return nil - } - - return teardown, &callCount -} diff --git a/test/fixtures/prView.json b/test/fixtures/prView.json index 444fa5e01..5ac892498 100644 --- a/test/fixtures/prView.json +++ b/test/fixtures/prView.json @@ -6,7 +6,7 @@ "node": { "number": 10, "title": "Blueberries are a good fruit", - "url": "https://github.com/github/gh-cli/pull/10", + "url": "https://github.com/OWNER/REPO/pull/10", "headRefName": "[blueberries]" } } @@ -19,7 +19,7 @@ "node": { "number": 8, "title": "Strawberries are not actually berries", - "url": "https://github.com/github/gh-cli/pull/8", + "url": "https://github.com/OWNER/REPO/pull/8", "headRefName": "[strawberries]" } } @@ -32,7 +32,7 @@ "node": { "number": 9, "title": "Apples are tasty", - "url": "https://github.com/github/gh-cli/pull/9", + "url": "https://github.com/OWNER/REPO/pull/9", "headRefName": "[apples]" } }, @@ -40,7 +40,7 @@ "node": { "number": 11, "title": "Figs are my favorite", - "url": "https://github.com/github/gh-cli/pull/1", + "url": "https://github.com/OWNER/REPO/pull/1", "headRefName": "[figs]" } } diff --git a/utils/utils.go b/utils/utils.go index ff1eb1993..b034a18d9 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -27,7 +27,7 @@ func ConcatPaths(paths ...string) string { return strings.Join(paths, "/") } -var OpenInBrowser = func(url string) error { +func OpenInBrowser(url string) error { browser := os.Getenv("BROWSER") if browser == "" { browser = searchBrowserLauncher(runtime.GOOS) @@ -45,7 +45,8 @@ var OpenInBrowser = func(url string) error { } endingArgs := append(browserArgs[1:], url) - return exec.Command(browserArgs[0], endingArgs...).Run() + browseCmd := exec.Command(browserArgs[0], endingArgs...) + return PrepareCmd(browseCmd).Run() } func searchBrowserLauncher(goos string) (browser string) { From 8ee97d72cd56ac12fac55d5cb0c8391a6f85e96d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 1 Nov 2019 23:20:15 +0100 Subject: [PATCH 3/5] Extract outputStub into `testing.go` --- command/pr_test.go | 12 ------------ command/testing.go | 14 ++++++++++++++ 2 files changed, 14 insertions(+), 12 deletions(-) create mode 100644 command/testing.go diff --git a/command/pr_test.go b/command/pr_test.go index 88eb6f2a8..6d5bf8869 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -89,18 +89,6 @@ func TestPRView(t *testing.T) { } } -type outputStub struct { - contents []byte -} - -func (s outputStub) Output() ([]byte, error) { - return s.contents, nil -} - -func (s outputStub) Run() error { - return nil -} - func TestPRView_NoActiveBranch(t *testing.T) { initBlankContext("OWNER/REPO", "master") http := initFakeHTTP() diff --git a/command/testing.go b/command/testing.go new file mode 100644 index 000000000..3d7284d23 --- /dev/null +++ b/command/testing.go @@ -0,0 +1,14 @@ +package command + +// outputStub implements a simple utils.Runnable +type outputStub struct { + output []byte +} + +func (s outputStub) Output() ([]byte, error) { + return s.output, nil +} + +func (s outputStub) Run() error { + return nil +} From 531db42c473e4985e056647eac82a05ec2086d0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 1 Nov 2019 23:20:45 +0100 Subject: [PATCH 4/5] Log executed git commands when DEBUG is active --- utils/prepare_cmd.go | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/utils/prepare_cmd.go b/utils/prepare_cmd.go index 41cfeecd0..0129d1a5b 100644 --- a/utils/prepare_cmd.go +++ b/utils/prepare_cmd.go @@ -3,6 +3,7 @@ package utils import ( "bytes" "fmt" + "os" "os/exec" "strings" ) @@ -28,12 +29,15 @@ func SetPrepareCmd(fn func(*exec.Cmd) Runnable) func() { } } -// cmdWithStderr augments Output() by adding stderr to the error message +// cmdWithStderr augments exec.Cmd by adding stderr to the error message type cmdWithStderr struct { *exec.Cmd } func (c cmdWithStderr) Output() ([]byte, error) { + if os.Getenv("DEBUG") != "" { + fmt.Fprintf(os.Stderr, "%v\n", c.Cmd.Args) + } errStream := &bytes.Buffer{} c.Cmd.Stderr = errStream out, err := c.Cmd.Output() @@ -43,6 +47,19 @@ func (c cmdWithStderr) Output() ([]byte, error) { return out, err } +func (c cmdWithStderr) Run() error { + if os.Getenv("DEBUG") != "" { + fmt.Fprintf(os.Stderr, "%v\n", c.Cmd.Args) + } + errStream := &bytes.Buffer{} + c.Cmd.Stderr = errStream + err := c.Cmd.Run() + if err != nil { + err = &CmdError{errStream, c.Cmd.Args, err} + } + return err +} + // CmdError provides more visibility into why an exec.Cmd had failed type CmdError struct { Stderr *bytes.Buffer From 524fe0a69b7121a7a00ac5d45ff839dc47cc8355 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 6 Nov 2019 19:41:18 +0100 Subject: [PATCH 5/5] :fire: last instance of mockOpenInBrowser --- command/issue_test.go | 18 ++++++++++++++---- command/pr_test.go | 1 + command/testing.go | 18 +----------------- 3 files changed, 16 insertions(+), 21 deletions(-) diff --git a/command/issue_test.go b/command/issue_test.go index 2181fcf37..4571bc43e 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -2,10 +2,12 @@ package command import ( "os" + "os/exec" "regexp" "testing" "github.com/github/gh-cli/test" + "github.com/github/gh-cli/utils" ) func TestIssueStatus(t *testing.T) { @@ -43,8 +45,12 @@ func TestIssueView(t *testing.T) { defer jsonFile.Close() http.StubResponse(200, jsonFile) - teardown, callCount := mockOpenInBrowser() - defer teardown() + var seenCmd *exec.Cmd + restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + seenCmd = cmd + return &outputStub{} + }) + defer restoreCmd() output, err := test.RunCommand(RootCmd, "issue view 8") if err != nil { @@ -55,7 +61,11 @@ func TestIssueView(t *testing.T) { t.Errorf("command output expected got an empty string") } - if *callCount != 1 { - t.Errorf("OpenInBrowser should be called 1 time but was called %d time(s)", *callCount) + if seenCmd == nil { + t.Fatal("expected a command to run") + } + url := seenCmd.Args[len(seenCmd.Args)-1] + if url != "https://github.com/OWNER/REPO/issues/8" { + t.Errorf("got: %q", url) } } diff --git a/command/pr_test.go b/command/pr_test.go index b51a3d26c..e4a4a7048 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/github/gh-cli/test" + "github.com/github/gh-cli/utils" ) func TestPRList(t *testing.T) { diff --git a/command/testing.go b/command/testing.go index 3b4717982..2e1cf9505 100644 --- a/command/testing.go +++ b/command/testing.go @@ -3,7 +3,6 @@ package command import ( "github.com/github/gh-cli/api" "github.com/github/gh-cli/context" - "github.com/github/gh-cli/utils" ) func initBlankContext(repo, branch string) { @@ -23,21 +22,6 @@ func initFakeHTTP() *api.FakeHTTP { return http } -func mockOpenInBrowser() (func(), *int) { - callCount := 0 - originalOpenInBrowser := utils.OpenInBrowser - teardown := func() { - utils.OpenInBrowser = originalOpenInBrowser - } - - utils.OpenInBrowser = func(_ string) error { - callCount++ - return nil - } - - return teardown, &callCount -} - // outputStub implements a simple utils.Runnable type outputStub struct { output []byte @@ -49,4 +33,4 @@ func (s outputStub) Output() ([]byte, error) { func (s outputStub) Run() error { return nil -} \ No newline at end of file +}