diff --git a/command/pr.go b/command/pr.go index c4377c0e0..8f35b9183 100644 --- a/command/pr.go +++ b/command/pr.go @@ -2,7 +2,6 @@ package command import ( "fmt" - "os/exec" "strconv" "github.com/github/gh-cli/api" @@ -92,18 +91,21 @@ func prView(cmd *cobra.Command, args []string) error { } } else { prPayload, err := api.PullRequests() - if err != nil || prPayload.CurrentPR == nil { + if err != nil { + return err + } else if prPayload.CurrentPR == nil { branch, err := context.Current().Branch() if err != nil { return err } - return fmt.Errorf("The [%s] branch has no open PRs", branch) + fmt.Printf("The [%s] branch has no open PRs", branch) + return nil } openURL = prPayload.CurrentPR.URL } fmt.Printf("Opening %s in your browser.\n", openURL) - return openInBrowser(openURL) + return utils.OpenInBrowser(openURL) } func printPrs(prs ...api.PullRequest) { @@ -128,12 +130,3 @@ func truncateTitle(title string) string { } return title } - -func openInBrowser(url string) error { - launcher, err := utils.BrowserLauncher() - if err != nil { - return err - } - endingArgs := append(launcher[1:], url) - return exec.Command(launcher[0], endingArgs...).Run() -} diff --git a/command/pr_test.go b/command/pr_test.go index fa1538304..10a47b5e9 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -6,6 +6,7 @@ import ( "github.com/github/gh-cli/context" "github.com/github/gh-cli/test" + "github.com/github/gh-cli/utils" ) func TestPRList(t *testing.T) { @@ -13,7 +14,7 @@ func TestPRList(t *testing.T) { ctx.SetBaseRepo("github/FAKE-GITHUB-REPO-NAME") ctx.SetBranch("master") - teardown := test.MockGraphQLResponse("test/fixtures/pr.json") + teardown := test.MockGraphQLResponse("test/fixtures/prList.json") defer teardown() output, err := test.RunCommand(RootCmd, "pr list") @@ -34,3 +35,80 @@ func TestPRList(t *testing.T) { } } } + +func TestPRView(t *testing.T) { + teardown := test.MockGraphQLResponse("test/fixtures/prView.json") + defer teardown() + + gitRepo := test.UseTempGitRepo() + defer gitRepo.TearDown() + + teardown, callCount := mockOpenInBrowser() + defer teardown() + + output, err := test.RunCommand(RootCmd, "pr view") + if err != nil { + t.Errorf("error running command `pr view`: %v", err) + } + + if output == "" { + 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) + } +} + +func TestPRView_NoActiveBranch(t *testing.T) { + teardown := test.MockGraphQLResponse("test/fixtures/prView_NoActiveBranch.json") + defer teardown() + + gitRepo := test.UseTempGitRepo() + defer gitRepo.TearDown() + + teardown, callCount := mockOpenInBrowser() + defer teardown() + + output, err := test.RunCommand(RootCmd, "pr view") + if err != nil { + t.Errorf("error running command `pr view`: %v", err) + } + + if output == "" { + t.Errorf("command output expected got an empty string") + } + + if *callCount > 0 { + t.Errorf("OpenInBrowser should NOT be called but was called %d time(s)", *callCount) + } + + // Now run again but provide a PR number + output, err = test.RunCommand(RootCmd, "pr view 23") + if err != nil { + t.Errorf("error running command `pr view`: %v", err) + } + + if output == "" { + 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) + } +} + +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/git/git.go b/git/git.go index 25a1e2b24..b9585024c 100644 --- a/git/git.go +++ b/git/git.go @@ -10,13 +10,7 @@ import ( "strings" ) -var cachedDir string - func Dir() (string, error) { - if cachedDir != "" { - return cachedDir, nil - } - dirCmd := exec.Command("git", "rev-parse", "-q", "--git-dir") dirCmd.Stderr = nil output, err := dirCmd.Output() @@ -33,7 +27,6 @@ func Dir() (string, error) { gitDir = filepath.Clean(gitDir) } - cachedDir = gitDir return gitDir, nil } diff --git a/test/fixtures/pr.json b/test/fixtures/prList.json similarity index 100% rename from test/fixtures/pr.json rename to test/fixtures/prList.json diff --git a/test/fixtures/prView.json b/test/fixtures/prView.json new file mode 100644 index 000000000..d3a7e89f2 --- /dev/null +++ b/test/fixtures/prView.json @@ -0,0 +1,50 @@ +{ + "repository": { + "pullRequests": { + "edges": [ + { + "node": { + "number": 10, + "title": "Blueberries are a good fruit", + "url": "https://github.com/github/gh-cli/pull/10", + "headRefName": "[blueberries]" + } + } + ] + } + }, + "viewerCreated": { + "edges": [ + { + "node": { + "number": 8, + "title": "Strawberries are not actually berries", + "url": "https://github.com/github/gh-cli/pull/8", + "headRefName": "[strawberries]" + } + } + ], + "pageInfo": { "hasNextPage": false } + }, + "reviewRequested": { + "edges": [ + { + "node": { + "number": 9, + "title": "Apples are tasty", + "url": "https://github.com/github/gh-cli/pull/9", + "headRefName": "[apples]" + } + }, + { + "node": { + "number": 11, + "title": "Figs are my favorite", + "url": "https://github.com/github/gh-cli/pull/1", + "headRefName": "[figs]" + } + } + ], + "pageInfo": { "hasNextPage": false } + } +} diff --git a/test/fixtures/prView_NoActiveBranch.json b/test/fixtures/prView_NoActiveBranch.json new file mode 100644 index 000000000..dd7ddbafd --- /dev/null +++ b/test/fixtures/prView_NoActiveBranch.json @@ -0,0 +1,15 @@ +{ + "repository": { + "pullRequests": { + "edges": [] + } + }, + "viewerCreated": { + "edges": [], + "pageInfo": { "hasNextPage": false } + }, + "reviewRequested": { + "edges": [], + "pageInfo": { "hasNextPage": false } + } +} diff --git a/utils/utils.go b/utils/utils.go index 4350324ab..ff1eb1993 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -27,7 +27,7 @@ func ConcatPaths(paths ...string) string { return strings.Join(paths, "/") } -func BrowserLauncher() ([]string, error) { +var OpenInBrowser = func(url string) error { browser := os.Getenv("BROWSER") if browser == "" { browser = searchBrowserLauncher(runtime.GOOS) @@ -36,10 +36,16 @@ func BrowserLauncher() ([]string, error) { } if browser == "" { - return nil, errors.New("Please set $BROWSER to a web launcher") + return errors.New("Please set $BROWSER to a web launcher") } - return shellquote.Split(browser) + browserArgs, err := shellquote.Split(browser) + if err != nil { + return err + } + + endingArgs := append(browserArgs[1:], url) + return exec.Command(browserArgs[0], endingArgs...).Run() } func searchBrowserLauncher(goos string) (browser string) {