From 4b32e3f2159bec8e7e1d1e9c514062d8fd3f5c64 Mon Sep 17 00:00:00 2001 From: rista404 Date: Thu, 23 Apr 2020 12:20:54 +0200 Subject: [PATCH 01/47] Enable interactive mode only if flags aren't passed --- command/issue.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/issue.go b/command/issue.go index 2f4e145af..4bc57e926 100644 --- a/command/issue.go +++ b/command/issue.go @@ -383,7 +383,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { action := SubmitAction - interactive := title == "" || body == "" + interactive := !cmd.Flags().Changed("title") || !cmd.Flags().Changed("body") if interactive { tb, err := titleBodySurvey(cmd, title, body, defaults{}, templateFiles) From 76c73db5a032215b0ea5f5cb7743064e7664a3a6 Mon Sep 17 00:00:00 2001 From: rista404 Date: Sat, 25 Apr 2020 23:47:07 +0200 Subject: [PATCH 02/47] Disable interactivity if any flag is passed - Error if title is an empty string --- command/issue.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/command/issue.go b/command/issue.go index 4bc57e926..67fc592b9 100644 --- a/command/issue.go +++ b/command/issue.go @@ -383,7 +383,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { action := SubmitAction - interactive := !cmd.Flags().Changed("title") || !cmd.Flags().Changed("body") + interactive := !cmd.Flags().Changed("title") && !cmd.Flags().Changed("body") if interactive { tb, err := titleBodySurvey(cmd, title, body, defaults{}, templateFiles) @@ -405,6 +405,10 @@ func issueCreate(cmd *cobra.Command, args []string) error { if body == "" { body = tb.Body } + } else { + if title == "" { + return fmt.Errorf("title can't be blank") + } } if action == PreviewAction { From c62dd557e3ae9248691b19001c55526a08547d6d Mon Sep 17 00:00:00 2001 From: Daniel Foad Date: Mon, 27 Apr 2020 21:14:00 +0100 Subject: [PATCH 03/47] Filter closed/merged PRs on default branch --- command/pr.go | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/command/pr.go b/command/pr.go index a5b1f0079..2bba161b1 100644 --- a/command/pr.go +++ b/command/pr.go @@ -73,11 +73,22 @@ func prStatus(cmd *cobra.Command, args []string) error { return err } + remotes, err := ctx.Remotes() + if err != nil { + return err + } + currentUser, err := ctx.AuthLogin() if err != nil { return err } + baseRepoOverride, _ := cmd.Flags().GetString("repo") + repoContext, err := context.ResolveRemotesToRepos(remotes, apiClient, baseRepoOverride) + if err != nil { + return err + } + baseRepo, err := determineBaseRepo(cmd, ctx) if err != nil { return err @@ -101,13 +112,20 @@ func prStatus(cmd *cobra.Command, args []string) error { fmt.Fprintln(out, "") printHeader(out, "Current branch") - if prPayload.CurrentPR != nil { - printPrs(out, 0, *prPayload.CurrentPR) + currentPR := prPayload.CurrentPR + currentBranch, _ := ctx.Branch() + remoteRepo, _ := repoContext.BaseRepo() + noPRMessage := fmt.Sprintf(" There is no pull request associated with %s", utils.Cyan("["+currentPRHeadRef+"]")) + if currentPR != nil { + if remoteRepo.DefaultBranchRef.Name == currentBranch && currentPR.State != "OPEN" { + printMessage(out, noPRMessage) + } else { + printPrs(out, 0, *currentPR) + } } else if currentPRHeadRef == "" { printMessage(out, " There is no current branch") } else { - message := fmt.Sprintf(" There is no pull request associated with %s", utils.Cyan("["+currentPRHeadRef+"]")) - printMessage(out, message) + printMessage(out, noPRMessage) } fmt.Fprintln(out) From 09157f1fc7001d5d737edadc1b0c38f3e011124f Mon Sep 17 00:00:00 2001 From: Daniel Foad Date: Mon, 27 Apr 2020 23:57:14 +0100 Subject: [PATCH 04/47] unit tests --- api/fake_http.go | 19 ++++++++++++ command/pr.go | 8 ++--- command/pr_test.go | 73 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 4 deletions(-) diff --git a/api/fake_http.go b/api/fake_http.go index a88c9df17..d8b7506a6 100644 --- a/api/fake_http.go +++ b/api/fake_http.go @@ -70,6 +70,25 @@ func (f *FakeHTTP) StubRepoResponseWithPermission(owner, repo, permission string f.responseStubs = append(f.responseStubs, resp) } +func (f *FakeHTTP) StubRepoResponseWithDefaultBranch(owner, repo, defaultBranch string) { + body := bytes.NewBufferString(fmt.Sprintf(` + { "data": { "repo_000": { + "id": "REPOID", + "name": "%s", + "owner": {"login": "%s"}, + "defaultBranchRef": { + "name": "%s" + }, + "viewerPermission": "READ" + } } } + `, repo, owner, defaultBranch)) + resp := &http.Response{ + StatusCode: 200, + Body: ioutil.NopCloser(body), + } + f.responseStubs = append(f.responseStubs, resp) +} + func (f *FakeHTTP) StubForkedRepoResponse(forkFullName, parentFullName string) { forkRepo := strings.SplitN(forkFullName, "/", 2) parentRepo := strings.SplitN(parentFullName, "/", 2) diff --git a/command/pr.go b/command/pr.go index 2bba161b1..3e7f0ccb2 100644 --- a/command/pr.go +++ b/command/pr.go @@ -83,8 +83,9 @@ func prStatus(cmd *cobra.Command, args []string) error { return err } - baseRepoOverride, _ := cmd.Flags().GetString("repo") - repoContext, err := context.ResolveRemotesToRepos(remotes, apiClient, baseRepoOverride) + repoOverride, _ := cmd.Flags().GetString("repo") + + remoteContext, err := context.ResolveRemotesToRepos(remotes, apiClient, repoOverride) if err != nil { return err } @@ -94,7 +95,6 @@ func prStatus(cmd *cobra.Command, args []string) error { return err } - repoOverride, _ := cmd.Flags().GetString("repo") currentPRNumber, currentPRHeadRef, err := prSelectorForCurrentBranch(ctx, baseRepo) if err != nil && repoOverride == "" && err.Error() != "git: not on any branch" { return fmt.Errorf("could not query for pull request for current branch: %w", err) @@ -114,7 +114,7 @@ func prStatus(cmd *cobra.Command, args []string) error { printHeader(out, "Current branch") currentPR := prPayload.CurrentPR currentBranch, _ := ctx.Branch() - remoteRepo, _ := repoContext.BaseRepo() + remoteRepo, _ := remoteContext.BaseRepo() noPRMessage := fmt.Sprintf(" There is no pull request associated with %s", utils.Cyan("["+currentPRHeadRef+"]")) if currentPR != nil { if remoteRepo.DefaultBranchRef.Name == currentBranch && currentPR.State != "OPEN" { diff --git a/command/pr_test.go b/command/pr_test.go index 074420deb..950c4af9e 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -77,6 +77,7 @@ func TestPRStatus(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") + http.StubRepoResponse("OWNER", "REPO") jsonFile, _ := os.Open("../test/fixtures/prStatus.json") defer jsonFile.Close() @@ -105,6 +106,7 @@ func TestPRStatus_fork(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() http.StubForkedRepoResponse("OWNER/REPO", "PARENT/REPO") + http.StubForkedRepoResponse("OWNER/REPO", "PARENT/REPO") jsonFile, _ := os.Open("../test/fixtures/prStatusFork.json") defer jsonFile.Close() @@ -135,6 +137,7 @@ func TestPRStatus_reviewsAndChecks(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") + http.StubRepoResponse("OWNER", "REPO") jsonFile, _ := os.Open("../test/fixtures/prStatusChecks.json") defer jsonFile.Close() @@ -162,6 +165,7 @@ func TestPRStatus_currentBranch_showTheMostRecentPR(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") + http.StubRepoResponse("OWNER", "REPO") jsonFile, _ := os.Open("../test/fixtures/prStatusCurrentBranch.json") defer jsonFile.Close() @@ -190,10 +194,33 @@ func TestPRStatus_currentBranch_showTheMostRecentPR(t *testing.T) { } } +func TestPRStatus_currentBranch_defaultBranch(t *testing.T) { + initBlankContext("", "OWNER/REPO", "blueberries") + http := initFakeHTTP() + http.StubRepoResponseWithDefaultBranch("OWNER", "REPO", "blueberries") + http.StubRepoResponse("OWNER", "REPO") + + jsonFile, _ := os.Open("../test/fixtures/prStatusCurrentBranch.json") + defer jsonFile.Close() + http.StubResponse(200, jsonFile) + + output, err := RunCommand(prStatusCmd, "pr status") + if err != nil { + t.Errorf("error running command `pr status`: %v", err) + } + + expectedLine := regexp.MustCompile(`#10 Blueberries are certainly a good fruit \[blueberries\]`) + if !expectedLine.MatchString(output.String()) { + t.Errorf("output did not match regexp /%s/\n> output\n%s\n", expectedLine, output) + return + } +} + func TestPRStatus_currentBranch_Closed(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") + http.StubRepoResponse("OWNER", "REPO") jsonFile, _ := os.Open("../test/fixtures/prStatusCurrentBranchClosed.json") defer jsonFile.Close() @@ -211,10 +238,33 @@ func TestPRStatus_currentBranch_Closed(t *testing.T) { } } +func TestPRStatus_currentBranch_Closed_defaultBranch(t *testing.T) { + initBlankContext("", "OWNER/REPO", "blueberries") + http := initFakeHTTP() + http.StubRepoResponseWithDefaultBranch("OWNER", "REPO", "blueberries") + http.StubRepoResponse("OWNER", "REPO") + + jsonFile, _ := os.Open("../test/fixtures/prStatusCurrentBranchClosed.json") + defer jsonFile.Close() + http.StubResponse(200, jsonFile) + + output, err := RunCommand(prStatusCmd, "pr status") + if err != nil { + t.Errorf("error running command `pr status`: %v", err) + } + + expectedLine := regexp.MustCompile(`There is no pull request associated with \[blueberries\]`) + if !expectedLine.MatchString(output.String()) { + t.Errorf("output did not match regexp /%s/\n> output\n%s\n", expectedLine, output) + return + } +} + func TestPRStatus_currentBranch_Merged(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") + http.StubRepoResponse("OWNER", "REPO") jsonFile, _ := os.Open("../test/fixtures/prStatusCurrentBranchMerged.json") defer jsonFile.Close() @@ -232,10 +282,33 @@ func TestPRStatus_currentBranch_Merged(t *testing.T) { } } +func TestPRStatus_currentBranch_Merged_defaultBranch(t *testing.T) { + initBlankContext("", "OWNER/REPO", "blueberries") + http := initFakeHTTP() + http.StubRepoResponseWithDefaultBranch("OWNER", "REPO", "blueberries") + http.StubRepoResponse("OWNER", "REPO") + + jsonFile, _ := os.Open("../test/fixtures/prStatusCurrentBranchMerged.json") + defer jsonFile.Close() + http.StubResponse(200, jsonFile) + + output, err := RunCommand(prStatusCmd, "pr status") + if err != nil { + t.Errorf("error running command `pr status`: %v", err) + } + + expectedLine := regexp.MustCompile(`There is no pull request associated with \[blueberries\]`) + if !expectedLine.MatchString(output.String()) { + t.Errorf("output did not match regexp /%s/\n> output\n%s\n", expectedLine, output) + return + } +} + func TestPRStatus_blankSlate(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") + http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": {} } From efd2da2a5ed194f1ab18db5c9c6f9208cdf8f133 Mon Sep 17 00:00:00 2001 From: Justin Hutchings Date: Tue, 28 Apr 2020 13:01:20 -0700 Subject: [PATCH 05/47] Add CodeQL Analysis workflow --- .github/workflows/workflows/codeql.yml | 46 ++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 .github/workflows/workflows/codeql.yml diff --git a/.github/workflows/workflows/codeql.yml b/.github/workflows/workflows/codeql.yml new file mode 100644 index 000000000..75a7e5b54 --- /dev/null +++ b/.github/workflows/workflows/codeql.yml @@ -0,0 +1,46 @@ +name: "Code Scanning - Action" + +on: + push: + schedule: + - cron: '0 0 * * 0' + +jobs: + CodeQL-Build: + + strategy: + fail-fast: false + + + # CodeQL runs on ubuntu-latest, windows-latest, and macos-latest + runs-on: ubuntu-latest + + steps: + - name: Checkout repository + uses: actions/checkout@v2 + + # Initializes the CodeQL tools for scanning. + - name: Initialize CodeQL + uses: github/codeql-action/init@v1 + # Override language selection by uncommenting this and choosing your languages + # with: + # languages: go, javascript, csharp, python, cpp, java + + # Autobuild attempts to build any compiled languages (C/C++, C#, or Java). + # If this step fails, then you should remove it and run the build manually (see below). + - name: Autobuild + uses: github/codeql-action/autobuild@v1 + + # ℹ️ Command-line programs to run using the OS shell. + # πŸ“š https://git.io/JvXDl + + # ✏️ If the Autobuild fails above, remove it and uncomment the following three lines + # and modify them (or add more) to build your code if your project + # uses a compiled language + + #- run: | + # make bootstrap + # make release + + - name: Perform CodeQL Analysis + uses: github/codeql-action/analyze@v1 \ No newline at end of file From 66abe9e62e138d01c107de2f40c3d2373dc3c3b4 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Thu, 30 Apr 2020 11:24:27 -0700 Subject: [PATCH 06/47] Add pr close test --- command/pr_test.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/command/pr_test.go b/command/pr_test.go index ba0989645..a133e04f9 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -800,3 +800,28 @@ func TestPrStateTitleWithColor(t *testing.T) { }) } } + +func TestPrClose(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "pullRequest": { "number": 96 } + } } } + `)) + + http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) + + output, err := RunCommand(prCloseCmd, "pr close 96") + if err != nil { + t.Fatalf("error running command `pr close`: %v", err) + } + + r := regexp.MustCompile(`Closed pull request #96`) + + if !r.MatchString(output.Stderr()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) + } +} From f9a55f4d8ed96b08d9081407d569695ff68dcf66 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Thu, 30 Apr 2020 11:24:43 -0700 Subject: [PATCH 07/47] Add close pull request code --- api/queries_pr.go | 23 +++++++++++++++++++++++ command/pr.go | 42 ++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 0b2fd378a..6af709947 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -1,11 +1,13 @@ package api import ( + "context" "fmt" "strings" "time" "github.com/cli/cli/internal/ghrepo" + "github.com/shurcooL/githubv4" ) type PullRequestsPayload struct { @@ -20,9 +22,11 @@ type PullRequestAndTotalCount struct { } type PullRequest struct { + ID string Number int Title string State string + Closed bool URL string BaseRefName string HeadRefName string @@ -755,6 +759,25 @@ loop: return &res, nil } +func PullRequestClose(client *Client, repo ghrepo.Interface, pr PullRequest) error { + var mutation struct { + ClosePullRequest struct { + PullRequest struct { + ID githubv4.ID + } + } `graphql:"closePullRequest(input: $input)"` + } + + input := githubv4.ClosePullRequestInput{ + PullRequestID: pr.ID, + } + + v4 := githubv4.NewClient(client.http) + err := v4.Mutate(context.Background(), &mutation, input, nil) + + return err +} + func min(a, b int) int { if a < b { return a diff --git a/command/pr.go b/command/pr.go index 009548061..49e1f3c9a 100644 --- a/command/pr.go +++ b/command/pr.go @@ -21,16 +21,17 @@ func init() { RootCmd.AddCommand(prCmd) prCmd.AddCommand(prCheckoutCmd) prCmd.AddCommand(prCreateCmd) - prCmd.AddCommand(prListCmd) prCmd.AddCommand(prStatusCmd) - prCmd.AddCommand(prViewCmd) + prCmd.AddCommand(prCloseCmd) + prCmd.AddCommand(prListCmd) prListCmd.Flags().IntP("limit", "L", 30, "Maximum number of items to fetch") prListCmd.Flags().StringP("state", "s", "open", "Filter by state: {open|closed|merged|all}") prListCmd.Flags().StringP("base", "B", "", "Filter by base branch") prListCmd.Flags().StringSliceP("label", "l", nil, "Filter by label") prListCmd.Flags().StringP("assignee", "a", "", "Filter by assignee") + prCmd.AddCommand(prViewCmd) prViewCmd.Flags().BoolP("web", "w", false, "Open a pull request in the browser") } @@ -65,6 +66,11 @@ is displayed. With '--web', open the pull request in a web browser instead.`, RunE: prView, } +var prCloseCmd = &cobra.Command{ + Use: "close [{ | }]", + Short: "Close a pull request", + RunE: prClose, +} func prStatus(cmd *cobra.Command, args []string) error { ctx := contextForCommand(cmd) @@ -328,6 +334,38 @@ func prView(cmd *cobra.Command, args []string) error { } } +func prClose(cmd *cobra.Command, args []string) error { + ctx := contextForCommand(cmd) + apiClient, err := apiClientForContext(ctx) + if err != nil { + return err + } + + baseRepo, err := determineBaseRepo(cmd, ctx) + if err != nil { + return err + } + + pr, err := prFromArg(apiClient, baseRepo, args[0]) + if err != nil { + return err + } + + if pr.Closed { + fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d is already closed\n", utils.Yellow("!"), pr.Number) + return nil + } + + err = api.PullRequestClose(apiClient, baseRepo, *pr) + if err != nil { + return fmt.Errorf("API call failed:%w", err) + } + + fmt.Fprintf(colorableErr(cmd), "%s Closed pull request #%d\n", utils.Red("βœ”"), pr.Number) + + return nil +} + func printPrPreview(out io.Writer, pr *api.PullRequest) error { // Header (Title and State) fmt.Fprintln(out, utils.Bold(pr.Title)) From 19f6c69854588c154d0fe4c456befc3f070b5635 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Thu, 30 Apr 2020 11:27:11 -0700 Subject: [PATCH 08/47] Actually make it work --- api/queries_pr.go | 2 ++ command/pr.go | 2 ++ 2 files changed, 4 insertions(+) diff --git a/api/queries_pr.go b/api/queries_pr.go index 6af709947..3bb56b80f 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -348,10 +348,12 @@ func PullRequestByNumber(client *Client, repo ghrepo.Interface, number int) (*Pu query($owner: String!, $repo: String!, $pr_number: Int!) { repository(owner: $owner, name: $repo) { pullRequest(number: $pr_number) { + id url number title state + closed body author { login diff --git a/command/pr.go b/command/pr.go index 49e1f3c9a..5799867c5 100644 --- a/command/pr.go +++ b/command/pr.go @@ -351,6 +351,8 @@ func prClose(cmd *cobra.Command, args []string) error { return err } + fmt.Printf("🌭 %+v\n", pr) + if pr.Closed { fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d is already closed\n", utils.Yellow("!"), pr.Number) return nil From af6eaa8ca2ec5ee37334b95851e0d13e4fb950d6 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Thu, 30 Apr 2020 11:41:32 -0700 Subject: [PATCH 09/47] Hot dog --- command/pr.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/command/pr.go b/command/pr.go index 5799867c5..49e1f3c9a 100644 --- a/command/pr.go +++ b/command/pr.go @@ -351,8 +351,6 @@ func prClose(cmd *cobra.Command, args []string) error { return err } - fmt.Printf("🌭 %+v\n", pr) - if pr.Closed { fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d is already closed\n", utils.Yellow("!"), pr.Number) return nil From 4bf778b1365adccf8cf4dcc1a6a703177d27e9b0 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Thu, 30 Apr 2020 11:41:39 -0700 Subject: [PATCH 10/47] require one arg --- command/pr.go | 1 + 1 file changed, 1 insertion(+) diff --git a/command/pr.go b/command/pr.go index 49e1f3c9a..d6873b4d7 100644 --- a/command/pr.go +++ b/command/pr.go @@ -69,6 +69,7 @@ With '--web', open the pull request in a web browser instead.`, var prCloseCmd = &cobra.Command{ Use: "close [{ | }]", Short: "Close a pull request", + Args: cobra.ExactArgs(1), RunE: prClose, } From e93b18a9af8bf745d98968834d82f14a98dd09ff Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Thu, 30 Apr 2020 11:41:48 -0700 Subject: [PATCH 11/47] Add already open test --- command/pr_test.go | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/command/pr_test.go b/command/pr_test.go index a133e04f9..bf21d7627 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -51,7 +51,6 @@ func RunCommand(cmd *cobra.Command, args string) (*cmdOut, error) { cmd.SetOut(&outBuf) errBuf := bytes.Buffer{} cmd.SetErr(&errBuf) - // Reset flag values so they don't leak between tests // FIXME: change how we initialize Cobra commands to render this hack unnecessary cmd.Flags().VisitAll(func(f *pflag.Flag) { @@ -825,3 +824,28 @@ func TestPrClose(t *testing.T) { t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) } } + +func TestPrClose_alreadyClosed(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "pullRequest": { "number": 101, "closed": true } + } } } + `)) + + http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) + + output, err := RunCommand(prCloseCmd, "pr close 101") + if err != nil { + t.Fatalf("error running command `pr close`: %v", err) + } + + r := regexp.MustCompile(`Pull request #101 is already closed`) + + if !r.MatchString(output.Stderr()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) + } +} From 210c47572c21bfd099d4d40c17a2bce5cf914ea8 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Thu, 30 Apr 2020 12:14:40 -0700 Subject: [PATCH 12/47] Move RunCommand to testing file --- command/pr_test.go | 49 --------------------------------------------- command/testing.go | 50 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 49 deletions(-) diff --git a/command/pr_test.go b/command/pr_test.go index ba0989645..cb4805f7b 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -15,9 +15,6 @@ import ( "github.com/cli/cli/internal/run" "github.com/cli/cli/test" "github.com/google/go-cmp/cmp" - "github.com/google/shlex" - "github.com/spf13/cobra" - "github.com/spf13/pflag" ) func eq(t *testing.T, got interface{}, expected interface{}) { @@ -27,52 +24,6 @@ func eq(t *testing.T, got interface{}, expected interface{}) { } } -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 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 - // FIXME: change how we initialize Cobra commands to render this hack unnecessary - cmd.Flags().VisitAll(func(f *pflag.Flag) { - switch v := f.Value.(type) { - case pflag.SliceValue: - _ = v.Replace([]string{}) - default: - switch v.Type() { - case "bool", "string", "int": - _ = v.Set(f.DefValue) - } - } - }) - - _, err = rootCmd.ExecuteC() - cmd.SetOut(nil) - cmd.SetErr(nil) - - return &cmdOut{&outBuf, &errBuf}, err -} - func TestPRStatus(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() diff --git a/command/testing.go b/command/testing.go index b9b0ba04a..e99e8cbfb 100644 --- a/command/testing.go +++ b/command/testing.go @@ -1,12 +1,16 @@ package command import ( + "bytes" "errors" "fmt" "reflect" "github.com/AlecAivazis/survey/v2" "github.com/AlecAivazis/survey/v2/core" + "github.com/google/shlex" + "github.com/spf13/cobra" + "github.com/spf13/pflag" "github.com/cli/cli/api" "github.com/cli/cli/context" @@ -99,6 +103,52 @@ func initFakeHTTP() *api.FakeHTTP { return http } +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 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 + // FIXME: change how we initialize Cobra commands to render this hack unnecessary + cmd.Flags().VisitAll(func(f *pflag.Flag) { + switch v := f.Value.(type) { + case pflag.SliceValue: + _ = v.Replace([]string{}) + default: + switch v.Type() { + case "bool", "string", "int": + _ = v.Set(f.DefValue) + } + } + }) + + _, err = rootCmd.ExecuteC() + cmd.SetOut(nil) + cmd.SetErr(nil) + + return &cmdOut{&outBuf, &errBuf}, err +} + type errorStub struct { message string } From 3a7bde2cce2d751f60628d4d5b45cbf8f2a753b6 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Fri, 1 May 2020 10:09:30 -0700 Subject: [PATCH 13/47] RunCommand doesn't take the command to be run --- command/testing.go | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/command/testing.go b/command/testing.go index e99e8cbfb..24f2e8b60 100644 --- a/command/testing.go +++ b/command/testing.go @@ -8,13 +8,11 @@ import ( "github.com/AlecAivazis/survey/v2" "github.com/AlecAivazis/survey/v2/core" - "github.com/google/shlex" - "github.com/spf13/cobra" - "github.com/spf13/pflag" - "github.com/cli/cli/api" "github.com/cli/cli/context" "github.com/cli/cli/internal/config" + "github.com/google/shlex" + "github.com/spf13/pflag" ) const defaultTestConfig = `hosts: @@ -115,13 +113,19 @@ 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) +func RunCommand(args string) (*cmdOut, error) { + rootCmd := RootCmd + rootArgv, err := shlex.Split(args) if err != nil { return nil, err } - rootCmd.SetArgs(argv) + + cmd, _, err := rootCmd.Traverse(rootArgv) + if err != nil { + return nil, err + } + + rootCmd.SetArgs(rootArgv) outBuf := bytes.Buffer{} cmd.SetOut(&outBuf) From 38f9e819c56762d5b25db9b7e2bfe4e009820f72 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Fri, 1 May 2020 12:00:02 -0700 Subject: [PATCH 14/47] Add test for reopen --- command/pr_test.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/command/pr_test.go b/command/pr_test.go index bf21d7627..af876bf0a 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -849,3 +849,28 @@ func TestPrClose_alreadyClosed(t *testing.T) { t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) } } + +func TestPRReopen(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "pullRequest": { "number": 666, "closed": true} + } } } + `)) + + http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) + + output, err := RunCommand(prReopenCmd, "pr reopen 666") + if err != nil { + t.Fatalf("error running command `pr reopen`: %v", err) + } + + r := regexp.MustCompile(`Reopened pull request #666`) + + if !r.MatchString(output.Stderr()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) + } +} From 0bb1d2018ab72b0ab8cbc8bf305f56880a489ecf Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Fri, 1 May 2020 12:00:13 -0700 Subject: [PATCH 15/47] Add code to reopen --- api/queries_pr.go | 19 +++++++++++++++++++ command/pr.go | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/api/queries_pr.go b/api/queries_pr.go index 3bb56b80f..3ae9ba98a 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -780,6 +780,25 @@ func PullRequestClose(client *Client, repo ghrepo.Interface, pr PullRequest) err return err } +func PullRequestReopen(client *Client, repo ghrepo.Interface, pr PullRequest) error { + var mutation struct { + ReopenPullRequest struct { + PullRequest struct { + ID githubv4.ID + } + } `graphql:"reopenPullRequest(input: $input)"` + } + + input := githubv4.ReopenPullRequestInput{ + PullRequestID: pr.ID, + } + + v4 := githubv4.NewClient(client.http) + err := v4.Mutate(context.Background(), &mutation, input, nil) + + return err +} + func min(a, b int) int { if a < b { return a diff --git a/command/pr.go b/command/pr.go index d6873b4d7..fe359f1d6 100644 --- a/command/pr.go +++ b/command/pr.go @@ -23,6 +23,7 @@ func init() { prCmd.AddCommand(prCreateCmd) prCmd.AddCommand(prStatusCmd) prCmd.AddCommand(prCloseCmd) + prCmd.AddCommand(prReopenCmd) prCmd.AddCommand(prListCmd) prListCmd.Flags().IntP("limit", "L", 30, "Maximum number of items to fetch") @@ -72,6 +73,12 @@ var prCloseCmd = &cobra.Command{ Args: cobra.ExactArgs(1), RunE: prClose, } +var prReopenCmd = &cobra.Command{ + Use: "reopen [{ | }]", + Short: "Reopen a pull request", + Args: cobra.ExactArgs(1), + RunE: prReopen, +} func prStatus(cmd *cobra.Command, args []string) error { ctx := contextForCommand(cmd) @@ -367,6 +374,38 @@ func prClose(cmd *cobra.Command, args []string) error { return nil } +func prReopen(cmd *cobra.Command, args []string) error { + ctx := contextForCommand(cmd) + apiClient, err := apiClientForContext(ctx) + if err != nil { + return err + } + + baseRepo, err := determineBaseRepo(cmd, ctx) + if err != nil { + return err + } + + pr, err := prFromArg(apiClient, baseRepo, args[0]) + if err != nil { + return err + } + + if pr.Closed == false { + fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d is already open\n", utils.Yellow("!"), pr.Number) + return nil + } + + err = api.PullRequestReopen(apiClient, baseRepo, *pr) + if err != nil { + return fmt.Errorf("API call failed:%w", err) + } + + fmt.Fprintf(colorableErr(cmd), "%s Reopened pull request #%d\n", utils.Red("βœ”"), pr.Number) + + return nil +} + func printPrPreview(out io.Writer, pr *api.PullRequest) error { // Header (Title and State) fmt.Fprintln(out, utils.Bold(pr.Title)) From bc623a7a23bc7062b25a6e7ab115947f2b2313e5 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Fri, 1 May 2020 12:07:23 -0700 Subject: [PATCH 16/47] Make it green --- command/pr.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/pr.go b/command/pr.go index fe359f1d6..5742ed158 100644 --- a/command/pr.go +++ b/command/pr.go @@ -401,7 +401,7 @@ func prReopen(cmd *cobra.Command, args []string) error { return fmt.Errorf("API call failed:%w", err) } - fmt.Fprintf(colorableErr(cmd), "%s Reopened pull request #%d\n", utils.Red("βœ”"), pr.Number) + fmt.Fprintf(colorableErr(cmd), "%s Reopened pull request #%d\n", utils.Green("βœ”"), pr.Number) return nil } From b8ff489bb5766c8c27bcf11247a94f3f2c41d3e8 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Fri, 1 May 2020 12:07:30 -0700 Subject: [PATCH 17/47] Add already open test --- command/pr_test.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/command/pr_test.go b/command/pr_test.go index af876bf0a..8585dd83f 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -874,3 +874,28 @@ func TestPRReopen(t *testing.T) { t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) } } + +func TestPRReopen_alreadyOpen(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "pullRequest": { "number": 666, "closed": false} + } } } + `)) + + http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) + + output, err := RunCommand(prReopenCmd, "pr reopen 666") + if err != nil { + t.Fatalf("error running command `pr reopen`: %v", err) + } + + r := regexp.MustCompile(`Pull request #666 is already open`) + + if !r.MatchString(output.Stderr()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) + } +} From 63b20baa1be4d2b45881eabe67b05e6680243f56 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Fri, 1 May 2020 13:58:52 -0700 Subject: [PATCH 18/47] Add merged test --- command/pr_test.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/command/pr_test.go b/command/pr_test.go index 8585dd83f..0c92f2beb 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -899,3 +899,28 @@ func TestPRReopen_alreadyOpen(t *testing.T) { t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) } } + +func TestPRReopen_merged(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "pullRequest": { "number": 666, "closed": true, "state": "MERGED"} + } } } + `)) + + http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) + + output, err := RunCommand(prReopenCmd, "pr reopen 666") + if err != nil { + t.Fatalf("error running command `pr reopen`: %v", err) + } + + r := regexp.MustCompile(`Pull request #666 can't be reopened because it was merged.`) + + if !r.MatchString(output.Stderr()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) + } +} From 922352753a9b5820503bee5b0353bacdceb43222 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Fri, 1 May 2020 14:08:30 -0700 Subject: [PATCH 19/47] Add merged exit --- command/pr.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/command/pr.go b/command/pr.go index 5742ed158..8e3bba4e3 100644 --- a/command/pr.go +++ b/command/pr.go @@ -391,6 +391,11 @@ func prReopen(cmd *cobra.Command, args []string) error { return err } + if pr.State == "MERGED" { + fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d can't be reopened because it was merged.", utils.Red("!"), pr.Number) + return nil + } + if pr.Closed == false { fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d is already open\n", utils.Yellow("!"), pr.Number) return nil From 846425fe5e834a394d4e8f5d3f660e07f8264f09 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Fri, 1 May 2020 14:16:49 -0700 Subject: [PATCH 20/47] simplified boolean comparison --- command/pr.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/command/pr.go b/command/pr.go index 8e3bba4e3..2d4b63d61 100644 --- a/command/pr.go +++ b/command/pr.go @@ -392,11 +392,11 @@ func prReopen(cmd *cobra.Command, args []string) error { } if pr.State == "MERGED" { - fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d can't be reopened because it was merged.", utils.Red("!"), pr.Number) + fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d can't be reopened because it was already merged.\n", utils.Red("!"), pr.Number) return nil } - if pr.Closed == false { + if !pr.Closed { fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d is already open\n", utils.Yellow("!"), pr.Number) return nil } From fad697cbaf145cf49597c977187c42ca124f0616 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Fri, 1 May 2020 14:18:01 -0700 Subject: [PATCH 21/47] Names --- command/pr_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/pr_test.go b/command/pr_test.go index 0c92f2beb..e89115e1a 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -900,7 +900,7 @@ func TestPRReopen_alreadyOpen(t *testing.T) { } } -func TestPRReopen_merged(t *testing.T) { +func TestPRReopen_alreadyMerged(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") From 4020084f029d02776c2ddab11971423e28acb5ad Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Fri, 1 May 2020 14:54:38 -0700 Subject: [PATCH 22/47] Fix test --- command/pr_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/command/pr_test.go b/command/pr_test.go index e89115e1a..c70a5ed8a 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -918,9 +918,10 @@ func TestPRReopen_alreadyMerged(t *testing.T) { t.Fatalf("error running command `pr reopen`: %v", err) } - r := regexp.MustCompile(`Pull request #666 can't be reopened because it was merged.`) + r := regexp.MustCompile(`Pull request #666 can't be reopened because it was already merged.`) if !r.MatchString(output.Stderr()) { t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) } + } From 1ee31db033bad37574d06dc0e56c02add0e3f8f6 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 4 May 2020 10:10:03 -0700 Subject: [PATCH 23/47] Add "merged" message --- command/pr.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/command/pr.go b/command/pr.go index 2d4b63d61..a9cbe57ed 100644 --- a/command/pr.go +++ b/command/pr.go @@ -359,7 +359,10 @@ func prClose(cmd *cobra.Command, args []string) error { return err } - if pr.Closed { + if pr.State == "MERGED" { + fmt.Fprintf(colorableErr(cmd), "%s Can't close pull request #%d because it was already merged\n", utils.Yellow("!"), pr.Number) + return nil + } else if pr.Closed { fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d is already closed\n", utils.Yellow("!"), pr.Number) return nil } From f7be93b135f447dee7e5216e4e39500e70ad4690 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 4 May 2020 10:11:24 -0700 Subject: [PATCH 24/47] use less words --- command/pr.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/pr.go b/command/pr.go index a9cbe57ed..16d1c1e93 100644 --- a/command/pr.go +++ b/command/pr.go @@ -360,7 +360,7 @@ func prClose(cmd *cobra.Command, args []string) error { } if pr.State == "MERGED" { - fmt.Fprintf(colorableErr(cmd), "%s Can't close pull request #%d because it was already merged\n", utils.Yellow("!"), pr.Number) + fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d was already merged\n", utils.Yellow("!"), pr.Number) return nil } else if pr.Closed { fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d is already closed\n", utils.Yellow("!"), pr.Number) From 083693bc051ea0c34967521fe4ae1581b2d113a3 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 4 May 2020 10:15:47 -0700 Subject: [PATCH 25/47] Update usage string --- command/pr.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/command/pr.go b/command/pr.go index 16d1c1e93..ebeb99fc2 100644 --- a/command/pr.go +++ b/command/pr.go @@ -68,13 +68,13 @@ With '--web', open the pull request in a web browser instead.`, RunE: prView, } var prCloseCmd = &cobra.Command{ - Use: "close [{ | }]", + Use: "close ", Short: "Close a pull request", Args: cobra.ExactArgs(1), RunE: prClose, } var prReopenCmd = &cobra.Command{ - Use: "reopen [{ | }]", + Use: "reopen ", Short: "Reopen a pull request", Args: cobra.ExactArgs(1), RunE: prReopen, From 1e88226b2b6bd47861d62ed657f9489e3c5bccda Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 4 May 2020 10:16:30 -0700 Subject: [PATCH 26/47] Add space --- command/pr.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/command/pr.go b/command/pr.go index ebeb99fc2..04863f3f8 100644 --- a/command/pr.go +++ b/command/pr.go @@ -369,7 +369,7 @@ func prClose(cmd *cobra.Command, args []string) error { err = api.PullRequestClose(apiClient, baseRepo, *pr) if err != nil { - return fmt.Errorf("API call failed:%w", err) + return fmt.Errorf("API call failed: %w", err) } fmt.Fprintf(colorableErr(cmd), "%s Closed pull request #%d\n", utils.Red("βœ”"), pr.Number) @@ -406,7 +406,7 @@ func prReopen(cmd *cobra.Command, args []string) error { err = api.PullRequestReopen(apiClient, baseRepo, *pr) if err != nil { - return fmt.Errorf("API call failed:%w", err) + return fmt.Errorf("API call failed: %w", err) } fmt.Fprintf(colorableErr(cmd), "%s Reopened pull request #%d\n", utils.Green("βœ”"), pr.Number) From 5cc60c669effef621a13d550b30f39f08acbb97d Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 4 May 2020 10:19:54 -0700 Subject: [PATCH 27/47] Take in a pointer --- api/queries_pr.go | 4 ++-- command/pr.go | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 3ae9ba98a..ccbe1f390 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -761,7 +761,7 @@ loop: return &res, nil } -func PullRequestClose(client *Client, repo ghrepo.Interface, pr PullRequest) error { +func PullRequestClose(client *Client, repo ghrepo.Interface, pr *PullRequest) error { var mutation struct { ClosePullRequest struct { PullRequest struct { @@ -780,7 +780,7 @@ func PullRequestClose(client *Client, repo ghrepo.Interface, pr PullRequest) err return err } -func PullRequestReopen(client *Client, repo ghrepo.Interface, pr PullRequest) error { +func PullRequestReopen(client *Client, repo ghrepo.Interface, pr *PullRequest) error { var mutation struct { ReopenPullRequest struct { PullRequest struct { diff --git a/command/pr.go b/command/pr.go index 04863f3f8..b1736169a 100644 --- a/command/pr.go +++ b/command/pr.go @@ -367,7 +367,7 @@ func prClose(cmd *cobra.Command, args []string) error { return nil } - err = api.PullRequestClose(apiClient, baseRepo, *pr) + err = api.PullRequestClose(apiClient, baseRepo, pr) if err != nil { return fmt.Errorf("API call failed: %w", err) } @@ -395,7 +395,7 @@ func prReopen(cmd *cobra.Command, args []string) error { } if pr.State == "MERGED" { - fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d can't be reopened because it was already merged.\n", utils.Red("!"), pr.Number) + fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d can't be reopened because it was already merged\n", utils.Red("!"), pr.Number) return nil } @@ -404,7 +404,7 @@ func prReopen(cmd *cobra.Command, args []string) error { return nil } - err = api.PullRequestReopen(apiClient, baseRepo, *pr) + err = api.PullRequestReopen(apiClient, baseRepo, pr) if err != nil { return fmt.Errorf("API call failed: %w", err) } From 2f075fca73495bed7849e0734cb24cb4be703ee6 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 4 May 2020 10:25:38 -0700 Subject: [PATCH 28/47] Return an error if the pr was already merged --- command/pr.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/command/pr.go b/command/pr.go index b1736169a..f57daed50 100644 --- a/command/pr.go +++ b/command/pr.go @@ -395,8 +395,8 @@ func prReopen(cmd *cobra.Command, args []string) error { } if pr.State == "MERGED" { - fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d can't be reopened because it was already merged\n", utils.Red("!"), pr.Number) - return nil + err := fmt.Errorf("%s Pull request #%d can't be reopened because it was already merged", utils.Red("!"), pr.Number) + return err } if !pr.Closed { From 3b8f4794b497a3c81a0fb05c86d8a876182b82c1 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 4 May 2020 10:31:31 -0700 Subject: [PATCH 29/47] Update test --- command/pr_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/command/pr_test.go b/command/pr_test.go index c70a5ed8a..d851eb838 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -914,13 +914,13 @@ func TestPRReopen_alreadyMerged(t *testing.T) { http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) output, err := RunCommand(prReopenCmd, "pr reopen 666") - if err != nil { - t.Fatalf("error running command `pr reopen`: %v", err) + if err == nil { + t.Fatalf("expected an error running command `pr reopen`: %v", err) } - r := regexp.MustCompile(`Pull request #666 can't be reopened because it was already merged.`) + r := regexp.MustCompile(`Pull request #666 can't be reopened because it was already merged`) - if !r.MatchString(output.Stderr()) { + if !r.MatchString(err.Error()) { t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) } From fef11b348947ac5583e3c871c9b898012970663c Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Mon, 4 May 2020 10:44:20 -0700 Subject: [PATCH 30/47] Merged prs return error --- command/pr.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/command/pr.go b/command/pr.go index f57daed50..c4b268abe 100644 --- a/command/pr.go +++ b/command/pr.go @@ -360,8 +360,8 @@ func prClose(cmd *cobra.Command, args []string) error { } if pr.State == "MERGED" { - fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d was already merged\n", utils.Yellow("!"), pr.Number) - return nil + err := fmt.Errorf("%s Pull request #%d can't be closed because it was already merged", utils.Red("!"), pr.Number) + return err } else if pr.Closed { fmt.Fprintf(colorableErr(cmd), "%s Pull request #%d is already closed\n", utils.Yellow("!"), pr.Number) return nil From e7e3a4f9b14ae0f73ea18f033dc18aedc517e92b Mon Sep 17 00:00:00 2001 From: Daniel Foad Date: Mon, 4 May 2020 23:47:47 +0100 Subject: [PATCH 31/47] pr comments --- command/pr.go | 16 ++-------------- command/pr_test.go | 10 ---------- 2 files changed, 2 insertions(+), 24 deletions(-) diff --git a/command/pr.go b/command/pr.go index 3e7f0ccb2..22f083b97 100644 --- a/command/pr.go +++ b/command/pr.go @@ -73,28 +73,17 @@ func prStatus(cmd *cobra.Command, args []string) error { return err } - remotes, err := ctx.Remotes() - if err != nil { - return err - } - currentUser, err := ctx.AuthLogin() if err != nil { return err } - repoOverride, _ := cmd.Flags().GetString("repo") - - remoteContext, err := context.ResolveRemotesToRepos(remotes, apiClient, repoOverride) - if err != nil { - return err - } - baseRepo, err := determineBaseRepo(cmd, ctx) if err != nil { return err } + repoOverride, _ := cmd.Flags().GetString("repo") currentPRNumber, currentPRHeadRef, err := prSelectorForCurrentBranch(ctx, baseRepo) if err != nil && repoOverride == "" && err.Error() != "git: not on any branch" { return fmt.Errorf("could not query for pull request for current branch: %w", err) @@ -114,10 +103,9 @@ func prStatus(cmd *cobra.Command, args []string) error { printHeader(out, "Current branch") currentPR := prPayload.CurrentPR currentBranch, _ := ctx.Branch() - remoteRepo, _ := remoteContext.BaseRepo() noPRMessage := fmt.Sprintf(" There is no pull request associated with %s", utils.Cyan("["+currentPRHeadRef+"]")) if currentPR != nil { - if remoteRepo.DefaultBranchRef.Name == currentBranch && currentPR.State != "OPEN" { + if baseRepo.(*api.Repository).DefaultBranchRef.Name == currentBranch && currentPR.State != "OPEN" { printMessage(out, noPRMessage) } else { printPrs(out, 0, *currentPR) diff --git a/command/pr_test.go b/command/pr_test.go index 950c4af9e..aa4633541 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -77,7 +77,6 @@ func TestPRStatus(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubRepoResponse("OWNER", "REPO") jsonFile, _ := os.Open("../test/fixtures/prStatus.json") defer jsonFile.Close() @@ -106,7 +105,6 @@ func TestPRStatus_fork(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() http.StubForkedRepoResponse("OWNER/REPO", "PARENT/REPO") - http.StubForkedRepoResponse("OWNER/REPO", "PARENT/REPO") jsonFile, _ := os.Open("../test/fixtures/prStatusFork.json") defer jsonFile.Close() @@ -137,7 +135,6 @@ func TestPRStatus_reviewsAndChecks(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubRepoResponse("OWNER", "REPO") jsonFile, _ := os.Open("../test/fixtures/prStatusChecks.json") defer jsonFile.Close() @@ -165,7 +162,6 @@ func TestPRStatus_currentBranch_showTheMostRecentPR(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubRepoResponse("OWNER", "REPO") jsonFile, _ := os.Open("../test/fixtures/prStatusCurrentBranch.json") defer jsonFile.Close() @@ -198,7 +194,6 @@ func TestPRStatus_currentBranch_defaultBranch(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() http.StubRepoResponseWithDefaultBranch("OWNER", "REPO", "blueberries") - http.StubRepoResponse("OWNER", "REPO") jsonFile, _ := os.Open("../test/fixtures/prStatusCurrentBranch.json") defer jsonFile.Close() @@ -220,7 +215,6 @@ func TestPRStatus_currentBranch_Closed(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubRepoResponse("OWNER", "REPO") jsonFile, _ := os.Open("../test/fixtures/prStatusCurrentBranchClosed.json") defer jsonFile.Close() @@ -242,7 +236,6 @@ func TestPRStatus_currentBranch_Closed_defaultBranch(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() http.StubRepoResponseWithDefaultBranch("OWNER", "REPO", "blueberries") - http.StubRepoResponse("OWNER", "REPO") jsonFile, _ := os.Open("../test/fixtures/prStatusCurrentBranchClosed.json") defer jsonFile.Close() @@ -264,7 +257,6 @@ func TestPRStatus_currentBranch_Merged(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubRepoResponse("OWNER", "REPO") jsonFile, _ := os.Open("../test/fixtures/prStatusCurrentBranchMerged.json") defer jsonFile.Close() @@ -286,7 +278,6 @@ func TestPRStatus_currentBranch_Merged_defaultBranch(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() http.StubRepoResponseWithDefaultBranch("OWNER", "REPO", "blueberries") - http.StubRepoResponse("OWNER", "REPO") jsonFile, _ := os.Open("../test/fixtures/prStatusCurrentBranchMerged.json") defer jsonFile.Close() @@ -308,7 +299,6 @@ func TestPRStatus_blankSlate(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": {} } From 0a86be0ba7703b4e47c3aaf378202f1fc1916d9c Mon Sep 17 00:00:00 2001 From: Justin Hutchings Date: Mon, 4 May 2020 17:49:39 -0700 Subject: [PATCH 32/47] Rename .github/workflows/workflows/codeql.yml to .github/workflows/codeql.yml --- .github/workflows/{workflows => }/codeql.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename .github/workflows/{workflows => }/codeql.yml (96%) diff --git a/.github/workflows/workflows/codeql.yml b/.github/workflows/codeql.yml similarity index 96% rename from .github/workflows/workflows/codeql.yml rename to .github/workflows/codeql.yml index 75a7e5b54..aabab068f 100644 --- a/.github/workflows/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -43,4 +43,4 @@ jobs: # make release - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@v1 \ No newline at end of file + uses: github/codeql-action/analyze@v1 From 1834d8cfd5e54ab6c427360efd9f5e7cea5c06a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 5 May 2020 13:32:42 +0200 Subject: [PATCH 33/47] Tweak syntax in `issue/pr close` docs This makes it consistent with other subcommands under `issue/pr`. --- command/issue.go | 4 ++-- command/pr.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/command/issue.go b/command/issue.go index f86ad2cd5..783c444c3 100644 --- a/command/issue.go +++ b/command/issue.go @@ -83,13 +83,13 @@ With '--web', open the issue in a web browser instead.`, RunE: issueView, } var issueCloseCmd = &cobra.Command{ - Use: "close ", + Use: "close { | }", Short: "close issue", Args: cobra.ExactArgs(1), RunE: issueClose, } var issueReopenCmd = &cobra.Command{ - Use: "reopen ", + Use: "reopen { | }", Short: "reopen issue", Args: cobra.ExactArgs(1), RunE: issueReopen, diff --git a/command/pr.go b/command/pr.go index 126d3db01..069396714 100644 --- a/command/pr.go +++ b/command/pr.go @@ -68,13 +68,13 @@ With '--web', open the pull request in a web browser instead.`, RunE: prView, } var prCloseCmd = &cobra.Command{ - Use: "close ", + Use: "close { | | }", Short: "Close a pull request", Args: cobra.ExactArgs(1), RunE: prClose, } var prReopenCmd = &cobra.Command{ - Use: "reopen ", + Use: "reopen { | | }", Short: "Reopen a pull request", Args: cobra.ExactArgs(1), RunE: prReopen, From 1f9b7c0fe09f3c8c6cc7715f462fd39f779d5c3f Mon Sep 17 00:00:00 2001 From: Nikola Ristic Date: Tue, 5 May 2020 16:34:31 +0200 Subject: [PATCH 34/47] Disable interactive only if both flags are passed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Mislav MarohniΔ‡ --- command/issue.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/issue.go b/command/issue.go index 67fc592b9..001785ddc 100644 --- a/command/issue.go +++ b/command/issue.go @@ -383,7 +383,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { action := SubmitAction - interactive := !cmd.Flags().Changed("title") && !cmd.Flags().Changed("body") + interactive := !(cmd.Flags().Changed("title") && cmd.Flags().Changed("body")) if interactive { tb, err := titleBodySurvey(cmd, title, body, defaults{}, templateFiles) From 4ef73a723defbe1ceb924039189c93b1de4c6194 Mon Sep 17 00:00:00 2001 From: Corey Johnson Date: Tue, 5 May 2020 09:20:01 -0700 Subject: [PATCH 35/47] Update all calls to RunCommand --- command/completion_test.go | 10 ++++---- command/config_test.go | 18 +++++++-------- command/issue_test.go | 46 ++++++++++++++++++------------------- command/pr_checkout_test.go | 20 ++++++++-------- command/pr_create_test.go | 26 ++++++++++----------- command/pr_test.go | 42 ++++++++++++++++----------------- command/repo_test.go | 42 ++++++++++++++++----------------- 7 files changed, 102 insertions(+), 102 deletions(-) diff --git a/command/completion_test.go b/command/completion_test.go index 3db5ce071..029330a16 100644 --- a/command/completion_test.go +++ b/command/completion_test.go @@ -6,7 +6,7 @@ import ( ) func TestCompletion_bash(t *testing.T) { - output, err := RunCommand(completionCmd, `completion`) + output, err := RunCommand(`completion`) if err != nil { t.Fatal(err) } @@ -17,7 +17,7 @@ func TestCompletion_bash(t *testing.T) { } func TestCompletion_zsh(t *testing.T) { - output, err := RunCommand(completionCmd, `completion -s zsh`) + output, err := RunCommand(`completion -s zsh`) if err != nil { t.Fatal(err) } @@ -28,7 +28,7 @@ func TestCompletion_zsh(t *testing.T) { } func TestCompletion_fish(t *testing.T) { - output, err := RunCommand(completionCmd, `completion -s fish`) + output, err := RunCommand(`completion -s fish`) if err != nil { t.Fatal(err) } @@ -39,7 +39,7 @@ func TestCompletion_fish(t *testing.T) { } func TestCompletion_powerShell(t *testing.T) { - output, err := RunCommand(completionCmd, `completion -s powershell`) + output, err := RunCommand(`completion -s powershell`) if err != nil { t.Fatal(err) } @@ -50,7 +50,7 @@ func TestCompletion_powerShell(t *testing.T) { } func TestCompletion_unsupported(t *testing.T) { - _, err := RunCommand(completionCmd, `completion -s csh`) + _, err := RunCommand(`completion -s csh`) if err == nil || err.Error() != `unsupported shell type "csh"` { t.Fatal(err) } diff --git a/command/config_test.go b/command/config_test.go index cdfeb743a..00148f3b4 100644 --- a/command/config_test.go +++ b/command/config_test.go @@ -17,7 +17,7 @@ editor: ed ` initBlankContext(cfg, "OWNER/REPO", "master") - output, err := RunCommand(configGetCmd, "config get editor") + output, err := RunCommand("config get editor") if err != nil { t.Fatalf("error running command `config get editor`: %v", err) } @@ -27,7 +27,7 @@ editor: ed func TestConfigGet_default(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") - output, err := RunCommand(configGetCmd, "config get git_protocol") + output, err := RunCommand("config get git_protocol") if err != nil { t.Fatalf("error running command `config get git_protocol`: %v", err) } @@ -38,7 +38,7 @@ func TestConfigGet_default(t *testing.T) { func TestConfigGet_not_found(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") - output, err := RunCommand(configGetCmd, "config get missing") + output, err := RunCommand("config get missing") if err != nil { t.Fatalf("error running command `config get missing`: %v", err) } @@ -51,7 +51,7 @@ func TestConfigSet(t *testing.T) { buf := bytes.NewBufferString("") defer config.StubWriteConfig(buf)() - output, err := RunCommand(configSetCmd, "config set editor ed") + output, err := RunCommand("config set editor ed") if err != nil { t.Fatalf("error running command `config set editor ed`: %v", err) } @@ -82,7 +82,7 @@ editor: ed buf := bytes.NewBufferString("") defer config.StubWriteConfig(buf)() - output, err := RunCommand(configSetCmd, "config set editor vim") + output, err := RunCommand("config set editor vim") if err != nil { t.Fatalf("error running command `config get editor`: %v", err) } @@ -110,7 +110,7 @@ git_protocol: https ` initBlankContext(cfg, "OWNER/REPO", "master") - output, err := RunCommand(configGetCmd, "config get -hgithub.com git_protocol") + output, err := RunCommand("config get -hgithub.com git_protocol") if err != nil { t.Fatalf("error running command `config get editor`: %v", err) } @@ -130,7 +130,7 @@ git_protocol: ssh ` initBlankContext(cfg, "OWNER/REPO", "master") - output, err := RunCommand(configGetCmd, "config get -hgithub.com git_protocol") + output, err := RunCommand("config get -hgithub.com git_protocol") if err != nil { t.Fatalf("error running command `config get -hgithub.com git_protocol`: %v", err) } @@ -143,7 +143,7 @@ func TestConfigSetHost(t *testing.T) { buf := bytes.NewBufferString("") defer config.StubWriteConfig(buf)() - output, err := RunCommand(configSetCmd, "config set -hgithub.com git_protocol ssh") + output, err := RunCommand("config set -hgithub.com git_protocol ssh") if err != nil { t.Fatalf("error running command `config set editor ed`: %v", err) } @@ -174,7 +174,7 @@ hosts: buf := bytes.NewBufferString("") defer config.StubWriteConfig(buf)() - output, err := RunCommand(configSetCmd, "config set -hgithub.com git_protocol https") + output, err := RunCommand("config set -hgithub.com git_protocol https") if err != nil { t.Fatalf("error running command `config get editor`: %v", err) } diff --git a/command/issue_test.go b/command/issue_test.go index e408ce3cc..f2985d428 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -24,7 +24,7 @@ func TestIssueStatus(t *testing.T) { defer jsonFile.Close() http.StubResponse(200, jsonFile) - output, err := RunCommand(issueStatusCmd, "issue status") + output, err := RunCommand("issue status") if err != nil { t.Errorf("error running command `issue status`: %v", err) } @@ -58,7 +58,7 @@ func TestIssueStatus_blankSlate(t *testing.T) { } } } `)) - output, err := RunCommand(issueStatusCmd, "issue status") + output, err := RunCommand("issue status") if err != nil { t.Errorf("error running command `issue status`: %v", err) } @@ -92,7 +92,7 @@ func TestIssueStatus_disabledIssues(t *testing.T) { } } } `)) - _, err := RunCommand(issueStatusCmd, "issue status") + _, err := RunCommand("issue status") if err == nil || err.Error() != "the 'OWNER/REPO' repository has disabled issues" { t.Errorf("error running command `issue status`: %v", err) } @@ -107,7 +107,7 @@ func TestIssueList(t *testing.T) { defer jsonFile.Close() http.StubResponse(200, jsonFile) - output, err := RunCommand(issueListCmd, "issue list") + output, err := RunCommand("issue list") if err != nil { t.Errorf("error running command `issue list`: %v", err) } @@ -143,7 +143,7 @@ func TestIssueList_withFlags(t *testing.T) { } } } `)) - output, err := RunCommand(issueListCmd, "issue list -a probablyCher -l web,bug -s open -A foo") + output, err := RunCommand("issue list -a probablyCher -l web,bug -s open -A foo") if err != nil { t.Errorf("error running command `issue list`: %v", err) } @@ -183,7 +183,7 @@ func TestIssueList_nullAssigneeLabels(t *testing.T) { } } } `)) - _, err := RunCommand(issueListCmd, "issue list") + _, err := RunCommand("issue list") if err != nil { t.Errorf("error running command `issue list`: %v", err) } @@ -211,7 +211,7 @@ func TestIssueList_disabledIssues(t *testing.T) { } } } `)) - _, err := RunCommand(issueListCmd, "issue list") + _, err := RunCommand("issue list") if err == nil || err.Error() != "the 'OWNER/REPO' repository has disabled issues" { t.Errorf("error running command `issue list`: %v", err) } @@ -236,7 +236,7 @@ func TestIssueView_web(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(issueViewCmd, "issue view -w 123") + output, err := RunCommand("issue view -w 123") if err != nil { t.Errorf("error running command `issue view`: %v", err) } @@ -270,7 +270,7 @@ func TestIssueView_web_numberArgWithHash(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(issueViewCmd, "issue view -w \"#123\"") + output, err := RunCommand("issue view -w \"#123\"") if err != nil { t.Errorf("error running command `issue view`: %v", err) } @@ -350,7 +350,7 @@ func TestIssueView_Preview(t *testing.T) { defer jsonFile.Close() http.StubResponse(200, jsonFile) - output, err := RunCommand(issueViewCmd, tc.command) + output, err := RunCommand(tc.command) if err != nil { t.Errorf("error running command `%v`: %v", tc.command, err) } @@ -379,7 +379,7 @@ func TestIssueView_web_notFound(t *testing.T) { }) defer restoreCmd() - _, err := RunCommand(issueViewCmd, "issue view -w 9999") + _, err := RunCommand("issue view -w 9999") if err == nil || err.Error() != "graphql error: 'Could not resolve to an Issue with the number of 9999.'" { t.Errorf("error running command `issue view`: %v", err) } @@ -401,7 +401,7 @@ func TestIssueView_disabledIssues(t *testing.T) { } } } `)) - _, err := RunCommand(issueViewCmd, `issue view 6666`) + _, err := RunCommand(`issue view 6666`) if err == nil || err.Error() != "the 'OWNER/REPO' repository has disabled issues" { t.Errorf("error running command `issue view`: %v", err) } @@ -426,7 +426,7 @@ func TestIssueView_web_urlArg(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(issueViewCmd, "issue view -w https://github.com/OWNER/REPO/issues/123") + output, err := RunCommand("issue view -w https://github.com/OWNER/REPO/issues/123") if err != nil { t.Errorf("error running command `issue view`: %v", err) } @@ -457,7 +457,7 @@ func TestIssueCreate(t *testing.T) { } } } } `)) - output, err := RunCommand(issueCreateCmd, `issue create -t hello -b "cash rules everything around me"`) + output, err := RunCommand(`issue create -t hello -b "cash rules everything around me"`) if err != nil { t.Errorf("error running command `issue create`: %v", err) } @@ -493,7 +493,7 @@ func TestIssueCreate_disabledIssues(t *testing.T) { } } } `)) - _, err := RunCommand(issueCreateCmd, `issue create -t heres -b johnny`) + _, err := RunCommand(`issue create -t heres -b johnny`) if err == nil || err.Error() != "the 'OWNER/REPO' repository has disabled issues" { t.Errorf("error running command `issue create`: %v", err) } @@ -511,7 +511,7 @@ func TestIssueCreate_web(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(issueCreateCmd, `issue create --web`) + output, err := RunCommand(`issue create --web`) if err != nil { t.Errorf("error running command `issue create`: %v", err) } @@ -537,7 +537,7 @@ func TestIssueCreate_webTitleBody(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(issueCreateCmd, `issue create -w -t mytitle -b mybody`) + output, err := RunCommand(`issue create -w -t mytitle -b mybody`) if err != nil { t.Errorf("error running command `issue create`: %v", err) } @@ -695,7 +695,7 @@ func TestIssueClose(t *testing.T) { http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) - output, err := RunCommand(issueCloseCmd, "issue close 13") + output, err := RunCommand("issue close 13") if err != nil { t.Fatalf("error running command `issue close`: %v", err) } @@ -721,7 +721,7 @@ func TestIssueClose_alreadyClosed(t *testing.T) { http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) - output, err := RunCommand(issueCloseCmd, "issue close 13") + output, err := RunCommand("issue close 13") if err != nil { t.Fatalf("error running command `issue close`: %v", err) } @@ -744,7 +744,7 @@ func TestIssueClose_issuesDisabled(t *testing.T) { } } } `)) - _, err := RunCommand(issueCloseCmd, "issue close 13") + _, err := RunCommand("issue close 13") if err == nil { t.Fatalf("expected error when issues are disabled") } @@ -768,7 +768,7 @@ func TestIssueReopen(t *testing.T) { http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) - output, err := RunCommand(issueReopenCmd, "issue reopen 2") + output, err := RunCommand("issue reopen 2") if err != nil { t.Fatalf("error running command `issue reopen`: %v", err) } @@ -794,7 +794,7 @@ func TestIssueReopen_alreadyOpen(t *testing.T) { http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) - output, err := RunCommand(issueReopenCmd, "issue reopen 2") + output, err := RunCommand("issue reopen 2") if err != nil { t.Fatalf("error running command `issue reopen`: %v", err) } @@ -817,7 +817,7 @@ func TestIssueReopen_issuesDisabled(t *testing.T) { } } } `)) - _, err := RunCommand(issueReopenCmd, "issue reopen 2") + _, err := RunCommand("issue reopen 2") if err == nil { t.Fatalf("expected error when issues are disabled") } diff --git a/command/pr_checkout_test.go b/command/pr_checkout_test.go index 77be4c15a..851d6ed22 100644 --- a/command/pr_checkout_test.go +++ b/command/pr_checkout_test.go @@ -55,7 +55,7 @@ func TestPRCheckout_sameRepo(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(prCheckoutCmd, `pr checkout 123`) + output, err := RunCommand(`pr checkout 123`) eq(t, err, nil) eq(t, output.String(), "") @@ -107,7 +107,7 @@ func TestPRCheckout_urlArg(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(prCheckoutCmd, `pr checkout https://github.com/OWNER/REPO/pull/123/files`) + output, err := RunCommand(`pr checkout https://github.com/OWNER/REPO/pull/123/files`) eq(t, err, nil) eq(t, output.String(), "") @@ -156,7 +156,7 @@ func TestPRCheckout_urlArg_differentBase(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(prCheckoutCmd, `pr checkout https://github.com/OTHER/POE/pull/123/files`) + output, err := RunCommand(`pr checkout https://github.com/OTHER/POE/pull/123/files`) eq(t, err, nil) eq(t, output.String(), "") @@ -219,7 +219,7 @@ func TestPRCheckout_branchArg(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(prCheckoutCmd, `pr checkout hubot:feature`) + output, err := RunCommand(`pr checkout hubot:feature`) eq(t, err, nil) eq(t, output.String(), "") @@ -269,7 +269,7 @@ func TestPRCheckout_existingBranch(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(prCheckoutCmd, `pr checkout 123`) + output, err := RunCommand(`pr checkout 123`) eq(t, err, nil) eq(t, output.String(), "") @@ -322,7 +322,7 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(prCheckoutCmd, `pr checkout 123`) + output, err := RunCommand(`pr checkout 123`) eq(t, err, nil) eq(t, output.String(), "") @@ -375,7 +375,7 @@ func TestPRCheckout_differentRepo(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(prCheckoutCmd, `pr checkout 123`) + output, err := RunCommand(`pr checkout 123`) eq(t, err, nil) eq(t, output.String(), "") @@ -428,7 +428,7 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(prCheckoutCmd, `pr checkout 123`) + output, err := RunCommand(`pr checkout 123`) eq(t, err, nil) eq(t, output.String(), "") @@ -479,7 +479,7 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(prCheckoutCmd, `pr checkout 123`) + output, err := RunCommand(`pr checkout 123`) eq(t, err, nil) eq(t, output.String(), "") @@ -530,7 +530,7 @@ func TestPRCheckout_maintainerCanModify(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(prCheckoutCmd, `pr checkout 123`) + output, err := RunCommand(`pr checkout 123`) eq(t, err, nil) eq(t, output.String(), "") diff --git a/command/pr_create_test.go b/command/pr_create_test.go index f3a0954c8..c84a8ab0c 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -39,7 +39,7 @@ func TestPRCreate(t *testing.T) { cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log cs.Stub("") // git push - output, err := RunCommand(prCreateCmd, `pr create -t "my title" -b "my body"`) + output, err := RunCommand(`pr create -t "my title" -b "my body"`) eq(t, err, nil) bodyBytes, _ := ioutil.ReadAll(http.Requests[3].Body) @@ -101,7 +101,7 @@ func TestPRCreate_withForking(t *testing.T) { cs.Stub("") // git remote add cs.Stub("") // git push - output, err := RunCommand(prCreateCmd, `pr create -t title -b body`) + output, err := RunCommand(`pr create -t title -b body`) eq(t, err, nil) eq(t, http.Requests[3].URL.Path, "/repos/OWNER/REPO/forks") @@ -132,7 +132,7 @@ func TestPRCreate_alreadyExists(t *testing.T) { cs.Stub("") // git status cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log - _, err := RunCommand(prCreateCmd, `pr create`) + _, err := RunCommand(`pr create`) if err == nil { t.Fatal("error expected, got nil") } @@ -167,7 +167,7 @@ func TestPRCreate_alreadyExistsDifferentBase(t *testing.T) { cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log cs.Stub("") // git rev-parse - _, err := RunCommand(prCreateCmd, `pr create -BanotherBase -t"cool" -b"nah"`) + _, err := RunCommand(`pr create -BanotherBase -t"cool" -b"nah"`) if err != nil { t.Errorf("got unexpected error %q", err) } @@ -192,7 +192,7 @@ func TestPRCreate_web(t *testing.T) { cs.Stub("") // git push cs.Stub("") // browser - output, err := RunCommand(prCreateCmd, `pr create --web`) + output, err := RunCommand(`pr create --web`) eq(t, err, nil) eq(t, output.String(), "") @@ -232,7 +232,7 @@ func TestPRCreate_ReportsUncommittedChanges(t *testing.T) { cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log cs.Stub("") // git push - output, err := RunCommand(prCreateCmd, `pr create -t "my title" -b "my body"`) + output, err := RunCommand(`pr create -t "my title" -b "my body"`) eq(t, err, nil) eq(t, output.String(), "https://github.com/OWNER/REPO/pull/12\n") @@ -301,7 +301,7 @@ func TestPRCreate_cross_repo_same_branch(t *testing.T) { cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log cs.Stub("") // git push - output, err := RunCommand(prCreateCmd, `pr create -t "cross repo" -b "same branch"`) + output, err := RunCommand(`pr create -t "cross repo" -b "same branch"`) eq(t, err, nil) bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) @@ -377,7 +377,7 @@ func TestPRCreate_survey_defaults_multicommit(t *testing.T) { }, }) - output, err := RunCommand(prCreateCmd, `pr create`) + output, err := RunCommand(`pr create`) eq(t, err, nil) bodyBytes, _ := ioutil.ReadAll(http.Requests[3].Body) @@ -454,7 +454,7 @@ func TestPRCreate_survey_defaults_monocommit(t *testing.T) { }, }) - output, err := RunCommand(prCreateCmd, `pr create`) + output, err := RunCommand(`pr create`) eq(t, err, nil) bodyBytes, _ := ioutil.ReadAll(http.Requests[3].Body) @@ -512,7 +512,7 @@ func TestPRCreate_survey_autofill(t *testing.T) { cs.Stub("") // git push cs.Stub("") // browser open - output, err := RunCommand(prCreateCmd, `pr create -f`) + output, err := RunCommand(`pr create -f`) eq(t, err, nil) bodyBytes, _ := ioutil.ReadAll(http.Requests[3].Body) @@ -553,7 +553,7 @@ func TestPRCreate_defaults_error_autofill(t *testing.T) { cs.Stub("") // git status cs.Stub("") // git log - _, err := RunCommand(prCreateCmd, "pr create -f") + _, err := RunCommand("pr create -f") eq(t, err.Error(), "could not compute title or body defaults: could not find any commits between origin/master and feature") } @@ -571,7 +571,7 @@ func TestPRCreate_defaults_error_web(t *testing.T) { cs.Stub("") // git status cs.Stub("") // git log - _, err := RunCommand(prCreateCmd, "pr create -w") + _, err := RunCommand("pr create -w") eq(t, err.Error(), "could not compute title or body defaults: could not find any commits between origin/master and feature") } @@ -621,7 +621,7 @@ func TestPRCreate_defaults_error_interactive(t *testing.T) { }, }) - output, err := RunCommand(prCreateCmd, `pr create`) + output, err := RunCommand(`pr create`) eq(t, err, nil) stderr := string(output.Stderr()) diff --git a/command/pr_test.go b/command/pr_test.go index cb4805f7b..a4845a1de 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -33,7 +33,7 @@ func TestPRStatus(t *testing.T) { defer jsonFile.Close() http.StubResponse(200, jsonFile) - output, err := RunCommand(prStatusCmd, "pr status") + output, err := RunCommand("pr status") if err != nil { t.Errorf("error running command `pr status`: %v", err) } @@ -71,7 +71,7 @@ branch.blueberries.merge refs/heads/blueberries`)} } })() - output, err := RunCommand(prStatusCmd, "pr status") + output, err := RunCommand("pr status") if err != nil { t.Fatalf("error running command `pr status`: %v", err) } @@ -91,7 +91,7 @@ func TestPRStatus_reviewsAndChecks(t *testing.T) { defer jsonFile.Close() http.StubResponse(200, jsonFile) - output, err := RunCommand(prStatusCmd, "pr status") + output, err := RunCommand("pr status") if err != nil { t.Errorf("error running command `pr status`: %v", err) } @@ -118,7 +118,7 @@ func TestPRStatus_currentBranch_showTheMostRecentPR(t *testing.T) { defer jsonFile.Close() http.StubResponse(200, jsonFile) - output, err := RunCommand(prStatusCmd, "pr status") + output, err := RunCommand("pr status") if err != nil { t.Errorf("error running command `pr status`: %v", err) } @@ -150,7 +150,7 @@ func TestPRStatus_currentBranch_Closed(t *testing.T) { defer jsonFile.Close() http.StubResponse(200, jsonFile) - output, err := RunCommand(prStatusCmd, "pr status") + output, err := RunCommand("pr status") if err != nil { t.Errorf("error running command `pr status`: %v", err) } @@ -171,7 +171,7 @@ func TestPRStatus_currentBranch_Merged(t *testing.T) { defer jsonFile.Close() http.StubResponse(200, jsonFile) - output, err := RunCommand(prStatusCmd, "pr status") + output, err := RunCommand("pr status") if err != nil { t.Errorf("error running command `pr status`: %v", err) } @@ -192,7 +192,7 @@ func TestPRStatus_blankSlate(t *testing.T) { { "data": {} } `)) - output, err := RunCommand(prStatusCmd, "pr status") + output, err := RunCommand("pr status") if err != nil { t.Errorf("error running command `pr status`: %v", err) } @@ -224,7 +224,7 @@ func TestPRList(t *testing.T) { defer jsonFile.Close() http.StubResponse(200, jsonFile) - output, err := RunCommand(prListCmd, "pr list") + output, err := RunCommand("pr list") if err != nil { t.Fatal(err) } @@ -247,7 +247,7 @@ func TestPRList_filtering(t *testing.T) { respBody := bytes.NewBufferString(`{ "data": {} }`) http.StubResponse(200, respBody) - output, err := RunCommand(prListCmd, `pr list -s all -l one,two -l three`) + output, err := RunCommand(`pr list -s all -l one,two -l three`) if err != nil { t.Fatal(err) } @@ -280,7 +280,7 @@ func TestPRList_filteringRemoveDuplicate(t *testing.T) { defer jsonFile.Close() http.StubResponse(200, jsonFile) - output, err := RunCommand(prListCmd, "pr list -l one,two") + output, err := RunCommand("pr list -l one,two") if err != nil { t.Fatal(err) } @@ -299,7 +299,7 @@ func TestPRList_filteringClosed(t *testing.T) { respBody := bytes.NewBufferString(`{ "data": {} }`) http.StubResponse(200, respBody) - _, err := RunCommand(prListCmd, `pr list -s closed`) + _, err := RunCommand(`pr list -s closed`) if err != nil { t.Fatal(err) } @@ -323,7 +323,7 @@ func TestPRList_filteringAssignee(t *testing.T) { respBody := bytes.NewBufferString(`{ "data": {} }`) http.StubResponse(200, respBody) - _, err := RunCommand(prListCmd, `pr list -s merged -l "needs tests" -a hubot -B develop`) + _, err := RunCommand(`pr list -s merged -l "needs tests" -a hubot -B develop`) if err != nil { t.Fatal(err) } @@ -347,7 +347,7 @@ func TestPRList_filteringAssigneeLabels(t *testing.T) { respBody := bytes.NewBufferString(`{ "data": {} }`) http.StubResponse(200, respBody) - _, err := RunCommand(prListCmd, `pr list -l one,two -a hubot`) + _, err := RunCommand(`pr list -l one,two -a hubot`) if err == nil && err.Error() != "multiple labels with --assignee are not supported" { t.Fatal(err) } @@ -478,7 +478,7 @@ func TestPRView_Preview(t *testing.T) { defer jsonFile.Close() http.StubResponse(200, jsonFile) - output, err := RunCommand(prViewCmd, tc.args) + output, err := RunCommand(tc.args) if err != nil { t.Errorf("error running command `%v`: %v", tc.args, err) } @@ -511,7 +511,7 @@ func TestPRView_web_currentBranch(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(prViewCmd, "pr view -w") + output, err := RunCommand("pr view -w") if err != nil { t.Errorf("error running command `pr view`: %v", err) } @@ -549,7 +549,7 @@ func TestPRView_web_noResultsForBranch(t *testing.T) { }) defer restoreCmd() - _, err := RunCommand(prViewCmd, "pr view -w") + _, err := RunCommand("pr view -w") if err == nil || err.Error() != `no open pull requests found for branch "blueberries"` { t.Errorf("error running command `pr view`: %v", err) } @@ -577,7 +577,7 @@ func TestPRView_web_numberArg(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(prViewCmd, "pr view -w 23") + output, err := RunCommand("pr view -w 23") if err != nil { t.Errorf("error running command `pr view`: %v", err) } @@ -609,7 +609,7 @@ func TestPRView_web_numberArgWithHash(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(prViewCmd, "pr view -w \"#23\"") + output, err := RunCommand("pr view -w \"#23\"") if err != nil { t.Errorf("error running command `pr view`: %v", err) } @@ -640,7 +640,7 @@ func TestPRView_web_urlArg(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(prViewCmd, "pr view -w https://github.com/OWNER/REPO/pull/23/files") + output, err := RunCommand("pr view -w https://github.com/OWNER/REPO/pull/23/files") if err != nil { t.Errorf("error running command `pr view`: %v", err) } @@ -674,7 +674,7 @@ func TestPRView_web_branchArg(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(prViewCmd, "pr view -w blueberries") + output, err := RunCommand("pr view -w blueberries") if err != nil { t.Errorf("error running command `pr view`: %v", err) } @@ -709,7 +709,7 @@ func TestPRView_web_branchWithOwnerArg(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(prViewCmd, "pr view -w hubot:blueberries") + output, err := RunCommand("pr view -w hubot:blueberries") if err != nil { t.Errorf("error running command `pr view`: %v", err) } diff --git a/command/repo_test.go b/command/repo_test.go index 3e9fca954..760298231 100644 --- a/command/repo_test.go +++ b/command/repo_test.go @@ -42,7 +42,7 @@ func TestRepoFork_already_forked(t *testing.T) { http.StubRepoResponse("OWNER", "REPO") defer http.StubWithFixture(200, "forkResult.json")() - output, err := RunCommand(repoForkCmd, "repo fork --remote=false") + output, err := RunCommand("repo fork --remote=false") if err != nil { t.Errorf("got unexpected error: %v", err) } @@ -69,7 +69,7 @@ func TestRepoFork_reuseRemote(t *testing.T) { http.StubRepoResponse("OWNER", "REPO") defer http.StubWithFixture(200, "forkResult.json")() - output, err := RunCommand(repoForkCmd, "repo fork") + output, err := RunCommand("repo fork") if err != nil { t.Errorf("got unexpected error: %v", err) } @@ -97,7 +97,7 @@ func TestRepoFork_in_parent(t *testing.T) { http.StubRepoResponse("OWNER", "REPO") defer http.StubWithFixture(200, "forkResult.json")() - output, err := RunCommand(repoForkCmd, "repo fork --remote=false") + output, err := RunCommand("repo fork --remote=false") if err != nil { t.Errorf("error running command `repo fork`: %v", err) } @@ -132,7 +132,7 @@ func TestRepoFork_outside(t *testing.T) { http := initFakeHTTP() defer http.StubWithFixture(200, "forkResult.json")() - output, err := RunCommand(repoForkCmd, tt.args) + output, err := RunCommand(tt.args) if err != nil { t.Errorf("error running command `repo fork`: %v", err) } @@ -162,7 +162,7 @@ func TestRepoFork_in_parent_yes(t *testing.T) { return &test.OutputStub{} })() - output, err := RunCommand(repoForkCmd, "repo fork --remote") + output, err := RunCommand("repo fork --remote") if err != nil { t.Errorf("error running command `repo fork`: %v", err) } @@ -195,7 +195,7 @@ func TestRepoFork_outside_yes(t *testing.T) { cs.Stub("") // git clone cs.Stub("") // git remote add - output, err := RunCommand(repoForkCmd, "repo fork --clone OWNER/REPO") + output, err := RunCommand("repo fork --clone OWNER/REPO") if err != nil { t.Errorf("error running command `repo fork`: %v", err) } @@ -229,7 +229,7 @@ func TestRepoFork_outside_survey_yes(t *testing.T) { } defer func() { Confirm = oldConfirm }() - output, err := RunCommand(repoForkCmd, "repo fork OWNER/REPO") + output, err := RunCommand("repo fork OWNER/REPO") if err != nil { t.Errorf("error running command `repo fork`: %v", err) } @@ -263,7 +263,7 @@ func TestRepoFork_outside_survey_no(t *testing.T) { } defer func() { Confirm = oldConfirm }() - output, err := RunCommand(repoForkCmd, "repo fork OWNER/REPO") + output, err := RunCommand("repo fork OWNER/REPO") if err != nil { t.Errorf("error running command `repo fork`: %v", err) } @@ -300,7 +300,7 @@ func TestRepoFork_in_parent_survey_yes(t *testing.T) { } defer func() { Confirm = oldConfirm }() - output, err := RunCommand(repoForkCmd, "repo fork") + output, err := RunCommand("repo fork") if err != nil { t.Errorf("error running command `repo fork`: %v", err) } @@ -343,7 +343,7 @@ func TestRepoFork_in_parent_survey_no(t *testing.T) { } defer func() { Confirm = oldConfirm }() - output, err := RunCommand(repoForkCmd, "repo fork") + output, err := RunCommand("repo fork") if err != nil { t.Errorf("error running command `repo fork`: %v", err) } @@ -469,7 +469,7 @@ func TestRepoClone(t *testing.T) { cs.Stub("") // git clone - output, err := RunCommand(repoCloneCmd, tt.args) + output, err := RunCommand(tt.args) if err != nil { t.Fatalf("error running command `repo clone`: %v", err) } @@ -499,7 +499,7 @@ func TestRepoClone_hasParent(t *testing.T) { cs.Stub("") // git clone cs.Stub("") // git remote add - _, err := RunCommand(repoCloneCmd, "repo clone OWNER/REPO") + _, err := RunCommand("repo clone OWNER/REPO") if err != nil { t.Fatalf("error running command `repo clone`: %v", err) } @@ -536,7 +536,7 @@ func TestRepoCreate(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(repoCreateCmd, "repo create REPO") + output, err := RunCommand("repo create REPO") if err != nil { t.Errorf("error running command `repo create`: %v", err) } @@ -605,7 +605,7 @@ func TestRepoCreate_org(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(repoCreateCmd, "repo create ORG/REPO") + output, err := RunCommand("repo create ORG/REPO") if err != nil { t.Errorf("error running command `repo create`: %v", err) } @@ -674,7 +674,7 @@ func TestRepoCreate_orgWithTeam(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(repoCreateCmd, "repo create ORG/REPO --team monkeys") + output, err := RunCommand("repo create ORG/REPO --team monkeys") if err != nil { t.Errorf("error running command `repo create`: %v", err) } @@ -725,7 +725,7 @@ func TestRepoView_web(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(repoViewCmd, "repo view -w") + output, err := RunCommand("repo view -w") if err != nil { t.Errorf("error running command `repo view`: %v", err) } @@ -758,7 +758,7 @@ func TestRepoView_web_ownerRepo(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(repoViewCmd, "repo view -w cli/cli") + output, err := RunCommand("repo view -w cli/cli") if err != nil { t.Errorf("error running command `repo view`: %v", err) } @@ -790,7 +790,7 @@ func TestRepoView_web_fullURL(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(repoViewCmd, "repo view -w https://github.com/cli/cli") + output, err := RunCommand("repo view -w https://github.com/cli/cli") if err != nil { t.Errorf("error running command `repo view`: %v", err) } @@ -820,7 +820,7 @@ func TestRepoView(t *testing.T) { "content": "IyB0cnVseSBjb29sIHJlYWRtZSBjaGVjayBpdCBvdXQ="} `)) - output, err := RunCommand(repoViewCmd, "repo view") + output, err := RunCommand("repo view") if err != nil { t.Errorf("error running command `repo view`: %v", err) } @@ -848,7 +848,7 @@ func TestRepoView_nonmarkdown_readme(t *testing.T) { "content": "IyB0cnVseSBjb29sIHJlYWRtZSBjaGVjayBpdCBvdXQ="} `)) - output, err := RunCommand(repoViewCmd, "repo view") + output, err := RunCommand("repo view") if err != nil { t.Errorf("error running command `repo view`: %v", err) } @@ -867,7 +867,7 @@ func TestRepoView_blanks(t *testing.T) { http.StubResponse(200, bytes.NewBufferString("{}")) http.StubResponse(200, bytes.NewBufferString("{}")) - output, err := RunCommand(repoViewCmd, "repo view") + output, err := RunCommand("repo view") if err != nil { t.Errorf("error running command `repo view`: %v", err) } From e0071329f5fc74539dd56b2777a972188c7fbdb1 Mon Sep 17 00:00:00 2001 From: Justin Hutchings Date: Tue, 5 May 2020 13:45:06 -0700 Subject: [PATCH 36/47] Remove strategy, remove autobuild --- .github/workflows/codeql.yml | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index aabab068f..cc908c3d9 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -8,9 +8,6 @@ on: jobs: CodeQL-Build: - strategy: - fail-fast: false - # CodeQL runs on ubuntu-latest, windows-latest, and macos-latest runs-on: ubuntu-latest @@ -26,21 +23,5 @@ jobs: # with: # languages: go, javascript, csharp, python, cpp, java - # Autobuild attempts to build any compiled languages (C/C++, C#, or Java). - # If this step fails, then you should remove it and run the build manually (see below). - - name: Autobuild - uses: github/codeql-action/autobuild@v1 - - # ℹ️ Command-line programs to run using the OS shell. - # πŸ“š https://git.io/JvXDl - - # ✏️ If the Autobuild fails above, remove it and uncomment the following three lines - # and modify them (or add more) to build your code if your project - # uses a compiled language - - #- run: | - # make bootstrap - # make release - - name: Perform CodeQL Analysis uses: github/codeql-action/analyze@v1 From c62c00761ecc31d07f6a5cd13168e9b1488916e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 6 May 2020 10:16:08 +0200 Subject: [PATCH 37/47] Tweak CodeQL workflow --- .github/workflows/codeql.yml | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index cc908c3d9..28d17464b 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -1,27 +1,22 @@ -name: "Code Scanning - Action" +name: Code Scanning on: push: schedule: - - cron: '0 0 * * 0' + - cron: "0 0 * * 0" jobs: CodeQL-Build: - - - # CodeQL runs on ubuntu-latest, windows-latest, and macos-latest runs-on: ubuntu-latest steps: - - name: Checkout repository - uses: actions/checkout@v2 + - name: Check out code + uses: actions/checkout@v2 - # Initializes the CodeQL tools for scanning. - - name: Initialize CodeQL - uses: github/codeql-action/init@v1 - # Override language selection by uncommenting this and choosing your languages - # with: - # languages: go, javascript, csharp, python, cpp, java + - name: Initialize CodeQL + uses: github/codeql-action/init@v1 + with: + languages: go - - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@v1 + - name: Perform CodeQL Analysis + uses: github/codeql-action/analyze@v1 From 1d4065e4c48f824978fb63869dc277d9ca2337dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 6 May 2020 22:03:32 +0200 Subject: [PATCH 38/47] Add a HTTP mocking interface that matches incoming requests This adds a thread-safe RoundTripper that can be used for mocking HTTP requests in tests. Incoming requests are matched based on their contents, not the order the stubs were registered in. --- pkg/httpmock/httpmock.go | 148 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 148 insertions(+) create mode 100644 pkg/httpmock/httpmock.go diff --git a/pkg/httpmock/httpmock.go b/pkg/httpmock/httpmock.go new file mode 100644 index 000000000..9b34b9063 --- /dev/null +++ b/pkg/httpmock/httpmock.go @@ -0,0 +1,148 @@ +package httpmock + +import ( + "bytes" + "encoding/json" + "fmt" + "io" + "io/ioutil" + "net/http" + "regexp" + "strings" + "sync" +) + +type Registry struct { + mu sync.Mutex + stubs []*Stub +} + +func (r *Registry) Register(m Matcher, resp Responder) { + r.stubs = append(r.stubs, &Stub{ + Matcher: m, + Responder: resp, + }) +} + +type Testing interface { + Errorf(string, ...interface{}) +} + +func (r *Registry) Verify(t Testing) { + n := 0 + for _, s := range r.stubs { + if !s.matched { + n++ + } + } + if n > 0 { + t.Errorf("%d unmatched HTTP stubs", n) + } +} + +// RoundTrip satisfies http.RoundTripper +func (r *Registry) RoundTrip(req *http.Request) (*http.Response, error) { + var stub *Stub + + r.mu.Lock() + for _, s := range r.stubs { + if s.matched || !s.Matcher(req) { + continue + } + if stub != nil { + r.mu.Unlock() + return nil, fmt.Errorf("more than 1 stub matched %v", req) + } + stub = s + } + if stub != nil { + stub.matched = true + } + r.mu.Unlock() + + if stub == nil { + return nil, fmt.Errorf("no registered stubs matched %v", req) + } + return stub.Responder(req) +} + +type Matcher func(req *http.Request) bool +type Responder func(req *http.Request) (*http.Response, error) + +type Stub struct { + matched bool + Matcher Matcher + Responder Responder +} + +func GraphQL(q string) Matcher { + re := regexp.MustCompile(q) + + return func(req *http.Request) bool { + if !strings.EqualFold(req.Method, "POST") { + return false + } + if req.URL.Path != "/graphql" { + return false + } + + var bodyData struct { + Query string + } + _ = decodeJSONBody(req, &bodyData) + + return re.MatchString(bodyData.Query) + } +} + +func readBody(req *http.Request) ([]byte, error) { + bodyCopy := &bytes.Buffer{} + r := io.TeeReader(req.Body, bodyCopy) + req.Body = ioutil.NopCloser(bodyCopy) + return ioutil.ReadAll(r) +} + +func decodeJSONBody(req *http.Request, dest interface{}) error { + b, err := readBody(req) + if err != nil { + return err + } + return json.Unmarshal(b, dest) +} + +func StringResponse(body string) Responder { + return func(*http.Request) (*http.Response, error) { + return httpResponse(200, bytes.NewBufferString(body)), nil + } +} + +func JSONResponse(body interface{}) Responder { + return func(*http.Request) (*http.Response, error) { + b, _ := json.Marshal(body) + return httpResponse(200, bytes.NewBuffer(b)), nil + } +} + +func GraphQLMutation(body string, cb func(map[string]interface{})) Responder { + return func(req *http.Request) (*http.Response, error) { + var bodyData struct { + Variables struct { + Input map[string]interface{} + } + } + err := decodeJSONBody(req, &bodyData) + if err != nil { + return nil, err + } + cb(bodyData.Variables.Input) + + return httpResponse(200, bytes.NewBufferString(body)), nil + } +} + +func httpResponse(status int, body io.Reader) *http.Response { + return &http.Response{ + StatusCode: status, + Body: ioutil.NopCloser(body), + } +} From 256d31950a6caa3f8b7d72223176e6a2f43adb0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 6 May 2020 22:06:37 +0200 Subject: [PATCH 39/47] Migrate a single test over to httpmock to demonstrate its use --- api/queries_repo.go | 14 ++++++++++++ command/pr_create_test.go | 48 ++++++++++++++------------------------- command/testing.go | 9 ++++++++ 3 files changed, 40 insertions(+), 31 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index b8ad760d0..c415aeec7 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -221,6 +221,20 @@ func RepoNetwork(client *Client, repos []ghrepo.Interface) (RepoNetworkResult, e return result, nil } +func RepoNetworkStubResponse(owner, repo, permission string) string { + return fmt.Sprintf(` + { "data": { "repo_000": { + "id": "REPOID", + "name": "%s", + "owner": {"login": "%s"}, + "defaultBranchRef": { + "name": "master" + }, + "viewerPermission": "%s" + } } } + `, repo, owner, permission) +} + // repositoryV3 is the repository result from GitHub API v3 type repositoryV3 struct { NodeID string diff --git a/command/pr_create_test.go b/command/pr_create_test.go index c84a8ab0c..7445f9a68 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -7,8 +7,10 @@ import ( "strings" "testing" + "github.com/cli/cli/api" "github.com/cli/cli/context" "github.com/cli/cli/git" + "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/test" ) @@ -407,21 +409,28 @@ func TestPRCreate_survey_defaults_multicommit(t *testing.T) { func TestPRCreate_survey_defaults_monocommit(t *testing.T) { initBlankContext("", "OWNER/REPO", "feature") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { "forks": { "nodes": [ - ] } } } } + http := initMockHTTP() + defer http.Verify(t) + http.Register(httpmock.GraphQL(`\bviewerPermission\b`), httpmock.StringResponse(api.RepoNetworkStubResponse("OWNER", "REPO", "WRITE"))) + http.Register(httpmock.GraphQL(`\bforks\(`), httpmock.StringResponse(` + { "data": { "repository": { "forks": { "nodes": [ + ] } } } } `)) - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`\bpullRequests\(`), httpmock.StringResponse(` { "data": { "repository": { "pullRequests": { "nodes" : [ ] } } } } `)) - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`\bcreatePullRequest\(`), httpmock.GraphQLMutation(` { "data": { "createPullRequest": { "pullRequest": { "URL": "https://github.com/OWNER/REPO/pull/12" } } } } - `)) + `, func(inputs map[string]interface{}) { + eq(t, inputs["repositoryId"], "REPOID") + eq(t, inputs["title"], "the sky above the port") + eq(t, inputs["body"], "was the color of a television, turned to a dead channel") + eq(t, inputs["baseRefName"], "master") + eq(t, inputs["headRefName"], "feature") + })) cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() @@ -456,29 +465,6 @@ func TestPRCreate_survey_defaults_monocommit(t *testing.T) { output, err := RunCommand(`pr create`) eq(t, err, nil) - - bodyBytes, _ := ioutil.ReadAll(http.Requests[3].Body) - reqBody := struct { - Variables struct { - Input struct { - RepositoryID string - Title string - Body string - BaseRefName string - HeadRefName string - } - } - }{} - _ = json.Unmarshal(bodyBytes, &reqBody) - - expectedBody := "was the color of a television, turned to a dead channel" - - eq(t, reqBody.Variables.Input.RepositoryID, "REPOID") - eq(t, reqBody.Variables.Input.Title, "the sky above the port") - eq(t, reqBody.Variables.Input.Body, expectedBody) - eq(t, reqBody.Variables.Input.BaseRefName, "master") - eq(t, reqBody.Variables.Input.HeadRefName, "feature") - eq(t, output.String(), "https://github.com/OWNER/REPO/pull/12\n") } diff --git a/command/testing.go b/command/testing.go index 24f2e8b60..05b6c6117 100644 --- a/command/testing.go +++ b/command/testing.go @@ -11,6 +11,7 @@ import ( "github.com/cli/cli/api" "github.com/cli/cli/context" "github.com/cli/cli/internal/config" + "github.com/cli/cli/pkg/httpmock" "github.com/google/shlex" "github.com/spf13/pflag" ) @@ -101,6 +102,14 @@ func initFakeHTTP() *api.FakeHTTP { return http } +func initMockHTTP() *httpmock.Registry { + http := &httpmock.Registry{} + apiClientForContext = func(context.Context) (*api.Client, error) { + return api.NewClient(api.ReplaceTripper(http)), nil + } + return http +} + type cmdOut struct { outBuf, errBuf *bytes.Buffer } From 0a4d4ee0074c393b04e55c1135436c542648b40a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 7 May 2020 15:19:14 +0200 Subject: [PATCH 40/47] Replace `FakeHTTP` with httpmock which is now compatible --- api/client_test.go | 8 +- api/fake_http.go | 120 -------------------------- api/queries_issue_test.go | 3 +- api/queries_repo.go | 14 --- api/queries_repo_test.go | 4 +- command/pr_create_test.go | 5 +- command/testing.go | 10 +-- context/remote_test.go | 5 +- pkg/httpmock/legacy.go | 89 +++++++++++++++++++ pkg/httpmock/registry.go | 70 +++++++++++++++ pkg/httpmock/{httpmock.go => stub.go} | 60 +------------ update/update_test.go | 3 +- 12 files changed, 181 insertions(+), 210 deletions(-) delete mode 100644 api/fake_http.go create mode 100644 pkg/httpmock/legacy.go create mode 100644 pkg/httpmock/registry.go rename pkg/httpmock/{httpmock.go => stub.go} (66%) diff --git a/api/client_test.go b/api/client_test.go index 492609e5c..4c81bf315 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -5,6 +5,8 @@ import ( "io/ioutil" "reflect" "testing" + + "github.com/cli/cli/pkg/httpmock" ) func eq(t *testing.T, got interface{}, expected interface{}) { @@ -15,7 +17,7 @@ func eq(t *testing.T, got interface{}, expected interface{}) { } func TestGraphQL(t *testing.T) { - http := &FakeHTTP{} + http := &httpmock.Registry{} client := NewClient( ReplaceTripper(http), AddHeader("Authorization", "token OTOKEN"), @@ -40,7 +42,7 @@ func TestGraphQL(t *testing.T) { } func TestGraphQLError(t *testing.T) { - http := &FakeHTTP{} + http := &httpmock.Registry{} client := NewClient(ReplaceTripper(http)) response := struct{}{} @@ -52,7 +54,7 @@ func TestGraphQLError(t *testing.T) { } func TestRESTGetDelete(t *testing.T) { - http := &FakeHTTP{} + http := &httpmock.Registry{} client := NewClient( ReplaceTripper(http), diff --git a/api/fake_http.go b/api/fake_http.go deleted file mode 100644 index d8b7506a6..000000000 --- a/api/fake_http.go +++ /dev/null @@ -1,120 +0,0 @@ -package api - -import ( - "bytes" - "fmt" - "io" - "io/ioutil" - "net/http" - "os" - "path" - "strings" -) - -// FakeHTTP provides a mechanism by which to stub HTTP responses through -type FakeHTTP struct { - // Requests stores references to sequential requests that RoundTrip has received - Requests []*http.Request - count int - responseStubs []*http.Response -} - -// StubResponse pre-records an HTTP response -func (f *FakeHTTP) StubResponse(status int, body io.Reader) { - resp := &http.Response{ - StatusCode: status, - Body: ioutil.NopCloser(body), - } - f.responseStubs = append(f.responseStubs, resp) -} - -// RoundTrip satisfies http.RoundTripper -func (f *FakeHTTP) RoundTrip(req *http.Request) (*http.Response, error) { - if len(f.responseStubs) <= f.count { - return nil, fmt.Errorf("FakeHTTP: missing response stub for request %d", f.count) - } - resp := f.responseStubs[f.count] - f.count++ - resp.Request = req - f.Requests = append(f.Requests, req) - return resp, nil -} - -func (f *FakeHTTP) StubWithFixture(status int, fixtureFileName string) func() { - fixturePath := path.Join("../test/fixtures/", fixtureFileName) - fixtureFile, _ := os.Open(fixturePath) - f.StubResponse(status, fixtureFile) - return func() { fixtureFile.Close() } -} - -func (f *FakeHTTP) StubRepoResponse(owner, repo string) { - f.StubRepoResponseWithPermission(owner, repo, "WRITE") -} - -func (f *FakeHTTP) StubRepoResponseWithPermission(owner, repo, permission string) { - body := bytes.NewBufferString(fmt.Sprintf(` - { "data": { "repo_000": { - "id": "REPOID", - "name": "%s", - "owner": {"login": "%s"}, - "defaultBranchRef": { - "name": "master" - }, - "viewerPermission": "%s" - } } } - `, repo, owner, permission)) - resp := &http.Response{ - StatusCode: 200, - Body: ioutil.NopCloser(body), - } - f.responseStubs = append(f.responseStubs, resp) -} - -func (f *FakeHTTP) StubRepoResponseWithDefaultBranch(owner, repo, defaultBranch string) { - body := bytes.NewBufferString(fmt.Sprintf(` - { "data": { "repo_000": { - "id": "REPOID", - "name": "%s", - "owner": {"login": "%s"}, - "defaultBranchRef": { - "name": "%s" - }, - "viewerPermission": "READ" - } } } - `, repo, owner, defaultBranch)) - resp := &http.Response{ - StatusCode: 200, - Body: ioutil.NopCloser(body), - } - f.responseStubs = append(f.responseStubs, resp) -} - -func (f *FakeHTTP) StubForkedRepoResponse(forkFullName, parentFullName string) { - forkRepo := strings.SplitN(forkFullName, "/", 2) - parentRepo := strings.SplitN(parentFullName, "/", 2) - body := bytes.NewBufferString(fmt.Sprintf(` - { "data": { "repo_000": { - "id": "REPOID2", - "name": "%s", - "owner": {"login": "%s"}, - "defaultBranchRef": { - "name": "master" - }, - "viewerPermission": "ADMIN", - "parent": { - "id": "REPOID1", - "name": "%s", - "owner": {"login": "%s"}, - "defaultBranchRef": { - "name": "master" - }, - "viewerPermission": "READ" - } - } } } - `, forkRepo[1], forkRepo[0], parentRepo[1], parentRepo[0])) - resp := &http.Response{ - StatusCode: 200, - Body: ioutil.NopCloser(body), - } - f.responseStubs = append(f.responseStubs, resp) -} diff --git a/api/queries_issue_test.go b/api/queries_issue_test.go index b2dc7c819..c8ee505cf 100644 --- a/api/queries_issue_test.go +++ b/api/queries_issue_test.go @@ -7,10 +7,11 @@ import ( "testing" "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/httpmock" ) func TestIssueList(t *testing.T) { - http := &FakeHTTP{} + http := &httpmock.Registry{} client := NewClient(ReplaceTripper(http)) http.StubResponse(200, bytes.NewBufferString(` diff --git a/api/queries_repo.go b/api/queries_repo.go index c415aeec7..b8ad760d0 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -221,20 +221,6 @@ func RepoNetwork(client *Client, repos []ghrepo.Interface) (RepoNetworkResult, e return result, nil } -func RepoNetworkStubResponse(owner, repo, permission string) string { - return fmt.Sprintf(` - { "data": { "repo_000": { - "id": "REPOID", - "name": "%s", - "owner": {"login": "%s"}, - "defaultBranchRef": { - "name": "master" - }, - "viewerPermission": "%s" - } } } - `, repo, owner, permission) -} - // repositoryV3 is the repository result from GitHub API v3 type repositoryV3 struct { NodeID string diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index 49202f628..d8a75f4ff 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -5,10 +5,12 @@ import ( "encoding/json" "io/ioutil" "testing" + + "github.com/cli/cli/pkg/httpmock" ) func Test_RepoCreate(t *testing.T) { - http := &FakeHTTP{} + http := &httpmock.Registry{} client := NewClient(ReplaceTripper(http)) http.StubResponse(200, bytes.NewBufferString(`{}`)) diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 7445f9a68..f69c5c0c6 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -7,7 +7,6 @@ import ( "strings" "testing" - "github.com/cli/cli/api" "github.com/cli/cli/context" "github.com/cli/cli/git" "github.com/cli/cli/pkg/httpmock" @@ -409,9 +408,9 @@ func TestPRCreate_survey_defaults_multicommit(t *testing.T) { func TestPRCreate_survey_defaults_monocommit(t *testing.T) { initBlankContext("", "OWNER/REPO", "feature") - http := initMockHTTP() + http := initFakeHTTP() defer http.Verify(t) - http.Register(httpmock.GraphQL(`\bviewerPermission\b`), httpmock.StringResponse(api.RepoNetworkStubResponse("OWNER", "REPO", "WRITE"))) + http.Register(httpmock.GraphQL(`\bviewerPermission\b`), httpmock.StringResponse(httpmock.RepoNetworkStubResponse("OWNER", "REPO", "master", "WRITE"))) http.Register(httpmock.GraphQL(`\bforks\(`), httpmock.StringResponse(` { "data": { "repository": { "forks": { "nodes": [ ] } } } } diff --git a/command/testing.go b/command/testing.go index 05b6c6117..06d9a90a1 100644 --- a/command/testing.go +++ b/command/testing.go @@ -94,15 +94,7 @@ func initBlankContext(cfg, repo, branch string) { } } -func initFakeHTTP() *api.FakeHTTP { - http := &api.FakeHTTP{} - apiClientForContext = func(context.Context) (*api.Client, error) { - return api.NewClient(api.ReplaceTripper(http)), nil - } - return http -} - -func initMockHTTP() *httpmock.Registry { +func initFakeHTTP() *httpmock.Registry { http := &httpmock.Registry{} apiClientForContext = func(context.Context) (*api.Client, error) { return api.NewClient(api.ReplaceTripper(http)), nil diff --git a/context/remote_test.go b/context/remote_test.go index 4b6838e23..9cf7a0adf 100644 --- a/context/remote_test.go +++ b/context/remote_test.go @@ -10,6 +10,7 @@ import ( "github.com/cli/cli/api" "github.com/cli/cli/git" "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/httpmock" ) func eq(t *testing.T, got interface{}, expected interface{}) { @@ -70,7 +71,7 @@ func Test_translateRemotes(t *testing.T) { } func Test_resolvedRemotes_triangularSetup(t *testing.T) { - http := &api.FakeHTTP{} + http := &httpmock.Registry{} apiClient := api.NewClient(api.ReplaceTripper(http)) http.StubResponse(200, bytes.NewBufferString(` @@ -137,7 +138,7 @@ func Test_resolvedRemotes_triangularSetup(t *testing.T) { } func Test_resolvedRemotes_forkLookup(t *testing.T) { - http := &api.FakeHTTP{} + http := &httpmock.Registry{} apiClient := api.NewClient(api.ReplaceTripper(http)) http.StubResponse(200, bytes.NewBufferString(` diff --git a/pkg/httpmock/legacy.go b/pkg/httpmock/legacy.go new file mode 100644 index 000000000..9474c3dd8 --- /dev/null +++ b/pkg/httpmock/legacy.go @@ -0,0 +1,89 @@ +package httpmock + +import ( + "fmt" + "io" + "net/http" + "os" + "path" + "strings" +) + +// TODO: clean up methods in this file when there are no more callers + +func (r *Registry) StubResponse(status int, body io.Reader) { + r.Register(MatchAny, func(*http.Request) (*http.Response, error) { + return httpResponse(status, body), nil + }) +} + +func (r *Registry) StubWithFixture(status int, fixtureFileName string) func() { + fixturePath := path.Join("../test/fixtures/", fixtureFileName) + fixtureFile, err := os.Open(fixturePath) + r.Register(MatchAny, func(*http.Request) (*http.Response, error) { + if err != nil { + return nil, err + } + return httpResponse(200, fixtureFile), nil + }) + return func() { + if err == nil { + fixtureFile.Close() + } + } +} + +func (r *Registry) StubRepoResponse(owner, repo string) { + r.StubRepoResponseWithPermission(owner, repo, "WRITE") +} + +func (r *Registry) StubRepoResponseWithPermission(owner, repo, permission string) { + r.Register(MatchAny, StringResponse(RepoNetworkStubResponse(owner, repo, "master", permission))) +} + +func (r *Registry) StubRepoResponseWithDefaultBranch(owner, repo, defaultBranch string) { + r.Register(MatchAny, StringResponse(RepoNetworkStubResponse(owner, repo, defaultBranch, "WRITE"))) +} + +func (r *Registry) StubForkedRepoResponse(ownRepo, parentRepo string) { + r.Register(MatchAny, StringResponse(RepoNetworkStubForkResponse(ownRepo, parentRepo))) +} + +func RepoNetworkStubResponse(owner, repo, defaultBranch, permission string) string { + return fmt.Sprintf(` + { "data": { "repo_000": { + "id": "REPOID", + "name": "%s", + "owner": {"login": "%s"}, + "defaultBranchRef": { + "name": "%s" + }, + "viewerPermission": "%s" + } } } + `, repo, owner, defaultBranch, permission) +} + +func RepoNetworkStubForkResponse(forkFullName, parentFullName string) string { + forkRepo := strings.SplitN(forkFullName, "/", 2) + parentRepo := strings.SplitN(parentFullName, "/", 2) + return fmt.Sprintf(` + { "data": { "repo_000": { + "id": "REPOID2", + "name": "%s", + "owner": {"login": "%s"}, + "defaultBranchRef": { + "name": "master" + }, + "viewerPermission": "ADMIN", + "parent": { + "id": "REPOID1", + "name": "%s", + "owner": {"login": "%s"}, + "defaultBranchRef": { + "name": "master" + }, + "viewerPermission": "READ" + } + } } } + `, forkRepo[1], forkRepo[0], parentRepo[1], parentRepo[0]) +} diff --git a/pkg/httpmock/registry.go b/pkg/httpmock/registry.go new file mode 100644 index 000000000..486d79a06 --- /dev/null +++ b/pkg/httpmock/registry.go @@ -0,0 +1,70 @@ +package httpmock + +import ( + "fmt" + "net/http" + "sync" +) + +type Registry struct { + mu sync.Mutex + stubs []*Stub + Requests []*http.Request +} + +func (r *Registry) Register(m Matcher, resp Responder) { + r.stubs = append(r.stubs, &Stub{ + Matcher: m, + Responder: resp, + }) +} + +type Testing interface { + Errorf(string, ...interface{}) +} + +func (r *Registry) Verify(t Testing) { + n := 0 + for _, s := range r.stubs { + if !s.matched { + n++ + } + } + if n > 0 { + // NOTE: stubs offer no useful reflection, so we can't print details + // about dead stubs and what they were trying to match + t.Errorf("%d unmatched HTTP stubs", n) + } +} + +// RoundTrip satisfies http.RoundTripper +func (r *Registry) RoundTrip(req *http.Request) (*http.Response, error) { + var stub *Stub + + r.mu.Lock() + for _, s := range r.stubs { + if s.matched || !s.Matcher(req) { + continue + } + // TODO: reinstate this check once the legacy layer has been cleaned up + // if stub != nil { + // r.mu.Unlock() + // return nil, fmt.Errorf("more than 1 stub matched %v", req) + // } + stub = s + break // TODO: remove + } + if stub != nil { + stub.matched = true + } + + if stub == nil { + r.mu.Unlock() + return nil, fmt.Errorf("no registered stubs matched %v", req) + } + + r.Requests = append(r.Requests, req) + r.mu.Unlock() + + return stub.Responder(req) +} diff --git a/pkg/httpmock/httpmock.go b/pkg/httpmock/stub.go similarity index 66% rename from pkg/httpmock/httpmock.go rename to pkg/httpmock/stub.go index 9b34b9063..b27ceea6c 100644 --- a/pkg/httpmock/httpmock.go +++ b/pkg/httpmock/stub.go @@ -3,69 +3,13 @@ package httpmock import ( "bytes" "encoding/json" - "fmt" "io" "io/ioutil" "net/http" "regexp" "strings" - "sync" ) -type Registry struct { - mu sync.Mutex - stubs []*Stub -} - -func (r *Registry) Register(m Matcher, resp Responder) { - r.stubs = append(r.stubs, &Stub{ - Matcher: m, - Responder: resp, - }) -} - -type Testing interface { - Errorf(string, ...interface{}) -} - -func (r *Registry) Verify(t Testing) { - n := 0 - for _, s := range r.stubs { - if !s.matched { - n++ - } - } - if n > 0 { - t.Errorf("%d unmatched HTTP stubs", n) - } -} - -// RoundTrip satisfies http.RoundTripper -func (r *Registry) RoundTrip(req *http.Request) (*http.Response, error) { - var stub *Stub - - r.mu.Lock() - for _, s := range r.stubs { - if s.matched || !s.Matcher(req) { - continue - } - if stub != nil { - r.mu.Unlock() - return nil, fmt.Errorf("more than 1 stub matched %v", req) - } - stub = s - } - if stub != nil { - stub.matched = true - } - r.mu.Unlock() - - if stub == nil { - return nil, fmt.Errorf("no registered stubs matched %v", req) - } - return stub.Responder(req) -} - type Matcher func(req *http.Request) bool type Responder func(req *http.Request) (*http.Response, error) @@ -75,6 +19,10 @@ type Stub struct { Responder Responder } +func MatchAny(*http.Request) bool { + return true +} + func GraphQL(q string) Matcher { re := regexp.MustCompile(q) diff --git a/update/update_test.go b/update/update_test.go index bd919f530..2fcb2d6ab 100644 --- a/update/update_test.go +++ b/update/update_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/cli/cli/api" + "github.com/cli/cli/pkg/httpmock" ) func TestCheckForUpdate(t *testing.T) { @@ -51,7 +52,7 @@ func TestCheckForUpdate(t *testing.T) { for _, s := range scenarios { t.Run(s.Name, func(t *testing.T) { - http := &api.FakeHTTP{} + http := &httpmock.Registry{} client := api.NewClient(api.ReplaceTripper(http)) http.StubResponse(200, bytes.NewBufferString(fmt.Sprintf(`{ "tag_name": "%s", From 8b9e7df705a22fa05e7b14ab7bb7a2a3a67a0620 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 7 May 2020 15:50:01 +0200 Subject: [PATCH 41/47] Fix parsing some issue template names YAML parsing sometimes gets sabotaged by asterisks that follow the end of frontmatter (`---`). We now scope YAML parsing to only frontmatter and we don't pass any contents that follow. --- pkg/githubtemplate/github_template.go | 5 +++-- pkg/githubtemplate/github_template_test.go | 4 +--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/pkg/githubtemplate/github_template.go b/pkg/githubtemplate/github_template.go index 3aac35554..89cceda4c 100644 --- a/pkg/githubtemplate/github_template.go +++ b/pkg/githubtemplate/github_template.go @@ -66,11 +66,12 @@ mainLoop: // ExtractName returns the name of the template from YAML front-matter func ExtractName(filePath string) string { contents, err := ioutil.ReadFile(filePath) - if err == nil && detectFrontmatter(contents)[0] == 0 { + frontmatterBoundaries := detectFrontmatter(contents) + if err == nil && frontmatterBoundaries[0] == 0 { templateData := struct { Name string }{} - if err := yaml.Unmarshal(contents, &templateData); err == nil && templateData.Name != "" { + if err := yaml.Unmarshal(contents[0:frontmatterBoundaries[1]], &templateData); err == nil && templateData.Name != "" { return templateData.Name } } diff --git a/pkg/githubtemplate/github_template_test.go b/pkg/githubtemplate/github_template_test.go index fa757873c..839e215b3 100644 --- a/pkg/githubtemplate/github_template_test.go +++ b/pkg/githubtemplate/github_template_test.go @@ -148,9 +148,7 @@ name: Bug Report about: This is how you report bugs --- -Template contents ---- -More of template +**Template contents** `, args: args{ filePath: tmpfile.Name(), From d81e62270071e9c6bbd15b3e73e131c031a64a0a Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 29 Apr 2020 13:42:35 -0500 Subject: [PATCH 42/47] lightest weight reviewing this commit add very basic non-interactive PR reviewing. You can either review the "current" or a passed PR (number or URL) as approved, changes requested, or commented via CLI flags. --- api/queries_pr.go | 44 +++++++- command/pr_review.go | 128 ++++++++++++++++++++++ command/pr_review_test.go | 224 ++++++++++++++++++++++++++++++++++++++ command/testing.go | 1 + 4 files changed, 396 insertions(+), 1 deletion(-) create mode 100644 command/pr_review.go create mode 100644 command/pr_review_test.go diff --git a/api/queries_pr.go b/api/queries_pr.go index ccbe1f390..26731c144 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -6,10 +6,24 @@ import ( "strings" "time" - "github.com/cli/cli/internal/ghrepo" "github.com/shurcooL/githubv4" + + "github.com/cli/cli/internal/ghrepo" ) +type PullRequestReviewState int + +const ( + ReviewApprove PullRequestReviewState = iota + ReviewRequestChanges + ReviewComment +) + +type PullRequestReviewInput struct { + Body string + State PullRequestReviewState +} + type PullRequestsPayload struct { ViewerCreated PullRequestAndTotalCount ReviewRequested PullRequestAndTotalCount @@ -593,6 +607,34 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter return &result.CreatePullRequest.PullRequest, nil } +func AddReview(client *Client, pr *PullRequest, input *PullRequestReviewInput) error { + var mutation struct { + AddPullRequestReview struct { + ClientMutationID string + } `graphql:"addPullRequestReview(input:$input)"` + } + + state := githubv4.PullRequestReviewEventComment + switch input.State { + case ReviewApprove: + state = githubv4.PullRequestReviewEventApprove + case ReviewRequestChanges: + state = githubv4.PullRequestReviewEventRequestChanges + } + + body := githubv4.String(input.Body) + + gqlInput := githubv4.AddPullRequestReviewInput{ + PullRequestID: pr.ID, + Event: &state, + Body: &body, + } + + v4 := githubv4.NewClient(client.http) + + return v4.Mutate(context.Background(), &mutation, gqlInput, nil) +} + func PullRequestList(client *Client, vars map[string]interface{}, limit int) (*PullRequestAndTotalCount, error) { type prBlock struct { Edges []struct { diff --git a/command/pr_review.go b/command/pr_review.go new file mode 100644 index 000000000..e445acfc8 --- /dev/null +++ b/command/pr_review.go @@ -0,0 +1,128 @@ +package command + +import ( + "errors" + "fmt" + "strconv" + "strings" + + "github.com/cli/cli/api" + "github.com/spf13/cobra" +) + +func init() { + prCmd.AddCommand(prReviewCmd) + + prReviewCmd.Flags().StringP("approve", "a", "", "Approve pull request") + prReviewCmd.Flags().StringP("request-changes", "r", "", "Request changes on a pull request") + prReviewCmd.Flags().StringP("comment", "c", "", "Comment on a pull request") + + // this is required; without it pflag complains that you must pass a string to string flags. + prReviewCmd.Flags().Lookup("approve").NoOptDefVal = " " + prReviewCmd.Flags().Lookup("request-changes").NoOptDefVal = " " + prReviewCmd.Flags().Lookup("comment").NoOptDefVal = " " +} + +var prReviewCmd = &cobra.Command{ + Use: "review", + Short: "TODO", + Args: cobra.MaximumNArgs(1), + Long: "TODO", + RunE: prReview, +} + +func processReviewOpt(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { + found := 0 + flag := "" + var state api.PullRequestReviewState + + if cmd.Flags().Changed("approve") { + found++ + flag = "approve" + state = api.ReviewApprove + } + if cmd.Flags().Changed("request-changes") { + found++ + flag = "request-changes" + state = api.ReviewRequestChanges + } + if cmd.Flags().Changed("comment") { + found++ + flag = "comment" + state = api.ReviewComment + } + + if found != 1 { + return nil, errors.New("need exactly one of approve, request-changes, or comment") + } + + val, err := cmd.Flags().GetString(flag) + if err != nil { + return nil, err + } + + body := "" + if val != " " { + body = val + } + + if flag == "comment" && (body == " " || len(body) == 0) { + return nil, errors.New("cannot leave blank comment") + } + + return &api.PullRequestReviewInput{ + Body: body, + State: state, + }, nil +} + +func prReview(cmd *cobra.Command, args []string) error { + ctx := contextForCommand(cmd) + baseRepo, err := determineBaseRepo(cmd, ctx) + if err != nil { + return fmt.Errorf("could not determine base repo: %w", err) + } + + apiClient, err := apiClientForContext(ctx) + if err != nil { + return err + } + + var prNum int + + if len(args) == 0 { + prNum, _, err = prSelectorForCurrentBranch(ctx, baseRepo) + if err != nil { + return fmt.Errorf("could not query for pull request for current branch: %w", err) + } + } else { + prArg, repo := prFromURL(args[0]) + if repo != nil { + baseRepo = repo + // TODO handle malformed URL; it falls through to Atoi + } else { + prArg = strings.TrimPrefix(args[0], "#") + } + prNum, err = strconv.Atoi(prArg) + if err != nil { + return fmt.Errorf("could not parse pull request number: %w", err) + } + } + + input, err := processReviewOpt(cmd) + if err != nil { + return fmt.Errorf("did not understand desired review action: %w", err) + } + + pr, err := api.PullRequestByNumber(apiClient, baseRepo, prNum) + if err != nil { + return fmt.Errorf("could not find pull request: %w", err) + } + + err = api.AddReview(apiClient, pr, input) + if err != nil { + return fmt.Errorf("failed to create review: %w", err) + } + + return nil +} diff --git a/command/pr_review_test.go b/command/pr_review_test.go new file mode 100644 index 000000000..eb96f8dd2 --- /dev/null +++ b/command/pr_review_test.go @@ -0,0 +1,224 @@ +package command + +import ( + "bytes" + "encoding/json" + "io/ioutil" + "testing" +) + +func TestPRReview_validation(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + for _, cmd := range []string{ + `pr review 123`, + `pr review --approve --comment 123`, + `pr review --approve="cool" --comment="rad" 123`, + } { + http.StubRepoResponse("OWNER", "REPO") + _, err := RunCommand(cmd) + eq(t, err.Error(), "did not understand desired review action: need exactly one of approve, request-changes, or comment") + } +} + +func TestPRReview_url_arg(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequest": { + "id": "foobar123", + "number": 123, + "headRefName": "feature", + "headRepositoryOwner": { + "login": "hubot" + }, + "headRepository": { + "name": "REPO", + "defaultBranchRef": { + "name": "master" + } + }, + "isCrossRepository": false, + "maintainerCanModify": false + } } } } `)) + http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) + + _, err := RunCommand("pr review --approve https://github.com/OWNER/REPO/pull/123") + if err != nil { + t.Fatalf("error running pr review: %s", err) + } + + bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + reqBody := struct { + Variables struct { + Input struct { + PullRequestID string + Event string + Body string + } + } + }{} + _ = json.Unmarshal(bodyBytes, &reqBody) + + eq(t, reqBody.Variables.Input.PullRequestID, "foobar123") + eq(t, reqBody.Variables.Input.Event, "APPROVE") + eq(t, reqBody.Variables.Input.Body, "") +} + +func TestPRReview_number_arg(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequest": { + "id": "foobar123", + "number": 123, + "headRefName": "feature", + "headRepositoryOwner": { + "login": "hubot" + }, + "headRepository": { + "name": "REPO", + "defaultBranchRef": { + "name": "master" + } + }, + "isCrossRepository": false, + "maintainerCanModify": false + } } } } `)) + http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) + + _, err := RunCommand("pr review --request-changes 123") + if err != nil { + t.Fatalf("error running pr review: %s", err) + } + + bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + reqBody := struct { + Variables struct { + Input struct { + PullRequestID string + Event string + Body string + } + } + }{} + _ = json.Unmarshal(bodyBytes, &reqBody) + + eq(t, reqBody.Variables.Input.PullRequestID, "foobar123") + eq(t, reqBody.Variables.Input.Event, "REQUEST_CHANGES") + eq(t, reqBody.Variables.Input.Body, "") +} + +func TestPRReview_no_arg(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequest": { + "id": "foobar123", + "number": 123, + "headRefName": "feature", + "headRepositoryOwner": { + "login": "hubot" + }, + "headRepository": { + "name": "REPO", + "defaultBranchRef": { + "name": "master" + } + }, + "isCrossRepository": false, + "maintainerCanModify": false + } } } } `)) + http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) + + _, err := RunCommand(`pr review --comment="cool story"`) + if err != nil { + t.Fatalf("error running pr review: %s", err) + } + + bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + reqBody := struct { + Variables struct { + Input struct { + PullRequestID string + Event string + Body string + } + } + }{} + _ = json.Unmarshal(bodyBytes, &reqBody) + + eq(t, reqBody.Variables.Input.PullRequestID, "foobar123") + eq(t, reqBody.Variables.Input.Event, "COMMENT") + eq(t, reqBody.Variables.Input.Body, "cool story") +} + +func TestPRReview_blank_comment(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + + _, err := RunCommand(`pr review --comment 123`) + eq(t, err.Error(), "did not understand desired review action: cannot leave blank comment") +} + +func TestPRReview(t *testing.T) { + type c struct { + Cmd string + ExpectedEvent string + ExpectedBody string + } + cases := []c{ + c{`pr review --request-changes="bad"`, "REQUEST_CHANGES", "bad"}, + c{`pr review --request-changes`, "REQUEST_CHANGES", ""}, + c{`pr review --approve`, "APPROVE", ""}, + c{`pr review --approve="hot damn"`, "APPROVE", "hot damn"}, + c{`pr review --comment="i donno"`, "COMMENT", "i donno"}, + } + + for _, kase := range cases { + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequest": { + "id": "foobar123", + "number": 123, + "headRefName": "feature", + "headRepositoryOwner": { + "login": "hubot" + }, + "headRepository": { + "name": "REPO", + "defaultBranchRef": { + "name": "master" + } + }, + "isCrossRepository": false, + "maintainerCanModify": false + } } } } `)) + http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) + + _, err := RunCommand(kase.Cmd) + if err != nil { + t.Fatalf("got unexpected error running %s: %s", kase.Cmd, err) + } + + bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + reqBody := struct { + Variables struct { + Input struct { + Event string + Body string + } + } + }{} + _ = json.Unmarshal(bodyBytes, &reqBody) + + eq(t, reqBody.Variables.Input.Event, kase.ExpectedEvent) + eq(t, reqBody.Variables.Input.Body, kase.ExpectedBody) + } +} diff --git a/command/testing.go b/command/testing.go index 24f2e8b60..5a7f68e84 100644 --- a/command/testing.go +++ b/command/testing.go @@ -135,6 +135,7 @@ func RunCommand(args string) (*cmdOut, error) { // Reset flag values so they don't leak between tests // FIXME: change how we initialize Cobra commands to render this hack unnecessary cmd.Flags().VisitAll(func(f *pflag.Flag) { + f.Changed = false switch v := f.Value.(type) { case pflag.SliceValue: _ = v.Replace([]string{}) From 5dbec25f449be011ecf8ec8e9bd7830143aac429 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Thu, 7 May 2020 14:27:33 -0500 Subject: [PATCH 43/47] handle pr for branch case --- command/pr_review.go | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/command/pr_review.go b/command/pr_review.go index e445acfc8..9754c178b 100644 --- a/command/pr_review.go +++ b/command/pr_review.go @@ -89,9 +89,10 @@ func prReview(cmd *cobra.Command, args []string) error { } var prNum int + branchWithOwner := "" if len(args) == 0 { - prNum, _, err = prSelectorForCurrentBranch(ctx, baseRepo) + prNum, branchWithOwner, err = prSelectorForCurrentBranch(ctx, baseRepo) if err != nil { return fmt.Errorf("could not query for pull request for current branch: %w", err) } @@ -114,9 +115,17 @@ func prReview(cmd *cobra.Command, args []string) error { return fmt.Errorf("did not understand desired review action: %w", err) } - pr, err := api.PullRequestByNumber(apiClient, baseRepo, prNum) - if err != nil { - return fmt.Errorf("could not find pull request: %w", err) + var pr *api.PullRequest + if prNum > 0 { + pr, err = api.PullRequestByNumber(apiClient, baseRepo, prNum) + if err != nil { + return fmt.Errorf("could not find pull request: %w", err) + } + } else { + pr, err = api.PullRequestForBranch(apiClient, baseRepo, "", branchWithOwner) + if err != nil { + return fmt.Errorf("could not find pull request: %w", err) + } } err = api.AddReview(apiClient, pr, input) From 8a5290d2b41ead7acb072c126ba2a1e57d6fccec Mon Sep 17 00:00:00 2001 From: vilmibm Date: Thu, 7 May 2020 14:51:08 -0500 Subject: [PATCH 44/47] clarify error msg --- command/pr_review.go | 2 +- command/pr_review_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/command/pr_review.go b/command/pr_review.go index 9754c178b..1ecc7f8bc 100644 --- a/command/pr_review.go +++ b/command/pr_review.go @@ -53,7 +53,7 @@ func processReviewOpt(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { } if found != 1 { - return nil, errors.New("need exactly one of approve, request-changes, or comment") + return nil, errors.New("need exactly one of --approve, --request-changes, or --comment") } val, err := cmd.Flags().GetString(flag) diff --git a/command/pr_review_test.go b/command/pr_review_test.go index eb96f8dd2..f37a609d8 100644 --- a/command/pr_review_test.go +++ b/command/pr_review_test.go @@ -17,7 +17,7 @@ func TestPRReview_validation(t *testing.T) { } { http.StubRepoResponse("OWNER", "REPO") _, err := RunCommand(cmd) - eq(t, err.Error(), "did not understand desired review action: need exactly one of approve, request-changes, or comment") + eq(t, err.Error(), "did not understand desired review action: need exactly one of --approve, --request-changes, or --comment") } } From 46522a2add6d903bf7d044fe6f91722d14efef34 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Thu, 7 May 2020 14:51:17 -0500 Subject: [PATCH 45/47] fix current branch tests --- command/pr_review_test.go | 50 +++++++++++++-------------------------- 1 file changed, 16 insertions(+), 34 deletions(-) diff --git a/command/pr_review_test.go b/command/pr_review_test.go index f37a609d8..50580450d 100644 --- a/command/pr_review_test.go +++ b/command/pr_review_test.go @@ -112,26 +112,17 @@ func TestPRReview_number_arg(t *testing.T) { } func TestPRReview_no_arg(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") + initBlankContext("", "OWNER/REPO", "feature") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { "pullRequest": { - "id": "foobar123", - "number": 123, - "headRefName": "feature", - "headRepositoryOwner": { - "login": "hubot" - }, - "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } - }, - "isCrossRepository": false, - "maintainerCanModify": false - } } } } `)) + { "data": { "repository": { "pullRequests": { "nodes": [ + { "url": "https://github.com/OWNER/REPO/pull/123", + "id": "foobar123", + "headRefName": "feature", + "baseRefName": "master" } + ] } } } } + `)) http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) _, err := RunCommand(`pr review --comment="cool story"`) @@ -180,26 +171,17 @@ func TestPRReview(t *testing.T) { } for _, kase := range cases { - initBlankContext("", "OWNER/REPO", "master") + initBlankContext("", "OWNER/REPO", "feature") http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { "pullRequest": { - "id": "foobar123", - "number": 123, - "headRefName": "feature", - "headRepositoryOwner": { - "login": "hubot" - }, - "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } - }, - "isCrossRepository": false, - "maintainerCanModify": false - } } } } `)) + { "data": { "repository": { "pullRequests": { "nodes": [ + { "url": "https://github.com/OWNER/REPO/pull/123", + "id": "foobar123", + "headRefName": "feature", + "baseRefName": "master" } + ] } } } } + `)) http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) _, err := RunCommand(kase.Cmd) From c40f064baa6210e52b29e6d10ec8e1783e1c8583 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Thu, 7 May 2020 14:58:00 -0500 Subject: [PATCH 46/47] sigh --- api/queries_pr.go | 1 + 1 file changed, 1 insertion(+) diff --git a/api/queries_pr.go b/api/queries_pr.go index 26731c144..2a78519cc 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -470,6 +470,7 @@ func PullRequestForBranch(client *Client, repo ghrepo.Interface, baseBranch, hea repository(owner: $owner, name: $repo) { pullRequests(headRefName: $headRefName, states: OPEN, first: 30) { nodes { + id number title state From 6538cca6930d4f4b19293da7034f2cce2667f98d Mon Sep 17 00:00:00 2001 From: vilmibm Date: Fri, 8 May 2020 10:07:20 -0500 Subject: [PATCH 47/47] switch to using body flag. small TODOs --- command/pr_review.go | 41 ++++++++++++++++++--------------------- command/pr_review_test.go | 29 ++++++++++++++++----------- 2 files changed, 37 insertions(+), 33 deletions(-) diff --git a/command/pr_review.go b/command/pr_review.go index 1ecc7f8bc..bd0453784 100644 --- a/command/pr_review.go +++ b/command/pr_review.go @@ -13,22 +13,25 @@ import ( func init() { prCmd.AddCommand(prReviewCmd) - prReviewCmd.Flags().StringP("approve", "a", "", "Approve pull request") - prReviewCmd.Flags().StringP("request-changes", "r", "", "Request changes on a pull request") - prReviewCmd.Flags().StringP("comment", "c", "", "Comment on a pull request") - - // this is required; without it pflag complains that you must pass a string to string flags. - prReviewCmd.Flags().Lookup("approve").NoOptDefVal = " " - prReviewCmd.Flags().Lookup("request-changes").NoOptDefVal = " " - prReviewCmd.Flags().Lookup("comment").NoOptDefVal = " " + prReviewCmd.Flags().BoolP("approve", "a", false, "Approve pull request") + prReviewCmd.Flags().BoolP("request-changes", "r", false, "Request changes on a pull request") + prReviewCmd.Flags().BoolP("comment", "c", false, "Comment on a pull request") + prReviewCmd.Flags().StringP("body", "b", "", "Specify the body of a review") } var prReviewCmd = &cobra.Command{ - Use: "review", - Short: "TODO", + Use: "review [{ | | ]", + Short: "Add a review to a pull request.", Args: cobra.MaximumNArgs(1), - Long: "TODO", - RunE: prReview, + Long: `Add a review to either a specified pull request or the pull request associated with the current branch. + +Examples: + + gh pr review -a # mark the current branch's pull request as approved + gh pr review -c -b "interesting" # comment on the current branch's pull request + gh pr review 123 -r -b "needs more ascii art" # request changes on pull request 123 + `, + RunE: prReview, } func processReviewOpt(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { @@ -56,18 +59,13 @@ func processReviewOpt(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { return nil, errors.New("need exactly one of --approve, --request-changes, or --comment") } - val, err := cmd.Flags().GetString(flag) + body, err := cmd.Flags().GetString("body") if err != nil { return nil, err } - body := "" - if val != " " { - body = val - } - - if flag == "comment" && (body == " " || len(body) == 0) { - return nil, errors.New("cannot leave blank comment") + if (flag == "request-changes" || flag == "comment") && body == "" { + return nil, fmt.Errorf("body cannot be blank for %s review", flag) } return &api.PullRequestReviewInput{ @@ -100,13 +98,12 @@ func prReview(cmd *cobra.Command, args []string) error { prArg, repo := prFromURL(args[0]) if repo != nil { baseRepo = repo - // TODO handle malformed URL; it falls through to Atoi } else { prArg = strings.TrimPrefix(args[0], "#") } prNum, err = strconv.Atoi(prArg) if err != nil { - return fmt.Errorf("could not parse pull request number: %w", err) + return errors.New("could not parse pull request argument") } } diff --git a/command/pr_review_test.go b/command/pr_review_test.go index 50580450d..96ceabd7e 100644 --- a/command/pr_review_test.go +++ b/command/pr_review_test.go @@ -13,7 +13,7 @@ func TestPRReview_validation(t *testing.T) { for _, cmd := range []string{ `pr review 123`, `pr review --approve --comment 123`, - `pr review --approve="cool" --comment="rad" 123`, + `pr review --approve --comment -b"hey" 123`, } { http.StubRepoResponse("OWNER", "REPO") _, err := RunCommand(cmd) @@ -89,7 +89,7 @@ func TestPRReview_number_arg(t *testing.T) { } } } } `)) http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) - _, err := RunCommand("pr review --request-changes 123") + _, err := RunCommand("pr review --approve 123") if err != nil { t.Fatalf("error running pr review: %s", err) } @@ -107,7 +107,7 @@ func TestPRReview_number_arg(t *testing.T) { _ = json.Unmarshal(bodyBytes, &reqBody) eq(t, reqBody.Variables.Input.PullRequestID, "foobar123") - eq(t, reqBody.Variables.Input.Event, "REQUEST_CHANGES") + eq(t, reqBody.Variables.Input.Event, "APPROVE") eq(t, reqBody.Variables.Input.Body, "") } @@ -121,11 +121,10 @@ func TestPRReview_no_arg(t *testing.T) { "id": "foobar123", "headRefName": "feature", "baseRefName": "master" } - ] } } } } - `)) + ] } } } }`)) http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) - _, err := RunCommand(`pr review --comment="cool story"`) + _, err := RunCommand(`pr review --comment -b "cool story"`) if err != nil { t.Fatalf("error running pr review: %s", err) } @@ -153,7 +152,16 @@ func TestPRReview_blank_comment(t *testing.T) { http.StubRepoResponse("OWNER", "REPO") _, err := RunCommand(`pr review --comment 123`) - eq(t, err.Error(), "did not understand desired review action: cannot leave blank comment") + eq(t, err.Error(), "did not understand desired review action: body cannot be blank for comment review") +} + +func TestPRReview_blank_request_changes(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + + _, err := RunCommand(`pr review -r 123`) + eq(t, err.Error(), "did not understand desired review action: body cannot be blank for request-changes review") } func TestPRReview(t *testing.T) { @@ -163,11 +171,10 @@ func TestPRReview(t *testing.T) { ExpectedBody string } cases := []c{ - c{`pr review --request-changes="bad"`, "REQUEST_CHANGES", "bad"}, - c{`pr review --request-changes`, "REQUEST_CHANGES", ""}, + c{`pr review --request-changes -b"bad"`, "REQUEST_CHANGES", "bad"}, c{`pr review --approve`, "APPROVE", ""}, - c{`pr review --approve="hot damn"`, "APPROVE", "hot damn"}, - c{`pr review --comment="i donno"`, "COMMENT", "i donno"}, + c{`pr review --approve -b"hot damn"`, "APPROVE", "hot damn"}, + c{`pr review --comment --body "i donno"`, "COMMENT", "i donno"}, } for _, kase := range cases {