From 168bd33bc979b9fa4f45db9774f56c27ec02dfd8 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 14 Jul 2020 16:47:32 -0500 Subject: [PATCH] pr command scriptability improvements This commit improves behavior and error handling for pr commands when run unattached to a tty. - error if pr create and no -t/-f - error if pr create and -w - machine readable pr list - machine readable pr view - various cleanup of informational messages --- command/pr.go | 74 +++++++- command/pr_create.go | 25 ++- command/pr_create_test.go | 155 +++++++++++++++++ command/pr_review.go | 19 ++- command/pr_review_test.go | 84 ++++++++- command/pr_test.go | 350 ++++++++++++++++++++++++++++++++++++-- test/fixtures/prList.json | 10 +- 7 files changed, 679 insertions(+), 38 deletions(-) diff --git a/command/pr.go b/command/pr.go index 91ceb1186..018488259 100644 --- a/command/pr.go +++ b/command/pr.go @@ -293,8 +293,9 @@ func prList(cmd *cobra.Command, args []string) error { }) title := listHeader(ghrepo.FullName(baseRepo), "pull request", len(listResult.PullRequests), listResult.TotalCount, hasFilters) - // TODO: avoid printing header if piped to a script - fmt.Fprintf(colorableErr(cmd), "\n%s\n\n", title) + if connectedToTerminal(cmd) { + fmt.Fprintf(colorableErr(cmd), "\n%s\n\n", title) + } table := utils.NewTablePrinter(cmd.OutOrStdout()) for _, pr := range listResult.PullRequests { @@ -303,6 +304,10 @@ func prList(cmd *cobra.Command, args []string) error { prNum = "#" + prNum } table.AddField(prNum, nil, colorFuncForPR(pr)) + if !table.IsTTY() { + table.AddField(pr.State, nil, nil) + table.AddField(prReadyState(&pr), nil, nil) + } table.AddField(replaceExcessiveWhitespace(pr.Title), nil, nil) table.AddField(pr.HeadLabel(), nil, utils.Cyan) table.EndRow() @@ -357,6 +362,10 @@ func prView(cmd *cobra.Command, args []string) error { return err } + if web && !connectedToTerminal(cmd) { + return errors.New("--web unsupported when not attached to a tty") + } + pr, _, err := prFromArgs(ctx, apiClient, cmd, args) if err != nil { return err @@ -367,8 +376,13 @@ func prView(cmd *cobra.Command, args []string) error { fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", openURL) return utils.OpenInBrowser(openURL) } - out := colorableOut(cmd) - return printPrPreview(out, pr) + + if connectedToTerminal(cmd) { + out := colorableOut(cmd) + return printHumanPrPreview(out, pr) + } + + return printRawPrPreview(cmd.OutOrStdout(), pr) } func prClose(cmd *cobra.Command, args []string) error { @@ -482,6 +496,9 @@ func prMerge(cmd *cobra.Command, args []string) error { } if enabledFlagCount == 0 { + if !connectedToTerminal(cmd) { + return errors.New("--merge, --rebase, or --squash required when not attached to a tty") + } isInteractive = true } else if enabledFlagCount > 1 { return errors.New("expected exactly one of --merge, --rebase, or --squash to be true") @@ -513,7 +530,9 @@ func prMerge(cmd *cobra.Command, args []string) error { return fmt.Errorf("API call failed: %w", err) } - fmt.Fprintf(colorableOut(cmd), "%s %s pull request #%d (%s)\n", utils.Magenta("✔"), action, pr.Number, pr.Title) + if connectedToTerminal(cmd) { + fmt.Fprintf(colorableOut(cmd), "%s %s pull request #%d (%s)\n", utils.Magenta("✔"), action, pr.Number, pr.Title) + } if deleteBranch { branchSwitchString := "" @@ -560,7 +579,9 @@ func prMerge(cmd *cobra.Command, args []string) error { } } - fmt.Fprintf(colorableOut(cmd), "%s Deleted branch %s%s\n", utils.Red("✔"), utils.Cyan(pr.HeadRefName), branchSwitchString) + if connectedToTerminal(cmd) { + fmt.Fprintf(colorableOut(cmd), "%s Deleted branch %s%s\n", utils.Red("✔"), utils.Cyan(pr.HeadRefName), branchSwitchString) + } } return nil @@ -620,7 +641,46 @@ func prInteractiveMerge(deleteLocalBranch bool, crossRepoPR bool) (api.PullReque return mergeMethod, deleteBranch, nil } -func printPrPreview(out io.Writer, pr *api.PullRequest) error { +func prReadyState(pr *api.PullRequest) string { + switch pr.State { + case "OPEN": + if pr.IsDraft { + return "draft" + } else { + return "ready" + } + case "CLOSED": + return "unreviewable" + case "MERGED": + return "unreviewable" + } + + return "unknown" +} + +func printRawPrPreview(out io.Writer, pr *api.PullRequest) error { + reviewers := prReviewerList(*pr) + assignees := prAssigneeList(*pr) + labels := prLabelList(*pr) + projects := prProjectList(*pr) + + fmt.Fprintf(out, "title:\t%s\n", pr.Title) + fmt.Fprintf(out, "state:\t%s\n", pr.State) + fmt.Fprintf(out, "ready:\t%s\n", prReadyState(pr)) + fmt.Fprintf(out, "author:\t%s\n", pr.Author.Login) + fmt.Fprintf(out, "labels:\t%s\n", labels) + fmt.Fprintf(out, "assignees:\t%s\n", assignees) + fmt.Fprintf(out, "reviewers:\t%s\n", reviewers) + fmt.Fprintf(out, "projects:\t%s\n", projects) + fmt.Fprintf(out, "milestone:\t%s\n", pr.Milestone.Title) + + fmt.Fprintln(out, "--") + fmt.Fprintln(out, pr.Body) + + return nil +} + +func printHumanPrPreview(out io.Writer, pr *api.PullRequest) error { // Header (Title and State) fmt.Fprintln(out, utils.Bold(pr.Title)) fmt.Fprintf(out, "%s", prStateTitleWithColor(*pr)) diff --git a/command/pr_create.go b/command/pr_create.go index 15406a27c..4b489774b 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -205,12 +205,14 @@ func prCreate(cmd *cobra.Command, _ []string) error { message = "\nCreating draft pull request for %s into %s in %s\n\n" } - fmt.Fprintf(colorableErr(cmd), message, - utils.Cyan(headBranch), - utils.Cyan(baseBranch), - ghrepo.FullName(baseRepo)) - if (title == "" || body == "") && defaultsErr != nil { - fmt.Fprintf(colorableErr(cmd), "%s warning: could not compute title or body defaults: %s\n", utils.Yellow("!"), defaultsErr) + if connectedToTerminal(cmd) { + fmt.Fprintf(colorableErr(cmd), message, + utils.Cyan(headBranch), + utils.Cyan(baseBranch), + ghrepo.FullName(baseRepo)) + if (title == "" || body == "") && defaultsErr != nil { + fmt.Fprintf(colorableErr(cmd), "%s warning: could not compute title or body defaults: %s\n", utils.Yellow("!"), defaultsErr) + } } } @@ -223,7 +225,16 @@ func prCreate(cmd *cobra.Command, _ []string) error { Milestones: milestoneTitles, } - interactive := !(cmd.Flags().Changed("title") && cmd.Flags().Changed("body")) + if !connectedToTerminal(cmd) { + if isWeb { + return errors.New("--web unsupported when not attached to a tty") + } + if !cmd.Flags().Changed("title") && !autofill { + return errors.New("--title or --fill required when not attached to a tty") + } + } + + interactive := connectedToTerminal(cmd) && !(cmd.Flags().Changed("title") && cmd.Flags().Changed("body")) if !isWeb && !autofill && interactive { var nonLegacyTemplateFiles []string diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 9f2a1ebdc..22ea4da72 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -12,10 +12,91 @@ import ( "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/test" + "github.com/stretchr/testify/assert" ) +func TestPRCreate_nontty_insufficient_flags(t *testing.T) { + initBlankContext("", "OWNER/REPO", "feature") + defer stubTerminal(false)() + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "forks": { "nodes": [ + ] } } } } + `)) + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequests": { "nodes" : [ + ] } } } } + `)) + + output, err := RunCommand("pr create") + if err == nil { + t.Fatal("expected error") + } + + assert.Equal(t, "--title or --fill required when not attached to a tty", err.Error()) + + assert.Equal(t, "", output.String()) +} + +func TestPRCreate_nontty(t *testing.T) { + initBlankContext("", "OWNER/REPO", "feature") + defer stubTerminal(false)() + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "forks": { "nodes": [ + ] } } } } + `)) + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequests": { "nodes" : [ + ] } } } } + `)) + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "createPullRequest": { "pullRequest": { + "URL": "https://github.com/OWNER/REPO/pull/12" + } } } } + `)) + + cs, cmdTeardown := test.InitCmdStubber() + defer cmdTeardown() + + cs.Stub("") // git config --get-regexp (determineTrackingBranch) + cs.Stub("") // git show-ref --verify (determineTrackingBranch) + cs.Stub("") // git status + cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log + cs.Stub("") // git push + + output, err := RunCommand(`pr create -t "my title" -b "my body"`) + 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) + + assert.Equal(t, "REPOID", reqBody.Variables.Input.RepositoryID) + assert.Equal(t, "my title", reqBody.Variables.Input.Title) + assert.Equal(t, "my body", reqBody.Variables.Input.Body) + assert.Equal(t, "master", reqBody.Variables.Input.BaseRefName) + assert.Equal(t, "feature", reqBody.Variables.Input.HeadRefName) + + assert.Equal(t, "", output.Stderr()) + assert.Equal(t, "https://github.com/OWNER/REPO/pull/12\n", output.String()) +} + func TestPRCreate(t *testing.T) { initBlankContext("", "OWNER/REPO", "feature") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` @@ -69,6 +150,7 @@ func TestPRCreate(t *testing.T) { func TestPRCreate_metadata(t *testing.T) { initBlankContext("", "OWNER/REPO", "feature") + defer stubTerminal(true)() http := initFakeHTTP() defer http.Verify(t) @@ -193,6 +275,7 @@ func TestPRCreate_metadata(t *testing.T) { func TestPRCreate_withForking(t *testing.T) { initBlankContext("", "OWNER/REPO", "feature") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponseWithPermission("OWNER", "REPO", "READ") http.StubResponse(200, bytes.NewBufferString(` @@ -236,6 +319,7 @@ func TestPRCreate_withForking(t *testing.T) { func TestPRCreate_alreadyExists(t *testing.T) { initBlankContext("", "OWNER/REPO", "feature") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` @@ -269,6 +353,7 @@ func TestPRCreate_alreadyExists(t *testing.T) { func TestPRCreate_alreadyExistsDifferentBase(t *testing.T) { initBlankContext("", "OWNER/REPO", "feature") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` @@ -301,6 +386,7 @@ func TestPRCreate_alreadyExistsDifferentBase(t *testing.T) { func TestPRCreate_web(t *testing.T) { initBlankContext("", "OWNER/REPO", "feature") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` @@ -332,6 +418,7 @@ func TestPRCreate_web(t *testing.T) { func TestPRCreate_ReportsUncommittedChanges(t *testing.T) { initBlankContext("", "OWNER/REPO", "feature") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") @@ -369,6 +456,7 @@ Creating pull request for feature into master in OWNER/REPO `) } func TestPRCreate_cross_repo_same_branch(t *testing.T) { + defer stubTerminal(true)() ctx := context.NewBlank() ctx.SetBranch("default") ctx.SetRemotes(map[string]string{ @@ -457,6 +545,7 @@ func TestPRCreate_cross_repo_same_branch(t *testing.T) { func TestPRCreate_survey_defaults_multicommit(t *testing.T) { initBlankContext("", "OWNER/REPO", "cool_bug-fixes") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` @@ -533,6 +622,7 @@ func TestPRCreate_survey_defaults_multicommit(t *testing.T) { func TestPRCreate_survey_defaults_monocommit(t *testing.T) { initBlankContext("", "OWNER/REPO", "feature") + defer stubTerminal(true)() http := initFakeHTTP() defer http.Verify(t) http.Register(httpmock.GraphQL(`query RepositoryNetwork\b`), httpmock.StringResponse(httpmock.RepoNetworkStubResponse("OWNER", "REPO", "master", "WRITE"))) @@ -592,8 +682,70 @@ func TestPRCreate_survey_defaults_monocommit(t *testing.T) { eq(t, output.String(), "https://github.com/OWNER/REPO/pull/12\n") } +func TestPRCreate_survey_autofill_nontty(t *testing.T) { + initBlankContext("", "OWNER/REPO", "feature") + defer stubTerminal(false)() + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "forks": { "nodes": [ + ] } } } } + `)) + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequests": { "nodes" : [ + ] } } } } + `)) + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "createPullRequest": { "pullRequest": { + "URL": "https://github.com/OWNER/REPO/pull/12" + } } } } + `)) + + cs, cmdTeardown := test.InitCmdStubber() + defer cmdTeardown() + + cs.Stub("") // git config --get-regexp (determineTrackingBranch) + cs.Stub("") // git show-ref --verify (determineTrackingBranch) + cs.Stub("") // git status + cs.Stub("1234567890,the sky above the port") // git log + cs.Stub("was the color of a television, turned to a dead channel") // git show + cs.Stub("") // git rev-parse + cs.Stub("") // git push + cs.Stub("") // browser open + + output, err := RunCommand(`pr create -f`) + 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" + + assert.Equal(t, "REPOID", reqBody.Variables.Input.RepositoryID) + assert.Equal(t, "the sky above the port", reqBody.Variables.Input.Title) + assert.Equal(t, expectedBody, reqBody.Variables.Input.Body) + assert.Equal(t, "master", reqBody.Variables.Input.BaseRefName) + assert.Equal(t, "feature", reqBody.Variables.Input.HeadRefName) + + assert.Equal(t, "https://github.com/OWNER/REPO/pull/12\n", output.String()) + + assert.Equal(t, "", output.Stderr()) +} + func TestPRCreate_survey_autofill(t *testing.T) { initBlankContext("", "OWNER/REPO", "feature") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` @@ -652,6 +804,7 @@ func TestPRCreate_survey_autofill(t *testing.T) { func TestPRCreate_defaults_error_autofill(t *testing.T) { initBlankContext("", "OWNER/REPO", "feature") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") @@ -670,6 +823,7 @@ func TestPRCreate_defaults_error_autofill(t *testing.T) { func TestPRCreate_defaults_error_web(t *testing.T) { initBlankContext("", "OWNER/REPO", "feature") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") @@ -688,6 +842,7 @@ func TestPRCreate_defaults_error_web(t *testing.T) { func TestPRCreate_defaults_error_interactive(t *testing.T) { initBlankContext("", "OWNER/REPO", "feature") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` diff --git a/command/pr_review.go b/command/pr_review.go index 08efc638d..96ac299f3 100644 --- a/command/pr_review.go +++ b/command/pr_review.go @@ -72,7 +72,10 @@ func processReviewOpt(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { } if found == 0 && body == "" { - return nil, nil // signal interactive mode + if connectedToTerminal(cmd) { + return nil, nil // signal interactive mode + } + return nil, errors.New("--approve, --request-changes, or --comment required when not attached to a tty") } else if found == 0 && body != "" { return nil, errors.New("--body unsupported without --approve, --request-changes, or --comment") } else if found > 1 { @@ -106,7 +109,7 @@ func prReview(cmd *cobra.Command, args []string) error { return fmt.Errorf("did not understand desired review action: %w", err) } - out := colorableOut(cmd) + stderr := cmd.ErrOrStderr() if reviewData == nil { reviewData, err = reviewSurvey(cmd) @@ -114,7 +117,7 @@ func prReview(cmd *cobra.Command, args []string) error { return err } if reviewData == nil && err == nil { - fmt.Fprint(out, "Discarding.\n") + fmt.Fprint(stderr, "Discarding.\n") return nil } } @@ -124,13 +127,17 @@ func prReview(cmd *cobra.Command, args []string) error { return fmt.Errorf("failed to create review: %w", err) } + if !connectedToTerminal(cmd) { + return nil + } + switch reviewData.State { case api.ReviewComment: - fmt.Fprintf(out, "%s Reviewed pull request #%d\n", utils.Gray("-"), pr.Number) + fmt.Fprintf(stderr, "%s Reviewed pull request #%d\n", utils.Gray("-"), pr.Number) case api.ReviewApprove: - fmt.Fprintf(out, "%s Approved pull request #%d\n", utils.Green("✓"), pr.Number) + fmt.Fprintf(stderr, "%s Approved pull request #%d\n", utils.Green("✓"), pr.Number) case api.ReviewRequestChanges: - fmt.Fprintf(out, "%s Requested changes to pull request #%d\n", utils.Red("+"), pr.Number) + fmt.Fprintf(stderr, "%s Requested changes to pull request #%d\n", utils.Red("+"), pr.Number) } return nil diff --git a/command/pr_review_test.go b/command/pr_review_test.go index 2c2673db4..fdbcc366c 100644 --- a/command/pr_review_test.go +++ b/command/pr_review_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/cli/cli/test" + "github.com/stretchr/testify/assert" ) func TestPRReview_validation(t *testing.T) { @@ -49,6 +50,7 @@ func TestPRReview_bad_body(t *testing.T) { func TestPRReview_url_arg(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequest": { @@ -74,7 +76,7 @@ func TestPRReview_url_arg(t *testing.T) { t.Fatalf("error running pr review: %s", err) } - test.ExpectLines(t, output.String(), "Approved pull request #123") + test.ExpectLines(t, output.Stderr(), "Approved pull request #123") bodyBytes, _ := ioutil.ReadAll(http.Requests[1].Body) reqBody := struct { @@ -95,6 +97,7 @@ func TestPRReview_url_arg(t *testing.T) { func TestPRReview_number_arg(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` @@ -121,7 +124,7 @@ func TestPRReview_number_arg(t *testing.T) { t.Fatalf("error running pr review: %s", err) } - test.ExpectLines(t, output.String(), "Approved pull request #123") + test.ExpectLines(t, output.Stderr(), "Approved pull request #123") bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) reqBody := struct { @@ -142,6 +145,7 @@ func TestPRReview_number_arg(t *testing.T) { func TestPRReview_no_arg(t *testing.T) { initBlankContext("", "OWNER/REPO", "feature") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` @@ -159,7 +163,7 @@ func TestPRReview_no_arg(t *testing.T) { t.Fatalf("error running pr review: %s", err) } - test.ExpectLines(t, output.String(), "- Reviewed pull request #123") + test.ExpectLines(t, output.Stderr(), "- Reviewed pull request #123") bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) reqBody := struct { @@ -254,8 +258,73 @@ func TestPRReview(t *testing.T) { } } +func TestPRReview_nontty_insufficient_flags(t *testing.T) { + initBlankContext("", "OWNER/REPO", "feature") + defer stubTerminal(false)() + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequests": { "nodes": [ + { "url": "https://github.com/OWNER/REPO/pull/123", + "number": 123, + "id": "foobar123", + "headRefName": "feature", + "baseRefName": "master" } + ] } } } } + `)) + + output, err := RunCommand("pr review") + if err == nil { + t.Fatal("expected error") + } + + assert.Equal(t, "", output.String()) + + assert.Equal(t, "did not understand desired review action: --approve, --request-changes, or --comment required when not attached to a tty", err.Error()) +} + +func TestPRReview_nontty(t *testing.T) { + initBlankContext("", "OWNER/REPO", "feature") + defer stubTerminal(false)() + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequests": { "nodes": [ + { "url": "https://github.com/OWNER/REPO/pull/123", + "number": 123, + "id": "foobar123", + "headRefName": "feature", + "baseRefName": "master" } + ] } } } } + `)) + + http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) + output, err := RunCommand("pr review -c -bcool") + if err != nil { + t.Fatalf("unexpected error running command: %s", err) + } + + assert.Equal(t, "", output.String()) + assert.Equal(t, "", output.Stderr()) + + bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + reqBody := struct { + Variables struct { + Input struct { + Event string + Body string + } + } + }{} + _ = json.Unmarshal(bodyBytes, &reqBody) + + assert.Equal(t, "COMMENT", reqBody.Variables.Input.Event) + assert.Equal(t, "cool", reqBody.Variables.Input.Body) +} + func TestPRReview_interactive(t *testing.T) { initBlankContext("", "OWNER/REPO", "feature") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` @@ -295,10 +364,11 @@ func TestPRReview_interactive(t *testing.T) { t.Fatalf("got unexpected error running pr review: %s", err) } + test.ExpectLines(t, output.Stderr(), "Approved pull request #123") + test.ExpectLines(t, output.String(), - "Approved pull request #123", "Got:", - "cool.*story") // weird because markdown rendering puts a bunch of junk between works + "cool.*story") bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) reqBody := struct { @@ -317,6 +387,7 @@ func TestPRReview_interactive(t *testing.T) { func TestPRReview_interactive_no_body(t *testing.T) { initBlankContext("", "OWNER/REPO", "feature") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` @@ -359,6 +430,7 @@ func TestPRReview_interactive_no_body(t *testing.T) { func TestPRReview_interactive_blank_approve(t *testing.T) { initBlankContext("", "OWNER/REPO", "feature") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` @@ -403,7 +475,7 @@ func TestPRReview_interactive_blank_approve(t *testing.T) { t.Errorf("did not expect to see body printed in %s", output.String()) } - test.ExpectLines(t, output.String(), "Approved pull request #123") + test.ExpectLines(t, output.Stderr(), "Approved pull request #123") bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) reqBody := struct { diff --git a/command/pr_test.go b/command/pr_test.go index 59ca67e9a..95f8dadef 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -295,6 +295,7 @@ Requesting a code review from you func TestPRList(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestList\b`), httpmock.FileResponse("../test/fixtures/prList.json")) @@ -304,18 +305,40 @@ func TestPRList(t *testing.T) { t.Fatal(err) } - eq(t, output.Stderr(), ` + assert.Equal(t, ` Showing 3 of 3 pull requests in OWNER/REPO -`) - eq(t, output.String(), `32 New feature feature -29 Fixed bad bug hubot:bug-fix -28 Improve documentation docs -`) +`, output.Stderr()) + + assert.Equal(t, `#32 New feature feature +#29 Fixed bad bug hubot:bug-fix +#28 Improve documentation docs +`, output.String()) +} + +func TestPRList_nontty(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(false)() + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.Register(httpmock.GraphQL(`query PullRequestList\b`), httpmock.FileResponse("../test/fixtures/prList.json")) + + output, err := RunCommand("pr list") + if err != nil { + t.Fatal(err) + } + + assert.Equal(t, "", output.Stderr()) + + assert.Equal(t, `32 OPEN draft New feature feature +29 OPEN ready Fixed bad bug hubot:bug-fix +28 MERGED unreviewable Improve documentation docs +`, output.String()) } func TestPRList_filtering(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.Register( @@ -339,6 +362,7 @@ No pull requests match your search in OWNER/REPO func TestPRList_filteringRemoveDuplicate(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.Register( @@ -350,14 +374,15 @@ func TestPRList_filteringRemoveDuplicate(t *testing.T) { t.Fatal(err) } - eq(t, output.String(), `32 New feature feature -29 Fixed bad bug hubot:bug-fix -28 Improve documentation docs -`) + assert.Equal(t, `#32 New feature feature +#29 Fixed bad bug hubot:bug-fix +#28 Improve documentation docs +`, output.String()) } func TestPRList_filteringClosed(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.Register( @@ -374,6 +399,7 @@ func TestPRList_filteringClosed(t *testing.T) { func TestPRList_filteringAssignee(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.Register( @@ -390,6 +416,7 @@ func TestPRList_filteringAssignee(t *testing.T) { func TestPRList_filteringAssigneeLabels(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() initFakeHTTP() _, err := RunCommand(`pr list -l one,two -a hubot`) @@ -400,6 +427,7 @@ func TestPRList_filteringAssigneeLabels(t *testing.T) { func TestPRList_withInvalidLimitFlag(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() initFakeHTTP() _, err := RunCommand(`pr list --limit=0`) @@ -410,6 +438,7 @@ func TestPRList_withInvalidLimitFlag(t *testing.T) { func TestPRList_web(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") @@ -437,7 +466,197 @@ func TestPRList_web(t *testing.T) { eq(t, url, expectedURL) } +func TestPRView_Preview_nontty(t *testing.T) { + defer stubTerminal(false)() + tests := map[string]struct { + ownerRepo string + args string + fixture string + expectedOutputs []string + }{ + "Open PR without metadata": { + ownerRepo: "master", + args: "pr view 12", + fixture: "../test/fixtures/prViewPreview.json", + expectedOutputs: []string{ + `title:\tBlueberries are from a fork\n`, + `state:\tOPEN\n`, + `author:\tnobody\n`, + `labels:\t\n`, + `ready:\tready\n`, + `assignees:\t\n`, + `reviewers:\t\n`, + `projects:\t\n`, + `milestone:\t\n`, + `blueberries taste good`, + }, + }, + "Open PR with metadata by number": { + ownerRepo: "master", + args: "pr view 12", + fixture: "../test/fixtures/prViewPreviewWithMetadataByNumber.json", + expectedOutputs: []string{ + `title:\tBlueberries are from a fork\n`, + `reviewers:\t2 \(Approved\), 3 \(Commented\), 1 \(Requested\)\n`, + `assignees:\tmarseilles, monaco\n`, + `labels:\tone, two, three, four, five\n`, + `ready:\tready\n`, + `projects:\tProject 1 \(column A\), Project 2 \(column B\), Project 3 \(column C\), Project 4 \(Awaiting triage\)\n`, + `milestone:\tuluru\n`, + `\*\*blueberries taste good\*\*`, + }, + }, + "Open PR with reviewers by number": { + ownerRepo: "master", + args: "pr view 12", + fixture: "../test/fixtures/prViewPreviewWithReviewersByNumber.json", + expectedOutputs: []string{ + `title:\tBlueberries are from a fork\n`, + `state:\tOPEN\n`, + `author:\tnobody\n`, + `labels:\t\n`, + `assignees:\t\n`, + `projects:\t\n`, + `ready:\tready\n`, + `milestone:\t\n`, + `reviewers:\tDEF \(Commented\), def \(Changes requested\), ghost \(Approved\), hubot \(Commented\), xyz \(Approved\), 123 \(Requested\), Team 1 \(Requested\), abc \(Requested\)\n`, + `\*\*blueberries taste good\*\*`, + }, + }, + "Open PR with metadata by branch": { + ownerRepo: "master", + args: "pr view blueberries", + fixture: "../test/fixtures/prViewPreviewWithMetadataByBranch.json", + expectedOutputs: []string{ + `title:\tBlueberries are a good fruit`, + `state:\tOPEN`, + `author:\tnobody`, + `assignees:\tmarseilles, monaco\n`, + `ready:\tready\n`, + `labels:\tone, two, three, four, five\n`, + `projects:\tProject 1 \(column A\), Project 2 \(column B\), Project 3 \(column C\)\n`, + `milestone:\tuluru\n`, + `blueberries taste good`, + }, + }, + "Open PR for the current branch": { + ownerRepo: "blueberries", + args: "pr view", + fixture: "../test/fixtures/prView.json", + expectedOutputs: []string{ + `title:\tBlueberries are a good fruit`, + `state:\tOPEN`, + `author:\tnobody`, + `ready:\tready\n`, + `assignees:\t\n`, + `labels:\t\n`, + `projects:\t\n`, + `milestone:\t\n`, + `\*\*blueberries taste good\*\*`, + }, + }, + "Open PR wth empty body for the current branch": { + ownerRepo: "blueberries", + args: "pr view", + fixture: "../test/fixtures/prView_EmptyBody.json", + expectedOutputs: []string{ + `title:\tBlueberries are a good fruit`, + `state:\tOPEN`, + `ready:\tready\n`, + `author:\tnobody`, + `assignees:\t\n`, + `labels:\t\n`, + `projects:\t\n`, + `milestone:\t\n`, + }, + }, + "Closed PR": { + ownerRepo: "master", + args: "pr view 12", + fixture: "../test/fixtures/prViewPreviewClosedState.json", + expectedOutputs: []string{ + `state:\tCLOSED\n`, + `author:\tnobody\n`, + `labels:\t\n`, + `ready:\tunreviewable\n`, + `assignees:\t\n`, + `reviewers:\t\n`, + `projects:\t\n`, + `milestone:\t\n`, + `\*\*blueberries taste good\*\*`, + }, + }, + "Merged PR": { + ownerRepo: "master", + args: "pr view 12", + fixture: "../test/fixtures/prViewPreviewMergedState.json", + expectedOutputs: []string{ + `state:\tMERGED\n`, + `author:\tnobody\n`, + `labels:\t\n`, + `assignees:\t\n`, + `ready:\tunreviewable\n`, + `reviewers:\t\n`, + `projects:\t\n`, + `milestone:\t\n`, + `\*\*blueberries taste good\*\*`, + }, + }, + "Draft PR": { + ownerRepo: "master", + args: "pr view 12", + fixture: "../test/fixtures/prViewPreviewDraftState.json", + expectedOutputs: []string{ + `title:\tBlueberries are from a fork\n`, + `state:\tOPEN\n`, + `author:\tnobody\n`, + `labels:`, + `ready:\tdraft\n`, + `assignees:`, + `projects:`, + `milestone:`, + `\*\*blueberries taste good\*\*`, + }, + }, + "Draft PR by branch": { + ownerRepo: "master", + args: "pr view blueberries", + fixture: "../test/fixtures/prViewPreviewDraftStatebyBranch.json", + expectedOutputs: []string{ + `title:\tBlueberries are a good fruit\n`, + `state:\tOPEN\n`, + `author:\tnobody\n`, + `labels:`, + `ready:\tdraft\n`, + `assignees:`, + `projects:`, + `milestone:`, + `\*\*blueberries taste good\*\*`, + }, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + initBlankContext("", "OWNER/REPO", tc.ownerRepo) + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.Register(httpmock.GraphQL(`query PullRequest(ByNumber|ForBranch)\b`), httpmock.FileResponse(tc.fixture)) + + output, err := RunCommand(tc.args) + if err != nil { + t.Errorf("error running command `%v`: %v", tc.args, err) + } + + eq(t, output.Stderr(), "") + + test.ExpectLines(t, output.String(), tc.expectedOutputs...) + }) + } +} + func TestPRView_Preview(t *testing.T) { + defer stubTerminal(true)() tests := map[string]struct { ownerRepo string args string @@ -585,6 +804,7 @@ func TestPRView_Preview(t *testing.T) { func TestPRView_web_currentBranch(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestForBranch\b`), httpmock.FileResponse("../test/fixtures/prView.json")) @@ -620,6 +840,7 @@ func TestPRView_web_currentBranch(t *testing.T) { func TestPRView_web_noResultsForBranch(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestForBranch\b`), httpmock.FileResponse("../test/fixtures/prView_NoActiveBranch.json")) @@ -648,6 +869,7 @@ func TestPRView_web_noResultsForBranch(t *testing.T) { func TestPRView_web_numberArg(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") @@ -680,6 +902,7 @@ func TestPRView_web_numberArg(t *testing.T) { func TestPRView_web_numberArgWithHash(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") @@ -712,6 +935,7 @@ func TestPRView_web_numberArgWithHash(t *testing.T) { func TestPRView_web_urlArg(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequest": { @@ -742,6 +966,7 @@ func TestPRView_web_urlArg(t *testing.T) { func TestPRView_web_branchArg(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") @@ -776,6 +1001,7 @@ func TestPRView_web_branchArg(t *testing.T) { func TestPRView_web_branchWithOwnerArg(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") @@ -809,6 +1035,21 @@ func TestPRView_web_branchWithOwnerArg(t *testing.T) { eq(t, url, "https://github.com/hubot/REPO/pull/23") } +func TestPrView_web_nontty(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(false)() + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + + output, err := RunCommand("pr view -w") + if err == nil { + t.Fatal("expected error") + } + + assert.Equal(t, "--web unsupported when not attached to a tty", err.Error()) + assert.Equal(t, "", output.String()) +} + func TestReplaceExcessiveWhitespace(t *testing.T) { eq(t, replaceExcessiveWhitespace("hello\ngoodbye"), "hello goodbye") eq(t, replaceExcessiveWhitespace(" hello goodbye "), "hello goodbye") @@ -965,6 +1206,7 @@ func TestPRReopen_alreadyMerged(t *testing.T) { func TestPrMerge(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") @@ -1010,8 +1252,78 @@ func TestPrMerge(t *testing.T) { } } +func TestPrMerge_nontty(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(false)() + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.Register( + httpmock.GraphQL(`query PullRequestByNumber\b`), + httpmock.StringResponse(` + { "data": { "repository": { + "pullRequest": { "number": 1, "title": "The title of the PR", "state": "OPEN", "id": "THE-ID"} + } } }`)) + http.Register( + httpmock.GraphQL(`mutation PullRequestMerge\b`), + httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { + assert.Equal(t, "THE-ID", input["pullRequestId"].(string)) + assert.Equal(t, "MERGE", input["mergeMethod"].(string)) + })) + http.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(`{ + "data": { + "repository": { + "defaultBranchRef": {"name": "master"} + } + } + }`)) + http.Register( + httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/blueberries"), + httpmock.StringResponse(`{}`)) + + cs, cmdTeardown := test.InitCmdStubber() + defer cmdTeardown() + + cs.Stub("branch.blueberries.remote origin\nbranch.blueberries.merge refs/heads/blueberries") // git config --get-regexp ^branch\.master\.(remote|merge) + cs.Stub("") // git config --get-regexp ^branch\.blueberries\.(remote|merge)$ + cs.Stub("") // git symbolic-ref --quiet --short HEAD + cs.Stub("") // git checkout master + cs.Stub("") + + output, err := RunCommand("pr merge 1 --merge") + if err != nil { + t.Fatalf("error running command `pr merge`: %v", err) + } + + assert.Equal(t, "", output.String()) + assert.Equal(t, "", output.Stderr()) +} + +func TestPrMerge_nontty_insufficient_flags(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(false)() + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.Register( + httpmock.GraphQL(`query PullRequestByNumber\b`), + httpmock.StringResponse(` + { "data": { "repository": { + "pullRequest": { "number": 1, "title": "The title of the PR", "state": "OPEN", "id": "THE-ID"} + } } }`)) + + output, err := RunCommand("pr merge 1") + if err == nil { + t.Fatal("expected error") + } + + assert.Equal(t, "--merge, --rebase, or --squash required when not attached to a tty", err.Error()) + assert.Equal(t, "", output.String()) +} + func TestPrMerge_withRepoFlag(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() defer http.Verify(t) http.Register( @@ -1049,6 +1361,7 @@ func TestPrMerge_withRepoFlag(t *testing.T) { func TestPrMerge_deleteBranch(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") + defer stubTerminal(true)() http := initFakeHTTP() defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") @@ -1084,6 +1397,7 @@ func TestPrMerge_deleteBranch(t *testing.T) { func TestPrMerge_deleteNonCurrentBranch(t *testing.T) { initBlankContext("", "OWNER/REPO", "another-branch") + defer stubTerminal(true)() http := initFakeHTTP() defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") @@ -1117,6 +1431,7 @@ func TestPrMerge_deleteNonCurrentBranch(t *testing.T) { func TestPrMerge_noPrNumberGiven(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") + defer stubTerminal(true)() http := initFakeHTTP() defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") @@ -1156,6 +1471,7 @@ func TestPrMerge_noPrNumberGiven(t *testing.T) { func TestPrMerge_rebase(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") @@ -1202,6 +1518,7 @@ func TestPrMerge_rebase(t *testing.T) { func TestPrMerge_squash(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") @@ -1245,6 +1562,7 @@ func TestPrMerge_squash(t *testing.T) { func TestPrMerge_alreadyMerged(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() http := initFakeHTTP() defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") @@ -1277,6 +1595,7 @@ func TestPrMerge_alreadyMerged(t *testing.T) { func TestPRMerge_interactive(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") + defer stubTerminal(true)() http := initFakeHTTP() defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") @@ -1332,6 +1651,17 @@ func TestPRMerge_interactive(t *testing.T) { func TestPrMerge_multipleMergeMethods(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(true)() + + _, err := RunCommand("pr merge 1 --merge --squash") + if err == nil { + t.Fatal("expected error running `pr merge` with multiple merge methods") + } +} + +func TestPrMerge_multipleMergeMethods_nontty(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + defer stubTerminal(false)() _, err := RunCommand("pr merge 1 --merge --squash") if err == nil { diff --git a/test/fixtures/prList.json b/test/fixtures/prList.json index abd0e4ee9..e050a27f6 100644 --- a/test/fixtures/prList.json +++ b/test/fixtures/prList.json @@ -9,7 +9,9 @@ "number": 32, "title": "New feature", "url": "https://github.com/monalisa/hello/pull/32", - "headRefName": "feature" + "headRefName": "feature", + "state": "OPEN", + "isDraft": true } }, { @@ -18,6 +20,8 @@ "title": "Fixed bad bug", "url": "https://github.com/monalisa/hello/pull/29", "headRefName": "bug-fix", + "state": "OPEN", + "isDraft": false, "isCrossRepository": true, "headRepositoryOwner": { "login": "hubot" @@ -27,6 +31,8 @@ { "node": { "number": 28, + "state": "MERGED", + "isDraft": false, "title": "Improve documentation", "url": "https://github.com/monalisa/hello/pull/28", "headRefName": "docs" @@ -40,4 +46,4 @@ } } } -} \ No newline at end of file +}