From 12637d02d6f1eef52e2543710bcaa0209263ca95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 29 Jul 2020 19:35:54 +0200 Subject: [PATCH] Isolate `pr review` command --- command/root.go | 12 + .../cmd/pr/review/review.go | 231 +++++++------ .../cmd/pr/review/review_test.go | 313 ++++++++++-------- pkg/cmdutil/factory.go | 1 + pkg/cmdutil/legacy.go | 23 ++ 5 files changed, 335 insertions(+), 245 deletions(-) rename command/pr_review.go => pkg/cmd/pr/review/review.go (50%) rename command/pr_review_test.go => pkg/cmd/pr/review/review_test.go (55%) create mode 100644 pkg/cmdutil/legacy.go diff --git a/command/root.go b/command/root.go index 3ee9f2203..e306e9fec 100644 --- a/command/root.go +++ b/command/root.go @@ -16,11 +16,13 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/api" "github.com/cli/cli/context" + "github.com/cli/cli/git" "github.com/cli/cli/internal/config" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/internal/run" apiCmd "github.com/cli/cli/pkg/cmd/api" gistCreateCmd "github.com/cli/cli/pkg/cmd/gist/create" + prReviewCmd "github.com/cli/cli/pkg/cmd/pr/review" repoCmd "github.com/cli/cli/pkg/cmd/repo" repoCloneCmd "github.com/cli/cli/pkg/cmd/repo/clone" repoCreateCmd "github.com/cli/cli/pkg/cmd/repo/create" @@ -113,6 +115,13 @@ func init() { } return cfg, nil }, + Branch: func() (string, error) { + currentBranch, err := git.CurrentBranch() + if err != nil { + return "", fmt.Errorf("could not determine current branch: %w", err) + } + return currentBranch, nil + }, } RootCmd.AddCommand(apiCmd.NewCmdApi(cmdFactory, nil)) @@ -160,6 +169,8 @@ func init() { repoCmd.Cmd.AddCommand(repoCreateCmd.NewCmdCreate(cmdFactory, nil)) repoCmd.Cmd.AddCommand(creditsCmd.NewCmdRepoCredits(&repoResolvingCmdFactory, nil)) + prCmd.AddCommand(prReviewCmd.NewCmdReview(&repoResolvingCmdFactory, nil)) + RootCmd.AddCommand(creditsCmd.NewCmdCredits(cmdFactory, nil)) } @@ -401,6 +412,7 @@ func formatRemoteURL(cmd *cobra.Command, repo ghrepo.Interface) string { return fmt.Sprintf("https://%s/%s/%s.git", repo.RepoHost(), repo.RepoOwner(), repo.RepoName()) } +// TODO there is a parallel implementation for isolated commands func determineEditor(cmd *cobra.Command) (string, error) { editorCommand := os.Getenv("GH_EDITOR") if editorCommand == "" { diff --git a/command/pr_review.go b/pkg/cmd/pr/review/review.go similarity index 50% rename from command/pr_review.go rename to pkg/cmd/pr/review/review.go index f2cbb68be..48ad1d0b4 100644 --- a/command/pr_review.go +++ b/pkg/cmd/pr/review/review.go @@ -1,79 +1,171 @@ -package command +package review import ( "errors" "fmt" + "net/http" "github.com/AlecAivazis/survey/v2" "github.com/MakeNowJust/heredoc" - "github.com/spf13/cobra" - "github.com/cli/cli/api" + "github.com/cli/cli/context" + "github.com/cli/cli/internal/config" + "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/cmd/pr/shared" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/pkg/prompt" "github.com/cli/cli/pkg/surveyext" "github.com/cli/cli/utils" + "github.com/spf13/cobra" ) -func init() { - prCmd.AddCommand(prReviewCmd) +type ReviewOptions struct { + HttpClient func() (*http.Client, error) + Config func() (config.Config, error) + IO *iostreams.IOStreams + BaseRepo func() (ghrepo.Interface, error) + Remotes func() (context.Remotes, error) + Branch func() (string, error) - 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") + SelectorArg string + Approve bool + RequestChanges bool + Comment bool + Body string } -var prReviewCmd = &cobra.Command{ - Use: "review [ | | ]", - Short: "Add a review to a pull request", - Long: `Add a review to a pull request. +func NewCmdReview(f *cmdutil.Factory, runF func(*ReviewOptions) error) *cobra.Command { + opts := &ReviewOptions{ + IO: f.IOStreams, + HttpClient: f.HttpClient, + Config: f.Config, + BaseRepo: f.BaseRepo, + Remotes: f.Remotes, + Branch: f.Branch, + } -Without an argument, the pull request that belongs to the current branch is reviewed.`, - Example: heredoc.Doc(` - # approve the pull request of the current branch - $ gh pr review --approve - - # leave a review comment for the current branch - $ gh pr review --comment -b "interesting" - - # add a review for a specific pull request - $ gh pr review 123 - - # request changes on a specific pull request - $ gh pr review 123 -r -b "needs more ASCII art" - `), - Args: cobra.MaximumNArgs(1), - RunE: prReview, + cmd := &cobra.Command{ + Use: "review [ | | ]", + Short: "Add a review to a pull request", + Long: heredoc.Doc(` + Add a review to a pull request. + + Without an argument, the pull request that belongs to the current branch is reviewed. + `), + Example: heredoc.Doc(` + # approve the pull request of the current branch + $ gh pr review --approve + + # leave a review comment for the current branch + $ gh pr review --comment -b "interesting" + + # add a review for a specific pull request + $ gh pr review 123 + + # request changes on a specific pull request + $ gh pr review 123 -r -b "needs more ASCII art" + `), + Args: cobra.MaximumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + if len(args) > 0 { + opts.SelectorArg = args[0] + } + + if runF != nil { + return runF(opts) + } + return reviewRun(opts) + }, + } + + cmd.Flags().BoolVarP(&opts.Approve, "approve", "a", false, "Approve pull request") + cmd.Flags().BoolVarP(&opts.RequestChanges, "request-changes", "r", false, "Request changes on a pull request") + cmd.Flags().BoolVarP(&opts.Comment, "comment", "c", false, "Comment on a pull request") + cmd.Flags().StringVarP(&opts.Body, "body", "b", "", "Specify the body of a review") + + return cmd } -func processReviewOpt(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { +func reviewRun(opts *ReviewOptions) error { + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + apiClient := api.NewClientFromHTTP(httpClient) + + reviewData, err := processReviewOpt(opts) + if err != nil { + return fmt.Errorf("did not understand desired review action: %w", err) + } + + pr, _, err := shared.PRFromArgs(apiClient, opts.BaseRepo, opts.Branch, opts.Remotes, opts.SelectorArg) + if err != nil { + return err + } + + if reviewData == nil { + editorCommand, err := cmdutil.DetermineEditor(opts.Config) + if err != nil { + return err + } + reviewData, err = reviewSurvey(opts.IO, editorCommand) + if err != nil { + return err + } + if reviewData == nil && err == nil { + fmt.Fprint(opts.IO.ErrOut, "Discarding.\n") + return nil + } + } + + err = api.AddReview(apiClient, pr, reviewData) + if err != nil { + return fmt.Errorf("failed to create review: %w", err) + } + + if !opts.IO.IsStdoutTTY() || !opts.IO.IsStderrTTY() { + return nil + } + + switch reviewData.State { + case api.ReviewComment: + fmt.Fprintf(opts.IO.ErrOut, "%s Reviewed pull request #%d\n", utils.Gray("-"), pr.Number) + case api.ReviewApprove: + fmt.Fprintf(opts.IO.ErrOut, "%s Approved pull request #%d\n", utils.Green("✓"), pr.Number) + case api.ReviewRequestChanges: + fmt.Fprintf(opts.IO.ErrOut, "%s Requested changes to pull request #%d\n", utils.Red("+"), pr.Number) + } + + return nil +} + +// TODO: move to Command.Args, raise FlagError +func processReviewOpt(opts *ReviewOptions) (*api.PullRequestReviewInput, error) { found := 0 flag := "" var state api.PullRequestReviewState - if cmd.Flags().Changed("approve") { + if opts.Approve { found++ flag = "approve" state = api.ReviewApprove } - if cmd.Flags().Changed("request-changes") { + if opts.RequestChanges { found++ flag = "request-changes" state = api.ReviewRequestChanges } - if cmd.Flags().Changed("comment") { + if opts.Comment { found++ flag = "comment" state = api.ReviewComment } - body, err := cmd.Flags().GetString("body") - if err != nil { - return nil, err - } + body := opts.Body if found == 0 && body == "" { - if connectedToTerminal(cmd) { + if opts.IO.IsStdoutTTY() && opts.IO.IsStderrTTY() { return nil, nil // signal interactive mode } return nil, errors.New("--approve, --request-changes, or --comment required when not attached to a tty") @@ -93,63 +185,7 @@ func processReviewOpt(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { }, nil } -func prReview(cmd *cobra.Command, args []string) error { - ctx := contextForCommand(cmd) - apiClient, err := apiClientForContext(ctx) - if err != nil { - return err - } - - pr, _, err := prFromArgs(ctx, apiClient, cmd, args) - if err != nil { - return err - } - - reviewData, err := processReviewOpt(cmd) - if err != nil { - return fmt.Errorf("did not understand desired review action: %w", err) - } - - stderr := colorableErr(cmd) - - if reviewData == nil { - reviewData, err = reviewSurvey(cmd) - if err != nil { - return err - } - if reviewData == nil && err == nil { - fmt.Fprint(stderr, "Discarding.\n") - return nil - } - } - - err = api.AddReview(apiClient, pr, reviewData) - if err != nil { - return fmt.Errorf("failed to create review: %w", err) - } - - if !connectedToTerminal(cmd) { - return nil - } - - switch reviewData.State { - case api.ReviewComment: - fmt.Fprintf(stderr, "%s Reviewed pull request #%d\n", utils.Gray("-"), pr.Number) - case api.ReviewApprove: - fmt.Fprintf(stderr, "%s Approved pull request #%d\n", utils.Green("✓"), pr.Number) - case api.ReviewRequestChanges: - fmt.Fprintf(stderr, "%s Requested changes to pull request #%d\n", utils.Red("+"), pr.Number) - } - - return nil -} - -func reviewSurvey(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { - editorCommand, err := determineEditor(cmd) - if err != nil { - return nil, err - } - +func reviewSurvey(io *iostreams.IOStreams, editorCommand string) (*api.PullRequestReviewInput, error) { typeAnswers := struct { ReviewType string }{} @@ -167,7 +203,7 @@ func reviewSurvey(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { }, } - err = prompt.SurveyAsk(typeQs, &typeAnswers) + err := prompt.SurveyAsk(typeQs, &typeAnswers) if err != nil { return nil, err } @@ -218,13 +254,12 @@ func reviewSurvey(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { } if len(bodyAnswers.Body) > 0 { - out := colorableOut(cmd) renderedBody, err := utils.RenderMarkdown(bodyAnswers.Body) if err != nil { return nil, err } - fmt.Fprintf(out, "Got:\n%s", renderedBody) + fmt.Fprintf(io.Out, "Got:\n%s", renderedBody) } confirm := false diff --git a/command/pr_review_test.go b/pkg/cmd/pr/review/review_test.go similarity index 55% rename from command/pr_review_test.go rename to pkg/cmd/pr/review/review_test.go index b2bb36536..fa033d0a5 100644 --- a/command/pr_review_test.go +++ b/pkg/cmd/pr/review/review_test.go @@ -1,58 +1,112 @@ -package command +package review import ( "bytes" "encoding/json" "io/ioutil" + "net/http" "regexp" "testing" + "github.com/cli/cli/context" + "github.com/cli/cli/git" + "github.com/cli/cli/internal/config" + "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/httpmock" + "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/pkg/prompt" "github.com/cli/cli/test" + "github.com/google/shlex" "github.com/stretchr/testify/assert" ) +func runCommand(rt http.RoundTripper, remotes context.Remotes, isTTY bool, cli string) (*test.CmdOut, error) { + io, _, stdout, stderr := iostreams.Test() + io.SetStdoutTTY(isTTY) + io.SetStdinTTY(isTTY) + io.SetStderrTTY(isTTY) + + factory := &cmdutil.Factory{ + IOStreams: io, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: rt}, nil + }, + Config: func() (config.Config, error) { + return config.NewBlankConfig(), nil + }, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("OWNER", "REPO"), nil + }, + Remotes: func() (context.Remotes, error) { + if remotes == nil { + return context.Remotes{ + { + Remote: &git.Remote{Name: "origin"}, + Repo: ghrepo.New("OWNER", "REPO"), + }, + }, nil + } + + return remotes, nil + }, + Branch: func() (string, error) { + return "feature", nil + }, + } + + cmd := NewCmdReview(factory, nil) + + argv, err := shlex.Split(cli) + if err != nil { + return nil, err + } + cmd.SetArgs(argv) + + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(ioutil.Discard) + cmd.SetErr(ioutil.Discard) + + _, err = cmd.ExecuteC() + return &test.CmdOut{ + OutBuf: stdout, + ErrBuf: stderr, + }, err +} + func TestPRReview_validation(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() for _, cmd := range []string{ - `pr review --approve --comment 123`, - `pr review --approve --comment -b"hey" 123`, + `--approve --comment 123`, + `--approve --comment -b"hey" 123`, } { - http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "pullRequest": { "number": 123 } - } } } - `)) - _, err := RunCommand(cmd) - if err == nil { - t.Fatal("expected error") - } - eq(t, err.Error(), "did not understand desired review action: need exactly one of --approve, --request-changes, or --comment") + t.Run(cmd, func(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + + _, err := runCommand(http, nil, false, cmd) + if err == nil { + t.Fatal("expected error") + } + assert.Equal(t, "did not understand desired review action: need exactly one of --approve, --request-changes, or --comment", err.Error()) + }) + } } func TestPRReview_bad_body(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "pullRequest": { "number": 123 } - } } } - `)) - _, err := RunCommand(`pr review 123 -b "radical"`) + http := &httpmock.Registry{} + defer http.Verify(t) + + _, err := runCommand(http, nil, false, `123 -b "radical"`) if err == nil { t.Fatal("expected error") } - eq(t, err.Error(), "did not understand desired review action: --body unsupported without --approve, --request-changes, or --comment") + assert.Equal(t, "did not understand desired review action: --body unsupported without --approve, --request-changes, or --comment", err.Error()) } func TestPRReview_url_arg(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - defer stubTerminal(true)() - http := initFakeHTTP() + http := &httpmock.Registry{} + defer http.Verify(t) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequest": { "id": "foobar123", @@ -72,7 +126,7 @@ func TestPRReview_url_arg(t *testing.T) { } } } } `)) http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) - output, err := RunCommand("pr review --approve https://github.com/OWNER/REPO/pull/123") + output, err := runCommand(http, nil, true, "--approve https://github.com/OWNER/REPO/pull/123") if err != nil { t.Fatalf("error running pr review: %s", err) } @@ -91,16 +145,14 @@ func TestPRReview_url_arg(t *testing.T) { }{} _ = json.Unmarshal(bodyBytes, &reqBody) - eq(t, reqBody.Variables.Input.PullRequestID, "foobar123") - eq(t, reqBody.Variables.Input.Event, "APPROVE") - eq(t, reqBody.Variables.Input.Body, "") + assert.Equal(t, "foobar123", reqBody.Variables.Input.PullRequestID) + assert.Equal(t, "APPROVE", reqBody.Variables.Input.Event) + assert.Equal(t, "", reqBody.Variables.Input.Body) } func TestPRReview_number_arg(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - defer stubTerminal(true)() - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") + http := &httpmock.Registry{} + defer http.Verify(t) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequest": { "id": "foobar123", @@ -120,14 +172,14 @@ func TestPRReview_number_arg(t *testing.T) { } } } } `)) http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) - output, err := RunCommand("pr review --approve 123") + output, err := runCommand(http, nil, true, "--approve 123") if err != nil { t.Fatalf("error running pr review: %s", err) } test.ExpectLines(t, output.Stderr(), "Approved pull request #123") - bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + bodyBytes, _ := ioutil.ReadAll(http.Requests[1].Body) reqBody := struct { Variables struct { Input struct { @@ -139,16 +191,14 @@ func TestPRReview_number_arg(t *testing.T) { }{} _ = json.Unmarshal(bodyBytes, &reqBody) - eq(t, reqBody.Variables.Input.PullRequestID, "foobar123") - eq(t, reqBody.Variables.Input.Event, "APPROVE") - eq(t, reqBody.Variables.Input.Body, "") + assert.Equal(t, "foobar123", reqBody.Variables.Input.PullRequestID) + assert.Equal(t, "APPROVE", reqBody.Variables.Input.Event) + assert.Equal(t, "", reqBody.Variables.Input.Body) } func TestPRReview_no_arg(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(true)() - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") + http := &httpmock.Registry{} + defer http.Verify(t) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes": [ { "url": "https://github.com/OWNER/REPO/pull/123", @@ -159,14 +209,14 @@ func TestPRReview_no_arg(t *testing.T) { ] } } } }`)) http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) - output, err := RunCommand(`pr review --comment -b "cool story"`) + output, err := runCommand(http, nil, true, `--comment -b "cool story"`) if err != nil { t.Fatalf("error running pr review: %s", err) } test.ExpectLines(t, output.Stderr(), "Reviewed pull request #123") - bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + bodyBytes, _ := ioutil.ReadAll(http.Requests[1].Body) reqBody := struct { Variables struct { Input struct { @@ -178,37 +228,25 @@ func TestPRReview_no_arg(t *testing.T) { }{} _ = 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") + assert.Equal(t, "foobar123", reqBody.Variables.Input.PullRequestID) + assert.Equal(t, "COMMENT", reqBody.Variables.Input.Event) + assert.Equal(t, "cool story", reqBody.Variables.Input.Body) } func TestPRReview_blank_comment(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "pullRequest": { "number": 123 } - } } } - `)) + http := &httpmock.Registry{} + defer http.Verify(t) - _, err := RunCommand(`pr review --comment 123`) - eq(t, err.Error(), "did not understand desired review action: body cannot be blank for comment review") + _, err := runCommand(http, nil, false, `--comment 123`) + assert.Equal(t, "did not understand desired review action: body cannot be blank for comment review", err.Error()) } func TestPRReview_blank_request_changes(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "pullRequest": { "number": 123 } - } } } - `)) + http := &httpmock.Registry{} + defer http.Verify(t) - _, err := RunCommand(`pr review -r 123`) - eq(t, err.Error(), "did not understand desired review action: body cannot be blank for request-changes review") + _, err := runCommand(http, nil, false, `-r 123`) + assert.Equal(t, "did not understand desired review action: body cannot be blank for request-changes review", err.Error()) } func TestPRReview(t *testing.T) { @@ -218,63 +256,53 @@ func TestPRReview(t *testing.T) { ExpectedBody string } cases := []c{ - {`pr review --request-changes -b"bad"`, "REQUEST_CHANGES", "bad"}, - {`pr review --approve`, "APPROVE", ""}, - {`pr review --approve -b"hot damn"`, "APPROVE", "hot damn"}, - {`pr review --comment --body "i donno"`, "COMMENT", "i donno"}, + {`--request-changes -b"bad"`, "REQUEST_CHANGES", "bad"}, + {`--approve`, "APPROVE", ""}, + {`--approve -b"hot damn"`, "APPROVE", "hot damn"}, + {`--comment --body "i donno"`, "COMMENT", "i donno"}, } for _, kase := range cases { - initBlankContext("", "OWNER/REPO", "feature") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { "pullRequests": { "nodes": [ - { "url": "https://github.com/OWNER/REPO/pull/123", - "id": "foobar123", - "headRefName": "feature", - "baseRefName": "master" } - ] } } } } - `)) - http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) + t.Run(kase.Cmd, func(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + http.StubResponse(200, bytes.NewBufferString(` + { "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) - 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 - } + _, err := runCommand(http, nil, false, kase.Cmd) + if err != nil { + t.Fatalf("got unexpected error running %s: %s", kase.Cmd, err) } - }{} - _ = json.Unmarshal(bodyBytes, &reqBody) - eq(t, reqBody.Variables.Input.Event, kase.ExpectedEvent) - eq(t, reqBody.Variables.Input.Body, kase.ExpectedBody) + bodyBytes, _ := ioutil.ReadAll(http.Requests[1].Body) + reqBody := struct { + Variables struct { + Input struct { + Event string + Body string + } + } + }{} + _ = json.Unmarshal(bodyBytes, &reqBody) + + assert.Equal(t, kase.ExpectedEvent, reqBody.Variables.Input.Event) + assert.Equal(t, kase.ExpectedBody, reqBody.Variables.Input.Body) + }) } } 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" } - ] } } } } - `)) + http := &httpmock.Registry{} + defer http.Verify(t) - output, err := RunCommand("pr review") + output, err := runCommand(http, nil, false, "") if err == nil { t.Fatal("expected error") } @@ -285,10 +313,8 @@ func TestPRReview_nontty_insufficient_flags(t *testing.T) { } func TestPRReview_nontty(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(false)() - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") + http := &httpmock.Registry{} + defer http.Verify(t) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes": [ { "url": "https://github.com/OWNER/REPO/pull/123", @@ -300,7 +326,7 @@ func TestPRReview_nontty(t *testing.T) { `)) http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) - output, err := RunCommand("pr review -c -bcool") + output, err := runCommand(http, nil, false, "-c -bcool") if err != nil { t.Fatalf("unexpected error running command: %s", err) } @@ -308,7 +334,7 @@ func TestPRReview_nontty(t *testing.T) { assert.Equal(t, "", output.String()) assert.Equal(t, "", output.Stderr()) - bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + bodyBytes, _ := ioutil.ReadAll(http.Requests[1].Body) reqBody := struct { Variables struct { Input struct { @@ -324,10 +350,8 @@ func TestPRReview_nontty(t *testing.T) { } func TestPRReview_interactive(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(true)() - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") + http := &httpmock.Registry{} + defer http.Verify(t) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes": [ { "url": "https://github.com/OWNER/REPO/pull/123", @@ -360,7 +384,7 @@ func TestPRReview_interactive(t *testing.T) { }, }) - output, err := RunCommand(`pr review`) + output, err := runCommand(http, nil, true, "") if err != nil { t.Fatalf("got unexpected error running pr review: %s", err) } @@ -371,7 +395,7 @@ func TestPRReview_interactive(t *testing.T) { "Got:", "cool.*story") - bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + bodyBytes, _ := ioutil.ReadAll(http.Requests[1].Body) reqBody := struct { Variables struct { Input struct { @@ -382,15 +406,13 @@ func TestPRReview_interactive(t *testing.T) { }{} _ = json.Unmarshal(bodyBytes, &reqBody) - eq(t, reqBody.Variables.Input.Event, "APPROVE") - eq(t, reqBody.Variables.Input.Body, "cool story") + assert.Equal(t, "APPROVE", reqBody.Variables.Input.Event) + assert.Equal(t, "cool story", reqBody.Variables.Input.Body) } func TestPRReview_interactive_no_body(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(true)() - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") + http := &httpmock.Registry{} + defer http.Verify(t) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes": [ { "url": "https://github.com/OWNER/REPO/pull/123", @@ -399,7 +421,7 @@ func TestPRReview_interactive_no_body(t *testing.T) { "baseRefName": "master" } ] } } } } `)) - http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) + as, teardown := prompt.InitAskStubber() defer teardown() @@ -422,18 +444,16 @@ func TestPRReview_interactive_no_body(t *testing.T) { }, }) - _, err := RunCommand(`pr review`) + _, err := runCommand(http, nil, true, "") if err == nil { t.Fatal("expected error") } - eq(t, err.Error(), "this type of review cannot be blank") + assert.Equal(t, "this type of review cannot be blank", err.Error()) } func TestPRReview_interactive_blank_approve(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(true)() - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") + http := &httpmock.Registry{} + defer http.Verify(t) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes": [ { "url": "https://github.com/OWNER/REPO/pull/123", @@ -466,7 +486,7 @@ func TestPRReview_interactive_blank_approve(t *testing.T) { }, }) - output, err := RunCommand(`pr review`) + output, err := runCommand(http, nil, true, "") if err != nil { t.Fatalf("got unexpected error running pr review: %s", err) } @@ -478,7 +498,7 @@ func TestPRReview_interactive_blank_approve(t *testing.T) { test.ExpectLines(t, output.Stderr(), "Approved pull request #123") - bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + bodyBytes, _ := ioutil.ReadAll(http.Requests[1].Body) reqBody := struct { Variables struct { Input struct { @@ -489,7 +509,6 @@ func TestPRReview_interactive_blank_approve(t *testing.T) { }{} _ = json.Unmarshal(bodyBytes, &reqBody) - eq(t, reqBody.Variables.Input.Event, "APPROVE") - eq(t, reqBody.Variables.Input.Body, "") - + assert.Equal(t, "APPROVE", reqBody.Variables.Input.Event) + assert.Equal(t, "", reqBody.Variables.Input.Body) } diff --git a/pkg/cmdutil/factory.go b/pkg/cmdutil/factory.go index 9a6cd5c6f..62ed5d802 100644 --- a/pkg/cmdutil/factory.go +++ b/pkg/cmdutil/factory.go @@ -15,4 +15,5 @@ type Factory struct { BaseRepo func() (ghrepo.Interface, error) Remotes func() (context.Remotes, error) Config func() (config.Config, error) + Branch func() (string, error) } diff --git a/pkg/cmdutil/legacy.go b/pkg/cmdutil/legacy.go new file mode 100644 index 000000000..57cdf855b --- /dev/null +++ b/pkg/cmdutil/legacy.go @@ -0,0 +1,23 @@ +package cmdutil + +import ( + "fmt" + "os" + + "github.com/cli/cli/internal/config" +) + +// TODO: consider passing via Factory +// TODO: support per-hostname settings +func DetermineEditor(cf func() (config.Config, error)) (string, error) { + editorCommand := os.Getenv("GH_EDITOR") + if editorCommand == "" { + cfg, err := cf() + if err != nil { + return "", fmt.Errorf("could not read config: %w", err) + } + editorCommand, _ = cfg.Get("", "editor") + } + + return editorCommand, nil +}