From a8b7e4c5f3c1ac8c4a440af840974d068f7f0c4f Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Wed, 16 Oct 2019 14:47:47 -0700 Subject: [PATCH 1/6] Add PRView tests --- command/pr.go | 16 +++------ command/pr_test.go | 31 +++++++++++++++- test/fixtures/{pr.json => prList.json} | 0 test/fixtures/prView.json | 50 ++++++++++++++++++++++++++ utils/utils.go | 12 +++++-- 5 files changed, 93 insertions(+), 16 deletions(-) rename test/fixtures/{pr.json => prList.json} (100%) create mode 100644 test/fixtures/prView.json diff --git a/command/pr.go b/command/pr.go index 596d9e988..a4966f5fb 100644 --- a/command/pr.go +++ b/command/pr.go @@ -4,7 +4,6 @@ import ( "fmt" "net/url" "os" - "os/exec" "strconv" "strings" @@ -88,7 +87,9 @@ 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 := currentBranch() return fmt.Errorf("The [%s] branch has no open PRs", branch) } @@ -96,7 +97,7 @@ func prView(cmd *cobra.Command, args []string) error { } fmt.Printf("Opening %s in your browser.\n", openURL) - return openInBrowser(openURL) + return utils.OpenInBrowser(openURL) } func printPrs(prs ...api.PullRequest) { @@ -122,15 +123,6 @@ 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() -} - // The functions below should be replaced at some point by the context package // 🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨🧨 func currentBranch() string { diff --git a/command/pr_test.go b/command/pr_test.go index ced88b3b8..d4d766f17 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -4,10 +4,12 @@ import ( "regexp" "testing" + "github.com/github/gh-cli/utils" + "github.com/github/gh-cli/test" ) -func TestPRList(t *testing.T) { +func FTestPRList(t *testing.T) { teardown := test.MockGraphQLResponse("test/fixtures/pr.json") defer teardown() @@ -32,3 +34,30 @@ 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() + + openInBrowserCalls := 0 + utils.OpenInBrowser = func(_ string) error { + openInBrowserCalls++ + return nil + } + + 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 openInBrowserCalls != 1 { + t.Errorf("OpenInBrowser should be called 1 time but was called %d time(s)", openInBrowserCalls) + } +} 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/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) { From 19d02404cd70b77be772a1e42d52fefc148a953c Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Wed, 16 Oct 2019 16:25:51 -0700 Subject: [PATCH 2/6] Add PRView test --- command/pr_test.go | 64 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 57 insertions(+), 7 deletions(-) diff --git a/command/pr_test.go b/command/pr_test.go index d4d766f17..e061bad2a 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -42,11 +42,8 @@ func TestPRView(t *testing.T) { gitRepo := test.UseTempGitRepo() defer gitRepo.TearDown() - openInBrowserCalls := 0 - utils.OpenInBrowser = func(_ string) error { - openInBrowserCalls++ - return nil - } + teardown, callCount := mockOpenInBrowser() + defer teardown() output, err := test.RunCommand(RootCmd, "pr view") if err != nil { @@ -57,7 +54,60 @@ func TestPRView(t *testing.T) { t.Errorf("command output expected got an empty string") } - if openInBrowserCalls != 1 { - t.Errorf("OpenInBrowser should be called 1 time but was called %d time(s)", openInBrowserCalls) + 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 +} From 5a10f2350ffa4076fefd3418f2743588c8058cfb Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Wed, 16 Oct 2019 16:26:05 -0700 Subject: [PATCH 3/6] Don't treat no active PR as an error --- command/pr.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/command/pr.go b/command/pr.go index a4966f5fb..7777f095d 100644 --- a/command/pr.go +++ b/command/pr.go @@ -91,7 +91,8 @@ func prView(cmd *cobra.Command, args []string) error { return err } else if prPayload.CurrentPR == nil { branch := currentBranch() - 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 } From 9e97712956706c42f292607b38670bd5d8ebd9e3 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Wed, 16 Oct 2019 16:26:13 -0700 Subject: [PATCH 4/6] Add fixture --- test/fixtures/prView_NoActiveBranch.json | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 test/fixtures/prView_NoActiveBranch.json 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 } + } +} From b69dff172c14fcbe0bc1fd1b50e63803cc4e1b49 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Wed, 16 Oct 2019 16:26:32 -0700 Subject: [PATCH 5/6] Remove caching I removed this because it was making tests fail! --- git/git.go | 7 ------- 1 file changed, 7 deletions(-) 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 } From 32c9cb98927053579861d69973509e04c13802fd Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Wed, 16 Oct 2019 16:28:56 -0700 Subject: [PATCH 6/6] Great work corey --- command/pr_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/command/pr_test.go b/command/pr_test.go index e061bad2a..d35a00223 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -9,8 +9,8 @@ import ( "github.com/github/gh-cli/test" ) -func FTestPRList(t *testing.T) { - teardown := test.MockGraphQLResponse("test/fixtures/pr.json") +func TestPRList(t *testing.T) { + teardown := test.MockGraphQLResponse("test/fixtures/prList.json") defer teardown() gitRepo := test.UseTempGitRepo()