From 9bdc63c4ca6de084d5a9db3b068aa6314e1e81e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 28 Apr 2021 19:25:27 +0200 Subject: [PATCH 1/6] Eliminate API overfetching in `pr` commands This completely rewrites the PR lookup mechanism so that the caller must specify the GraphQL fields to query for each PR. Additionally, this fixes some export problems with `pr view --json`. Features: - Each pr command now gets assigned a concept of a Finder. This makes it easier to stub the PR in tests without having to stub the underlying HTTP calls or git invocations. - `pr view --web` is much faster since it only fetches the "url" field. - `pr diff 123` now skips a whole API call where a whole PR was unnecessarily preloaded just to access its diff in a subsequent call. - PullRequestGraphQL query builder is now used to construct queries. - A bunch of individual commands are now freed of having to know about concepts such as BaseRepo, Branch, Config, or Remotes. --- api/queries_comments.go | 18 - api/queries_issue.go | 4 + api/queries_pr.go | 339 ++----------- api/queries_pr_test.go | 29 -- api/reaction_groups.go | 9 - pkg/cmd/pr/checkout/checkout.go | 32 +- pkg/cmd/pr/checks/checks.go | 34 +- pkg/cmd/pr/checks/checks_test.go | 23 +- pkg/cmd/pr/close/close.go | 25 +- pkg/cmd/pr/comment/comment.go | 32 +- pkg/cmd/pr/comment/comment_test.go | 33 +- pkg/cmd/pr/create/create.go | 13 +- pkg/cmd/pr/diff/diff.go | 26 +- pkg/cmd/pr/edit/edit.go | 25 +- pkg/cmd/pr/edit/edit_test.go | 2 - pkg/cmd/pr/merge/merge.go | 30 +- pkg/cmd/pr/merge/merge_test.go | 479 ++++++++---------- pkg/cmd/pr/ready/ready.go | 20 +- pkg/cmd/pr/reopen/reopen.go | 25 +- pkg/cmd/pr/review/review.go | 27 +- pkg/cmd/pr/shared/finder.go | 328 ++++++++++++ .../shared/{lookup_test.go => finder_test.go} | 30 +- pkg/cmd/pr/shared/lookup.go | 121 ----- pkg/cmd/pr/view/view.go | 96 +--- pkg/cmd/run/shared/shared.go | 5 +- 25 files changed, 769 insertions(+), 1036 deletions(-) create mode 100644 pkg/cmd/pr/shared/finder.go rename pkg/cmd/pr/shared/{lookup_test.go => finder_test.go} (78%) delete mode 100644 pkg/cmd/pr/shared/lookup.go diff --git a/api/queries_comments.go b/api/queries_comments.go index db6ad25e7..f02322c22 100644 --- a/api/queries_comments.go +++ b/api/queries_comments.go @@ -135,24 +135,6 @@ func CommentCreate(client *Client, repoHost string, params CommentCreateInput) ( return mutation.AddComment.CommentEdge.Node.URL, nil } -func commentsFragment() string { - return `comments(last: 1) { - nodes { - author { - login - } - authorAssociation - body - createdAt - includesCreatedEdit - isMinimized - minimizedReason - ` + reactionGroupsFragment() + ` - } - totalCount - }` -} - func (c Comment) AuthorLogin() string { return c.Author.Login } diff --git a/api/queries_issue.go b/api/queries_issue.go index 7804a7613..35e4e418f 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -98,6 +98,10 @@ type IssuesDisabledError struct { error } +type Owner struct { + Login string `json:"login"` +} + type Author struct { Login string `json:"login"` } diff --git a/api/queries_pr.go b/api/queries_pr.go index e3a575562..38f9f5ee4 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -6,7 +6,6 @@ import ( "fmt" "io" "net/http" - "sort" "strings" "time" @@ -34,6 +33,7 @@ type PullRequest struct { Number int Title string State string + Closed bool URL string BaseRefName string HeadRefName string @@ -57,10 +57,8 @@ type PullRequest struct { Author Author MergedBy *Author - HeadRepositoryOwner struct { - Login string `json:"login"` - } - HeadRepository struct { + HeadRepositoryOwner Owner + HeadRepository struct { Name string } IsCrossRepository bool @@ -77,27 +75,7 @@ type PullRequest struct { Commits struct { TotalCount int - Nodes []struct { - Commit struct { - Oid string - StatusCheckRollup struct { - Contexts struct { - Nodes []struct { - TypeName string `json:"__typename"` - Name string `json:"name"` - Context string `json:"context,omitempty"` - State string `json:"state,omitempty"` - Status string `json:"status"` - Conclusion string `json:"conclusion"` - StartedAt time.Time `json:"startedAt"` - CompletedAt time.Time `json:"completedAt"` - DetailsURL string `json:"detailsUrl"` - TargetURL string `json:"targetUrl,omitempty"` - } - } - } - } - } + Nodes []PullRequestCommit } Assignees Assignees Labels Labels @@ -113,6 +91,37 @@ type Commit struct { OID string `json:"oid"` } +type PullRequestCommit struct { + Commit PullRequestCommitCommit +} + +// PullRequestCommitCommit is like "Commit" but with StatusCheckRollup +type PullRequestCommitCommit struct { + Oid string + StatusCheckRollup struct { + Contexts struct { + Nodes []struct { + TypeName string `json:"__typename"` + Name string `json:"name"` + Context string `json:"context,omitempty"` + State string `json:"state,omitempty"` + Status string `json:"status"` + Conclusion string `json:"conclusion"` + StartedAt time.Time `json:"startedAt"` + CompletedAt time.Time `json:"completedAt"` + DetailsURL string `json:"detailsUrl"` + TargetURL string `json:"targetUrl,omitempty"` + } + } + } +} + +func (pr *PullRequest) StubCommit(oid string) { + pr.Commits.Nodes = append(pr.Commits.Nodes, PullRequestCommit{ + Commit: PullRequestCommitCommit{Oid: oid}, + }) +} + type PullRequestFile struct { Path string `json:"path"` Additions int `json:"additions"` @@ -138,14 +147,6 @@ func (r ReviewRequests) Logins() []string { return logins } -type NotFoundError struct { - error -} - -func (err *NotFoundError) Unwrap() error { - return err.error -} - func (pr PullRequest) HeadLabel() string { if pr.IsCrossRepository { return fmt.Sprintf("%s:%s", pr.HeadRepositoryOwner.Login, pr.HeadRefName) @@ -247,7 +248,7 @@ func (c Client) PullRequestDiff(baseRepo ghrepo.Interface, prNumber int) (io.Rea } if resp.StatusCode == 404 { - return nil, &NotFoundError{errors.New("pull request not found")} + return nil, errors.New("pull request not found") } else if resp.StatusCode != 200 { return nil, HandleHTTPError(resp) } @@ -560,274 +561,6 @@ func pullRequestFragment(httpClient *http.Client, hostname string) (string, erro return fragments, nil } -func prCommitsFragment(httpClient *http.Client, hostname string) (string, error) { - cachedClient := NewCachedClient(httpClient, time.Hour*24) - if prFeatures, err := determinePullRequestFeatures(cachedClient, hostname); err != nil { - return "", err - } else if !prFeatures.HasStatusCheckRollup { - return "", nil - } - - return ` - commits(last: 1) { - totalCount - nodes { - commit { - oid - statusCheckRollup { - contexts(last: 100) { - nodes { - ...on StatusContext { - context - state - targetUrl - } - ...on CheckRun { - name - status - conclusion - startedAt - completedAt - detailsUrl - } - } - } - } - } - } - } - `, nil -} - -func PullRequestByNumber(client *Client, repo ghrepo.Interface, number int) (*PullRequest, error) { - type response struct { - Repository struct { - PullRequest PullRequest - } - } - - statusesFragment, err := prCommitsFragment(client.http, repo.RepoHost()) - if err != nil { - return nil, err - } - - query := ` - query PullRequestByNumber($owner: String!, $repo: String!, $pr_number: Int!) { - repository(owner: $owner, name: $repo) { - pullRequest(number: $pr_number) { - id - url - number - title - state - closed - body - mergeable - additions - deletions - author { - login - } - ` + statusesFragment + ` - baseRefName - headRefName - headRepositoryOwner { - login - } - headRepository { - name - } - isCrossRepository - isDraft - maintainerCanModify - reviewRequests(first: 100) { - nodes { - requestedReviewer { - __typename - ...on User { - login - } - ...on Team { - name - } - } - } - totalCount - } - assignees(first: 100) { - nodes { - login - } - totalCount - } - labels(first: 100) { - nodes { - name - } - totalCount - } - projectCards(first: 100) { - nodes { - project { - name - } - column { - name - } - } - totalCount - } - milestone{ - title - } - ` + commentsFragment() + ` - ` + reactionGroupsFragment() + ` - } - } - }` - - variables := map[string]interface{}{ - "owner": repo.RepoOwner(), - "repo": repo.RepoName(), - "pr_number": number, - } - - var resp response - err = client.GraphQL(repo.RepoHost(), query, variables, &resp) - if err != nil { - return nil, err - } - - return &resp.Repository.PullRequest, nil -} - -func PullRequestForBranch(client *Client, repo ghrepo.Interface, baseBranch, headBranch string, stateFilters []string) (*PullRequest, error) { - type response struct { - Repository struct { - PullRequests struct { - Nodes []PullRequest - } - } - } - - statusesFragment, err := prCommitsFragment(client.http, repo.RepoHost()) - if err != nil { - return nil, err - } - - query := ` - query PullRequestForBranch($owner: String!, $repo: String!, $headRefName: String!, $states: [PullRequestState!]) { - repository(owner: $owner, name: $repo) { - pullRequests(headRefName: $headRefName, states: $states, first: 30, orderBy: { field: CREATED_AT, direction: DESC }) { - nodes { - id - number - title - state - body - mergeable - additions - deletions - author { - login - } - ` + statusesFragment + ` - url - baseRefName - headRefName - headRepositoryOwner { - login - } - headRepository { - name - } - isCrossRepository - isDraft - maintainerCanModify - reviewRequests(first: 100) { - nodes { - requestedReviewer { - __typename - ...on User { - login - } - ...on Team { - name - } - } - } - totalCount - } - assignees(first: 100) { - nodes { - login - } - totalCount - } - labels(first: 100) { - nodes { - name - } - totalCount - } - projectCards(first: 100) { - nodes { - project { - name - } - column { - name - } - } - totalCount - } - milestone{ - title - } - ` + commentsFragment() + ` - ` + reactionGroupsFragment() + ` - } - } - } - }` - - branchWithoutOwner := headBranch - if idx := strings.Index(headBranch, ":"); idx >= 0 { - branchWithoutOwner = headBranch[idx+1:] - } - - variables := map[string]interface{}{ - "owner": repo.RepoOwner(), - "repo": repo.RepoName(), - "headRefName": branchWithoutOwner, - "states": stateFilters, - } - - var resp response - err = client.GraphQL(repo.RepoHost(), query, variables, &resp) - if err != nil { - return nil, err - } - - prs := resp.Repository.PullRequests.Nodes - sortPullRequestsByState(prs) - - for _, pr := range prs { - if pr.HeadLabel() == headBranch && (baseBranch == "" || pr.BaseRefName == baseBranch) { - return &pr, nil - } - } - - return nil, &NotFoundError{fmt.Errorf("no pull requests found for branch %q", headBranch)} -} - -// sortPullRequestsByState sorts a PullRequest slice by open-first -func sortPullRequestsByState(prs []PullRequest) { - sort.SliceStable(prs, func(a, b int) bool { - return prs[a].State == "OPEN" - }) -} - // CreatePullRequest creates a pull request in a GitHub repository func CreatePullRequest(client *Client, repo *Repository, params map[string]interface{}) (*PullRequest, error) { query := ` diff --git a/api/queries_pr_test.go b/api/queries_pr_test.go index 5441be950..886f16dd0 100644 --- a/api/queries_pr_test.go +++ b/api/queries_pr_test.go @@ -158,32 +158,3 @@ func Test_determinePullRequestFeatures(t *testing.T) { }) } } - -func Test_sortPullRequestsByState(t *testing.T) { - prs := []PullRequest{ - { - BaseRefName: "test1", - State: "MERGED", - }, - { - BaseRefName: "test2", - State: "CLOSED", - }, - { - BaseRefName: "test3", - State: "OPEN", - }, - } - - sortPullRequestsByState(prs) - - if prs[0].BaseRefName != "test3" { - t.Errorf("prs[0]: got %s, want %q", prs[0].BaseRefName, "test3") - } - if prs[1].BaseRefName != "test1" { - t.Errorf("prs[1]: got %s, want %q", prs[1].BaseRefName, "test1") - } - if prs[2].BaseRefName != "test2" { - t.Errorf("prs[2]: got %s, want %q", prs[2].BaseRefName, "test2") - } -} diff --git a/api/reaction_groups.go b/api/reaction_groups.go index 769edc6aa..08ae53040 100644 --- a/api/reaction_groups.go +++ b/api/reaction_groups.go @@ -57,12 +57,3 @@ var reactionEmoji = map[string]string{ "ROCKET": "\U0001f680", "EYES": "\U0001f440", } - -func reactionGroupsFragment() string { - return `reactionGroups { - content - users { - totalCount - } - }` -} diff --git a/pkg/cmd/pr/checkout/checkout.go b/pkg/cmd/pr/checkout/checkout.go index f7f73bb28..03d04a1a9 100644 --- a/pkg/cmd/pr/checkout/checkout.go +++ b/pkg/cmd/pr/checkout/checkout.go @@ -24,10 +24,11 @@ type CheckoutOptions 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) + Finder shared.PRFinder + SelectorArg string RecurseSubmodules bool Force bool @@ -48,8 +49,7 @@ func NewCmdCheckout(f *cmdutil.Factory, runF func(*CheckoutOptions) error) *cobr Short: "Check out a pull request in git", Args: cmdutil.ExactArgs(1, "argument required"), RunE: func(cmd *cobra.Command, args []string) error { - // support `-R, --repo` override - opts.BaseRepo = f.BaseRepo + opts.Finder = shared.NewFinder(f) if len(args) > 0 { opts.SelectorArg = args[0] @@ -70,18 +70,10 @@ func NewCmdCheckout(f *cmdutil.Factory, runF func(*CheckoutOptions) error) *cobr } func checkoutRun(opts *CheckoutOptions) error { - remotes, err := opts.Remotes() - if err != nil { - return err + findOptions := shared.FindOptions{ + Selector: opts.SelectorArg, } - - httpClient, err := opts.HttpClient() - if err != nil { - return err - } - apiClient := api.NewClientFromHTTP(httpClient) - - pr, baseRepo, err := shared.PRFromArgs(apiClient, opts.BaseRepo, opts.Branch, opts.Remotes, opts.SelectorArg) + pr, baseRepo, err := opts.Finder.Find(findOptions) if err != nil { return err } @@ -90,8 +82,12 @@ func checkoutRun(opts *CheckoutOptions) error { if err != nil { return err } - protocol, _ := cfg.Get(baseRepo.RepoHost(), "git_protocol") + + remotes, err := opts.Remotes() + if err != nil { + return err + } baseRemote, _ := remotes.FindByRepo(baseRepo.RepoOwner(), baseRepo.RepoName()) baseURLOrName := ghrepo.FormatRemoteURL(baseRepo, protocol) if baseRemote != nil { @@ -112,6 +108,12 @@ func checkoutRun(opts *CheckoutOptions) error { if headRemote != nil { cmdQueue = append(cmdQueue, cmdsForExistingRemote(headRemote, pr, opts)...) } else { + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + apiClient := api.NewClientFromHTTP(httpClient) + defaultBranch, err := api.RepoDefaultBranch(apiClient, baseRepo) if err != nil { return err diff --git a/pkg/cmd/pr/checks/checks.go b/pkg/cmd/pr/checks/checks.go index 1a11f3d44..b9091379f 100644 --- a/pkg/cmd/pr/checks/checks.go +++ b/pkg/cmd/pr/checks/checks.go @@ -3,13 +3,10 @@ package checks import ( "errors" "fmt" - "net/http" "sort" "time" "github.com/MakeNowJust/heredoc" - "github.com/cli/cli/api" - "github.com/cli/cli/context" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/cmd/pr/shared" "github.com/cli/cli/pkg/cmdutil" @@ -23,26 +20,19 @@ type browser interface { } type ChecksOptions struct { - HttpClient func() (*http.Client, error) - IO *iostreams.IOStreams - Browser browser - BaseRepo func() (ghrepo.Interface, error) - Branch func() (string, error) - Remotes func() (context.Remotes, error) + IO *iostreams.IOStreams + Browser browser - WebMode bool + Finder shared.PRFinder SelectorArg string + WebMode bool } func NewCmdChecks(f *cmdutil.Factory, runF func(*ChecksOptions) error) *cobra.Command { opts := &ChecksOptions{ - IO: f.IOStreams, - HttpClient: f.HttpClient, - Branch: f.Branch, - Remotes: f.Remotes, - BaseRepo: f.BaseRepo, - Browser: f.Browser, + IO: f.IOStreams, + Browser: f.Browser, } cmd := &cobra.Command{ @@ -56,8 +46,7 @@ func NewCmdChecks(f *cmdutil.Factory, runF func(*ChecksOptions) error) *cobra.Co `), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - // support `-R, --repo` override - opts.BaseRepo = f.BaseRepo + opts.Finder = shared.NewFinder(f) if repoOverride, _ := cmd.Flags().GetString("repo"); repoOverride != "" && len(args) == 0 { return &cmdutil.FlagError{Err: errors.New("argument required when using the --repo flag")} @@ -81,13 +70,10 @@ func NewCmdChecks(f *cmdutil.Factory, runF func(*ChecksOptions) error) *cobra.Co } func checksRun(opts *ChecksOptions) error { - httpClient, err := opts.HttpClient() - if err != nil { - return err + findOptions := shared.FindOptions{ + Selector: opts.SelectorArg, } - apiClient := api.NewClientFromHTTP(httpClient) - - pr, baseRepo, err := shared.PRFromArgs(apiClient, opts.BaseRepo, opts.Branch, opts.Remotes, opts.SelectorArg) + pr, baseRepo, err := opts.Finder.Find(findOptions) if err != nil { return err } diff --git a/pkg/cmd/pr/checks/checks_test.go b/pkg/cmd/pr/checks/checks_test.go index fdcce3f9e..ca743d856 100644 --- a/pkg/cmd/pr/checks/checks_test.go +++ b/pkg/cmd/pr/checks/checks_test.go @@ -2,10 +2,8 @@ package checks import ( "bytes" - "net/http" "testing" - "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/internal/run" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/httpmock" @@ -174,10 +172,7 @@ func Test_checksRun(t *testing.T) { io.SetStdoutTTY(!tt.nontty) opts := &ChecksOptions{ - IO: io, - BaseRepo: func() (ghrepo.Interface, error) { - return ghrepo.New("OWNER", "REPO"), nil - }, + IO: io, SelectorArg: "123", } @@ -190,10 +185,6 @@ func Test_checksRun(t *testing.T) { reg.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.FileResponse(tt.fixture)) } - opts.HttpClient = func() (*http.Client, error) { - return &http.Client{Transport: reg}, nil - } - err := checksRun(opts) if tt.wantErr != "" { assert.EqualError(t, err, tt.wantErr) @@ -246,15 +237,9 @@ func TestChecksRun_web(t *testing.T) { defer teardown(t) err := checksRun(&ChecksOptions{ - IO: io, - Browser: browser, - WebMode: true, - HttpClient: func() (*http.Client, error) { - return &http.Client{Transport: reg}, nil - }, - BaseRepo: func() (ghrepo.Interface, error) { - return ghrepo.New("OWNER", "REPO"), nil - }, + IO: io, + Browser: browser, + WebMode: true, SelectorArg: "123", }) assert.NoError(t, err) diff --git a/pkg/cmd/pr/close/close.go b/pkg/cmd/pr/close/close.go index 850bd7631..a1e6e3dff 100644 --- a/pkg/cmd/pr/close/close.go +++ b/pkg/cmd/pr/close/close.go @@ -6,8 +6,6 @@ import ( "github.com/cli/cli/api" "github.com/cli/cli/git" - "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" @@ -16,11 +14,11 @@ import ( type CloseOptions struct { HttpClient func() (*http.Client, error) - Config func() (config.Config, error) IO *iostreams.IOStreams - BaseRepo func() (ghrepo.Interface, error) Branch func() (string, error) + Finder shared.PRFinder + SelectorArg string DeleteBranch bool DeleteLocalBranch bool @@ -30,7 +28,6 @@ func NewCmdClose(f *cmdutil.Factory, runF func(*CloseOptions) error) *cobra.Comm opts := &CloseOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, - Config: f.Config, Branch: f.Branch, } @@ -39,8 +36,7 @@ func NewCmdClose(f *cmdutil.Factory, runF func(*CloseOptions) error) *cobra.Comm Short: "Close a pull request", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - // support `-R, --repo` override - opts.BaseRepo = f.BaseRepo + opts.Finder = shared.NewFinder(f) if len(args) > 0 { opts.SelectorArg = args[0] @@ -62,13 +58,10 @@ func NewCmdClose(f *cmdutil.Factory, runF func(*CloseOptions) error) *cobra.Comm func closeRun(opts *CloseOptions) error { cs := opts.IO.ColorScheme() - httpClient, err := opts.HttpClient() - if err != nil { - return err + findOptions := shared.FindOptions{ + Selector: opts.SelectorArg, } - apiClient := api.NewClientFromHTTP(httpClient) - - pr, baseRepo, err := shared.PRFromArgs(apiClient, opts.BaseRepo, nil, nil, opts.SelectorArg) + pr, baseRepo, err := opts.Finder.Find(findOptions) if err != nil { return err } @@ -81,6 +74,12 @@ func closeRun(opts *CloseOptions) error { return nil } + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + apiClient := api.NewClientFromHTTP(httpClient) + err = api.PullRequestClose(apiClient, baseRepo, pr) if err != nil { return fmt.Errorf("API call failed: %w", err) diff --git a/pkg/cmd/pr/comment/comment.go b/pkg/cmd/pr/comment/comment.go index a9ec5e9d3..85845259c 100644 --- a/pkg/cmd/pr/comment/comment.go +++ b/pkg/cmd/pr/comment/comment.go @@ -2,11 +2,8 @@ package comment import ( "errors" - "net/http" "github.com/MakeNowJust/heredoc" - "github.com/cli/cli/api" - "github.com/cli/cli/context" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/cmd/pr/shared" "github.com/cli/cli/pkg/cmdutil" @@ -48,7 +45,13 @@ func NewCmdComment(f *cmdutil.Factory, runF func(*shared.CommentableOptions) err if len(args) > 0 { selector = args[0] } - opts.RetrieveCommentable = retrievePR(f.HttpClient, f.BaseRepo, f.Branch, f.Remotes, selector) + finder := shared.NewFinder(f) + opts.RetrieveCommentable = func() (shared.Commentable, ghrepo.Interface, error) { + return finder.Find(shared.FindOptions{ + Selector: selector, + Fields: []string{"id", "url"}, + }) + } return shared.CommentablePreRun(cmd, opts) }, RunE: func(cmd *cobra.Command, args []string) error { @@ -74,24 +77,3 @@ func NewCmdComment(f *cmdutil.Factory, runF func(*shared.CommentableOptions) err return cmd } - -func retrievePR(httpClient func() (*http.Client, error), - baseRepo func() (ghrepo.Interface, error), - branch func() (string, error), - remotes func() (context.Remotes, error), - selector string) func() (shared.Commentable, ghrepo.Interface, error) { - return func() (shared.Commentable, ghrepo.Interface, error) { - httpClient, err := httpClient() - if err != nil { - return nil, nil, err - } - apiClient := api.NewClientFromHTTP(httpClient) - - pr, repo, err := shared.PRFromArgs(apiClient, baseRepo, branch, remotes, selector) - if err != nil { - return nil, nil, err - } - - return pr, repo, nil - } -} diff --git a/pkg/cmd/pr/comment/comment_test.go b/pkg/cmd/pr/comment/comment_test.go index 429af7cda..859a57069 100644 --- a/pkg/cmd/pr/comment/comment_test.go +++ b/pkg/cmd/pr/comment/comment_test.go @@ -8,7 +8,7 @@ import ( "path/filepath" "testing" - "github.com/cli/cli/context" + "github.com/cli/cli/api" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/cmd/pr/shared" "github.com/cli/cli/pkg/cmdutil" @@ -224,7 +224,6 @@ func Test_commentRun(t *testing.T) { ConfirmSubmitSurvey: func() (bool, error) { return true, nil }, }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - mockPullRequestFromNumber(t, reg) mockCommentCreate(t, reg) }, stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-456\n", @@ -238,9 +237,6 @@ func Test_commentRun(t *testing.T) { OpenInBrowser: func(string) error { return nil }, }, - httpStubs: func(t *testing.T, reg *httpmock.Registry) { - mockPullRequestFromNumber(t, reg) - }, stderr: "Opening github.com/OWNER/REPO/pull/123 in your browser.\n", }, { @@ -253,7 +249,6 @@ func Test_commentRun(t *testing.T) { EditSurvey: func() (string, error) { return "comment body", nil }, }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - mockPullRequestFromNumber(t, reg) mockCommentCreate(t, reg) }, stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-456\n", @@ -266,7 +261,6 @@ func Test_commentRun(t *testing.T) { Body: "comment body", }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - mockPullRequestFromNumber(t, reg) mockCommentCreate(t, reg) }, stdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-456\n", @@ -280,16 +274,20 @@ func Test_commentRun(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) - tt.httpStubs(t, reg) + if tt.httpStubs != nil { + tt.httpStubs(t, reg) + } httpClient := func() (*http.Client, error) { return &http.Client{Transport: reg}, nil } - baseRepo := func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil } - branch := func() (string, error) { return "", nil } - remotes := func() (context.Remotes, error) { return nil, nil } tt.input.IO = io tt.input.HttpClient = httpClient - tt.input.RetrieveCommentable = retrievePR(httpClient, baseRepo, branch, remotes, "123") + tt.input.RetrieveCommentable = func() (shared.Commentable, ghrepo.Interface, error) { + return &api.PullRequest{ + Number: 123, + URL: "https://github.com/OWNER/REPO/pull/123", + }, ghrepo.New("OWNER", "REPO"), nil + } t.Run(tt.name, func(t *testing.T) { err := shared.CommentableRun(tt.input) @@ -300,17 +298,6 @@ func Test_commentRun(t *testing.T) { } } -func mockPullRequestFromNumber(_ *testing.T, reg *httpmock.Registry) { - reg.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "number": 123, - "url": "https://github.com/OWNER/REPO/pull/123" - } } } }`), - ) -} - func mockCommentCreate(t *testing.T, reg *httpmock.Registry) { reg.Register( httpmock.GraphQL(`mutation CommentCreate\b`), diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index fc2e70e26..c368e4e43 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -36,6 +36,7 @@ type CreateOptions struct { Remotes func() (context.Remotes, error) Branch func() (string, error) Browser browser + Finder shared.PRFinder TitleProvided bool BodyProvided bool @@ -117,6 +118,8 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co `), Args: cmdutil.NoArgsQuoteReminder, RunE: func(cmd *cobra.Command, args []string) error { + opts.Finder = shared.NewFinder(f) + opts.TitleProvided = cmd.Flags().Changed("title") opts.RepoOverride, _ = cmd.Flags().GetString("repo") noMaintainerEdit, _ := cmd.Flags().GetBool("no-maintainer-edit") @@ -220,9 +223,13 @@ func createRun(opts *CreateOptions) (err error) { state.Body = opts.Body } - existingPR, err := api.PullRequestForBranch( - client, ctx.BaseRepo, ctx.BaseBranch, ctx.HeadBranchLabel, []string{"OPEN"}) - var notFound *api.NotFoundError + existingPR, _, err := opts.Finder.Find(shared.FindOptions{ + Selector: ctx.HeadBranchLabel, + BaseBranch: ctx.BaseBranch, + States: []string{"OPEN"}, + Fields: []string{"url"}, + }) + var notFound *shared.NotFoundError if err != nil && !errors.As(err, ¬Found) { return fmt.Errorf("error checking for existing pull request: %w", err) } diff --git a/pkg/cmd/pr/diff/diff.go b/pkg/cmd/pr/diff/diff.go index 00a41c657..fa040a4aa 100644 --- a/pkg/cmd/pr/diff/diff.go +++ b/pkg/cmd/pr/diff/diff.go @@ -11,8 +11,6 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/api" - "github.com/cli/cli/context" - "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" @@ -22,9 +20,8 @@ import ( type DiffOptions struct { HttpClient func() (*http.Client, error) IO *iostreams.IOStreams - BaseRepo func() (ghrepo.Interface, error) - Remotes func() (context.Remotes, error) - Branch func() (string, error) + + Finder shared.PRFinder SelectorArg string UseColor string @@ -34,8 +31,6 @@ func NewCmdDiff(f *cmdutil.Factory, runF func(*DiffOptions) error) *cobra.Comman opts := &DiffOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, - Remotes: f.Remotes, - Branch: f.Branch, } cmd := &cobra.Command{ @@ -49,8 +44,7 @@ func NewCmdDiff(f *cmdutil.Factory, runF func(*DiffOptions) error) *cobra.Comman `), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - // support `-R, --repo` override - opts.BaseRepo = f.BaseRepo + opts.Finder = shared.NewFinder(f) if repoOverride, _ := cmd.Flags().GetString("repo"); repoOverride != "" && len(args) == 0 { return &cmdutil.FlagError{Err: errors.New("argument required when using the --repo flag")} @@ -81,17 +75,21 @@ func NewCmdDiff(f *cmdutil.Factory, runF func(*DiffOptions) error) *cobra.Comman } func diffRun(opts *DiffOptions) error { + findOptions := shared.FindOptions{ + Selector: opts.SelectorArg, + Fields: []string{"number"}, + } + pr, baseRepo, err := opts.Finder.Find(findOptions) + if err != nil { + return err + } + httpClient, err := opts.HttpClient() if err != nil { return err } apiClient := api.NewClientFromHTTP(httpClient) - pr, baseRepo, err := shared.PRFromArgs(apiClient, opts.BaseRepo, opts.Branch, opts.Remotes, opts.SelectorArg) - if err != nil { - return err - } - diff, err := apiClient.PullRequestDiff(baseRepo, pr.Number) if err != nil { return fmt.Errorf("could not find pull request diff: %w", err) diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index dbf0321f9..888dc6ec2 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -7,7 +7,6 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/api" - "github.com/cli/cli/context" "github.com/cli/cli/internal/config" "github.com/cli/cli/internal/ghrepo" shared "github.com/cli/cli/pkg/cmd/pr/shared" @@ -20,10 +19,8 @@ import ( type EditOptions struct { HttpClient func() (*http.Client, error) IO *iostreams.IOStreams - BaseRepo func() (ghrepo.Interface, error) - Remotes func() (context.Remotes, error) - Branch func() (string, error) + Finder shared.PRFinder Surveyor Surveyor Fetcher EditableOptionsFetcher EditorRetriever EditorRetriever @@ -38,8 +35,6 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman opts := &EditOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, - Remotes: f.Remotes, - Branch: f.Branch, Surveyor: surveyor{}, Fetcher: fetcher{}, EditorRetriever: editorRetriever{config: f.Config}, @@ -66,8 +61,7 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman `), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - // support `-R, --repo` override - opts.BaseRepo = f.BaseRepo + opts.Finder = shared.NewFinder(f) if len(args) > 0 { opts.SelectorArg = args[0] @@ -155,13 +149,10 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman } func editRun(opts *EditOptions) error { - httpClient, err := opts.HttpClient() - if err != nil { - return err + findOptions := shared.FindOptions{ + Selector: opts.SelectorArg, } - apiClient := api.NewClientFromHTTP(httpClient) - - pr, repo, err := shared.PRFromArgs(apiClient, opts.BaseRepo, opts.Branch, opts.Remotes, opts.SelectorArg) + pr, repo, err := opts.Finder.Find(findOptions) if err != nil { return err } @@ -184,6 +175,12 @@ func editRun(opts *EditOptions) error { } } + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + apiClient := api.NewClientFromHTTP(httpClient) + opts.IO.StartProgressIndicator() err = opts.Fetcher.EditableOptionsFetch(apiClient, repo, &editable) opts.IO.StopProgressIndicator() diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index 586036910..c918f4a4c 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -450,11 +450,9 @@ func Test_editRun(t *testing.T) { tt.httpStubs(t, reg) httpClient := func() (*http.Client, error) { return &http.Client{Transport: reg}, nil } - baseRepo := func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil } tt.input.IO = io tt.input.HttpClient = httpClient - tt.input.BaseRepo = baseRepo t.Run(tt.name, func(t *testing.T) { err := editRun(tt.input) diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index 31a91d6be..298d59b7f 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -8,10 +8,8 @@ import ( "github.com/AlecAivazis/survey/v2" "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/pkg/cmd/pr/shared" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" @@ -26,12 +24,11 @@ type editor interface { type MergeOptions 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) + Finder shared.PRFinder + SelectorArg string DeleteBranch bool MergeMethod PullRequestMergeMethod @@ -52,8 +49,6 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm opts := &MergeOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, - Config: f.Config, - Remotes: f.Remotes, Branch: f.Branch, } @@ -76,8 +71,7 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm `), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - // support `-R, --repo` override - opts.BaseRepo = f.BaseRepo + opts.Finder = shared.NewFinder(f) if repoOverride, _ := cmd.Flags().GetString("repo"); repoOverride != "" && len(args) == 0 { return &cmdutil.FlagError{Err: errors.New("argument required when using the --repo flag")} @@ -136,7 +130,7 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm opts.Editor = &userEditor{ io: opts.IO, - config: opts.Config, + config: f.Config, } if runF != nil { @@ -160,19 +154,23 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm func mergeRun(opts *MergeOptions) error { cs := opts.IO.ColorScheme() - httpClient, err := opts.HttpClient() - if err != nil { - return err + findOptions := shared.FindOptions{ + Selector: opts.SelectorArg, + Fields: []string{"id", "number", "state", "title", "commits", "mergeable", "headRepositoryOwner", "headRefName"}, } - apiClient := api.NewClientFromHTTP(httpClient) - - pr, baseRepo, err := shared.PRFromArgs(apiClient, opts.BaseRepo, opts.Branch, opts.Remotes, opts.SelectorArg) + pr, baseRepo, err := opts.Finder.Find(findOptions) if err != nil { return err } isTerminal := opts.IO.IsStdoutTTY() + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + apiClient := api.NewClientFromHTTP(httpClient) + if opts.AutoMergeDisable { err := disableAutoMerge(httpClient, baseRepo, pr.ID) if err != nil { diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index 16ad0ed6d..df562e194 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -13,11 +13,9 @@ 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" + "github.com/cli/cli/pkg/cmd/pr/shared" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/pkg/iostreams" @@ -197,6 +195,14 @@ func Test_NewCmdMerge(t *testing.T) { } } +func baseRepo(owner, repo, branch string) ghrepo.Interface { + return api.InitRepoHostname(&api.Repository{ + Name: repo, + Owner: api.RepositoryOwner{Login: owner}, + DefaultBranchRef: api.BranchRef{Name: branch}, + }, "github.com") +} + func runCommand(rt http.RoundTripper, branch string, isTTY bool, cli string) (*test.CmdOut, error) { io, _, stdout, stderr := iostreams.Test() io.SetStdoutTTY(isTTY) @@ -208,24 +214,6 @@ func runCommand(rt http.RoundTripper, branch string, isTTY bool, cli string) (*t 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 api.InitRepoHostname(&api.Repository{ - Name: "REPO", - Owner: api.RepositoryOwner{Login: "OWNER"}, - DefaultBranchRef: api.BranchRef{Name: "master"}, - }, "github.com"), nil - }, - Remotes: func() (context.Remotes, error) { - return context.Remotes{ - { - Remote: &git.Remote{Name: "origin"}, - Repo: ghrepo.New("OWNER", "REPO"), - }, - }, nil - }, Branch: func() (string, error) { return branch, nil }, @@ -259,17 +247,18 @@ func initFakeHTTP() *httpmock.Registry { func TestPrMerge(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "id": "THE-ID", - "number": 1, - "title": "The title of the PR", - "state": "OPEN", - "headRefName": "blueberries", - "headRepositoryOwner": {"login": "OWNER"} - } } } }`)) + + shared.RunCommandFinder( + "1", + &api.PullRequest{ + ID: "THE-ID", + Number: 1, + State: "OPEN", + Title: "The title of the PR", + }, + baseRepo("OWNER", "REPO", "master"), + ) + http.Register( httpmock.GraphQL(`mutation PullRequestMerge\b`), httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { @@ -296,17 +285,18 @@ func TestPrMerge(t *testing.T) { func TestPrMerge_nontty(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "id": "THE-ID", - "number": 1, - "title": "The title of the PR", - "state": "OPEN", - "headRefName": "blueberries", - "headRepositoryOwner": {"login": "OWNER"} - } } } }`)) + + shared.RunCommandFinder( + "1", + &api.PullRequest{ + ID: "THE-ID", + Number: 1, + State: "OPEN", + Title: "The title of the PR", + }, + baseRepo("OWNER", "REPO", "master"), + ) + http.Register( httpmock.GraphQL(`mutation PullRequestMerge\b`), httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { @@ -330,17 +320,18 @@ func TestPrMerge_nontty(t *testing.T) { func TestPrMerge_withRepoFlag(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "id": "THE-ID", - "number": 1, - "title": "The title of the PR", - "state": "OPEN", - "headRefName": "blueberries", - "headRepositoryOwner": {"login": "OWNER"} - } } } }`)) + + shared.RunCommandFinder( + "1", + &api.PullRequest{ + ID: "THE-ID", + Number: 1, + State: "OPEN", + Title: "The title of the PR", + }, + baseRepo("OWNER", "REPO", "master"), + ) + http.Register( httpmock.GraphQL(`mutation PullRequestMerge\b`), httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { @@ -367,10 +358,19 @@ func TestPrMerge_withRepoFlag(t *testing.T) { func TestPrMerge_deleteBranch(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - // FIXME: references fixture from another package - httpmock.FileResponse("../view/fixtures/prViewPreviewWithMetadataByBranch.json")) + + shared.RunCommandFinder( + "", + &api.PullRequest{ + ID: "PR_10", + Number: 10, + State: "OPEN", + Title: "Blueberries are a good fruit", + HeadRefName: "blueberries", + }, + baseRepo("OWNER", "REPO", "master"), + ) + http.Register( httpmock.GraphQL(`mutation PullRequestMerge\b`), httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { @@ -385,8 +385,6 @@ func TestPrMerge_deleteBranch(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) - cs.Register(`git .+ show .+ HEAD`, 1, "") - cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "") cs.Register(`git checkout master`, 0, "") cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "") cs.Register(`git branch -D blueberries`, 0, "") @@ -406,10 +404,19 @@ func TestPrMerge_deleteBranch(t *testing.T) { func TestPrMerge_deleteNonCurrentBranch(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - // FIXME: references fixture from another package - httpmock.FileResponse("../view/fixtures/prViewPreviewWithMetadataByBranch.json")) + + shared.RunCommandFinder( + "blueberries", + &api.PullRequest{ + ID: "PR_10", + Number: 10, + State: "OPEN", + Title: "Blueberries are a good fruit", + HeadRefName: "blueberries", + }, + baseRepo("OWNER", "REPO", "master"), + ) + http.Register( httpmock.GraphQL(`mutation PullRequestMerge\b`), httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { @@ -439,59 +446,24 @@ func TestPrMerge_deleteNonCurrentBranch(t *testing.T) { `), output.Stderr()) } -func TestPrMerge_noPrNumberGiven(t *testing.T) { - http := initFakeHTTP() - defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - // FIXME: references fixture from another package - httpmock.FileResponse("../view/fixtures/prViewPreviewWithMetadataByBranch.json")) - http.Register( - httpmock.GraphQL(`mutation PullRequestMerge\b`), - httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { - assert.Equal(t, "PR_10", input["pullRequestId"].(string)) - assert.Equal(t, "MERGE", input["mergeMethod"].(string)) - assert.NotContains(t, input, "commitHeadline") - })) - - cs, cmdTeardown := run.Stub() - defer cmdTeardown(t) - - cs.Register(`git .+ show .+ HEAD`, 1, "") - cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "") - - output, err := runCommand(http, "blueberries", true, "pr merge --merge") - if err != nil { - t.Fatalf("error running command `pr merge`: %v", err) - } - - assert.Equal(t, "", output.String()) - assert.Equal(t, heredoc.Doc(` - ✓ Merged pull request #10 (Blueberries are a good fruit) - `), output.Stderr()) -} - func Test_nonDivergingPullRequest(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes": [{ - "headRefName": "blueberries", - "headRepositoryOwner": {"login": "OWNER"}, - "id": "PR_10", - "title": "Blueberries are a good fruit", - "number": 10, - "commits": { - "nodes": [{ - "commit": { - "oid": "COMMITSHA1" - } - }], - "totalCount": 1 - } - }] } } } }`)) + + pr := &api.PullRequest{ + ID: "PR_10", + Number: 10, + Title: "Blueberries are a good fruit", + State: "OPEN", + } + pr.StubCommit("COMMITSHA1") + + shared.RunCommandFinder( + "", + pr, + baseRepo("OWNER", "REPO", "master"), + ) + http.Register( httpmock.GraphQL(`mutation PullRequestMerge\b`), httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { @@ -504,7 +476,6 @@ func Test_nonDivergingPullRequest(t *testing.T) { defer cmdTeardown(t) cs.Register(`git .+ show .+ HEAD`, 0, "COMMITSHA1,title") - cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "") output, err := runCommand(http, "blueberries", true, "pr merge --merge") if err != nil { @@ -519,24 +490,21 @@ func Test_nonDivergingPullRequest(t *testing.T) { func Test_divergingPullRequestWarning(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes": [{ - "headRefName": "blueberries", - "headRepositoryOwner": {"login": "OWNER"}, - "id": "PR_10", - "title": "Blueberries are a good fruit", - "number": 10, - "commits": { - "nodes": [{ - "commit": { - "oid": "COMMITSHA1" - } - }], - "totalCount": 1 - } - }] } } } }`)) + + pr := &api.PullRequest{ + ID: "PR_10", + Number: 10, + Title: "Blueberries are a good fruit", + State: "OPEN", + } + pr.StubCommit("COMMITSHA1") + + shared.RunCommandFinder( + "", + pr, + baseRepo("OWNER", "REPO", "master"), + ) + http.Register( httpmock.GraphQL(`mutation PullRequestMerge\b`), httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { @@ -549,7 +517,6 @@ func Test_divergingPullRequestWarning(t *testing.T) { defer cmdTeardown(t) cs.Register(`git .+ show .+ HEAD`, 0, "COMMITSHA2,title") - cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "") output, err := runCommand(http, "blueberries", true, "pr merge --merge") if err != nil { @@ -565,20 +532,18 @@ func Test_divergingPullRequestWarning(t *testing.T) { func Test_pullRequestWithoutCommits(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes": [{ - "headRefName": "blueberries", - "headRepositoryOwner": {"login": "OWNER"}, - "id": "PR_10", - "title": "Blueberries are a good fruit", - "number": 10, - "commits": { - "nodes": [], - "totalCount": 0 - } - }] } } } }`)) + + shared.RunCommandFinder( + "", + &api.PullRequest{ + ID: "PR_10", + Number: 10, + Title: "Blueberries are a good fruit", + State: "OPEN", + }, + baseRepo("OWNER", "REPO", "master"), + ) + http.Register( httpmock.GraphQL(`mutation PullRequestMerge\b`), httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { @@ -587,11 +552,9 @@ func Test_pullRequestWithoutCommits(t *testing.T) { assert.NotContains(t, input, "commitHeadline") })) - cs, cmdTeardown := run.Stub() + _, cmdTeardown := run.Stub() defer cmdTeardown(t) - cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "") - output, err := runCommand(http, "blueberries", true, "pr merge --merge") if err != nil { t.Fatalf("error running command `pr merge`: %v", err) @@ -605,17 +568,18 @@ func Test_pullRequestWithoutCommits(t *testing.T) { func TestPrMerge_rebase(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "id": "THE-ID", - "number": 2, - "title": "The title of the PR", - "state": "OPEN", - "headRefName": "blueberries", - "headRepositoryOwner": {"login": "OWNER"} - } } } }`)) + + shared.RunCommandFinder( + "2", + &api.PullRequest{ + ID: "THE-ID", + Number: 2, + Title: "The title of the PR", + State: "OPEN", + }, + baseRepo("OWNER", "REPO", "master"), + ) + http.Register( httpmock.GraphQL(`mutation PullRequestMerge\b`), httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { @@ -642,17 +606,18 @@ func TestPrMerge_rebase(t *testing.T) { func TestPrMerge_squash(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "id": "THE-ID", - "number": 3, - "title": "The title of the PR", - "state": "OPEN", - "headRefName": "blueberries", - "headRepositoryOwner": {"login": "OWNER"} - } } } }`)) + + shared.RunCommandFinder( + "3", + &api.PullRequest{ + ID: "THE-ID", + Number: 3, + Title: "The title of the PR", + State: "OPEN", + }, + baseRepo("OWNER", "REPO", "master"), + ) + http.Register( httpmock.GraphQL(`mutation PullRequestMerge\b`), httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { @@ -678,22 +643,18 @@ func TestPrMerge_squash(t *testing.T) { func TestPrMerge_alreadyMerged(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { - "pullRequest": { - "number": 4, - "title": "The title of the PR", - "state": "MERGED", - "baseRefName": "master", - "headRefName": "blueberries", - "headRepositoryOwner": { - "login": "OWNER" - }, - "isCrossRepository": false - } - } } }`)) + + shared.RunCommandFinder( + "4", + &api.PullRequest{ + ID: "THE-ID", + Number: 4, + State: "MERGED", + HeadRefName: "blueberries", + BaseRefName: "master", + }, + baseRepo("OWNER", "REPO", "master"), + ) cs, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -718,12 +679,17 @@ func TestPrMerge_alreadyMerged(t *testing.T) { func TestPrMerge_alreadyMerged_nonInteractive(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { - "pullRequest": { "number": 4, "title": "The title of the PR", "state": "MERGED"} - } } }`)) + + shared.RunCommandFinder( + "4", + &api.PullRequest{ + ID: "THE-ID", + Number: 4, + State: "MERGED", + HeadRepositoryOwner: api.Owner{Login: "monalisa"}, + }, + baseRepo("OWNER", "REPO", "master"), + ) _, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -740,15 +706,18 @@ func TestPrMerge_alreadyMerged_nonInteractive(t *testing.T) { func TestPRMerge_interactive(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes": [{ - "headRefName": "blueberries", - "headRepositoryOwner": {"login": "OWNER"}, - "id": "THE-ID", - "number": 3 - }] } } } }`)) + + shared.RunCommandFinder( + "", + &api.PullRequest{ + ID: "THE-ID", + Number: 3, + Title: "It was the best of times", + HeadRefName: "blueberries", + }, + baseRepo("OWNER", "REPO", "master"), + ) + http.Register( httpmock.GraphQL(`query RepositoryInfo\b`), httpmock.StringResponse(` @@ -765,11 +734,9 @@ func TestPRMerge_interactive(t *testing.T) { assert.NotContains(t, input, "commitHeadline") })) - cs, cmdTeardown := run.Stub() + _, cmdTeardown := run.Stub() defer cmdTeardown(t) - cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "") - as, surveyTeardown := prompt.InitAskStubber() defer surveyTeardown() @@ -789,16 +756,18 @@ func TestPRMerge_interactive(t *testing.T) { func TestPRMerge_interactiveWithDeleteBranch(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes": [{ - "headRefName": "blueberries", - "headRepositoryOwner": {"login": "OWNER"}, - "id": "THE-ID", - "title": "It was the best of times", - "number": 3 - }] } } } }`)) + + shared.RunCommandFinder( + "", + &api.PullRequest{ + ID: "THE-ID", + Number: 3, + Title: "It was the best of times", + HeadRefName: "blueberries", + }, + baseRepo("OWNER", "REPO", "master"), + ) + http.Register( httpmock.GraphQL(`query RepositoryInfo\b`), httpmock.StringResponse(` @@ -821,7 +790,6 @@ func TestPRMerge_interactiveWithDeleteBranch(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) - cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "") cs.Register(`git checkout master`, 0, "") cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "") cs.Register(`git branch -D blueberries`, 0, "") @@ -851,16 +819,6 @@ func TestPRMerge_interactiveSquashEditCommitMsg(t *testing.T) { tr := initFakeHTTP() defer tr.Verify(t) - - tr.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "headRepositoryOwner": {"login": "OWNER"}, - "id": "THE-ID", - "number": 3, - "title": "title" - } } } }`)) tr.Register( httpmock.GraphQL(`query RepositoryInfo\b`), httpmock.StringResponse(` @@ -902,25 +860,28 @@ func TestPRMerge_interactiveSquashEditCommitMsg(t *testing.T) { }, SelectorArg: "https://github.com/OWNER/REPO/pull/123", InteractiveMode: true, + Finder: shared.NewMockFinder( + "https://github.com/OWNER/REPO/pull/123", + &api.PullRequest{ID: "THE-ID", Number: 123, Title: "title"}, + ghrepo.New("OWNER", "REPO"), + ), }) assert.NoError(t, err) assert.Equal(t, "", stdout.String()) - assert.Equal(t, "✓ Squashed and merged pull request #3 (title)\n", stderr.String()) + assert.Equal(t, "✓ Squashed and merged pull request #123 (title)\n", stderr.String()) } func TestPRMerge_interactiveCancelled(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes": [{ - "headRefName": "blueberries", - "headRepositoryOwner": {"login": "OWNER"}, - "id": "THE-ID", - "number": 3 - }] } } } }`)) + + shared.RunCommandFinder( + "", + &api.PullRequest{ID: "THE-ID", Number: 123}, + ghrepo.New("OWNER", "REPO"), + ) + http.Register( httpmock.GraphQL(`query RepositoryInfo\b`), httpmock.StringResponse(` @@ -930,11 +891,9 @@ func TestPRMerge_interactiveCancelled(t *testing.T) { "squashMergeAllowed": true } } }`)) - cs, cmdTeardown := run.Stub() + _, cmdTeardown := run.Stub() defer cmdTeardown(t) - cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "") - as, surveyTeardown := prompt.InitAskStubber() defer surveyTeardown() @@ -971,18 +930,6 @@ func TestMergeRun_autoMerge(t *testing.T) { tr := initFakeHTTP() defer tr.Verify(t) - - tr.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "id": "THE-ID", - "number": 123, - "title": "The title of the PR", - "state": "OPEN", - "headRefName": "blueberries", - "headRepositoryOwner": {"login": "OWNER"} - } } } }`)) tr.Register( httpmock.GraphQL(`mutation PullRequestAutoMerge\b`), httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { @@ -1001,6 +948,11 @@ func TestMergeRun_autoMerge(t *testing.T) { SelectorArg: "https://github.com/OWNER/REPO/pull/123", AutoMergeEnable: true, MergeMethod: PullRequestMergeMethodSquash, + Finder: shared.NewMockFinder( + "https://github.com/OWNER/REPO/pull/123", + &api.PullRequest{ID: "THE-ID", Number: 123}, + ghrepo.New("OWNER", "REPO"), + ), }) assert.NoError(t, err) @@ -1015,21 +967,11 @@ func TestMergeRun_disableAutoMerge(t *testing.T) { tr := initFakeHTTP() defer tr.Verify(t) - - tr.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "id": "THE-ID", - "number": 123, - "title": "The title of the PR", - "state": "OPEN", - "headRefName": "blueberries", - "headRepositoryOwner": {"login": "OWNER"} - } } } }`)) tr.Register( httpmock.GraphQL(`mutation PullRequestAutoMergeDisable\b`), - httpmock.StringResponse(`{}`)) + httpmock.GraphQLQuery(`{}`, func(s string, m map[string]interface{}) { + assert.Equal(t, map[string]interface{}{"prID": "THE-ID"}, m) + })) _, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -1041,6 +983,11 @@ func TestMergeRun_disableAutoMerge(t *testing.T) { }, SelectorArg: "https://github.com/OWNER/REPO/pull/123", AutoMergeDisable: true, + Finder: shared.NewMockFinder( + "https://github.com/OWNER/REPO/pull/123", + &api.PullRequest{ID: "THE-ID", Number: 123}, + ghrepo.New("OWNER", "REPO"), + ), }) assert.NoError(t, err) diff --git a/pkg/cmd/pr/ready/ready.go b/pkg/cmd/pr/ready/ready.go index 78b20532a..a00563b4b 100644 --- a/pkg/cmd/pr/ready/ready.go +++ b/pkg/cmd/pr/ready/ready.go @@ -24,6 +24,8 @@ type ReadyOptions struct { Remotes func() (context.Remotes, error) Branch func() (string, error) + Finder shared.PRFinder + SelectorArg string } @@ -47,8 +49,7 @@ func NewCmdReady(f *cmdutil.Factory, runF func(*ReadyOptions) error) *cobra.Comm `), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - // support `-R, --repo` override - opts.BaseRepo = f.BaseRepo + opts.Finder = shared.NewFinder(f) if repoOverride, _ := cmd.Flags().GetString("repo"); repoOverride != "" && len(args) == 0 { return &cmdutil.FlagError{Err: errors.New("argument required when using the --repo flag")} @@ -71,13 +72,10 @@ func NewCmdReady(f *cmdutil.Factory, runF func(*ReadyOptions) error) *cobra.Comm func readyRun(opts *ReadyOptions) error { cs := opts.IO.ColorScheme() - httpClient, err := opts.HttpClient() - if err != nil { - return err + findOptions := shared.FindOptions{ + Selector: opts.SelectorArg, } - apiClient := api.NewClientFromHTTP(httpClient) - - pr, baseRepo, err := shared.PRFromArgs(apiClient, opts.BaseRepo, opts.Branch, opts.Remotes, opts.SelectorArg) + pr, baseRepo, err := opts.Finder.Find(findOptions) if err != nil { return err } @@ -90,6 +88,12 @@ func readyRun(opts *ReadyOptions) error { return nil } + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + apiClient := api.NewClientFromHTTP(httpClient) + err = api.PullRequestReady(apiClient, baseRepo, pr) if err != nil { return fmt.Errorf("API call failed: %w", err) diff --git a/pkg/cmd/pr/reopen/reopen.go b/pkg/cmd/pr/reopen/reopen.go index f22b8bfd2..72fb13659 100644 --- a/pkg/cmd/pr/reopen/reopen.go +++ b/pkg/cmd/pr/reopen/reopen.go @@ -5,8 +5,6 @@ import ( "net/http" "github.com/cli/cli/api" - "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" @@ -15,9 +13,9 @@ import ( type ReopenOptions struct { HttpClient func() (*http.Client, error) - Config func() (config.Config, error) IO *iostreams.IOStreams - BaseRepo func() (ghrepo.Interface, error) + + Finder shared.PRFinder SelectorArg string } @@ -26,7 +24,6 @@ func NewCmdReopen(f *cmdutil.Factory, runF func(*ReopenOptions) error) *cobra.Co opts := &ReopenOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, - Config: f.Config, } cmd := &cobra.Command{ @@ -34,8 +31,7 @@ func NewCmdReopen(f *cmdutil.Factory, runF func(*ReopenOptions) error) *cobra.Co Short: "Reopen a pull request", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - // support `-R, --repo` override - opts.BaseRepo = f.BaseRepo + opts.Finder = shared.NewFinder(f) if len(args) > 0 { opts.SelectorArg = args[0] @@ -54,13 +50,10 @@ func NewCmdReopen(f *cmdutil.Factory, runF func(*ReopenOptions) error) *cobra.Co func reopenRun(opts *ReopenOptions) error { cs := opts.IO.ColorScheme() - httpClient, err := opts.HttpClient() - if err != nil { - return err + findOptions := shared.FindOptions{ + Selector: opts.SelectorArg, } - apiClient := api.NewClientFromHTTP(httpClient) - - pr, baseRepo, err := shared.PRFromArgs(apiClient, opts.BaseRepo, nil, nil, opts.SelectorArg) + pr, baseRepo, err := opts.Finder.Find(findOptions) if err != nil { return err } @@ -75,6 +68,12 @@ func reopenRun(opts *ReopenOptions) error { return nil } + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + apiClient := api.NewClientFromHTTP(httpClient) + err = api.PullRequestReopen(apiClient, baseRepo, pr) if err != nil { return fmt.Errorf("API call failed: %w", err) diff --git a/pkg/cmd/pr/review/review.go b/pkg/cmd/pr/review/review.go index 1ff213bcb..606af85dc 100644 --- a/pkg/cmd/pr/review/review.go +++ b/pkg/cmd/pr/review/review.go @@ -8,9 +8,7 @@ import ( "github.com/AlecAivazis/survey/v2" "github.com/MakeNowJust/heredoc" "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" @@ -24,9 +22,8 @@ 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) + + Finder shared.PRFinder SelectorArg string InteractiveMode bool @@ -39,8 +36,6 @@ func NewCmdReview(f *cmdutil.Factory, runF func(*ReviewOptions) error) *cobra.Co IO: f.IOStreams, HttpClient: f.HttpClient, Config: f.Config, - Remotes: f.Remotes, - Branch: f.Branch, } var ( @@ -74,8 +69,7 @@ func NewCmdReview(f *cmdutil.Factory, runF func(*ReviewOptions) error) *cobra.Co `), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - // support `-R, --repo` override - opts.BaseRepo = f.BaseRepo + opts.Finder = shared.NewFinder(f) if repoOverride, _ := cmd.Flags().GetString("repo"); repoOverride != "" && len(args) == 0 { return &cmdutil.FlagError{Err: errors.New("argument required when using the --repo flag")} @@ -151,13 +145,10 @@ func NewCmdReview(f *cmdutil.Factory, runF func(*ReviewOptions) error) *cobra.Co } func reviewRun(opts *ReviewOptions) error { - httpClient, err := opts.HttpClient() - if err != nil { - return err + findOptions := shared.FindOptions{ + Selector: opts.SelectorArg, } - apiClient := api.NewClientFromHTTP(httpClient) - - pr, baseRepo, err := shared.PRFromArgs(apiClient, opts.BaseRepo, opts.Branch, opts.Remotes, opts.SelectorArg) + pr, baseRepo, err := opts.Finder.Find(findOptions) if err != nil { return err } @@ -183,6 +174,12 @@ func reviewRun(opts *ReviewOptions) error { } } + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + apiClient := api.NewClientFromHTTP(httpClient) + err = api.AddReview(apiClient, baseRepo, pr, reviewData) if err != nil { return fmt.Errorf("failed to create review: %w", err) diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go new file mode 100644 index 000000000..cd7e3c314 --- /dev/null +++ b/pkg/cmd/pr/shared/finder.go @@ -0,0 +1,328 @@ +package shared + +import ( + "errors" + "fmt" + "net/http" + "net/url" + "regexp" + "sort" + "strconv" + "strings" + + "github.com/cli/cli/api" + "github.com/cli/cli/context" + "github.com/cli/cli/git" + "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/set" +) + +type PRFinder interface { + Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, error) +} + +type progressIndicator interface { + StartProgressIndicator() + StopProgressIndicator() +} + +type finder struct { + baseRepoFn func() (ghrepo.Interface, error) + branchFn func() (string, error) + remotesFn func() (context.Remotes, error) + httpClient func() (*http.Client, error) + progress progressIndicator + + repo ghrepo.Interface + prNumber int + branchName string +} + +func NewFinder(factory *cmdutil.Factory) PRFinder { + if runCommandFinder != nil { + f := runCommandFinder + runCommandFinder = &mockFinder{err: errors.New("you must use a RunCommandFinder to stub PR lookups")} + return f + } + + return &finder{ + baseRepoFn: factory.BaseRepo, + branchFn: factory.Branch, + remotesFn: factory.Remotes, + httpClient: factory.HttpClient, + progress: factory.IOStreams, + } +} + +var runCommandFinder PRFinder + +// RunCommandFinder is the NewMockFinder substitute to be used ONLY in runCommand-style tests. +func RunCommandFinder(selector string, pr *api.PullRequest, repo ghrepo.Interface) { + runCommandFinder = NewMockFinder(selector, pr, repo) +} + +type FindOptions struct { + // Selector can be a number with optional `#` prefix, a branch name with optional `:` prefix, or + // a PR URL. + Selector string + // Fields lists the GraphQL fields to fetch for the PullRequest. + Fields []string + // BaseBranch is the name of the base branch to scope the PR-for-branch lookup to. + BaseBranch string + // States lists the possible PR states to scope the PR-for-branch lookup to. + States []string +} + +func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, error) { + if len(opts.Fields) == 0 { + return nil, nil, errors.New("Find error: no fields specified") + } + + _ = f.parseURL(opts.Selector) + + if f.repo == nil { + repo, err := f.baseRepoFn() + if err != nil { + return nil, nil, fmt.Errorf("could not determine base repo: %w", err) + } + f.repo = repo + } + + if opts.Selector == "" { + if err := f.parseCurrentBranch(); err != nil { + return nil, nil, err + } + } else if f.prNumber == 0 { + if prNumber, err := strconv.Atoi(strings.TrimPrefix(opts.Selector, "#")); err == nil { + f.prNumber = prNumber + } else { + f.branchName = opts.Selector + } + } + + httpClient, err := f.httpClient() + if err != nil { + return nil, nil, err + } + + if f.progress != nil { + f.progress.StartProgressIndicator() + defer f.progress.StopProgressIndicator() + } + + if f.prNumber > 0 { + if len(opts.Fields) == 1 && opts.Fields[0] == "number" { + // avoid hitting the API if we already have all the information + return &api.PullRequest{Number: f.prNumber}, f.repo, nil + } + pr, err := findByNumber(httpClient, f.repo, f.prNumber, opts.Fields) + return pr, f.repo, err + } + + pr, err := findForBranch(httpClient, f.repo, opts.BaseBranch, f.branchName, opts.States, opts.Fields) + + // TODO: preload view: api.ReviewsForPullRequest, api.CommentsForPullRequest + // TODO: preload checks: get all checks + return pr, f.repo, err +} + +var pullURLRE = regexp.MustCompile(`^/([^/]+)/([^/]+)/pull/(\d+)`) + +func (f *finder) parseURL(prURL string) error { + if prURL == "" { + return fmt.Errorf("invalid URL: %q", prURL) + } + + u, err := url.Parse(prURL) + if err != nil { + return err + } + + if u.Scheme != "https" && u.Scheme != "http" { + return fmt.Errorf("invalid scheme: %s", u.Scheme) + } + + m := pullURLRE.FindStringSubmatch(u.Path) + if m == nil { + return fmt.Errorf("not a pull request URL: %s", prURL) + } + + f.repo = ghrepo.NewWithHost(m[1], m[2], u.Hostname()) + f.prNumber, _ = strconv.Atoi(m[3]) + return nil +} + +var prHeadRE = regexp.MustCompile(`^refs/pull/(\d+)/head$`) + +func (f *finder) parseCurrentBranch() error { + prHeadRef, err := f.branchFn() + if err != nil { + return err + } + + branchConfig := git.ReadBranchConfig(prHeadRef) + + // the branch is configured to merge a special PR head ref + if m := prHeadRE.FindStringSubmatch(branchConfig.MergeRef); m != nil { + f.prNumber, _ = strconv.Atoi(m[1]) + return nil + } + + var branchOwner string + if branchConfig.RemoteURL != nil { + // the branch merges from a remote specified by URL + if r, err := ghrepo.FromURL(branchConfig.RemoteURL); err == nil { + branchOwner = r.RepoOwner() + } + } else if branchConfig.RemoteName != "" { + // the branch merges from a remote specified by name + rem, _ := f.remotesFn() + if r, err := rem.FindByName(branchConfig.RemoteName); err == nil { + branchOwner = r.RepoOwner() + } + } + + if branchOwner != "" { + if strings.HasPrefix(branchConfig.MergeRef, "refs/heads/") { + prHeadRef = strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/") + } + // prepend `OWNER:` if this branch is pushed to a fork + if !strings.EqualFold(branchOwner, f.repo.RepoOwner()) { + prHeadRef = fmt.Sprintf("%s:%s", branchOwner, prHeadRef) + } + } + + f.branchName = prHeadRef + return nil +} + +func findByNumber(httpClient *http.Client, repo ghrepo.Interface, number int, fields []string) (*api.PullRequest, error) { + type response struct { + Repository struct { + PullRequest api.PullRequest + } + } + + query := fmt.Sprintf(` + query PullRequestByNumber($owner: String!, $repo: String!, $pr_number: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $pr_number) {%s} + } + }`, api.PullRequestGraphQL(fields)) + + variables := map[string]interface{}{ + "owner": repo.RepoOwner(), + "repo": repo.RepoName(), + "pr_number": number, + } + + var resp response + client := api.NewClientFromHTTP(httpClient) + err := client.GraphQL(repo.RepoHost(), query, variables, &resp) + if err != nil { + return nil, err + } + + return &resp.Repository.PullRequest, nil +} + +func findForBranch(httpClient *http.Client, repo ghrepo.Interface, baseBranch, headBranch string, stateFilters, fields []string) (*api.PullRequest, error) { + type response struct { + Repository struct { + PullRequests struct { + Nodes []api.PullRequest + } + } + } + + fieldSet := set.NewStringSet() + fieldSet.AddValues(fields) + // these fields are required for filtering below + fieldSet.AddValues([]string{"state", "baseRefName", "headRefName", "isCrossRepository", "headRepositoryOwner"}) + + query := fmt.Sprintf(` + query PullRequestForBranch($owner: String!, $repo: String!, $headRefName: String!, $states: [PullRequestState!]) { + repository(owner: $owner, name: $repo) { + pullRequests(headRefName: $headRefName, states: $states, first: 30, orderBy: { field: CREATED_AT, direction: DESC }) { + nodes {%s} + } + } + }`, api.PullRequestGraphQL(fieldSet.ToSlice())) + + branchWithoutOwner := headBranch + if idx := strings.Index(headBranch, ":"); idx >= 0 { + branchWithoutOwner = headBranch[idx+1:] + } + + variables := map[string]interface{}{ + "owner": repo.RepoOwner(), + "repo": repo.RepoName(), + "headRefName": branchWithoutOwner, + "states": stateFilters, + } + + var resp response + client := api.NewClientFromHTTP(httpClient) + err := client.GraphQL(repo.RepoHost(), query, variables, &resp) + if err != nil { + return nil, err + } + + prs := resp.Repository.PullRequests.Nodes + sort.SliceStable(prs, func(a, b int) bool { + return prs[a].State == "OPEN" && prs[b].State != "OPEN" + }) + + for _, pr := range prs { + if pr.HeadLabel() == headBranch && (baseBranch == "" || pr.BaseRefName == baseBranch) { + return &pr, nil + } + } + + return nil, &NotFoundError{fmt.Errorf("no pull requests found for branch %q", headBranch)} +} + +type NotFoundError struct { + error +} + +func (err *NotFoundError) Unwrap() error { + return err.error +} + +func NewMockFinder(selector string, pr *api.PullRequest, repo ghrepo.Interface) PRFinder { + return &mockFinder{ + expectSelector: selector, + pr: pr, + repo: repo, + } +} + +type mockFinder struct { + called bool + expectSelector string + pr *api.PullRequest + repo ghrepo.Interface + err error +} + +func (m *mockFinder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, error) { + if m.err != nil { + return nil, nil, m.err + } + if m.expectSelector != opts.Selector { + return nil, nil, fmt.Errorf("mockFinder: expected selector %q, got %q", m.expectSelector, opts.Selector) + } + if m.called { + return nil, nil, errors.New("mockFinder used more than once") + } + m.called = true + + if m.pr.HeadRepositoryOwner.Login == "" { + // pose as same-repo PR by default + m.pr.HeadRepositoryOwner.Login = m.repo.RepoOwner() + } + + return m.pr, m.repo, nil +} diff --git a/pkg/cmd/pr/shared/lookup_test.go b/pkg/cmd/pr/shared/finder_test.go similarity index 78% rename from pkg/cmd/pr/shared/lookup_test.go rename to pkg/cmd/pr/shared/finder_test.go index 4d843d7ae..f3600962e 100644 --- a/pkg/cmd/pr/shared/lookup_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -4,13 +4,12 @@ import ( "net/http" "testing" - "github.com/cli/cli/api" "github.com/cli/cli/context" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/httpmock" ) -func TestPRFromArgs(t *testing.T) { +func TestFind(t *testing.T) { type args struct { baseRepoFn func() (ghrepo.Interface, error) branchFn func() (string, error) @@ -68,12 +67,6 @@ func TestPRFromArgs(t *testing.T) { baseRepoFn: nil, }, httpStub: func(r *httpmock.Registry) { - r.Register( - httpmock.GraphQL(`query PullRequest_fields\b`), - httpmock.StringResponse(`{"data":{}}`)) - r.Register( - httpmock.GraphQL(`query PullRequest_fields2\b`), - httpmock.StringResponse(`{"data":{}}`)) r.Register( httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(`{"data":{"repository":{ @@ -87,17 +80,30 @@ func TestPRFromArgs(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { reg := &httpmock.Registry{} + defer reg.Verify(t) if tt.httpStub != nil { tt.httpStub(reg) } - httpClient := &http.Client{Transport: reg} - pr, repo, err := PRFromArgs(api.NewClientFromHTTP(httpClient), tt.args.baseRepoFn, tt.args.branchFn, tt.args.remotesFn, tt.args.selector) + + f := finder{ + httpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + baseRepoFn: tt.args.baseRepoFn, + branchFn: tt.args.branchFn, + remotesFn: tt.args.remotesFn, + } + + pr, repo, err := f.Find(FindOptions{ + Selector: tt.args.selector, + }) if (err != nil) != tt.wantErr { - t.Errorf("IssueFromArg() error = %v, wantErr %v", err, tt.wantErr) + t.Errorf("Find() error = %v, wantErr %v", err, tt.wantErr) return } + if pr.Number != tt.wantPR { - t.Errorf("want issue #%d, got #%d", tt.wantPR, pr.Number) + t.Errorf("want pr #%d, got #%d", tt.wantPR, pr.Number) } repoURL := ghrepo.GenerateRepoURL(repo, "") if repoURL != tt.wantRepo { diff --git a/pkg/cmd/pr/shared/lookup.go b/pkg/cmd/pr/shared/lookup.go deleted file mode 100644 index 06e9221c0..000000000 --- a/pkg/cmd/pr/shared/lookup.go +++ /dev/null @@ -1,121 +0,0 @@ -package shared - -import ( - "fmt" - "net/url" - "regexp" - "strconv" - "strings" - - "github.com/cli/cli/api" - "github.com/cli/cli/context" - "github.com/cli/cli/git" - "github.com/cli/cli/internal/ghrepo" -) - -// PRFromArgs looks up the pull request from either the number/branch/URL argument or one belonging to the current branch -// -// NOTE: this API isn't great, but is here as a compatibility layer between old-style and new-style commands -func PRFromArgs(apiClient *api.Client, baseRepoFn func() (ghrepo.Interface, error), branchFn func() (string, error), remotesFn func() (context.Remotes, error), arg string) (*api.PullRequest, ghrepo.Interface, error) { - if arg != "" { - // First check to see if the prString is a url, return repo from url if found. This - // is run first because we don't need to run determineBaseRepo for this path - pr, r, err := prFromURL(apiClient, arg) - if pr != nil || err != nil { - return pr, r, err - } - } - - repo, err := baseRepoFn() - if err != nil { - return nil, nil, fmt.Errorf("could not determine base repo: %w", err) - } - - // If there are no args see if we can guess the PR from the current branch - if arg == "" { - pr, err := prForCurrentBranch(apiClient, repo, branchFn, remotesFn) - return pr, repo, err - } else { - // Next see if the prString is a number and use that to look up the url - pr, err := prFromNumberString(apiClient, repo, arg) - if pr != nil || err != nil { - return pr, repo, err - } - - // Last see if it is a branch name - pr, err = api.PullRequestForBranch(apiClient, repo, "", arg, nil) - return pr, repo, err - } -} - -func prFromNumberString(apiClient *api.Client, repo ghrepo.Interface, s string) (*api.PullRequest, error) { - if prNumber, err := strconv.Atoi(strings.TrimPrefix(s, "#")); err == nil { - return api.PullRequestByNumber(apiClient, repo, prNumber) - } - - return nil, nil -} - -var pullURLRE = regexp.MustCompile(`^/([^/]+)/([^/]+)/pull/(\d+)`) - -func prFromURL(apiClient *api.Client, s string) (*api.PullRequest, ghrepo.Interface, error) { - u, err := url.Parse(s) - if err != nil { - return nil, nil, nil - } - - if u.Scheme != "https" && u.Scheme != "http" { - return nil, nil, nil - } - - m := pullURLRE.FindStringSubmatch(u.Path) - if m == nil { - return nil, nil, nil - } - - repo := ghrepo.NewWithHost(m[1], m[2], u.Hostname()) - prNumberString := m[3] - pr, err := prFromNumberString(apiClient, repo, prNumberString) - return pr, repo, err -} - -func prForCurrentBranch(apiClient *api.Client, repo ghrepo.Interface, branchFn func() (string, error), remotesFn func() (context.Remotes, error)) (*api.PullRequest, error) { - prHeadRef, err := branchFn() - if err != nil { - return nil, err - } - - branchConfig := git.ReadBranchConfig(prHeadRef) - - // the branch is configured to merge a special PR head ref - prHeadRE := regexp.MustCompile(`^refs/pull/(\d+)/head$`) - if m := prHeadRE.FindStringSubmatch(branchConfig.MergeRef); m != nil { - return prFromNumberString(apiClient, repo, m[1]) - } - - var branchOwner string - if branchConfig.RemoteURL != nil { - // the branch merges from a remote specified by URL - if r, err := ghrepo.FromURL(branchConfig.RemoteURL); err == nil { - branchOwner = r.RepoOwner() - } - } else if branchConfig.RemoteName != "" { - // the branch merges from a remote specified by name - rem, _ := remotesFn() - if r, err := rem.FindByName(branchConfig.RemoteName); err == nil { - branchOwner = r.RepoOwner() - } - } - - if branchOwner != "" { - if strings.HasPrefix(branchConfig.MergeRef, "refs/heads/") { - prHeadRef = strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/") - } - // prepend `OWNER:` if this branch is pushed to a fork - if !strings.EqualFold(branchOwner, repo.RepoOwner()) { - prHeadRef = fmt.Sprintf("%s:%s", branchOwner, prHeadRef) - } - } - - return api.PullRequestForBranch(apiClient, repo, "", prHeadRef, nil) -} diff --git a/pkg/cmd/pr/view/view.go b/pkg/cmd/pr/view/view.go index 4e6300297..b8e32189d 100644 --- a/pkg/cmd/pr/view/view.go +++ b/pkg/cmd/pr/view/view.go @@ -3,17 +3,12 @@ package view import ( "errors" "fmt" - "net/http" "sort" "strconv" "strings" - "sync" "github.com/MakeNowJust/heredoc" "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" @@ -27,14 +22,10 @@ type browser interface { } type ViewOptions struct { - HttpClient func() (*http.Client, error) - Config func() (config.Config, error) - IO *iostreams.IOStreams - Browser browser - BaseRepo func() (ghrepo.Interface, error) - Remotes func() (context.Remotes, error) - Branch func() (string, error) + IO *iostreams.IOStreams + Browser browser + Finder shared.PRFinder Exporter cmdutil.Exporter SelectorArg string @@ -44,12 +35,8 @@ type ViewOptions struct { func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Command { opts := &ViewOptions{ - IO: f.IOStreams, - HttpClient: f.HttpClient, - Config: f.Config, - Remotes: f.Remotes, - Branch: f.Branch, - Browser: f.Browser, + IO: f.IOStreams, + Browser: f.Browser, } cmd := &cobra.Command{ @@ -65,8 +52,7 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman `), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - // support `-R, --repo` override - opts.BaseRepo = f.BaseRepo + opts.Finder = shared.NewFinder(f) if repoOverride, _ := cmd.Flags().GetString("repo"); repoOverride != "" && len(args) == 0 { return &cmdutil.FlagError{Err: errors.New("argument required when using the --repo flag")} @@ -90,10 +76,26 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman return cmd } +var defaultFields = []string{ + "url", "number", "title", "state", "body", "author", + "isDraft", "maintainerCanModify", "mergeable", "additions", "deletions", + "baseRefName", "headRefName", "headRepositoryOwner", "headRepository", "isCrossRepository", + "reviewRequests", "reviews", "assignees", "labels", "projectCards", "milestone", + "comments", // TODO: fetch only 1 last comment unless `opts.Comments` was set + "reactionGroups", +} + func viewRun(opts *ViewOptions) error { - opts.IO.StartProgressIndicator() - pr, err := retrievePullRequest(opts) - opts.IO.StopProgressIndicator() + findOptions := shared.FindOptions{ + Selector: opts.SelectorArg, + Fields: defaultFields, + } + if opts.BrowserMode { + findOptions.Fields = []string{"url"} + } else if opts.Exporter != nil { + findOptions.Fields = opts.Exporter.Fields() + } + pr, _, err := opts.Finder.Find(findOptions) if err != nil { return err } @@ -413,51 +415,3 @@ func prStateWithDraft(pr *api.PullRequest) string { return pr.State } - -func retrievePullRequest(opts *ViewOptions) (*api.PullRequest, error) { - httpClient, err := opts.HttpClient() - if err != nil { - return nil, err - } - - apiClient := api.NewClientFromHTTP(httpClient) - - pr, repo, err := shared.PRFromArgs(apiClient, opts.BaseRepo, opts.Branch, opts.Remotes, opts.SelectorArg) - if err != nil { - return nil, err - } - - if opts.BrowserMode { - return pr, nil - } - - var errp, errc error - var wg sync.WaitGroup - wg.Add(1) - go func() { - defer wg.Done() - var reviews *api.PullRequestReviews - reviews, errp = api.ReviewsForPullRequest(apiClient, repo, pr) - pr.Reviews = *reviews - }() - - if opts.Comments { - wg.Add(1) - go func() { - defer wg.Done() - var comments *api.Comments - comments, errc = api.CommentsForPullRequest(apiClient, repo, pr) - pr.Comments = *comments - }() - } - - wg.Wait() - - if errp != nil { - err = errp - } - if errc != nil { - err = errc - } - return pr, err -} diff --git a/pkg/cmd/run/shared/shared.go b/pkg/cmd/run/shared/shared.go index afd926114..4fcf73250 100644 --- a/pkg/cmd/run/shared/shared.go +++ b/pkg/cmd/run/shared/shared.go @@ -134,11 +134,10 @@ func GetAnnotations(client *api.Client, repo ghrepo.Interface, job Job) ([]Annot err := client.REST(repo.RepoHost(), "GET", path, nil, &result) if err != nil { - var notFound *api.NotFoundError - if !errors.As(err, ¬Found) { + var httpError api.HTTPError + if errors.As(err, &httpError) && httpError.StatusCode == 404 { return []Annotation{}, nil } - return nil, err } From c50d390cf52c7726368f9928cadf9f0266a32814 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 7 May 2021 22:09:58 +0200 Subject: [PATCH 2/6] Fix tests --- api/queries_pr.go | 8 +- pkg/cmd/pr/checkout/checkout.go | 1 + pkg/cmd/pr/checkout/checkout_test.go | 367 ++++-------------- pkg/cmd/pr/checks/checks.go | 22 +- pkg/cmd/pr/checks/checks_test.go | 83 ++-- pkg/cmd/pr/checks/fixtures/allPassing.json | 7 +- pkg/cmd/pr/checks/fixtures/someFailing.json | 7 +- pkg/cmd/pr/checks/fixtures/somePending.json | 7 +- pkg/cmd/pr/checks/fixtures/withStatuses.json | 7 +- pkg/cmd/pr/close/close.go | 28 +- pkg/cmd/pr/close/close_test.go | 195 +++++++--- pkg/cmd/pr/create/create_test.go | 74 +--- pkg/cmd/pr/diff/diff_test.go | 92 +---- pkg/cmd/pr/edit/edit.go | 1 + pkg/cmd/pr/edit/edit_test.go | 46 +-- pkg/cmd/pr/merge/merge_test.go | 9 +- pkg/cmd/pr/ready/ready.go | 15 +- pkg/cmd/pr/ready/ready_test.go | 105 ++--- pkg/cmd/pr/reopen/reopen.go | 5 +- pkg/cmd/pr/reopen/reopen_test.go | 125 ++---- pkg/cmd/pr/review/review.go | 1 + pkg/cmd/pr/review/review_test.go | 310 +++------------ pkg/cmd/pr/shared/finder.go | 5 + pkg/cmd/pr/shared/finder_test.go | 18 + pkg/cmd/pr/view/fixtures/prView.json | 54 --- .../prViewPreviewDraftStatebyBranch.json | 54 --- .../view/fixtures/prViewPreviewNoReviews.json | 1 - .../prViewPreviewWithMetadataByBranch.json | 139 ------- .../pr/view/fixtures/prView_EmptyBody.json | 52 --- .../view/fixtures/prView_NoActiveBranch.json | 15 - pkg/cmd/pr/view/view_test.go | 319 +++++---------- 31 files changed, 573 insertions(+), 1599 deletions(-) delete mode 100644 pkg/cmd/pr/view/fixtures/prView.json delete mode 100644 pkg/cmd/pr/view/fixtures/prViewPreviewDraftStatebyBranch.json delete mode 100644 pkg/cmd/pr/view/fixtures/prViewPreviewNoReviews.json delete mode 100644 pkg/cmd/pr/view/fixtures/prViewPreviewWithMetadataByBranch.json delete mode 100644 pkg/cmd/pr/view/fixtures/prView_EmptyBody.json delete mode 100644 pkg/cmd/pr/view/fixtures/prView_NoActiveBranch.json diff --git a/api/queries_pr.go b/api/queries_pr.go index 38f9f5ee4..b5441a359 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -58,9 +58,7 @@ type PullRequest struct { Author Author MergedBy *Author HeadRepositoryOwner Owner - HeadRepository struct { - Name string - } + HeadRepository PRRepository IsCrossRepository bool IsDraft bool MaintainerCanModify bool @@ -87,6 +85,10 @@ type PullRequest struct { ReviewRequests ReviewRequests } +type PRRepository struct { + Name string +} + type Commit struct { OID string `json:"oid"` } diff --git a/pkg/cmd/pr/checkout/checkout.go b/pkg/cmd/pr/checkout/checkout.go index 03d04a1a9..84b16709f 100644 --- a/pkg/cmd/pr/checkout/checkout.go +++ b/pkg/cmd/pr/checkout/checkout.go @@ -72,6 +72,7 @@ func NewCmdCheckout(f *cmdutil.Factory, runF func(*CheckoutOptions) error) *cobr func checkoutRun(opts *CheckoutOptions) error { findOptions := shared.FindOptions{ Selector: opts.SelectorArg, + Fields: []string{"number", "headRefName", "headRepository", "headRepositoryOwner"}, } pr, baseRepo, err := opts.Finder.Find(findOptions) if err != nil { diff --git a/pkg/cmd/pr/checkout/checkout_test.go b/pkg/cmd/pr/checkout/checkout_test.go index ed1775f44..583602628 100644 --- a/pkg/cmd/pr/checkout/checkout_test.go +++ b/pkg/cmd/pr/checkout/checkout_test.go @@ -2,9 +2,9 @@ package checkout import ( "bytes" - "encoding/json" "io/ioutil" "net/http" + "strings" "testing" "github.com/cli/cli/api" @@ -13,6 +13,7 @@ import ( "github.com/cli/cli/internal/config" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/internal/run" + "github.com/cli/cli/pkg/cmd/pr/shared" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/pkg/iostreams" @@ -21,6 +22,43 @@ import ( "github.com/stretchr/testify/assert" ) +// repo: either "baseOwner/baseRepo" or "baseOwner/baseRepo:defaultBranch" +// prHead: "headOwner/headRepo:headBranch" +func stubPR(repo, prHead string) (ghrepo.Interface, *api.PullRequest) { + defaultBranch := "" + if idx := strings.IndexRune(repo, ':'); idx >= 0 { + defaultBranch = repo[idx+1:] + repo = repo[:idx] + } + baseRepo, err := ghrepo.FromFullName(repo) + if err != nil { + panic(err) + } + if defaultBranch != "" { + baseRepo = api.InitRepoHostname(&api.Repository{ + Name: baseRepo.RepoName(), + Owner: api.RepositoryOwner{Login: baseRepo.RepoOwner()}, + DefaultBranchRef: api.BranchRef{Name: defaultBranch}, + }, baseRepo.RepoHost()) + } + + idx := strings.IndexRune(prHead, ':') + headRefName := prHead[idx+1:] + headRepo, err := ghrepo.FromFullName(prHead[:idx]) + if err != nil { + panic(err) + } + + return baseRepo, &api.PullRequest{ + Number: 123, + HeadRefName: headRefName, + HeadRepositoryOwner: api.Owner{Login: headRepo.RepoOwner()}, + HeadRepository: api.PRRepository{Name: headRepo.RepoName()}, + IsCrossRepository: !ghrepo.IsSame(baseRepo, headRepo), + MaintainerCanModify: false, + } +} + func runCommand(rt http.RoundTripper, remotes context.Remotes, branch string, cli string) (*test.CmdOut, error) { io, _, stdout, stderr := iostreams.Test() @@ -32,13 +70,6 @@ func runCommand(rt http.RoundTripper, remotes context.Remotes, branch string, cl Config: func() (config.Config, error) { return config.NewBlankConfig(), nil }, - BaseRepo: func() (ghrepo.Interface, error) { - return api.InitRepoHostname(&api.Repository{ - Name: "REPO", - Owner: api.RepositoryOwner{Login: "OWNER"}, - DefaultBranchRef: api.BranchRef{Name: "master"}, - }, "github.com"), nil - }, Remotes: func() (context.Remotes, error) { if remotes == nil { return context.Remotes{ @@ -78,20 +109,8 @@ func TestPRCheckout_sameRepo(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "number": 123, - "headRefName": "feature", - "headRepositoryOwner": { - "login": "hubot" - }, - "headRepository": { - "name": "REPO" - }, - "isCrossRepository": false, - "maintainerCanModify": false - } } } } - `)) + baseRepo, pr := stubPR("OWNER/REPO", "OWNER/REPO:feature") + shared.RunCommandFinder("123", pr, baseRepo) cs, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -103,141 +122,17 @@ func TestPRCheckout_sameRepo(t *testing.T) { cs.Register(`git config branch\.feature\.merge refs/heads/feature`, 0, "") output, err := runCommand(http, nil, "master", `123`) - if !assert.NoError(t, err) { - return - } - - assert.Equal(t, "", output.String()) -} - -func TestPRCheckout_urlArg(t *testing.T) { - http := &httpmock.Registry{} - defer http.Verify(t) - http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "number": 123, - "headRefName": "feature", - "headRepositoryOwner": { - "login": "hubot" - }, - "headRepository": { - "name": "REPO" - }, - "isCrossRepository": false, - "maintainerCanModify": false - } } } } - `)) - - cs, cmdTeardown := run.Stub() - defer cmdTeardown(t) - - cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "") - cs.Register(`git show-ref --verify -- refs/heads/feature`, 1, "") - cs.Register(`git checkout -b feature --no-track origin/feature`, 0, "") - cs.Register(`git config branch\.feature\.remote origin`, 0, "") - cs.Register(`git config branch\.feature\.merge refs/heads/feature`, 0, "") - - output, err := runCommand(http, nil, "master", `https://github.com/OWNER/REPO/pull/123/files`) - assert.NoError(t, err) - assert.Equal(t, "", output.String()) -} - -func TestPRCheckout_urlArg_differentBase(t *testing.T) { - http := &httpmock.Registry{} - defer http.Verify(t) - http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "number": 123, - "headRefName": "feature", - "headRepositoryOwner": { - "login": "hubot" - }, - "headRepository": { - "name": "POE" - }, - "isCrossRepository": false, - "maintainerCanModify": false - } } } } - `)) - http.StubRepoInfoResponse("OWNER", "REPO", "master") - - cs, cmdTeardown := run.Stub() - defer cmdTeardown(t) - - cs.Register(`git fetch https://github\.com/OTHER/POE\.git refs/pull/123/head:feature`, 0, "") - cs.Register(`git config branch\.feature\.merge`, 1, "") - cs.Register(`git checkout feature`, 0, "") - cs.Register(`git config branch\.feature\.remote https://github\.com/OTHER/POE\.git`, 0, "") - cs.Register(`git config branch\.feature\.merge refs/pull/123/head`, 0, "") - - output, err := runCommand(http, nil, "master", `https://github.com/OTHER/POE/pull/123/files`) - assert.NoError(t, err) - assert.Equal(t, "", output.String()) - - bodyBytes, _ := ioutil.ReadAll(http.Requests[0].Body) - reqBody := struct { - Variables struct { - Owner string - Repo string - } - }{} - _ = json.Unmarshal(bodyBytes, &reqBody) - - assert.Equal(t, "OTHER", reqBody.Variables.Owner) - assert.Equal(t, "POE", reqBody.Variables.Repo) -} - -func TestPRCheckout_branchArg(t *testing.T) { - http := &httpmock.Registry{} - defer http.Verify(t) - - http.Register(httpmock.GraphQL(`query PullRequestForBranch\b`), httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes": [ - { "number": 123, - "headRefName": "feature", - "headRepositoryOwner": { - "login": "hubot" - }, - "headRepository": { - "name": "REPO" - }, - "isCrossRepository": true, - "maintainerCanModify": false } - ] } } } } - `)) - - cs, cmdTeardown := run.Stub() - defer cmdTeardown(t) - - cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "") - cs.Register(`git config branch\.feature\.merge`, 1, "") - cs.Register(`git checkout feature`, 0, "") - cs.Register(`git config branch\.feature\.remote origin`, 0, "") - cs.Register(`git config branch\.feature\.merge refs/pull/123/head`, 0, "") - - output, err := runCommand(http, nil, "master", `hubot:feature`) assert.NoError(t, err) assert.Equal(t, "", output.String()) + assert.Equal(t, "", output.Stderr()) } func TestPRCheckout_existingBranch(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "number": 123, - "headRefName": "feature", - "headRepositoryOwner": { - "login": "hubot" - }, - "headRepository": { - "name": "REPO" - }, - "isCrossRepository": false, - "maintainerCanModify": false - } } } } - `)) + baseRepo, pr := stubPR("OWNER/REPO", "OWNER/REPO:feature") + shared.RunCommandFinder("123", pr, baseRepo) cs, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -250,6 +145,7 @@ func TestPRCheckout_existingBranch(t *testing.T) { output, err := runCommand(http, nil, "master", `123`) assert.NoError(t, err) assert.Equal(t, "", output.String()) + assert.Equal(t, "", output.Stderr()) } func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { @@ -267,20 +163,8 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "number": 123, - "headRefName": "feature", - "headRepositoryOwner": { - "login": "hubot" - }, - "headRepository": { - "name": "REPO" - }, - "isCrossRepository": true, - "maintainerCanModify": false - } } } } - `)) + baseRepo, pr := stubPR("OWNER/REPO", "hubot/REPO:feature") + shared.RunCommandFinder("123", pr, baseRepo) cs, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -294,26 +178,15 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { output, err := runCommand(http, remotes, "master", `123`) assert.NoError(t, err) assert.Equal(t, "", output.String()) + assert.Equal(t, "", output.Stderr()) } func TestPRCheckout_differentRepo(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "number": 123, - "headRefName": "feature", - "headRepositoryOwner": { - "login": "hubot" - }, - "headRepository": { - "name": "REPO" - }, - "isCrossRepository": true, - "maintainerCanModify": false - } } } } - `)) + baseRepo, pr := stubPR("OWNER/REPO:master", "hubot/REPO:feature") + shared.RunCommandFinder("123", pr, baseRepo) cs, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -327,26 +200,15 @@ func TestPRCheckout_differentRepo(t *testing.T) { output, err := runCommand(http, nil, "master", `123`) assert.NoError(t, err) assert.Equal(t, "", output.String()) + assert.Equal(t, "", output.Stderr()) } func TestPRCheckout_differentRepo_existingBranch(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "number": 123, - "headRefName": "feature", - "headRepositoryOwner": { - "login": "hubot" - }, - "headRepository": { - "name": "REPO" - }, - "isCrossRepository": true, - "maintainerCanModify": false - } } } } - `)) + baseRepo, pr := stubPR("OWNER/REPO:master", "hubot/REPO:feature") + shared.RunCommandFinder("123", pr, baseRepo) cs, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -358,26 +220,15 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) { output, err := runCommand(http, nil, "master", `123`) assert.NoError(t, err) assert.Equal(t, "", output.String()) + assert.Equal(t, "", output.Stderr()) } func TestPRCheckout_detachedHead(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "number": 123, - "headRefName": "feature", - "headRepositoryOwner": { - "login": "hubot" - }, - "headRepository": { - "name": "REPO" - }, - "isCrossRepository": true, - "maintainerCanModify": true - } } } } - `)) + baseRepo, pr := stubPR("OWNER/REPO:master", "hubot/REPO:feature") + shared.RunCommandFinder("123", pr, baseRepo) cs, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -389,26 +240,15 @@ func TestPRCheckout_detachedHead(t *testing.T) { output, err := runCommand(http, nil, "", `123`) assert.NoError(t, err) assert.Equal(t, "", output.String()) + assert.Equal(t, "", output.Stderr()) } func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "number": 123, - "headRefName": "feature", - "headRepositoryOwner": { - "login": "hubot" - }, - "headRepository": { - "name": "REPO" - }, - "isCrossRepository": true, - "maintainerCanModify": false - } } } } - `)) + baseRepo, pr := stubPR("OWNER/REPO:master", "hubot/REPO:feature") + shared.RunCommandFinder("123", pr, baseRepo) cs, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -420,26 +260,15 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { output, err := runCommand(http, nil, "feature", `123`) assert.NoError(t, err) assert.Equal(t, "", output.String()) + assert.Equal(t, "", output.Stderr()) } func TestPRCheckout_differentRepo_invalidBranchName(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "number": 123, - "headRefName": "-foo", - "headRepositoryOwner": { - "login": "hubot" - }, - "headRepository": { - "name": "REPO" - }, - "isCrossRepository": true, - "maintainerCanModify": false - } } } } - `)) + baseRepo, pr := stubPR("OWNER/REPO", "hubot/REPO:-foo") + shared.RunCommandFinder("123", pr, baseRepo) _, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -447,26 +276,16 @@ func TestPRCheckout_differentRepo_invalidBranchName(t *testing.T) { output, err := runCommand(http, nil, "master", `123`) assert.EqualError(t, err, `invalid branch name: "-foo"`) assert.Equal(t, "", output.Stderr()) + assert.Equal(t, "", output.Stderr()) } func TestPRCheckout_maintainerCanModify(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "number": 123, - "headRefName": "feature", - "headRepositoryOwner": { - "login": "hubot" - }, - "headRepository": { - "name": "REPO" - }, - "isCrossRepository": true, - "maintainerCanModify": true - } } } } - `)) + baseRepo, pr := stubPR("OWNER/REPO:master", "hubot/REPO:feature") + pr.MaintainerCanModify = true + shared.RunCommandFinder("123", pr, baseRepo) cs, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -480,25 +299,14 @@ func TestPRCheckout_maintainerCanModify(t *testing.T) { output, err := runCommand(http, nil, "master", `123`) assert.NoError(t, err) assert.Equal(t, "", output.String()) + assert.Equal(t, "", output.Stderr()) } func TestPRCheckout_recurseSubmodules(t *testing.T) { http := &httpmock.Registry{} - http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "number": 123, - "headRefName": "feature", - "headRepositoryOwner": { - "login": "hubot" - }, - "headRepository": { - "name": "REPO" - }, - "isCrossRepository": false, - "maintainerCanModify": false - } } } } - `)) + baseRepo, pr := stubPR("OWNER/REPO", "OWNER/REPO:feature") + shared.RunCommandFinder("123", pr, baseRepo) cs, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -513,25 +321,14 @@ func TestPRCheckout_recurseSubmodules(t *testing.T) { output, err := runCommand(http, nil, "master", `123 --recurse-submodules`) assert.NoError(t, err) assert.Equal(t, "", output.String()) + assert.Equal(t, "", output.Stderr()) } func TestPRCheckout_force(t *testing.T) { http := &httpmock.Registry{} - http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "number": 123, - "headRefName": "feature", - "headRepositoryOwner": { - "login": "hubot" - }, - "headRepository": { - "name": "REPO" - }, - "isCrossRepository": false, - "maintainerCanModify": false - } } } } - `)) + baseRepo, pr := stubPR("OWNER/REPO", "OWNER/REPO:feature") + shared.RunCommandFinder("123", pr, baseRepo) cs, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -545,26 +342,15 @@ func TestPRCheckout_force(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "", output.String()) + assert.Equal(t, "", output.Stderr()) } func TestPRCheckout_detach(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "number": 123, - "headRef": "f8f8f8", - "headRepositoryOwner": { - "login": "hubot" - }, - "headRepository": { - "name": "REPO" - }, - "isCrossRepository": true, - "maintainerCanModify": true - } } } } - `)) + baseRepo, pr := stubPR("OWNER/REPO:master", "hubot/REPO:feature") + shared.RunCommandFinder("123", pr, baseRepo) cs, cmdTeardown := run.Stub() defer cmdTeardown(t) @@ -573,6 +359,7 @@ func TestPRCheckout_detach(t *testing.T) { cs.Register(`git fetch origin refs/pull/123/head`, 0, "") output, err := runCommand(http, nil, "", `123 --detach`) - assert.Nil(t, err) + assert.NoError(t, err) assert.Equal(t, "", output.String()) + assert.Equal(t, "", output.Stderr()) } diff --git a/pkg/cmd/pr/checks/checks.go b/pkg/cmd/pr/checks/checks.go index b9091379f..e97d1e46f 100644 --- a/pkg/cmd/pr/checks/checks.go +++ b/pkg/cmd/pr/checks/checks.go @@ -72,21 +72,16 @@ func NewCmdChecks(f *cmdutil.Factory, runF func(*ChecksOptions) error) *cobra.Co func checksRun(opts *ChecksOptions) error { findOptions := shared.FindOptions{ Selector: opts.SelectorArg, + Fields: []string{"number", "baseRefName", "statusCheckRollup"}, + } + if opts.WebMode { + findOptions.Fields = []string{"number"} } pr, baseRepo, err := opts.Finder.Find(findOptions) if err != nil { return err } - if len(pr.Commits.Nodes) == 0 { - return fmt.Errorf("no commit found on the pull request") - } - - rollup := pr.Commits.Nodes[0].Commit.StatusCheckRollup.Contexts.Nodes - if len(rollup) == 0 { - return fmt.Errorf("no checks reported on the '%s' branch", pr.BaseRefName) - } - isTerminal := opts.IO.IsStdoutTTY() if opts.WebMode { @@ -97,6 +92,15 @@ func checksRun(opts *ChecksOptions) error { return opts.Browser.Browse(openURL) } + if len(pr.Commits.Nodes) == 0 { + return fmt.Errorf("no commit found on the pull request") + } + + rollup := pr.Commits.Nodes[0].Commit.StatusCheckRollup.Contexts.Nodes + if len(rollup) == 0 { + return fmt.Errorf("no checks reported on the '%s' branch", pr.BaseRefName) + } + passing := 0 failing := 0 pending := 0 diff --git a/pkg/cmd/pr/checks/checks_test.go b/pkg/cmd/pr/checks/checks_test.go index ca743d856..6cedd6d32 100644 --- a/pkg/cmd/pr/checks/checks_test.go +++ b/pkg/cmd/pr/checks/checks_test.go @@ -2,14 +2,20 @@ package checks import ( "bytes" + "encoding/json" + "io" + "os" "testing" + "github.com/cli/cli/api" + "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/internal/run" + "github.com/cli/cli/pkg/cmd/pr/shared" "github.com/cli/cli/pkg/cmdutil" - "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestNewCmdChecks(t *testing.T) { @@ -64,36 +70,20 @@ func Test_checksRun(t *testing.T) { tests := []struct { name string fixture string - stubs func(*httpmock.Registry) + prJSON string nontty bool wantOut string wantErr string }{ { - name: "no commits", - stubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { - "pullRequest": { "number": 123 } - } } } - `)) - }, + name: "no commits", + prJSON: `{ "number": 123 }`, wantOut: "", wantErr: "no commit found on the pull request", }, { - name: "no checks", - stubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { - "pullRequest": { "number": 123, "commits": { "nodes": [{"commit": {"oid": "abc"}}]}, "baseRefName": "master" } - } } } - `)) - }, + name: "no checks", + prJSON: `{ "number": 123, "commits": { "nodes": [{"commit": {"oid": "abc"}}]}, "baseRefName": "master" }`, wantOut: "", wantErr: "no checks reported on the 'master' branch", }, @@ -122,17 +112,9 @@ func Test_checksRun(t *testing.T) { wantErr: "SilentError", }, { - name: "no checks", - nontty: true, - stubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { - "pullRequest": { "number": 123, "commits": { "nodes": [{"commit": {"oid": "abc"}}]}, "baseRefName": "master" } - } } } - `)) - }, + name: "no checks", + nontty: true, + prJSON: `{ "number": 123, "commits": { "nodes": [{"commit": {"oid": "abc"}}]}, "baseRefName": "master" }`, wantOut: "", wantErr: "no checks reported on the 'master' branch", }, @@ -168,21 +150,26 @@ func Test_checksRun(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - io, _, stdout, _ := iostreams.Test() - io.SetStdoutTTY(!tt.nontty) + ios, _, stdout, _ := iostreams.Test() + ios.SetStdoutTTY(!tt.nontty) + + var response *api.PullRequest + var jsonReader io.Reader + if tt.fixture != "" { + ff, err := os.Open(tt.fixture) + require.NoError(t, err) + defer ff.Close() + jsonReader = ff + } else { + jsonReader = bytes.NewBufferString(tt.prJSON) + } + dec := json.NewDecoder(jsonReader) + require.NoError(t, dec.Decode(&response)) opts := &ChecksOptions{ - IO: io, + IO: ios, SelectorArg: "123", - } - - reg := &httpmock.Registry{} - defer reg.Verify(t) - - if tt.stubs != nil { - tt.stubs(reg) - } else if tt.fixture != "" { - reg.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.FileResponse(tt.fixture)) + Finder: shared.NewMockFinder("123", response, ghrepo.New("OWNER", "REPO")), } err := checksRun(opts) @@ -223,10 +210,6 @@ func TestChecksRun_web(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { browser := &cmdutil.TestBrowser{} - reg := &httpmock.Registry{} - - reg.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.FileResponse("./fixtures/allPassing.json")) io, _, stdout, stderr := iostreams.Test() io.SetStdoutTTY(tc.isTTY) @@ -241,11 +224,11 @@ func TestChecksRun_web(t *testing.T) { Browser: browser, WebMode: true, SelectorArg: "123", + Finder: shared.NewMockFinder("123", &api.PullRequest{Number: 123}, ghrepo.New("OWNER", "REPO")), }) assert.NoError(t, err) assert.Equal(t, tc.wantStdout, stdout.String()) assert.Equal(t, tc.wantStderr, stderr.String()) - reg.Verify(t) browser.Verify(t, tc.wantBrowse) }) } diff --git a/pkg/cmd/pr/checks/fixtures/allPassing.json b/pkg/cmd/pr/checks/fixtures/allPassing.json index c75e3fc29..5116b5916 100644 --- a/pkg/cmd/pr/checks/fixtures/allPassing.json +++ b/pkg/cmd/pr/checks/fixtures/allPassing.json @@ -1,4 +1,4 @@ -{ "data": { "repository": { "pullRequest": { +{ "number": 123, "commits": { "nodes": [ @@ -37,5 +37,6 @@ } } } - ]} -} } } } + ] + } +} diff --git a/pkg/cmd/pr/checks/fixtures/someFailing.json b/pkg/cmd/pr/checks/fixtures/someFailing.json index 0e53cdb79..887e22ab3 100644 --- a/pkg/cmd/pr/checks/fixtures/someFailing.json +++ b/pkg/cmd/pr/checks/fixtures/someFailing.json @@ -1,4 +1,4 @@ -{ "data": { "repository": { "pullRequest": { +{ "number": 123, "commits": { "nodes": [ @@ -37,5 +37,6 @@ } } } - ]} -} } } } + ] + } +} diff --git a/pkg/cmd/pr/checks/fixtures/somePending.json b/pkg/cmd/pr/checks/fixtures/somePending.json index 6e36a5cd3..5214a7930 100644 --- a/pkg/cmd/pr/checks/fixtures/somePending.json +++ b/pkg/cmd/pr/checks/fixtures/somePending.json @@ -1,4 +1,4 @@ -{ "data": { "repository": { "pullRequest": { +{ "number": 123, "commits": { "nodes": [ @@ -37,5 +37,6 @@ } } } - ]} -} } } } + ] + } +} diff --git a/pkg/cmd/pr/checks/fixtures/withStatuses.json b/pkg/cmd/pr/checks/fixtures/withStatuses.json index 0ce8b9c66..2b4a808c7 100644 --- a/pkg/cmd/pr/checks/fixtures/withStatuses.json +++ b/pkg/cmd/pr/checks/fixtures/withStatuses.json @@ -1,4 +1,4 @@ -{ "data": { "repository": { "pullRequest": { +{ "number": 123, "commits": { "nodes": [ @@ -34,5 +34,6 @@ } } } - ]} -} } } } + ] + } +} diff --git a/pkg/cmd/pr/close/close.go b/pkg/cmd/pr/close/close.go index a1e6e3dff..041d48edc 100644 --- a/pkg/cmd/pr/close/close.go +++ b/pkg/cmd/pr/close/close.go @@ -60,6 +60,7 @@ func closeRun(opts *CloseOptions) error { findOptions := shared.FindOptions{ Selector: opts.SelectorArg, + Fields: []string{"state", "number", "title", "isCrossRepository", "headRefName"}, } pr, baseRepo, err := opts.Finder.Find(findOptions) if err != nil { @@ -67,10 +68,10 @@ func closeRun(opts *CloseOptions) error { } if pr.State == "MERGED" { - fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d (%s) can't be closed because it was already merged", cs.Red("!"), pr.Number, pr.Title) + fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d (%s) can't be closed because it was already merged\n", cs.FailureIcon(), pr.Number, pr.Title) return cmdutil.SilentError } else if !pr.IsOpen() { - fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d (%s) is already closed\n", cs.Yellow("!"), pr.Number, pr.Title) + fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d (%s) is already closed\n", cs.WarningIcon(), pr.Number, pr.Title) return nil } @@ -87,12 +88,10 @@ func closeRun(opts *CloseOptions) error { fmt.Fprintf(opts.IO.ErrOut, "%s Closed pull request #%d (%s)\n", cs.SuccessIconWithColor(cs.Red), pr.Number, pr.Title) - crossRepoPR := pr.HeadRepositoryOwner.Login != baseRepo.RepoOwner() - if opts.DeleteBranch { branchSwitchString := "" - if opts.DeleteLocalBranch && !crossRepoPR { + if opts.DeleteLocalBranch { currentBranch, err := opts.Branch() if err != nil { return err @@ -112,10 +111,8 @@ func closeRun(opts *CloseOptions) error { localBranchExists := git.HasLocalBranch(pr.HeadRefName) if localBranchExists { - err = git.DeleteLocalBranch(pr.HeadRefName) - if err != nil { - err = fmt.Errorf("failed to delete local branch %s: %w", cs.Cyan(pr.HeadRefName), err) - return err + if err := git.DeleteLocalBranch(pr.HeadRefName); err != nil { + return fmt.Errorf("failed to delete local branch %s: %w", cs.Cyan(pr.HeadRefName), err) } } @@ -124,11 +121,14 @@ func closeRun(opts *CloseOptions) error { } } - if !crossRepoPR { - err = api.BranchDeleteRemote(apiClient, baseRepo, pr.HeadRefName) - if err != nil { - err = fmt.Errorf("failed to delete remote branch %s: %w", cs.Cyan(pr.HeadRefName), err) - return err + if pr.IsCrossRepository { + fmt.Fprintf(opts.IO.ErrOut, "%s Avoiding deleting the remote branch of a pull request from fork\n", cs.WarningIcon()) + if !opts.DeleteLocalBranch { + return nil + } + } else { + if err := api.BranchDeleteRemote(apiClient, baseRepo, pr.HeadRefName); err != nil { + return fmt.Errorf("failed to delete remote branch %s: %w", cs.Cyan(pr.HeadRefName), err) } } fmt.Fprintf(opts.IO.ErrOut, "%s Deleted branch %s%s\n", cs.SuccessIconWithColor(cs.Red), cs.Cyan(pr.HeadRefName), branchSwitchString) diff --git a/pkg/cmd/pr/close/close_test.go b/pkg/cmd/pr/close/close_test.go index 4aa239384..4024398dd 100644 --- a/pkg/cmd/pr/close/close_test.go +++ b/pkg/cmd/pr/close/close_test.go @@ -4,12 +4,14 @@ import ( "bytes" "io/ioutil" "net/http" - "regexp" + "strings" "testing" - "github.com/cli/cli/internal/config" + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/api" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/internal/run" + "github.com/cli/cli/pkg/cmd/pr/shared" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/pkg/iostreams" @@ -18,6 +20,44 @@ import ( "github.com/stretchr/testify/assert" ) +// repo: either "baseOwner/baseRepo" or "baseOwner/baseRepo:defaultBranch" +// prHead: "headOwner/headRepo:headBranch" +func stubPR(repo, prHead string) (ghrepo.Interface, *api.PullRequest) { + defaultBranch := "" + if idx := strings.IndexRune(repo, ':'); idx >= 0 { + defaultBranch = repo[idx+1:] + repo = repo[:idx] + } + baseRepo, err := ghrepo.FromFullName(repo) + if err != nil { + panic(err) + } + if defaultBranch != "" { + baseRepo = api.InitRepoHostname(&api.Repository{ + Name: baseRepo.RepoName(), + Owner: api.RepositoryOwner{Login: baseRepo.RepoOwner()}, + DefaultBranchRef: api.BranchRef{Name: defaultBranch}, + }, baseRepo.RepoHost()) + } + + idx := strings.IndexRune(prHead, ':') + headRefName := prHead[idx+1:] + headRepo, err := ghrepo.FromFullName(prHead[:idx]) + if err != nil { + panic(err) + } + + return baseRepo, &api.PullRequest{ + ID: "THE-ID", + Number: 96, + State: "OPEN", + HeadRefName: headRefName, + HeadRepositoryOwner: api.Owner{Login: headRepo.RepoOwner()}, + HeadRepository: api.PRRepository{Name: headRepo.RepoName()}, + IsCrossRepository: !ghrepo.IsSame(baseRepo, headRepo), + } +} + func runCommand(rt http.RoundTripper, isTTY bool, cli string) (*test.CmdOut, error) { io, _, stdout, stderr := iostreams.Test() io.SetStdoutTTY(isTTY) @@ -29,12 +69,6 @@ func runCommand(rt http.RoundTripper, isTTY bool, cli string) (*test.CmdOut, err 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 - }, Branch: func() (string, error) { return "trunk", nil }, @@ -63,13 +97,10 @@ func TestPrClose(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { - "pullRequest": { "id": "THE-ID", "number": 96, "title": "The title of the PR", "state": "OPEN" } - } } }`), - ) + baseRepo, pr := stubPR("OWNER/REPO", "OWNER/REPO:feature") + pr.Title = "The title of the PR" + shared.RunCommandFinder("96", pr, baseRepo) + http.Register( httpmock.GraphQL(`mutation PullRequestClose\b`), httpmock.GraphQLMutation(`{"id": "THE-ID"}`, @@ -79,57 +110,34 @@ func TestPrClose(t *testing.T) { ) output, err := runCommand(http, true, "96") - if err != nil { - t.Fatalf("error running command `pr close`: %v", err) - } - - r := regexp.MustCompile(`Closed pull request #96 \(The title of the PR\)`) - - if !r.MatchString(output.Stderr()) { - t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) - } + assert.NoError(t, err) + assert.Equal(t, "", output.String()) + assert.Equal(t, "✓ Closed pull request #96 (The title of the PR)\n", output.Stderr()) } func TestPrClose_alreadyClosed(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { - "pullRequest": { "number": 101, "title": "The title of the PR", "state": "CLOSED" } - } } }`), - ) + baseRepo, pr := stubPR("OWNER/REPO", "OWNER/REPO:feature") + pr.State = "CLOSED" + pr.Title = "The title of the PR" + shared.RunCommandFinder("96", pr, baseRepo) - output, err := runCommand(http, true, "101") - if err != nil { - t.Fatalf("error running command `pr close`: %v", err) - } - - r := regexp.MustCompile(`Pull request #101 \(The title of the PR\) is already closed`) - - if !r.MatchString(output.Stderr()) { - t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) - } + output, err := runCommand(http, true, "96") + assert.NoError(t, err) + assert.Equal(t, "", output.String()) + assert.Equal(t, "! Pull request #96 (The title of the PR) is already closed\n", output.Stderr()) } -func TestPrClose_deleteBranch(t *testing.T) { +func TestPrClose_deleteBranch_sameRepo(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "id": "THE-ID", - "number": 96, - "title": "The title of the PR", - "headRefName":"blueberries", - "headRepositoryOwner": {"login": "OWNER"}, - "state": "OPEN" } - } } }`), - ) + baseRepo, pr := stubPR("OWNER/REPO", "OWNER/REPO:blueberries") + pr.Title = "The title of the PR" + shared.RunCommandFinder("96", pr, baseRepo) + http.Register( httpmock.GraphQL(`mutation PullRequestClose\b`), httpmock.GraphQLMutation(`{"id": "THE-ID"}`, @@ -148,10 +156,77 @@ func TestPrClose_deleteBranch(t *testing.T) { cs.Register(`git branch -D blueberries`, 0, "") output, err := runCommand(http, true, `96 --delete-branch`) - if err != nil { - t.Fatalf("Got unexpected error running `pr close` %s", err) - } - - //nolint:staticcheck // prefer exact matchers over ExpectLines - test.ExpectLines(t, output.Stderr(), `Closed pull request #96 \(The title of the PR\)`, `Deleted branch blueberries`) + assert.NoError(t, err) + assert.Equal(t, "", output.String()) + assert.Equal(t, heredoc.Doc(` + ✓ Closed pull request #96 (The title of the PR) + ✓ Deleted branch blueberries + `), output.Stderr()) +} + +func TestPrClose_deleteBranch_crossRepo(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + + baseRepo, pr := stubPR("OWNER/REPO", "hubot/REPO:blueberries") + pr.Title = "The title of the PR" + shared.RunCommandFinder("96", pr, baseRepo) + + http.Register( + httpmock.GraphQL(`mutation PullRequestClose\b`), + httpmock.GraphQLMutation(`{"id": "THE-ID"}`, + func(inputs map[string]interface{}) { + assert.Equal(t, inputs["pullRequestId"], "THE-ID") + }), + ) + + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "") + cs.Register(`git branch -D blueberries`, 0, "") + + output, err := runCommand(http, true, `96 --delete-branch`) + assert.NoError(t, err) + assert.Equal(t, "", output.String()) + assert.Equal(t, heredoc.Doc(` + ✓ Closed pull request #96 (The title of the PR) + ! Avoiding deleting the remote branch of a pull request from fork + ✓ Deleted branch blueberries + `), output.Stderr()) +} + +func TestPrClose_deleteBranch_sameBranch(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + + baseRepo, pr := stubPR("OWNER/REPO:main", "OWNER/REPO:trunk") + pr.Title = "The title of the PR" + shared.RunCommandFinder("96", pr, baseRepo) + + http.Register( + httpmock.GraphQL(`mutation PullRequestClose\b`), + httpmock.GraphQLMutation(`{"id": "THE-ID"}`, + func(inputs map[string]interface{}) { + assert.Equal(t, inputs["pullRequestId"], "THE-ID") + }), + ) + http.Register( + httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/trunk"), + httpmock.StringResponse(`{}`)) + + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + cs.Register(`git checkout main`, 0, "") + cs.Register(`git rev-parse --verify refs/heads/trunk`, 0, "") + cs.Register(`git branch -D trunk`, 0, "") + + output, err := runCommand(http, true, `96 --delete-branch`) + assert.NoError(t, err) + assert.Equal(t, "", output.String()) + assert.Equal(t, heredoc.Doc(` + ✓ Closed pull request #96 (The title of the PR) + ✓ Deleted branch trunk and switched to branch main + `), output.Stderr()) } diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 8480462ed..8cc2549eb 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -17,6 +17,7 @@ import ( "github.com/cli/cli/internal/config" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/internal/run" + "github.com/cli/cli/pkg/cmd/pr/shared" prShared "github.com/cli/cli/pkg/cmd/pr/shared" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/httpmock" @@ -247,12 +248,7 @@ func TestPRCreate_recover(t *testing.T) { defer http.Verify(t) http.StubRepoInfoResponse("OWNER", "REPO", "master") - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes" : [ - ] } } } } - `)) + shared.RunCommandFinder("feature", nil, nil) http.Register( httpmock.GraphQL(`query RepositoryResolveMetadataIDs\b`), httpmock.StringResponse(` @@ -337,12 +333,7 @@ func TestPRCreate_nontty(t *testing.T) { defer http.Verify(t) http.StubRepoInfoResponse("OWNER", "REPO", "master") - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes" : [ - ] } } } }`), - ) + shared.RunCommandFinder("feature", nil, nil) http.Register( httpmock.GraphQL(`mutation PullRequestCreate\b`), httpmock.GraphQLMutation(` @@ -379,12 +370,7 @@ func TestPRCreate(t *testing.T) { http.Register( httpmock.GraphQL(`query UserCurrent\b`), httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes" : [ - ] } } } } - `)) + shared.RunCommandFinder("feature", nil, nil) http.Register( httpmock.GraphQL(`mutation PullRequestCreate\b`), httpmock.GraphQLMutation(` @@ -428,12 +414,7 @@ func TestPRCreate_NoMaintainerModify(t *testing.T) { http.Register( httpmock.GraphQL(`query UserCurrent\b`), httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes" : [ - ] } } } } - `)) + shared.RunCommandFinder("feature", nil, nil) http.Register( httpmock.GraphQL(`mutation PullRequestCreate\b`), httpmock.GraphQLMutation(` @@ -477,12 +458,7 @@ func TestPRCreate_createFork(t *testing.T) { http.Register( httpmock.GraphQL(`query UserCurrent\b`), httpmock.StringResponse(`{"data": {"viewer": {"login": "monalisa"} } }`)) - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes" : [ - ] } } } } - `)) + shared.RunCommandFinder("feature", nil, nil) http.Register( httpmock.REST("POST", "repos/OWNER/REPO/forks"), httpmock.StatusStringResponse(201, ` @@ -544,12 +520,7 @@ func TestPRCreate_pushedToNonBaseRepo(t *testing.T) { defer http.Verify(t) http.StubRepoInfoResponse("OWNER", "REPO", "master") - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes" : [ - ] } } } } - `)) + shared.RunCommandFinder("feature", nil, nil) http.Register( httpmock.GraphQL(`mutation PullRequestCreate\b`), httpmock.GraphQLMutation(` @@ -588,12 +559,7 @@ func TestPRCreate_pushedToDifferentBranchName(t *testing.T) { defer http.Verify(t) http.StubRepoInfoResponse("OWNER", "REPO", "master") - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes" : [ - ] } } } } - `)) + shared.RunCommandFinder("feature", nil, nil) http.Register( httpmock.GraphQL(`mutation PullRequestCreate\b`), httpmock.GraphQLMutation(` @@ -634,12 +600,7 @@ func TestPRCreate_nonLegacyTemplate(t *testing.T) { defer http.Verify(t) http.StubRepoInfoResponse("OWNER", "REPO", "master") - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes" : [ - ] } } } } - `)) + shared.RunCommandFinder("feature", nil, nil) http.Register( httpmock.GraphQL(`mutation PullRequestCreate\b`), httpmock.GraphQLMutation(` @@ -684,12 +645,7 @@ func TestPRCreate_metadata(t *testing.T) { defer http.Verify(t) http.StubRepoInfoResponse("OWNER", "REPO", "master") - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes": [ - ] } } } } - `)) + shared.RunCommandFinder("feature", nil, nil) http.Register( httpmock.GraphQL(`query RepositoryResolveMetadataIDs\b`), httpmock.StringResponse(` @@ -790,15 +746,7 @@ func TestPRCreate_alreadyExists(t *testing.T) { defer http.Verify(t) http.StubRepoInfoResponse("OWNER", "REPO", "master") - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes": [ - { "url": "https://github.com/OWNER/REPO/pull/123", - "headRefName": "feature", - "baseRefName": "master" } - ] } } } }`), - ) + shared.RunCommandFinder("feature", &api.PullRequest{URL: "https://github.com/OWNER/REPO/pull/123"}, ghrepo.New("OWNER", "REPO")) _, err := runCommand(http, nil, "feature", true, `-t title -b body -H feature`) assert.EqualError(t, err, "a pull request for branch \"feature\" into branch \"master\" already exists:\nhttps://github.com/OWNER/REPO/pull/123") diff --git a/pkg/cmd/pr/diff/diff_test.go b/pkg/cmd/pr/diff/diff_test.go index 2e81116a4..e98690f93 100644 --- a/pkg/cmd/pr/diff/diff_test.go +++ b/pkg/cmd/pr/diff/diff_test.go @@ -6,10 +6,10 @@ import ( "net/http" "testing" + "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/pkg/cmd/pr/shared" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/pkg/iostreams" @@ -119,27 +119,6 @@ func runCommand(rt http.RoundTripper, remotes context.Remotes, isTTY bool, cli s 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 := NewCmdDiff(factory, nil) @@ -161,61 +140,15 @@ func runCommand(rt http.RoundTripper, remotes context.Remotes, isTTY bool, cli s }, err } -func TestPRDiff_no_current_pr(t *testing.T) { - http := &httpmock.Registry{} - defer http.Verify(t) - - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { - "pullRequests": { "nodes": [] } - } } }`), - ) - - _, err := runCommand(http, nil, false, "") - assert.EqualError(t, err, `no pull requests found for branch "feature"`) -} - -func TestPRDiff_argument_not_found(t *testing.T) { - http := &httpmock.Registry{} - defer http.Verify(t) - - http.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { - "pullRequest": { "number": 123 } - } } }`), - ) - http.Register( - httpmock.REST("GET", "repos/OWNER/REPO/pulls/123"), - httpmock.StatusStringResponse(404, ""), - ) - - _, err := runCommand(http, nil, false, "123") - assert.EqualError(t, err, `could not find pull request diff: pull request not found`) -} - func TestPRDiff_notty(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes": [ - { "url": "https://github.com/OWNER/REPO/pull/123", - "number": 123, - "id": "foobar123", - "headRefName": "feature", - "baseRefName": "master" } - ] } } } }`), - ) + shared.RunCommandFinder("", &api.PullRequest{Number: 123}, ghrepo.New("OWNER", "REPO")) + http.Register( httpmock.REST("GET", "repos/OWNER/REPO/pulls/123"), - httpmock.StringResponse(testDiff), - ) + httpmock.StringResponse(testDiff)) output, err := runCommand(http, nil, false, "") if err != nil { @@ -230,23 +163,14 @@ func TestPRDiff_tty(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes": [ - { "url": "https://github.com/OWNER/REPO/pull/123", - "number": 123, - "id": "foobar123", - "headRefName": "feature", - "baseRefName": "master" } - ] } } } }`), - ) + shared.RunCommandFinder("123", &api.PullRequest{Number: 123}, ghrepo.New("OWNER", "REPO")) + http.Register( httpmock.REST("GET", "repos/OWNER/REPO/pulls/123"), httpmock.StringResponse(testDiff), ) - output, err := runCommand(http, nil, true, "") + output, err := runCommand(http, nil, true, "123") if err != nil { t.Fatalf("unexpected error: %s", err) } diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index 888dc6ec2..ea1b73562 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -151,6 +151,7 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman func editRun(opts *EditOptions) error { findOptions := shared.FindOptions{ Selector: opts.SelectorArg, + Fields: []string{"id", "url", "title", "body", "baseRefName", "reviewRequests", "assignees", "labels", "projectCards", "milestone"}, } pr, repo, err := opts.Finder.Find(findOptions) if err != nil { diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index c918f4a4c..12203b99a 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -309,6 +309,9 @@ func Test_editRun(t *testing.T) { name: "non-interactive", input: &EditOptions{ SelectorArg: "123", + Finder: shared.NewMockFinder("123", &api.PullRequest{ + URL: "https://github.com/OWNER/REPO/pull/123", + }, ghrepo.New("OWNER", "REPO")), Interactive: false, Editable: shared.Editable{ Title: shared.EditableString{ @@ -351,7 +354,6 @@ func Test_editRun(t *testing.T) { Fetcher: testFetcher{}, }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - mockPullRequestGet(t, reg) mockRepoMetadata(t, reg, false) mockPullRequestUpdate(t, reg) mockPullRequestReviewersUpdate(t, reg) @@ -362,6 +364,9 @@ func Test_editRun(t *testing.T) { name: "non-interactive skip reviewers", input: &EditOptions{ SelectorArg: "123", + Finder: shared.NewMockFinder("123", &api.PullRequest{ + URL: "https://github.com/OWNER/REPO/pull/123", + }, ghrepo.New("OWNER", "REPO")), Interactive: false, Editable: shared.Editable{ Title: shared.EditableString{ @@ -399,7 +404,6 @@ func Test_editRun(t *testing.T) { Fetcher: testFetcher{}, }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - mockPullRequestGet(t, reg) mockRepoMetadata(t, reg, true) mockPullRequestUpdate(t, reg) }, @@ -408,14 +412,16 @@ func Test_editRun(t *testing.T) { { name: "interactive", input: &EditOptions{ - SelectorArg: "123", + SelectorArg: "123", + Finder: shared.NewMockFinder("123", &api.PullRequest{ + URL: "https://github.com/OWNER/REPO/pull/123", + }, ghrepo.New("OWNER", "REPO")), Interactive: true, Surveyor: testSurveyor{}, Fetcher: testFetcher{}, EditorRetriever: testEditorRetriever{}, }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - mockPullRequestGet(t, reg) mockRepoMetadata(t, reg, false) mockPullRequestUpdate(t, reg) mockPullRequestReviewersUpdate(t, reg) @@ -425,14 +431,16 @@ func Test_editRun(t *testing.T) { { name: "interactive skip reviewers", input: &EditOptions{ - SelectorArg: "123", + SelectorArg: "123", + Finder: shared.NewMockFinder("123", &api.PullRequest{ + URL: "https://github.com/OWNER/REPO/pull/123", + }, ghrepo.New("OWNER", "REPO")), Interactive: true, Surveyor: testSurveyor{skipReviewers: true}, Fetcher: testFetcher{}, EditorRetriever: testEditorRetriever{}, }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - mockPullRequestGet(t, reg) mockRepoMetadata(t, reg, true) mockPullRequestUpdate(t, reg) }, @@ -463,18 +471,6 @@ func Test_editRun(t *testing.T) { } } -func mockPullRequestGet(_ *testing.T, reg *httpmock.Registry) { - reg.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "id": "456", - "number": 123, - "url": "https://github.com/OWNER/REPO/pull/123" - } } } }`), - ) -} - func mockRepoMetadata(_ *testing.T, reg *httpmock.Registry, skipReviewers bool) { reg.Register( httpmock.GraphQL(`query RepositoryAssignableUsers\b`), @@ -549,23 +545,13 @@ func mockRepoMetadata(_ *testing.T, reg *httpmock.Registry, skipReviewers bool) func mockPullRequestUpdate(t *testing.T, reg *httpmock.Registry) { reg.Register( httpmock.GraphQL(`mutation PullRequestUpdate\b`), - httpmock.GraphQLMutation(` - { "data": { "updatePullRequest": { "pullRequest": { - "id": "456" - } } } }`, - func(inputs map[string]interface{}) {}), - ) + httpmock.StringResponse(`{}`)) } func mockPullRequestReviewersUpdate(t *testing.T, reg *httpmock.Registry) { reg.Register( httpmock.GraphQL(`mutation PullRequestUpdateRequestReviews\b`), - httpmock.GraphQLMutation(` - { "data": { "requestReviews": { "pullRequest": { - "id": "456" - } } } }`, - func(inputs map[string]interface{}) {}), - ) + httpmock.StringResponse(`{}`)) } type testFetcher struct{} diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index df562e194..a91b9740d 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -668,12 +668,9 @@ func TestPrMerge_alreadyMerged(t *testing.T) { as.StubOne(true) output, err := runCommand(http, "blueberries", true, "pr merge 4") - if err != nil { - t.Fatalf("Got unexpected error running `pr merge` %s", err) - } - - //nolint:staticcheck // prefer exact matchers over ExpectLines - test.ExpectLines(t, output.Stderr(), "✓ Deleted branch blueberries and switched to branch master") + assert.NoError(t, err) + assert.Equal(t, "", output.String()) + assert.Equal(t, "✓ Deleted branch blueberries and switched to branch master\n", output.Stderr()) } func TestPrMerge_alreadyMerged_nonInteractive(t *testing.T) { diff --git a/pkg/cmd/pr/ready/ready.go b/pkg/cmd/pr/ready/ready.go index a00563b4b..6f4212057 100644 --- a/pkg/cmd/pr/ready/ready.go +++ b/pkg/cmd/pr/ready/ready.go @@ -7,9 +7,6 @@ import ( "github.com/MakeNowJust/heredoc" "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" @@ -18,11 +15,7 @@ import ( type ReadyOptions 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) Finder shared.PRFinder @@ -33,9 +26,6 @@ func NewCmdReady(f *cmdutil.Factory, runF func(*ReadyOptions) error) *cobra.Comm opts := &ReadyOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, - Config: f.Config, - Remotes: f.Remotes, - Branch: f.Branch, } cmd := &cobra.Command{ @@ -74,6 +64,7 @@ func readyRun(opts *ReadyOptions) error { findOptions := shared.FindOptions{ Selector: opts.SelectorArg, + Fields: []string{"id", "number", "state", "isDraft"}, } pr, baseRepo, err := opts.Finder.Find(findOptions) if err != nil { @@ -81,10 +72,10 @@ func readyRun(opts *ReadyOptions) error { } if !pr.IsOpen() { - fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d is closed. Only draft pull requests can be marked as \"ready for review\"", cs.Red("!"), pr.Number) + fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d is closed. Only draft pull requests can be marked as \"ready for review\"\n", cs.FailureIcon(), pr.Number) return cmdutil.SilentError } else if !pr.IsDraft { - fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d is already \"ready for review\"\n", cs.Yellow("!"), pr.Number) + fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d is already \"ready for review\"\n", cs.WarningIcon(), pr.Number) return nil } diff --git a/pkg/cmd/pr/ready/ready_test.go b/pkg/cmd/pr/ready/ready_test.go index a53a15d24..b4a39d452 100644 --- a/pkg/cmd/pr/ready/ready_test.go +++ b/pkg/cmd/pr/ready/ready_test.go @@ -4,13 +4,11 @@ import ( "bytes" "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/api" "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/httpmock" "github.com/cli/cli/pkg/iostreams" @@ -101,23 +99,6 @@ func runCommand(rt http.RoundTripper, isTTY bool, cli string) (*test.CmdOut, err 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) { - return context.Remotes{ - { - Remote: &git.Remote{Name: "origin"}, - Repo: ghrepo.New("OWNER", "REPO"), - }, - }, nil - }, - Branch: func() (string, error) { - return "main", nil - }, } cmd := NewCmdReady(factory, nil) @@ -143,13 +124,13 @@ func TestPRReady(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { - "pullRequest": { "id": "THE-ID", "number": 444, "state": "OPEN", "isDraft": true} - } } }`), - ) + shared.RunCommandFinder("123", &api.PullRequest{ + ID: "THE-ID", + Number: 123, + State: "OPEN", + IsDraft: true, + }, ghrepo.New("OWNER", "REPO")) + http.Register( httpmock.GraphQL(`mutation PullRequestReadyForReview\b`), httpmock.GraphQLMutation(`{"id": "THE-ID"}`, @@ -158,62 +139,42 @@ func TestPRReady(t *testing.T) { }), ) - output, err := runCommand(http, true, "444") - if err != nil { - t.Fatalf("error running command `pr ready`: %v", err) - } - - r := regexp.MustCompile(`Pull request #444 is marked as "ready for review"`) - - if !r.MatchString(output.Stderr()) { - t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) - } + output, err := runCommand(http, true, "123") + assert.NoError(t, err) + assert.Equal(t, "", output.String()) + assert.Equal(t, "✓ Pull request #123 is marked as \"ready for review\"\n", output.Stderr()) } func TestPRReady_alreadyReady(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { - "pullRequest": { "number": 445, "state": "OPEN", "isDraft": false} - } } }`), - ) + shared.RunCommandFinder("123", &api.PullRequest{ + ID: "THE-ID", + Number: 123, + State: "OPEN", + IsDraft: false, + }, ghrepo.New("OWNER", "REPO")) - output, err := runCommand(http, true, "445") - if err != nil { - t.Fatalf("error running command `pr ready`: %v", err) - } - - r := regexp.MustCompile(`Pull request #445 is already "ready for review"`) - - if !r.MatchString(output.Stderr()) { - t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) - } + output, err := runCommand(http, true, "123") + assert.NoError(t, err) + assert.Equal(t, "", output.String()) + assert.Equal(t, "! Pull request #123 is already \"ready for review\"\n", output.Stderr()) } func TestPRReady_closed(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { - "pullRequest": { "number": 446, "state": "CLOSED", "isDraft": true} - } } }`), - ) + shared.RunCommandFinder("123", &api.PullRequest{ + ID: "THE-ID", + Number: 123, + State: "CLOSED", + IsDraft: true, + }, ghrepo.New("OWNER", "REPO")) - output, err := runCommand(http, true, "446") - if err == nil { - t.Fatalf("expected an error running command `pr ready` on a review that is closed!: %v", err) - } - - r := regexp.MustCompile(`Pull request #446 is closed. Only draft pull requests can be marked as "ready for review"`) - - if !r.MatchString(output.Stderr()) { - t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) - } + output, err := runCommand(http, true, "123") + assert.EqualError(t, err, "SilentError") + assert.Equal(t, "", output.String()) + assert.Equal(t, "X Pull request #123 is closed. Only draft pull requests can be marked as \"ready for review\"\n", output.Stderr()) } diff --git a/pkg/cmd/pr/reopen/reopen.go b/pkg/cmd/pr/reopen/reopen.go index 72fb13659..2750d1cec 100644 --- a/pkg/cmd/pr/reopen/reopen.go +++ b/pkg/cmd/pr/reopen/reopen.go @@ -52,6 +52,7 @@ func reopenRun(opts *ReopenOptions) error { findOptions := shared.FindOptions{ Selector: opts.SelectorArg, + Fields: []string{"id", "number", "state", "title"}, } pr, baseRepo, err := opts.Finder.Find(findOptions) if err != nil { @@ -59,12 +60,12 @@ func reopenRun(opts *ReopenOptions) error { } if pr.State == "MERGED" { - fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d (%s) can't be reopened because it was already merged", cs.Red("!"), pr.Number, pr.Title) + fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d (%s) can't be reopened because it was already merged\n", cs.FailureIcon(), pr.Number, pr.Title) return cmdutil.SilentError } if pr.IsOpen() { - fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d (%s) is already open\n", cs.Yellow("!"), pr.Number, pr.Title) + fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d (%s) is already open\n", cs.WarningIcon(), pr.Number, pr.Title) return nil } diff --git a/pkg/cmd/pr/reopen/reopen_test.go b/pkg/cmd/pr/reopen/reopen_test.go index 9c94f11b8..f04db2c06 100644 --- a/pkg/cmd/pr/reopen/reopen_test.go +++ b/pkg/cmd/pr/reopen/reopen_test.go @@ -4,11 +4,11 @@ import ( "bytes" "io/ioutil" "net/http" - "regexp" "testing" - "github.com/cli/cli/internal/config" + "github.com/cli/cli/api" "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/httpmock" "github.com/cli/cli/pkg/iostreams" @@ -28,12 +28,6 @@ func runCommand(rt http.RoundTripper, isTTY bool, cli string) (*test.CmdOut, err 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 - }, } cmd := NewCmdReopen(factory, nil) @@ -59,13 +53,13 @@ func TestPRReopen(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { - "pullRequest": { "id": "THE-ID", "number": 666, "title": "The title of the PR", "state": "CLOSED" } - } } }`), - ) + shared.RunCommandFinder("123", &api.PullRequest{ + ID: "THE-ID", + Number: 123, + State: "CLOSED", + Title: "The title of the PR", + }, ghrepo.New("OWNER", "REPO")) + http.Register( httpmock.GraphQL(`mutation PullRequestReopen\b`), httpmock.GraphQLMutation(`{"id": "THE-ID"}`, @@ -74,95 +68,42 @@ func TestPRReopen(t *testing.T) { }), ) - output, err := runCommand(http, true, "666") - if err != nil { - t.Fatalf("error running command `pr reopen`: %v", err) - } - - r := regexp.MustCompile(`Reopened pull request #666 \(The title of the PR\)`) - - if !r.MatchString(output.Stderr()) { - t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) - } -} - -func TestPRReopen_BranchArg(t *testing.T) { - http := &httpmock.Registry{} - defer http.Verify(t) - - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { - "nodes": [ - { "id": "THE-ID", "number": 666, "title": "The title of the PR", "headRefName": "fix-bug", "state": "CLOSED" } - ] - } } } }`), - ) - http.Register( - httpmock.GraphQL(`mutation PullRequestReopen\b`), - httpmock.GraphQLMutation(`{"id": "THE-ID"}`, - func(inputs map[string]interface{}) { - assert.Equal(t, inputs["pullRequestId"], "THE-ID") - }), - ) - - output, err := runCommand(http, true, "fix-bug") - if err != nil { - t.Fatalf("error running command `pr reopen`: %v", err) - } - - r := regexp.MustCompile(`Reopened pull request #666 \(The title of the PR\)`) - - if !r.MatchString(output.Stderr()) { - t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) - } + output, err := runCommand(http, true, "123") + assert.NoError(t, err) + assert.Equal(t, "", output.String()) + assert.Equal(t, "✓ Reopened pull request #123 (The title of the PR)\n", output.Stderr()) } func TestPRReopen_alreadyOpen(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { - "pullRequest": { "number": 666, "title": "The title of the PR", "state": "OPEN" } - } } }`), - ) + shared.RunCommandFinder("123", &api.PullRequest{ + ID: "THE-ID", + Number: 123, + State: "OPEN", + Title: "The title of the PR", + }, ghrepo.New("OWNER", "REPO")) - output, err := runCommand(http, true, "666") - if err != nil { - t.Fatalf("error running command `pr reopen`: %v", err) - } - - r := regexp.MustCompile(`Pull request #666 \(The title of the PR\) is already open`) - - if !r.MatchString(output.Stderr()) { - t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) - } + output, err := runCommand(http, true, "123") + assert.NoError(t, err) + assert.Equal(t, "", output.String()) + assert.Equal(t, "! Pull request #123 (The title of the PR) is already open\n", output.Stderr()) } func TestPRReopen_alreadyMerged(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { - "pullRequest": { "number": 666, "title": "The title of the PR", "state": "MERGED"} - } } }`), - ) + shared.RunCommandFinder("123", &api.PullRequest{ + ID: "THE-ID", + Number: 123, + State: "MERGED", + Title: "The title of the PR", + }, ghrepo.New("OWNER", "REPO")) - output, err := runCommand(http, true, "666") - if err == nil { - t.Fatalf("expected an error running command `pr reopen`: %v", err) - } - - r := regexp.MustCompile(`Pull request #666 \(The title of the PR\) 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()) - } + output, err := runCommand(http, true, "123") + assert.EqualError(t, err, "SilentError") + assert.Equal(t, "", output.String()) + assert.Equal(t, "X Pull request #123 (The title of the PR) can't be reopened because it was already merged\n", output.Stderr()) } diff --git a/pkg/cmd/pr/review/review.go b/pkg/cmd/pr/review/review.go index 606af85dc..06452149f 100644 --- a/pkg/cmd/pr/review/review.go +++ b/pkg/cmd/pr/review/review.go @@ -147,6 +147,7 @@ func NewCmdReview(f *cmdutil.Factory, runF func(*ReviewOptions) error) *cobra.Co func reviewRun(opts *ReviewOptions) error { findOptions := shared.FindOptions{ Selector: opts.SelectorArg, + Fields: []string{"id", "number"}, } pr, baseRepo, err := opts.Finder.Find(findOptions) if err != nil { diff --git a/pkg/cmd/pr/review/review_test.go b/pkg/cmd/pr/review/review_test.go index 84d526368..de65e471c 100644 --- a/pkg/cmd/pr/review/review_test.go +++ b/pkg/cmd/pr/review/review_test.go @@ -6,13 +6,14 @@ import ( "io/ioutil" "net/http" "path/filepath" - "regexp" "testing" + "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/pkg/cmd/pr/shared" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/pkg/iostreams" @@ -178,24 +179,6 @@ func runCommand(rt http.RoundTripper, remotes context.Remotes, isTTY bool, cli s 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) @@ -217,219 +200,67 @@ func runCommand(rt http.RoundTripper, remotes context.Remotes, isTTY bool, cli s }, err } -func TestPRReview_url_arg(t *testing.T) { - http := &httpmock.Registry{} - defer http.Verify(t) - - http.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "id": "foobar123", - "number": 123, - "headRefName": "feature", - "headRepositoryOwner": { - "login": "hubot" - }, - "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } - }, - "isCrossRepository": false, - "maintainerCanModify": false - } } } }`), - ) - http.Register( - httpmock.GraphQL(`mutation PullRequestReviewAdd\b`), - httpmock.GraphQLMutation(`{"data": {} }`, - func(inputs map[string]interface{}) { - assert.Equal(t, inputs["pullRequestId"], "foobar123") - assert.Equal(t, inputs["event"], "APPROVE") - assert.Equal(t, inputs["body"], "") - }), - ) - - 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) - } - - //nolint:staticcheck // prefer exact matchers over ExpectLines - test.ExpectLines(t, output.Stderr(), "Approved pull request #123") -} - -func TestPRReview_number_arg(t *testing.T) { - http := &httpmock.Registry{} - defer http.Verify(t) - - http.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "id": "foobar123", - "number": 123, - "headRefName": "feature", - "headRepositoryOwner": { - "login": "hubot" - }, - "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } - }, - "isCrossRepository": false, - "maintainerCanModify": false - } } } } `), - ) - http.Register( - httpmock.GraphQL(`mutation PullRequestReviewAdd`), - httpmock.GraphQLMutation(`{"data": {} }`, - func(inputs map[string]interface{}) { - assert.Equal(t, inputs["pullRequestId"], "foobar123") - assert.Equal(t, inputs["event"], "APPROVE") - assert.Equal(t, inputs["body"], "") - }), - ) - - output, err := runCommand(http, nil, true, "--approve 123") - if err != nil { - t.Fatalf("error running pr review: %s", err) - } - - //nolint:staticcheck // prefer exact matchers over ExpectLines - test.ExpectLines(t, output.Stderr(), "Approved pull request #123") -} - -func TestPRReview_no_arg(t *testing.T) { - http := &httpmock.Registry{} - defer http.Verify(t) - - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes": [ - { "url": "https://github.com/OWNER/REPO/pull/123", - "number": 123, - "id": "foobar123", - "headRefName": "feature", - "baseRefName": "master" } - ] } } } }`), - ) - http.Register( - httpmock.GraphQL(`mutation PullRequestReviewAdd\b`), - httpmock.GraphQLMutation(`{"data": {} }`, - func(inputs map[string]interface{}) { - assert.Equal(t, inputs["pullRequestId"], "foobar123") - assert.Equal(t, inputs["event"], "COMMENT") - assert.Equal(t, inputs["body"], "cool story") - }), - ) - - output, err := runCommand(http, nil, true, `--comment -b "cool story"`) - if err != nil { - t.Fatalf("error running pr review: %s", err) - } - - //nolint:staticcheck // prefer exact matchers over ExpectLines - test.ExpectLines(t, output.Stderr(), "Reviewed pull request #123") -} - func TestPRReview(t *testing.T) { - type c struct { - Cmd string - ExpectedEvent string - ExpectedBody string - } - cases := []c{ - {`--request-changes -b"bad"`, "REQUEST_CHANGES", "bad"}, - {`--approve`, "APPROVE", ""}, - {`--approve -b"hot damn"`, "APPROVE", "hot damn"}, - {`--comment --body "i dunno"`, "COMMENT", "i dunno"}, + tests := []struct { + args string + wantEvent string + wantBody string + }{ + { + args: `--request-changes -b"bad"`, + wantEvent: "REQUEST_CHANGES", + wantBody: "bad", + }, + { + args: `--approve`, + wantEvent: "APPROVE", + wantBody: "", + }, + { + args: `--approve -b"hot damn"`, + wantEvent: "APPROVE", + wantBody: "hot damn", + }, + { + args: `--comment --body "i dunno"`, + wantEvent: "COMMENT", + wantBody: "i dunno", + }, } - for _, kase := range cases { - t.Run(kase.Cmd, func(t *testing.T) { + for _, tt := range tests { + t.Run(tt.args, func(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes": [ - { "url": "https://github.com/OWNER/REPO/pull/123", - "id": "foobar123", - "headRefName": "feature", - "baseRefName": "master" } - ] } } } }`), - ) + shared.RunCommandFinder("", &api.PullRequest{ID: "THE-ID"}, ghrepo.New("OWNER", "REPO")) + http.Register( httpmock.GraphQL(`mutation PullRequestReviewAdd\b`), httpmock.GraphQLMutation(`{"data": {} }`, func(inputs map[string]interface{}) { - assert.Equal(t, inputs["event"], kase.ExpectedEvent) - assert.Equal(t, inputs["body"], kase.ExpectedBody) + assert.Equal(t, map[string]interface{}{ + "pullRequestId": "THE-ID", + "event": tt.wantEvent, + "body": tt.wantBody, + }, inputs) }), ) - _, err := runCommand(http, nil, false, kase.Cmd) - if err != nil { - t.Fatalf("got unexpected error running %s: %s", kase.Cmd, err) - } + output, err := runCommand(http, nil, false, tt.args) + assert.NoError(t, err) + assert.Equal(t, "", output.String()) + assert.Equal(t, "", output.Stderr()) }) } } -func TestPRReview_nontty(t *testing.T) { - http := &httpmock.Registry{} - defer http.Verify(t) - - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes": [ - { "url": "https://github.com/OWNER/REPO/pull/123", - "number": 123, - "id": "foobar123", - "headRefName": "feature", - "baseRefName": "master" } - ] } } } }`), - ) - http.Register( - httpmock.GraphQL(`mutation PullRequestReviewAdd\b`), - httpmock.GraphQLMutation(`{"data": {} }`, - func(inputs map[string]interface{}) { - assert.Equal(t, inputs["event"], "COMMENT") - assert.Equal(t, inputs["body"], "cool") - }), - ) - - output, err := runCommand(http, nil, false, "-c -bcool") - if err != nil { - t.Fatalf("unexpected error running command: %s", err) - } - - assert.Equal(t, "", output.String()) - assert.Equal(t, "", output.Stderr()) -} - func TestPRReview_interactive(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes": [ - { "url": "https://github.com/OWNER/REPO/pull/123", - "number": 123, - "id": "foobar123", - "headRefName": "feature", - "baseRefName": "master" } - ] } } } }`), - ) + shared.RunCommandFinder("", &api.PullRequest{ID: "THE-ID", Number: 123}, ghrepo.New("OWNER", "REPO")) + http.Register( httpmock.GraphQL(`mutation PullRequestReviewAdd\b`), httpmock.GraphQLMutation(`{"data": {} }`, @@ -462,33 +293,21 @@ func TestPRReview_interactive(t *testing.T) { }) output, err := runCommand(http, nil, true, "") - if err != nil { - t.Fatalf("got unexpected error running pr review: %s", err) - } + assert.NoError(t, err) + assert.Equal(t, heredoc.Doc(` + Got: - //nolint:staticcheck // prefer exact matchers over ExpectLines - test.ExpectLines(t, output.Stderr(), "Approved pull request #123") - - //nolint:staticcheck // prefer exact matchers over ExpectLines - test.ExpectLines(t, output.String(), - "Got:", - "cool.*story") + cool story + + `), output.String()) + assert.Equal(t, "✓ Approved pull request #123\n", output.Stderr()) } func TestPRReview_interactive_no_body(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes": [ - { "url": "https://github.com/OWNER/REPO/pull/123", - "id": "foobar123", - "headRefName": "feature", - "baseRefName": "master" } - ] } } } }`), - ) + shared.RunCommandFinder("", &api.PullRequest{ID: "THE-ID", Number: 123}, ghrepo.New("OWNER", "REPO")) as, teardown := prompt.InitAskStubber() defer teardown() @@ -520,17 +339,8 @@ func TestPRReview_interactive_blank_approve(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes": [ - { "url": "https://github.com/OWNER/REPO/pull/123", - "number": 123, - "id": "foobar123", - "headRefName": "feature", - "baseRefName": "master" } - ] } } } }`), - ) + shared.RunCommandFinder("", &api.PullRequest{ID: "THE-ID", Number: 123}, ghrepo.New("OWNER", "REPO")) + http.Register( httpmock.GraphQL(`mutation PullRequestReviewAdd\b`), httpmock.GraphQLMutation(`{"data": {} }`, @@ -563,15 +373,7 @@ func TestPRReview_interactive_blank_approve(t *testing.T) { }) output, err := runCommand(http, nil, true, "") - if err != nil { - t.Fatalf("got unexpected error running pr review: %s", err) - } - - unexpect := regexp.MustCompile("Got:") - if unexpect.MatchString(output.String()) { - t.Errorf("did not expect to see body printed in %s", output.String()) - } - - //nolint:staticcheck // prefer exact matchers over ExpectLines - test.ExpectLines(t, output.Stderr(), "Approved pull request #123") + assert.NoError(t, err) + assert.Equal(t, "", output.String()) + assert.Equal(t, "✓ Approved pull request #123\n", output.Stderr()) } diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index cd7e3c314..c17885ab2 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -292,10 +292,15 @@ func (err *NotFoundError) Unwrap() error { } func NewMockFinder(selector string, pr *api.PullRequest, repo ghrepo.Interface) PRFinder { + var err error + if pr == nil { + err = &NotFoundError{errors.New("no pull requests found")} + } return &mockFinder{ expectSelector: selector, pr: pr, repo: repo, + err: err, } } diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go index f3600962e..7149bd4f8 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -15,6 +15,7 @@ func TestFind(t *testing.T) { branchFn func() (string, error) remotesFn func() (context.Remotes, error) selector string + fields []string } tests := []struct { name string @@ -28,6 +29,7 @@ func TestFind(t *testing.T) { name: "number argument", args: args{ selector: "13", + fields: []string{"id", "number"}, baseRepoFn: func() (ghrepo.Interface, error) { return ghrepo.FromFullName("OWNER/REPO") }, @@ -42,10 +44,24 @@ func TestFind(t *testing.T) { wantPR: 13, wantRepo: "https://github.com/OWNER/REPO", }, + { + name: "number only", + args: args{ + selector: "13", + fields: []string{"number"}, + baseRepoFn: func() (ghrepo.Interface, error) { + return ghrepo.FromFullName("OWNER/REPO") + }, + }, + httpStub: nil, + wantPR: 13, + wantRepo: "https://github.com/OWNER/REPO", + }, { name: "number with hash argument", args: args{ selector: "#13", + fields: []string{"id", "number"}, baseRepoFn: func() (ghrepo.Interface, error) { return ghrepo.FromFullName("OWNER/REPO") }, @@ -64,6 +80,7 @@ func TestFind(t *testing.T) { name: "URL argument", args: args{ selector: "https://example.org/OWNER/REPO/pull/13/files", + fields: []string{"id", "number"}, baseRepoFn: nil, }, httpStub: func(r *httpmock.Registry) { @@ -96,6 +113,7 @@ func TestFind(t *testing.T) { pr, repo, err := f.Find(FindOptions{ Selector: tt.args.selector, + Fields: tt.args.fields, }) if (err != nil) != tt.wantErr { t.Errorf("Find() error = %v, wantErr %v", err, tt.wantErr) diff --git a/pkg/cmd/pr/view/fixtures/prView.json b/pkg/cmd/pr/view/fixtures/prView.json deleted file mode 100644 index c15a828a6..000000000 --- a/pkg/cmd/pr/view/fixtures/prView.json +++ /dev/null @@ -1,54 +0,0 @@ -{ - "data": { - "repository": { - "pullRequests": { - "nodes": [ - { - "number": 12, - "title": "Blueberries are from a fork", - "state": "OPEN", - "body": "yeah", - "url": "https://github.com/OWNER/REPO/pull/12", - "headRefName": "blueberries", - "baseRefName": "master", - "headRepositoryOwner": { - "login": "hubot" - }, - "additions": 100, - "deletions": 10, - "commits": { - "totalCount": 12 - }, - "author": { - "login": "nobody" - }, - "isCrossRepository": true, - "isDraft": false - }, - { - "number": 10, - "title": "Blueberries are a good fruit", - "state": "OPEN", - "body": "**blueberries taste good**", - "url": "https://github.com/OWNER/REPO/pull/10", - "baseRefName": "master", - "headRefName": "blueberries", - "author": { - "login": "nobody" - }, - "additions": 100, - "deletions": 10, - "headRepositoryOwner": { - "login": "OWNER" - }, - "commits": { - "totalCount": 8 - }, - "isCrossRepository": false, - "isDraft": false - } - ] - } - } - } -} diff --git a/pkg/cmd/pr/view/fixtures/prViewPreviewDraftStatebyBranch.json b/pkg/cmd/pr/view/fixtures/prViewPreviewDraftStatebyBranch.json deleted file mode 100644 index 03a55dbc5..000000000 --- a/pkg/cmd/pr/view/fixtures/prViewPreviewDraftStatebyBranch.json +++ /dev/null @@ -1,54 +0,0 @@ -{ - "data": { - "repository": { - "pullRequests": { - "nodes": [ - { - "number": 12, - "title": "Blueberries are from a fork", - "state": "OPEN", - "body": "yeah", - "url": "https://github.com/OWNER/REPO/pull/12", - "headRefName": "blueberries", - "baseRefName": "master", - "headRepositoryOwner": { - "login": "hubot" - }, - "additions": 100, - "deletions": 10, - "commits": { - "totalCount": 12 - }, - "author": { - "login": "nobody" - }, - "isCrossRepository": true, - "isDraft": false - }, - { - "number": 10, - "title": "Blueberries are a good fruit", - "state": "OPEN", - "body": "**blueberries taste good**", - "url": "https://github.com/OWNER/REPO/pull/10", - "baseRefName": "master", - "headRefName": "blueberries", - "author": { - "login": "nobody" - }, - "headRepositoryOwner": { - "login": "OWNER" - }, - "additions": 100, - "deletions": 10, - "commits": { - "totalCount": 8 - }, - "isCrossRepository": false, - "isDraft": true - } - ] - } - } - } -} diff --git a/pkg/cmd/pr/view/fixtures/prViewPreviewNoReviews.json b/pkg/cmd/pr/view/fixtures/prViewPreviewNoReviews.json deleted file mode 100644 index 92e1a5a75..000000000 --- a/pkg/cmd/pr/view/fixtures/prViewPreviewNoReviews.json +++ /dev/null @@ -1 +0,0 @@ -{ "data": { "repository": { "pullRequest": { "reviews": { } } } } } diff --git a/pkg/cmd/pr/view/fixtures/prViewPreviewWithMetadataByBranch.json b/pkg/cmd/pr/view/fixtures/prViewPreviewWithMetadataByBranch.json deleted file mode 100644 index 9893ac523..000000000 --- a/pkg/cmd/pr/view/fixtures/prViewPreviewWithMetadataByBranch.json +++ /dev/null @@ -1,139 +0,0 @@ -{ - "data": { - "repository": { - "pullRequests": { - "nodes": [ - { - "id": "PR_12", - "number": 12, - "title": "Blueberries are from a fork", - "state": "OPEN", - "body": "yeah", - "url": "https://github.com/OWNER/REPO/pull/12", - "headRefName": "blueberries", - "baseRefName": "master", - "additions": 100, - "deletions": 10, - "headRepositoryOwner": { - "login": "hubot" - }, - "assignees": { - "nodes": [], - "totalcount": 0 - }, - "labels": { - "nodes": [], - "totalcount": 0 - }, - "projectcards": { - "nodes": [], - "totalcount": 0 - }, - "milestone": {}, - "commits": { - "totalCount": 12 - }, - "author": { - "login": "nobody" - }, - "isCrossRepository": true, - "isDraft": false - }, - { - "id": "PR_10", - "number": 10, - "title": "Blueberries are a good fruit", - "state": "OPEN", - "body": "**blueberries taste good**", - "url": "https://github.com/OWNER/REPO/pull/10", - "baseRefName": "master", - "headRefName": "blueberries", - "author": { - "login": "nobody" - }, - "additions": 100, - "deletions": 10, - "assignees": { - "nodes": [ - { - "login": "marseilles" - }, - { - "login": "monaco" - } - ], - "totalcount": 2 - }, - "labels": { - "nodes": [ - { - "name": "one" - }, - { - "name": "two" - }, - { - "name": "three" - }, - { - "name": "four" - }, - { - "name": "five" - } - ], - "totalcount": 5 - }, - "projectcards": { - "nodes": [ - { - "project": { - "name": "Project 1" - }, - "column": { - "name": "column A" - } - }, - { - "project": { - "name": "Project 2" - }, - "column": { - "name": "column B" - } - }, - { - "project": { - "name": "Project 3" - }, - "column": { - "name": "column C" - } - } - ], - "totalcount": 3 - }, - "milestone": { - "title": "uluru" - }, - "headRepositoryOwner": { - "login": "OWNER" - }, - "commits": { - "nodes": [ - { - "commit": { - "oid": "123456789" - } - } - ], - "totalCount": 8 - }, - "isCrossRepository": false, - "isDraft": false - } - ] - } - } - } -} diff --git a/pkg/cmd/pr/view/fixtures/prView_EmptyBody.json b/pkg/cmd/pr/view/fixtures/prView_EmptyBody.json deleted file mode 100644 index dcc2be64b..000000000 --- a/pkg/cmd/pr/view/fixtures/prView_EmptyBody.json +++ /dev/null @@ -1,52 +0,0 @@ -{ - "data": { - "repository": { - "pullRequests": { - "nodes": [ - { - "number": 12, - "title": "Blueberries are from a fork", - "state": "OPEN", - "body": "yeah", - "url": "https://github.com/OWNER/REPO/pull/12", - "headRefName": "blueberries", - "baseRefName": "master", - "headRepositoryOwner": { - "login": "hubot" - }, - "additions": 100, - "deletions": 10, - "commits": { - "totalCount": 12 - }, - "author": { - "login": "nobody" - }, - "isCrossRepository": true - }, - { - "number": 10, - "title": "Blueberries are a good fruit", - "state": "OPEN", - "body": "", - "url": "https://github.com/OWNER/REPO/pull/10", - "baseRefName": "master", - "headRefName": "blueberries", - "additions": 100, - "deletions": 10, - "author": { - "login": "nobody" - }, - "headRepositoryOwner": { - "login": "OWNER" - }, - "commits": { - "totalCount": 8 - }, - "isCrossRepository": false - } - ] - } - } - } -} diff --git a/pkg/cmd/pr/view/fixtures/prView_NoActiveBranch.json b/pkg/cmd/pr/view/fixtures/prView_NoActiveBranch.json deleted file mode 100644 index 7c1fb0c05..000000000 --- a/pkg/cmd/pr/view/fixtures/prView_NoActiveBranch.json +++ /dev/null @@ -1,15 +0,0 @@ -{"data":{ - "repository": { - "pullRequests": { - "edges": [] - } - }, - "viewerCreated": { - "edges": [], - "pageInfo": { "hasNextPage": false } - }, - "reviewRequested": { - "edges": [], - "pageInfo": { "hasNextPage": false } - } -}} \ No newline at end of file diff --git a/pkg/cmd/pr/view/view_test.go b/pkg/cmd/pr/view/view_test.go index cda0e266c..78d588f1e 100644 --- a/pkg/cmd/pr/view/view_test.go +++ b/pkg/cmd/pr/view/view_test.go @@ -2,16 +2,17 @@ package view import ( "bytes" + "encoding/json" "fmt" "io/ioutil" "net/http" + "os" "testing" - "github.com/cli/cli/context" - "github.com/cli/cli/git" - "github.com/cli/cli/internal/config" + "github.com/cli/cli/api" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/internal/run" + "github.com/cli/cli/pkg/cmd/pr/shared" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/pkg/iostreams" @@ -121,26 +122,6 @@ func runCommand(rt http.RoundTripper, branch string, isTTY bool, cli string) (*t factory := &cmdutil.Factory{ IOStreams: io, Browser: browser, - 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) { - return context.Remotes{ - { - Remote: &git.Remote{Name: "origin"}, - Repo: ghrepo.New("OWNER", "REPO"), - }, - }, nil - }, - Branch: func() (string, error) { - return branch, nil - }, } cmd := NewCmdView(factory, nil) @@ -163,6 +144,50 @@ func runCommand(rt http.RoundTripper, branch string, isTTY bool, cli string) (*t }, err } +// hack for compatibility with old JSON fixture files +func prFromFixtures(fixtures map[string]string) (*api.PullRequest, error) { + var response struct { + Data struct { + Repository struct { + PullRequest *api.PullRequest + } + } + } + + ff, err := os.Open(fixtures["PullRequestByNumber"]) + if err != nil { + return nil, err + } + defer ff.Close() + + dec := json.NewDecoder(ff) + err = dec.Decode(&response) + if err != nil { + return nil, err + } + + for name := range fixtures { + switch name { + case "PullRequestByNumber": + case "ReviewsForPullRequest", "CommentsForPullRequest": + ff, err := os.Open(fixtures[name]) + if err != nil { + return nil, err + } + defer ff.Close() + dec := json.NewDecoder(ff) + err = dec.Decode(&response) + if err != nil { + return nil, err + } + default: + return nil, fmt.Errorf("unrecognized fixture type: %q", name) + } + } + + return response.Data.Repository.PullRequest, nil +} + func TestPRView_Preview_nontty(t *testing.T) { tests := map[string]struct { branch string @@ -174,8 +199,7 @@ func TestPRView_Preview_nontty(t *testing.T) { branch: "master", args: "12", fixtures: map[string]string{ - "PullRequestByNumber": "./fixtures/prViewPreview.json", - "ReviewsForPullRequest": "./fixtures/prViewPreviewNoReviews.json", + "PullRequestByNumber": "./fixtures/prViewPreview.json", }, expectedOutputs: []string{ `title:\tBlueberries are from a fork\n`, @@ -197,8 +221,7 @@ func TestPRView_Preview_nontty(t *testing.T) { branch: "master", args: "12", fixtures: map[string]string{ - "PullRequestByNumber": "./fixtures/prViewPreviewWithMetadataByNumber.json", - "ReviewsForPullRequest": "./fixtures/prViewPreviewNoReviews.json", + "PullRequestByNumber": "./fixtures/prViewPreviewWithMetadataByNumber.json", }, expectedOutputs: []string{ `title:\tBlueberries are from a fork\n`, @@ -231,74 +254,11 @@ func TestPRView_Preview_nontty(t *testing.T) { `\*\*blueberries taste good\*\*`, }, }, - "Open PR with metadata by branch": { - branch: "master", - args: "blueberries", - fixtures: map[string]string{ - "PullRequestForBranch": "./fixtures/prViewPreviewWithMetadataByBranch.json", - "ReviewsForPullRequest": "./fixtures/prViewPreviewNoReviews.json", - }, - expectedOutputs: []string{ - `title:\tBlueberries are a good fruit`, - `state:\tOPEN`, - `author:\tnobody`, - `assignees:\tmarseilles, monaco\n`, - `reviewers:\t\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`, - `additions:\t100\n`, - `deletions:\t10\n`, - `blueberries taste good`, - }, - }, - "Open PR for the current branch": { - branch: "blueberries", - args: "", - fixtures: map[string]string{ - "PullRequestForBranch": "./fixtures/prView.json", - "ReviewsForPullRequest": "./fixtures/prViewPreviewNoReviews.json", - }, - expectedOutputs: []string{ - `title:\tBlueberries are a good fruit`, - `state:\tOPEN`, - `author:\tnobody`, - `assignees:\t\n`, - `reviewers:\t\n`, - `labels:\t\n`, - `projects:\t\n`, - `milestone:\t\n`, - `additions:\t100\n`, - `deletions:\t10\n`, - `\*\*blueberries taste good\*\*`, - }, - }, - "Open PR wth empty body for the current branch": { - branch: "blueberries", - args: "", - fixtures: map[string]string{ - "PullRequestForBranch": "./fixtures/prView_EmptyBody.json", - "ReviewsForPullRequest": "./fixtures/prViewPreviewNoReviews.json", - }, - expectedOutputs: []string{ - `title:\tBlueberries are a good fruit`, - `state:\tOPEN`, - `author:\tnobody`, - `assignees:\t\n`, - `reviewers:\t\n`, - `labels:\t\n`, - `projects:\t\n`, - `milestone:\t\n`, - `additions:\t100\n`, - `deletions:\t10\n`, - }, - }, "Closed PR": { branch: "master", args: "12", fixtures: map[string]string{ - "PullRequestByNumber": "./fixtures/prViewPreviewClosedState.json", - "ReviewsForPullRequest": "./fixtures/prViewPreviewNoReviews.json", + "PullRequestByNumber": "./fixtures/prViewPreviewClosedState.json", }, expectedOutputs: []string{ `state:\tCLOSED\n`, @@ -317,8 +277,7 @@ func TestPRView_Preview_nontty(t *testing.T) { branch: "master", args: "12", fixtures: map[string]string{ - "PullRequestByNumber": "./fixtures/prViewPreviewMergedState.json", - "ReviewsForPullRequest": "./fixtures/prViewPreviewNoReviews.json", + "PullRequestByNumber": "./fixtures/prViewPreviewMergedState.json", }, expectedOutputs: []string{ `state:\tMERGED\n`, @@ -337,8 +296,7 @@ func TestPRView_Preview_nontty(t *testing.T) { branch: "master", args: "12", fixtures: map[string]string{ - "PullRequestByNumber": "./fixtures/prViewPreviewDraftState.json", - "ReviewsForPullRequest": "./fixtures/prViewPreviewNoReviews.json", + "PullRequestByNumber": "./fixtures/prViewPreviewDraftState.json", }, expectedOutputs: []string{ `title:\tBlueberries are from a fork\n`, @@ -354,37 +312,16 @@ func TestPRView_Preview_nontty(t *testing.T) { `\*\*blueberries taste good\*\*`, }, }, - "Draft PR by branch": { - branch: "master", - args: "blueberries", - fixtures: map[string]string{ - "PullRequestForBranch": "./fixtures/prViewPreviewDraftStatebyBranch.json", - "ReviewsForPullRequest": "./fixtures/prViewPreviewNoReviews.json", - }, - expectedOutputs: []string{ - `title:\tBlueberries are a good fruit\n`, - `state:\tDRAFT\n`, - `author:\tnobody\n`, - `labels:`, - `assignees:`, - `reviewers:`, - `projects:`, - `milestone:`, - `additions:\t100\n`, - `deletions:\t10\n`, - `\*\*blueberries taste good\*\*`, - }, - }, } for name, tc := range tests { t.Run(name, func(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - for name, file := range tc.fixtures { - name := fmt.Sprintf(`query %s\b`, name) - http.Register(httpmock.GraphQL(name), httpmock.FileResponse(file)) - } + + pr, err := prFromFixtures(tc.fixtures) + require.NoError(t, err) + shared.RunCommandFinder("12", pr, ghrepo.New("OWNER", "REPO")) output, err := runCommand(http, tc.branch, false, tc.args) if err != nil { @@ -410,8 +347,7 @@ func TestPRView_Preview(t *testing.T) { branch: "master", args: "12", fixtures: map[string]string{ - "PullRequestByNumber": "./fixtures/prViewPreview.json", - "ReviewsForPullRequest": "./fixtures/prViewPreviewNoReviews.json", + "PullRequestByNumber": "./fixtures/prViewPreview.json", }, expectedOutputs: []string{ `Blueberries are from a fork`, @@ -424,8 +360,7 @@ func TestPRView_Preview(t *testing.T) { branch: "master", args: "12", fixtures: map[string]string{ - "PullRequestByNumber": "./fixtures/prViewPreviewWithMetadataByNumber.json", - "ReviewsForPullRequest": "./fixtures/prViewPreviewNoReviews.json", + "PullRequestByNumber": "./fixtures/prViewPreviewWithMetadataByNumber.json", }, expectedOutputs: []string{ `Blueberries are from a fork`, @@ -453,57 +388,11 @@ func TestPRView_Preview(t *testing.T) { `View this pull request on GitHub: https://github.com/OWNER/REPO/pull/12`, }, }, - "Open PR with metadata by branch": { - branch: "master", - args: "blueberries", - fixtures: map[string]string{ - "PullRequestForBranch": "./fixtures/prViewPreviewWithMetadataByBranch.json", - "ReviewsForPullRequest": "./fixtures/prViewPreviewNoReviews.json", - }, - expectedOutputs: []string{ - `Blueberries are a good fruit`, - `Open.*nobody wants to merge 8 commits into master from blueberries.+100.-10`, - `Assignees:.*marseilles, monaco\n`, - `Labels:.*one, two, three, four, five\n`, - `Projects:.*Project 1 \(column A\), Project 2 \(column B\), Project 3 \(column C\)\n`, - `Milestone:.*uluru\n`, - `blueberries taste good`, - `View this pull request on GitHub: https://github.com/OWNER/REPO/pull/10`, - }, - }, - "Open PR for the current branch": { - branch: "blueberries", - args: "", - fixtures: map[string]string{ - "PullRequestForBranch": "./fixtures/prView.json", - "ReviewsForPullRequest": "./fixtures/prViewPreviewNoReviews.json", - }, - expectedOutputs: []string{ - `Blueberries are a good fruit`, - `Open.*nobody wants to merge 8 commits into master from blueberries.+100.-10`, - `blueberries taste good`, - `View this pull request on GitHub: https://github.com/OWNER/REPO/pull/10`, - }, - }, - "Open PR wth empty body for the current branch": { - branch: "blueberries", - args: "", - fixtures: map[string]string{ - "PullRequestForBranch": "./fixtures/prView_EmptyBody.json", - "ReviewsForPullRequest": "./fixtures/prViewPreviewNoReviews.json", - }, - expectedOutputs: []string{ - `Blueberries are a good fruit`, - `Open.*nobody wants to merge 8 commits into master from blueberries.+100.-10`, - `View this pull request on GitHub: https://github.com/OWNER/REPO/pull/10`, - }, - }, "Closed PR": { branch: "master", args: "12", fixtures: map[string]string{ - "PullRequestByNumber": "./fixtures/prViewPreviewClosedState.json", - "ReviewsForPullRequest": "./fixtures/prViewPreviewNoReviews.json", + "PullRequestByNumber": "./fixtures/prViewPreviewClosedState.json", }, expectedOutputs: []string{ `Blueberries are from a fork`, @@ -516,8 +405,7 @@ func TestPRView_Preview(t *testing.T) { branch: "master", args: "12", fixtures: map[string]string{ - "PullRequestByNumber": "./fixtures/prViewPreviewMergedState.json", - "ReviewsForPullRequest": "./fixtures/prViewPreviewNoReviews.json", + "PullRequestByNumber": "./fixtures/prViewPreviewMergedState.json", }, expectedOutputs: []string{ `Blueberries are from a fork`, @@ -530,8 +418,7 @@ func TestPRView_Preview(t *testing.T) { branch: "master", args: "12", fixtures: map[string]string{ - "PullRequestByNumber": "./fixtures/prViewPreviewDraftState.json", - "ReviewsForPullRequest": "./fixtures/prViewPreviewNoReviews.json", + "PullRequestByNumber": "./fixtures/prViewPreviewDraftState.json", }, expectedOutputs: []string{ `Blueberries are from a fork`, @@ -540,30 +427,16 @@ func TestPRView_Preview(t *testing.T) { `View this pull request on GitHub: https://github.com/OWNER/REPO/pull/12`, }, }, - "Draft PR by branch": { - branch: "master", - args: "blueberries", - fixtures: map[string]string{ - "PullRequestForBranch": "./fixtures/prViewPreviewDraftStatebyBranch.json", - "ReviewsForPullRequest": "./fixtures/prViewPreviewNoReviews.json", - }, - expectedOutputs: []string{ - `Blueberries are a good fruit`, - `Draft.*nobody wants to merge 8 commits into master from blueberries.+100.-10`, - `blueberries taste good`, - `View this pull request on GitHub: https://github.com/OWNER/REPO/pull/10`, - }, - }, } for name, tc := range tests { t.Run(name, func(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - for name, file := range tc.fixtures { - name := fmt.Sprintf(`query %s\b`, name) - http.Register(httpmock.GraphQL(name), httpmock.FileResponse(file)) - } + + pr, err := prFromFixtures(tc.fixtures) + require.NoError(t, err) + shared.RunCommandFinder("12", pr, ghrepo.New("OWNER", "REPO")) output, err := runCommand(http, tc.branch, true, tc.args) if err != nil { @@ -581,13 +454,12 @@ func TestPRView_Preview(t *testing.T) { func TestPRView_web_currentBranch(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register(httpmock.GraphQL(`query PullRequestForBranch\b`), httpmock.FileResponse("./fixtures/prView.json")) - cs, cmdTeardown := run.Stub() + shared.RunCommandFinder("", &api.PullRequest{URL: "https://github.com/OWNER/REPO/pull/10"}, ghrepo.New("OWNER", "REPO")) + + _, cmdTeardown := run.Stub() defer cmdTeardown(t) - cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "") - output, err := runCommand(http, "blueberries", true, "-w") if err != nil { t.Errorf("error running command `pr view`: %v", err) @@ -601,41 +473,16 @@ func TestPRView_web_currentBranch(t *testing.T) { func TestPRView_web_noResultsForBranch(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - http.Register(httpmock.GraphQL(`query PullRequestForBranch\b`), httpmock.FileResponse("./fixtures/prView_NoActiveBranch.json")) - cs, cmdTeardown := run.Stub() - defer cmdTeardown(t) - - cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "") - - _, err := runCommand(http, "blueberries", true, "-w") - if err == nil || err.Error() != `no pull requests found for branch "blueberries"` { - t.Errorf("error running command `pr view`: %v", err) - } -} - -func TestPRView_web_numberArg(t *testing.T) { - http := &httpmock.Registry{} - defer http.Verify(t) - - http.Register( - httpmock.GraphQL(`query PullRequestByNumber\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequest": { - "url": "https://github.com/OWNER/REPO/pull/23" - } } } }`), - ) + shared.RunCommandFinder("", nil, nil) _, cmdTeardown := run.Stub() defer cmdTeardown(t) - output, err := runCommand(http, "master", true, "-w 23") - if err != nil { + _, err := runCommand(http, "blueberries", true, "-w") + if err == nil || err.Error() != `no pull requests found` { t.Errorf("error running command `pr view`: %v", err) } - - assert.Equal(t, "", output.String()) - assert.Equal(t, "https://github.com/OWNER/REPO/pull/23", output.BrowsedURL) } func TestPRView_tty_Comments(t *testing.T) { @@ -715,10 +562,15 @@ func TestPRView_tty_Comments(t *testing.T) { t.Run(name, func(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - for name, file := range tt.fixtures { - name := fmt.Sprintf(`query %s\b`, name) - http.Register(httpmock.GraphQL(name), httpmock.FileResponse(file)) + + if len(tt.fixtures) > 0 { + pr, err := prFromFixtures(tt.fixtures) + require.NoError(t, err) + shared.RunCommandFinder("123", pr, ghrepo.New("OWNER", "REPO")) + } else { + shared.RunCommandFinder("123", nil, nil) } + output, err := runCommand(http, tt.branch, true, tt.cli) if tt.wantsErr { assert.Error(t, err) @@ -821,10 +673,15 @@ func TestPRView_nontty_Comments(t *testing.T) { t.Run(name, func(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) - for name, file := range tt.fixtures { - name := fmt.Sprintf(`query %s\b`, name) - http.Register(httpmock.GraphQL(name), httpmock.FileResponse(file)) + + if len(tt.fixtures) > 0 { + pr, err := prFromFixtures(tt.fixtures) + require.NoError(t, err) + shared.RunCommandFinder("123", pr, ghrepo.New("OWNER", "REPO")) + } else { + shared.RunCommandFinder("123", nil, nil) } + output, err := runCommand(http, tt.branch, false, tt.cli) if tt.wantsErr { assert.Error(t, err) From 51f7cbdfde0a533b99db23f2e3d2f607ea693d16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 18 May 2021 09:58:21 +0200 Subject: [PATCH 3/6] :nail_care: cleanup and tests for PR finder --- api/queries_pr.go | 6 - pkg/cmd/pr/close/close.go | 2 +- pkg/cmd/pr/close/close_test.go | 2 +- pkg/cmd/pr/merge/merge_test.go | 10 +- pkg/cmd/pr/shared/finder.go | 62 ++++--- pkg/cmd/pr/shared/finder_test.go | 282 +++++++++++++++++++++++++++++-- 6 files changed, 317 insertions(+), 47 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index b5441a359..dfeb8608d 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -118,12 +118,6 @@ type PullRequestCommitCommit struct { } } -func (pr *PullRequest) StubCommit(oid string) { - pr.Commits.Nodes = append(pr.Commits.Nodes, PullRequestCommit{ - Commit: PullRequestCommitCommit{Oid: oid}, - }) -} - type PullRequestFile struct { Path string `json:"path"` Additions int `json:"additions"` diff --git a/pkg/cmd/pr/close/close.go b/pkg/cmd/pr/close/close.go index 041d48edc..2c9442ecb 100644 --- a/pkg/cmd/pr/close/close.go +++ b/pkg/cmd/pr/close/close.go @@ -122,7 +122,7 @@ func closeRun(opts *CloseOptions) error { } if pr.IsCrossRepository { - fmt.Fprintf(opts.IO.ErrOut, "%s Avoiding deleting the remote branch of a pull request from fork\n", cs.WarningIcon()) + fmt.Fprintf(opts.IO.ErrOut, "%s Skipped deleting the remote branch of a pull request from fork\n", cs.WarningIcon()) if !opts.DeleteLocalBranch { return nil } diff --git a/pkg/cmd/pr/close/close_test.go b/pkg/cmd/pr/close/close_test.go index 4024398dd..4a02dc13d 100644 --- a/pkg/cmd/pr/close/close_test.go +++ b/pkg/cmd/pr/close/close_test.go @@ -191,7 +191,7 @@ func TestPrClose_deleteBranch_crossRepo(t *testing.T) { assert.Equal(t, "", output.String()) assert.Equal(t, heredoc.Doc(` ✓ Closed pull request #96 (The title of the PR) - ! Avoiding deleting the remote branch of a pull request from fork + ! Skipped deleting the remote branch of a pull request from fork ✓ Deleted branch blueberries `), output.Stderr()) } diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index a91b9740d..4abb88aca 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -203,6 +203,12 @@ func baseRepo(owner, repo, branch string) ghrepo.Interface { }, "github.com") } +func stubCommit(pr *api.PullRequest, oid string) { + pr.Commits.Nodes = append(pr.Commits.Nodes, api.PullRequestCommit{ + Commit: api.PullRequestCommitCommit{Oid: oid}, + }) +} + func runCommand(rt http.RoundTripper, branch string, isTTY bool, cli string) (*test.CmdOut, error) { io, _, stdout, stderr := iostreams.Test() io.SetStdoutTTY(isTTY) @@ -456,7 +462,7 @@ func Test_nonDivergingPullRequest(t *testing.T) { Title: "Blueberries are a good fruit", State: "OPEN", } - pr.StubCommit("COMMITSHA1") + stubCommit(pr, "COMMITSHA1") shared.RunCommandFinder( "", @@ -497,7 +503,7 @@ func Test_divergingPullRequestWarning(t *testing.T) { Title: "Blueberries are a good fruit", State: "OPEN", } - pr.StubCommit("COMMITSHA1") + stubCommit(pr, "COMMITSHA1") shared.RunCommandFinder( "", diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index c17885ab2..52b2a436a 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -28,11 +28,12 @@ type progressIndicator interface { } type finder struct { - baseRepoFn func() (ghrepo.Interface, error) - branchFn func() (string, error) - remotesFn func() (context.Remotes, error) - httpClient func() (*http.Client, error) - progress progressIndicator + baseRepoFn func() (ghrepo.Interface, error) + branchFn func() (string, error) + remotesFn func() (context.Remotes, error) + httpClient func() (*http.Client, error) + branchConfig func(string) git.BranchConfig + progress progressIndicator repo ghrepo.Interface prNumber int @@ -47,11 +48,12 @@ func NewFinder(factory *cmdutil.Factory) PRFinder { } return &finder{ - baseRepoFn: factory.BaseRepo, - branchFn: factory.Branch, - remotesFn: factory.Remotes, - httpClient: factory.HttpClient, - progress: factory.IOStreams, + baseRepoFn: factory.BaseRepo, + branchFn: factory.Branch, + remotesFn: factory.Remotes, + httpClient: factory.HttpClient, + progress: factory.IOStreams, + branchConfig: git.ReadBranchConfig, } } @@ -79,7 +81,10 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err return nil, nil, errors.New("Find error: no fields specified") } - _ = f.parseURL(opts.Selector) + if repo, prNumber, err := f.parseURL(opts.Selector); err == nil { + f.prNumber = prNumber + f.repo = repo + } if f.repo == nil { repo, err := f.baseRepoFn() @@ -90,8 +95,12 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err } if opts.Selector == "" { - if err := f.parseCurrentBranch(); err != nil { + if branch, prNumber, err := f.parseCurrentBranch(); err != nil { return nil, nil, err + } else if prNumber > 0 { + f.prNumber = prNumber + } else { + f.branchName = branch } } else if f.prNumber == 0 { if prNumber, err := strconv.Atoi(strings.TrimPrefix(opts.Selector, "#")); err == nil { @@ -129,44 +138,44 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err var pullURLRE = regexp.MustCompile(`^/([^/]+)/([^/]+)/pull/(\d+)`) -func (f *finder) parseURL(prURL string) error { +func (f *finder) parseURL(prURL string) (ghrepo.Interface, int, error) { if prURL == "" { - return fmt.Errorf("invalid URL: %q", prURL) + return nil, 0, fmt.Errorf("invalid URL: %q", prURL) } u, err := url.Parse(prURL) if err != nil { - return err + return nil, 0, err } if u.Scheme != "https" && u.Scheme != "http" { - return fmt.Errorf("invalid scheme: %s", u.Scheme) + return nil, 0, fmt.Errorf("invalid scheme: %s", u.Scheme) } m := pullURLRE.FindStringSubmatch(u.Path) if m == nil { - return fmt.Errorf("not a pull request URL: %s", prURL) + return nil, 0, fmt.Errorf("not a pull request URL: %s", prURL) } - f.repo = ghrepo.NewWithHost(m[1], m[2], u.Hostname()) - f.prNumber, _ = strconv.Atoi(m[3]) - return nil + repo := ghrepo.NewWithHost(m[1], m[2], u.Hostname()) + prNumber, _ := strconv.Atoi(m[3]) + return repo, prNumber, nil } var prHeadRE = regexp.MustCompile(`^refs/pull/(\d+)/head$`) -func (f *finder) parseCurrentBranch() error { +func (f *finder) parseCurrentBranch() (string, int, error) { prHeadRef, err := f.branchFn() if err != nil { - return err + return "", 0, err } - branchConfig := git.ReadBranchConfig(prHeadRef) + branchConfig := f.branchConfig(prHeadRef) // the branch is configured to merge a special PR head ref if m := prHeadRE.FindStringSubmatch(branchConfig.MergeRef); m != nil { - f.prNumber, _ = strconv.Atoi(m[1]) - return nil + prNumber, _ := strconv.Atoi(m[1]) + return "", prNumber, nil } var branchOwner string @@ -193,8 +202,7 @@ func (f *finder) parseCurrentBranch() error { } } - f.branchName = prHeadRef - return nil + return prHeadRef, 0, nil } func findByNumber(httpClient *http.Client, repo ghrepo.Interface, number int, fields []string) (*api.PullRequest, error) { diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go index 7149bd4f8..488e470ff 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -1,21 +1,26 @@ package shared import ( + "errors" "net/http" + "net/url" "testing" "github.com/cli/cli/context" + "github.com/cli/cli/git" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/httpmock" ) func TestFind(t *testing.T) { type args struct { - baseRepoFn func() (ghrepo.Interface, error) - branchFn func() (string, error) - remotesFn func() (context.Remotes, error) - selector string - fields []string + baseRepoFn func() (ghrepo.Interface, error) + branchFn func() (string, error) + branchConfig func(string) git.BranchConfig + remotesFn func() (context.Remotes, error) + selector string + fields []string + baseBranch string } tests := []struct { name string @@ -44,6 +49,25 @@ func TestFind(t *testing.T) { wantPR: 13, wantRepo: "https://github.com/OWNER/REPO", }, + { + name: "baseRepo is error", + args: args{ + selector: "13", + fields: []string{"id", "number"}, + baseRepoFn: func() (ghrepo.Interface, error) { + return nil, errors.New("baseRepoErr") + }, + }, + wantErr: true, + }, + { + name: "blank fields is error", + args: args{ + selector: "13", + fields: []string{}, + }, + wantErr: true, + }, { name: "number only", args: args{ @@ -93,6 +117,233 @@ func TestFind(t *testing.T) { wantPR: 13, wantRepo: "https://example.org/OWNER/REPO", }, + { + name: "branch argument", + args: args{ + selector: "blueberries", + fields: []string{"id", "number"}, + baseRepoFn: func() (ghrepo.Interface, error) { + return ghrepo.FromFullName("OWNER/REPO") + }, + }, + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.GraphQL(`query PullRequestForBranch\b`), + httpmock.StringResponse(`{"data":{"repository":{ + "pullRequests":{"nodes":[ + { + "number": 14, + "state": "CLOSED", + "baseRefName": "main", + "headRefName": "blueberries", + "isCrossRepository": false, + "headRepositoryOwner": {"login":"OWNER"} + }, + { + "number": 13, + "state": "OPEN", + "baseRefName": "main", + "headRefName": "blueberries", + "isCrossRepository": false, + "headRepositoryOwner": {"login":"OWNER"} + } + ]} + }}}`)) + }, + wantPR: 13, + wantRepo: "https://github.com/OWNER/REPO", + }, + { + name: "branch argument with base branch", + args: args{ + selector: "blueberries", + baseBranch: "main", + fields: []string{"id", "number"}, + baseRepoFn: func() (ghrepo.Interface, error) { + return ghrepo.FromFullName("OWNER/REPO") + }, + }, + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.GraphQL(`query PullRequestForBranch\b`), + httpmock.StringResponse(`{"data":{"repository":{ + "pullRequests":{"nodes":[ + { + "number": 14, + "state": "OPEN", + "baseRefName": "dev", + "headRefName": "blueberries", + "isCrossRepository": false, + "headRepositoryOwner": {"login":"OWNER"} + }, + { + "number": 13, + "state": "OPEN", + "baseRefName": "main", + "headRefName": "blueberries", + "isCrossRepository": false, + "headRepositoryOwner": {"login":"OWNER"} + } + ]} + }}}`)) + }, + wantPR: 13, + wantRepo: "https://github.com/OWNER/REPO", + }, + { + name: "no argument reads current branch", + args: args{ + selector: "", + fields: []string{"id", "number"}, + baseRepoFn: func() (ghrepo.Interface, error) { + return ghrepo.FromFullName("OWNER/REPO") + }, + branchFn: func() (string, error) { + return "blueberries", nil + }, + branchConfig: func(branch string) (c git.BranchConfig) { + return + }, + }, + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.GraphQL(`query PullRequestForBranch\b`), + httpmock.StringResponse(`{"data":{"repository":{ + "pullRequests":{"nodes":[ + { + "number": 13, + "state": "OPEN", + "baseRefName": "main", + "headRefName": "blueberries", + "isCrossRepository": false, + "headRepositoryOwner": {"login":"OWNER"} + } + ]} + }}}`)) + }, + wantPR: 13, + wantRepo: "https://github.com/OWNER/REPO", + }, + { + name: "current branch is error", + args: args{ + selector: "", + fields: []string{"id", "number"}, + baseRepoFn: func() (ghrepo.Interface, error) { + return ghrepo.FromFullName("OWNER/REPO") + }, + branchFn: func() (string, error) { + return "", errors.New("branchErr") + }, + }, + wantErr: true, + }, + { + name: "current branch with upstream configuration", + args: args{ + selector: "", + fields: []string{"id", "number"}, + baseRepoFn: func() (ghrepo.Interface, error) { + return ghrepo.FromFullName("OWNER/REPO") + }, + branchFn: func() (string, error) { + return "blueberries", nil + }, + branchConfig: func(branch string) (c git.BranchConfig) { + c.MergeRef = "refs/heads/blue-upstream-berries" + c.RemoteName = "origin" + return + }, + remotesFn: func() (context.Remotes, error) { + return context.Remotes{{ + Remote: &git.Remote{Name: "origin"}, + Repo: ghrepo.New("UPSTREAMOWNER", "REPO"), + }}, nil + }, + }, + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.GraphQL(`query PullRequestForBranch\b`), + httpmock.StringResponse(`{"data":{"repository":{ + "pullRequests":{"nodes":[ + { + "number": 13, + "state": "OPEN", + "baseRefName": "main", + "headRefName": "blue-upstream-berries", + "isCrossRepository": true, + "headRepositoryOwner": {"login":"UPSTREAMOWNER"} + } + ]} + }}}`)) + }, + wantPR: 13, + wantRepo: "https://github.com/OWNER/REPO", + }, + { + name: "current branch with upstream configuration", + args: args{ + selector: "", + fields: []string{"id", "number"}, + baseRepoFn: func() (ghrepo.Interface, error) { + return ghrepo.FromFullName("OWNER/REPO") + }, + branchFn: func() (string, error) { + return "blueberries", nil + }, + branchConfig: func(branch string) (c git.BranchConfig) { + u, _ := url.Parse("https://github.com/UPSTREAMOWNER/REPO") + c.MergeRef = "refs/heads/blue-upstream-berries" + c.RemoteURL = u + return + }, + remotesFn: nil, + }, + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.GraphQL(`query PullRequestForBranch\b`), + httpmock.StringResponse(`{"data":{"repository":{ + "pullRequests":{"nodes":[ + { + "number": 13, + "state": "OPEN", + "baseRefName": "main", + "headRefName": "blue-upstream-berries", + "isCrossRepository": true, + "headRepositoryOwner": {"login":"UPSTREAMOWNER"} + } + ]} + }}}`)) + }, + wantPR: 13, + wantRepo: "https://github.com/OWNER/REPO", + }, + { + name: "current branch made by pr checkout", + args: args{ + selector: "", + fields: []string{"id", "number"}, + baseRepoFn: func() (ghrepo.Interface, error) { + return ghrepo.FromFullName("OWNER/REPO") + }, + branchFn: func() (string, error) { + return "blueberries", nil + }, + branchConfig: func(branch string) (c git.BranchConfig) { + c.MergeRef = "refs/pull/13/head" + return + }, + }, + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.GraphQL(`query PullRequestByNumber\b`), + httpmock.StringResponse(`{"data":{"repository":{ + "pullRequest":{"number":13} + }}}`)) + }, + wantPR: 13, + wantRepo: "https://github.com/OWNER/REPO", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -106,19 +357,30 @@ func TestFind(t *testing.T) { httpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - baseRepoFn: tt.args.baseRepoFn, - branchFn: tt.args.branchFn, - remotesFn: tt.args.remotesFn, + baseRepoFn: tt.args.baseRepoFn, + branchFn: tt.args.branchFn, + branchConfig: tt.args.branchConfig, + remotesFn: tt.args.remotesFn, } pr, repo, err := f.Find(FindOptions{ - Selector: tt.args.selector, - Fields: tt.args.fields, + Selector: tt.args.selector, + Fields: tt.args.fields, + BaseBranch: tt.args.baseBranch, }) if (err != nil) != tt.wantErr { t.Errorf("Find() error = %v, wantErr %v", err, tt.wantErr) return } + if tt.wantErr { + if tt.wantPR > 0 { + t.Error("wantPR field is not checked in error case") + } + if tt.wantRepo != "" { + t.Error("wantRepo field is not checked in error case") + } + return + } if pr.Number != tt.wantPR { t.Errorf("want pr #%d, got #%d", tt.wantPR, pr.Number) From e758f30073002dcb06fa4b3bb10e69706b5982be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 18 May 2021 16:59:03 +0200 Subject: [PATCH 4/6] Fix preloading of pr reviews, checks, and issue/pr comments --- api/export_pr.go | 25 ++- api/export_pr_test.go | 2 +- api/pull_request_test.go | 2 +- api/queries_comments.go | 83 +------- api/queries_pr.go | 60 ++++-- api/queries_pr_review.go | 43 +---- api/query_builder.go | 58 ++++-- .../issueView_previewFullComments.json | 4 +- pkg/cmd/issue/view/http.go | 50 +++++ pkg/cmd/issue/view/view.go | 42 +++-- pkg/cmd/pr/checks/checks.go | 6 +- pkg/cmd/pr/checks/checks_test.go | 4 +- pkg/cmd/pr/checks/fixtures/allPassing.json | 2 +- pkg/cmd/pr/checks/fixtures/someFailing.json | 2 +- pkg/cmd/pr/checks/fixtures/somePending.json | 2 +- pkg/cmd/pr/checks/fixtures/withStatuses.json | 2 +- pkg/cmd/pr/merge/merge.go | 2 +- pkg/cmd/pr/merge/merge_test.go | 2 +- pkg/cmd/pr/shared/finder.go | 177 +++++++++++++++++- .../pr/status/fixtures/prStatusChecks.json | 6 +- pkg/cmd/pr/view/view.go | 5 +- 21 files changed, 383 insertions(+), 196 deletions(-) create mode 100644 pkg/cmd/issue/view/http.go diff --git a/api/export_pr.go b/api/export_pr.go index 9a0e10702..96411aead 100644 --- a/api/export_pr.go +++ b/api/export_pr.go @@ -49,11 +49,34 @@ func (pr *PullRequest) ExportData(fields []string) *map[string]interface{} { data[f] = nil } case "statusCheckRollup": - if n := pr.Commits.Nodes; len(n) > 0 { + if n := pr.StatusCheckRollup.Nodes; len(n) > 0 { data[f] = n[0].Commit.StatusCheckRollup.Contexts.Nodes } else { data[f] = nil } + case "commits": + commits := make([]interface{}, 0, len(pr.Commits.Nodes)) + for _, c := range pr.Commits.Nodes { + commit := c.Commit + authors := make([]interface{}, 0, len(commit.Authors.Nodes)) + for _, author := range commit.Authors.Nodes { + authors = append(authors, map[string]interface{}{ + "name": author.Name, + "email": author.Email, + "id": author.User.ID, + "login": author.User.Login, + }) + } + commits = append(commits, map[string]interface{}{ + "oid": commit.OID, + "messageHeadline": commit.MessageHeadline, + "messageBody": commit.MessageBody, + "committedDate": commit.CommittedDate, + "authoredDate": commit.AuthoredDate, + "authors": authors, + }) + } + data[f] = commits case "comments": data[f] = pr.Comments.Nodes case "assignees": diff --git a/api/export_pr_test.go b/api/export_pr_test.go index 4e02f9f09..9243e6a8d 100644 --- a/api/export_pr_test.go +++ b/api/export_pr_test.go @@ -129,7 +129,7 @@ func TestPullRequest_ExportData(t *testing.T) { name: "status checks", fields: []string{"statusCheckRollup"}, inputJSON: heredoc.Doc(` - { "commits": { "nodes": [ + { "statusCheckRollup": { "nodes": [ { "commit": { "statusCheckRollup": { "contexts": { "nodes": [ { "__typename": "CheckRun", diff --git a/api/pull_request_test.go b/api/pull_request_test.go index 9fb1d9e72..2e4fa73b1 100644 --- a/api/pull_request_test.go +++ b/api/pull_request_test.go @@ -10,7 +10,7 @@ import ( func TestPullRequest_ChecksStatus(t *testing.T) { pr := PullRequest{} payload := ` - { "commits": { "nodes": [{ "commit": { + { "statusCheckRollup": { "nodes": [{ "commit": { "statusCheckRollup": { "contexts": { "nodes": [ diff --git a/api/queries_comments.go b/api/queries_comments.go index f02322c22..999c39033 100644 --- a/api/queries_comments.go +++ b/api/queries_comments.go @@ -4,7 +4,6 @@ import ( "context" "time" - "github.com/cli/cli/internal/ghrepo" "github.com/shurcooL/githubv4" "github.com/shurcooL/graphql" ) @@ -12,7 +11,10 @@ import ( type Comments struct { Nodes []Comment TotalCount int - PageInfo PageInfo + PageInfo struct { + HasNextPage bool + EndCursor string + } } type Comment struct { @@ -26,83 +28,6 @@ type Comment struct { ReactionGroups ReactionGroups `json:"reactionGroups"` } -type PageInfo struct { - HasNextPage bool - EndCursor string -} - -func CommentsForIssue(client *Client, repo ghrepo.Interface, issue *Issue) (*Comments, error) { - type response struct { - Repository struct { - Issue struct { - Comments Comments `graphql:"comments(first: 100, after: $endCursor)"` - } `graphql:"issue(number: $number)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } - - variables := map[string]interface{}{ - "owner": githubv4.String(repo.RepoOwner()), - "repo": githubv4.String(repo.RepoName()), - "number": githubv4.Int(issue.Number), - "endCursor": (*githubv4.String)(nil), - } - - gql := graphQLClient(client.http, repo.RepoHost()) - - var comments []Comment - for { - var query response - err := gql.QueryNamed(context.Background(), "CommentsForIssue", &query, variables) - if err != nil { - return nil, err - } - - comments = append(comments, query.Repository.Issue.Comments.Nodes...) - if !query.Repository.Issue.Comments.PageInfo.HasNextPage { - break - } - variables["endCursor"] = githubv4.String(query.Repository.Issue.Comments.PageInfo.EndCursor) - } - - return &Comments{Nodes: comments, TotalCount: len(comments)}, nil -} - -func CommentsForPullRequest(client *Client, repo ghrepo.Interface, pr *PullRequest) (*Comments, error) { - type response struct { - Repository struct { - PullRequest struct { - Comments Comments `graphql:"comments(first: 100, after: $endCursor)"` - } `graphql:"pullRequest(number: $number)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } - - variables := map[string]interface{}{ - "owner": githubv4.String(repo.RepoOwner()), - "repo": githubv4.String(repo.RepoName()), - "number": githubv4.Int(pr.Number), - "endCursor": (*githubv4.String)(nil), - } - - gql := graphQLClient(client.http, repo.RepoHost()) - - var comments []Comment - for { - var query response - err := gql.QueryNamed(context.Background(), "CommentsForPullRequest", &query, variables) - if err != nil { - return nil, err - } - - comments = append(comments, query.Repository.PullRequest.Comments.Nodes...) - if !query.Repository.PullRequest.Comments.PageInfo.HasNextPage { - break - } - variables["endCursor"] = githubv4.String(query.Repository.PullRequest.Comments.PageInfo.EndCursor) - } - - return &Comments{Nodes: comments, TotalCount: len(comments)}, nil -} - type CommentCreateInput struct { Body string SubjectId string diff --git a/api/queries_pr.go b/api/queries_pr.go index dfeb8608d..eefbcdd49 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -75,6 +75,33 @@ type PullRequest struct { TotalCount int Nodes []PullRequestCommit } + StatusCheckRollup struct { + Nodes []struct { + Commit struct { + StatusCheckRollup struct { + Contexts struct { + Nodes []struct { + TypeName string `json:"__typename"` + Name string `json:"name"` + Context string `json:"context,omitempty"` + State string `json:"state,omitempty"` + Status string `json:"status"` + Conclusion string `json:"conclusion"` + StartedAt time.Time `json:"startedAt"` + CompletedAt time.Time `json:"completedAt"` + DetailsURL string `json:"detailsUrl"` + TargetURL string `json:"targetUrl,omitempty"` + } + PageInfo struct { + HasNextPage bool + EndCursor string + } + } + } + } + } + } + Assignees Assignees Labels Labels ProjectCards ProjectCards @@ -89,6 +116,7 @@ type PRRepository struct { Name string } +// Commit loads just the commit SHA and nothing else type Commit struct { OID string `json:"oid"` } @@ -97,25 +125,20 @@ type PullRequestCommit struct { Commit PullRequestCommitCommit } -// PullRequestCommitCommit is like "Commit" but with StatusCheckRollup +// PullRequestCommitCommit contains full information about a commit type PullRequestCommitCommit struct { - Oid string - StatusCheckRollup struct { - Contexts struct { - Nodes []struct { - TypeName string `json:"__typename"` - Name string `json:"name"` - Context string `json:"context,omitempty"` - State string `json:"state,omitempty"` - Status string `json:"status"` - Conclusion string `json:"conclusion"` - StartedAt time.Time `json:"startedAt"` - CompletedAt time.Time `json:"completedAt"` - DetailsURL string `json:"detailsUrl"` - TargetURL string `json:"targetUrl,omitempty"` - } + OID string `json:"oid"` + Authors struct { + Nodes []struct { + Name string + Email string + User GitHubUser } } + MessageHeadline string + MessageBody string + CommittedDate time.Time + AuthoredDate time.Time } type PullRequestFile struct { @@ -132,7 +155,6 @@ type ReviewRequests struct { Name string `json:"name"` } } - TotalCount int } func (r ReviewRequests) Logins() []string { @@ -189,10 +211,10 @@ type PullRequestChecksStatus struct { } func (pr *PullRequest) ChecksStatus() (summary PullRequestChecksStatus) { - if len(pr.Commits.Nodes) == 0 { + if len(pr.StatusCheckRollup.Nodes) == 0 { return } - commit := pr.Commits.Nodes[0].Commit + commit := pr.StatusCheckRollup.Nodes[0].Commit for _, c := range commit.StatusCheckRollup.Contexts.Nodes { state := c.State // StatusContext if state == "" { diff --git a/api/queries_pr_review.go b/api/queries_pr_review.go index 030472d75..53eac3e78 100644 --- a/api/queries_pr_review.go +++ b/api/queries_pr_review.go @@ -22,8 +22,11 @@ type PullRequestReviewInput struct { } type PullRequestReviews struct { - Nodes []PullRequestReview - PageInfo PageInfo + Nodes []PullRequestReview + PageInfo struct { + HasNextPage bool + EndCursor string + } TotalCount int } @@ -66,42 +69,6 @@ func AddReview(client *Client, repo ghrepo.Interface, pr *PullRequest, input *Pu return gql.MutateNamed(context.Background(), "PullRequestReviewAdd", &mutation, variables) } -func ReviewsForPullRequest(client *Client, repo ghrepo.Interface, pr *PullRequest) (*PullRequestReviews, error) { - type response struct { - Repository struct { - PullRequest struct { - Reviews PullRequestReviews `graphql:"reviews(first: 100, after: $endCursor)"` - } `graphql:"pullRequest(number: $number)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - } - - variables := map[string]interface{}{ - "owner": githubv4.String(repo.RepoOwner()), - "repo": githubv4.String(repo.RepoName()), - "number": githubv4.Int(pr.Number), - "endCursor": (*githubv4.String)(nil), - } - - gql := graphQLClient(client.http, repo.RepoHost()) - - var reviews []PullRequestReview - for { - var query response - err := gql.QueryNamed(context.Background(), "ReviewsForPullRequest", &query, variables) - if err != nil { - return nil, err - } - - reviews = append(reviews, query.Repository.PullRequest.Reviews.Nodes...) - if !query.Repository.PullRequest.Reviews.PageInfo.HasNextPage { - break - } - variables["endCursor"] = githubv4.String(query.Repository.PullRequest.Reviews.PageInfo.EndCursor) - } - - return &PullRequestReviews{Nodes: reviews, TotalCount: len(reviews)}, nil -} - func (prr PullRequestReview) AuthorLogin() string { return prr.Author.Login } diff --git a/api/query_builder.go b/api/query_builder.go index 25862e179..a97d5eee9 100644 --- a/api/query_builder.go +++ b/api/query_builder.go @@ -1,6 +1,7 @@ package api import ( + "fmt" "strings" ) @@ -18,7 +19,7 @@ func shortenQuery(q string) string { } var issueComments = shortenQuery(` - comments(last: 100) { + comments(first: 100) { nodes { author{login}, authorAssociation, @@ -29,25 +30,25 @@ var issueComments = shortenQuery(` minimizedReason, reactionGroups{content,users{totalCount}} }, + pageInfo{hasNextPage,endCursor}, totalCount } `) var prReviewRequests = shortenQuery(` - reviewRequests(last: 100) { + reviewRequests(first: 100) { nodes { requestedReviewer { __typename, ...on User{login}, ...on Team{name} } - }, - totalCount + } } `) var prReviews = shortenQuery(` - reviews(last: 100) { + reviews(first: 100) { nodes { author{login}, authorAssociation, @@ -56,6 +57,7 @@ var prReviews = shortenQuery(` state, reactionGroups{content,users{totalCount}} } + pageInfo{hasNextPage,endCursor} } `) @@ -69,14 +71,38 @@ var prFiles = shortenQuery(` } `) -var prStatusCheckRollup = shortenQuery(` - commits(last: 1) { - totalCount, +var prCommits = shortenQuery(` + commits(first: 100) { nodes { commit { + authors(first:100) { + nodes { + name, + email, + user{id,login} + } + }, + messageHeadline, + messageBody, oid, + committedDate, + authoredDate + } + } + } +`) + +func StatusCheckRollupGraphQL(after string) string { + var afterClause string + if after != "" { + afterClause = ",after:" + after + } + return fmt.Sprintf(shortenQuery(` + statusCheckRollup: commits(last: 1) { + nodes { + commit { statusCheckRollup { - contexts(last: 100) { + contexts(first:100%s) { nodes { __typename ...on StatusContext { @@ -92,13 +118,14 @@ var prStatusCheckRollup = shortenQuery(` completedAt, detailsUrl } - } + }, + pageInfo{hasNextPage,endCursor} } } } } - } -`) + }`), afterClause) +} var IssueFields = []string{ "assignees", @@ -124,6 +151,7 @@ var PullRequestFields = append(IssueFields, "additions", "baseRefName", "changedFiles", + "commits", "deletions", "files", "headRefName", @@ -178,8 +206,12 @@ func PullRequestGraphQL(fields []string) string { q = append(q, prReviews) case "files": q = append(q, prFiles) + case "commits": + q = append(q, prCommits) + case "commitsCount": // pseudo-field + q = append(q, `commits{totalCount}`) case "statusCheckRollup": - q = append(q, prStatusCheckRollup) + q = append(q, StatusCheckRollupGraphQL("")) default: q = append(q, field) } diff --git a/pkg/cmd/issue/view/fixtures/issueView_previewFullComments.json b/pkg/cmd/issue/view/fixtures/issueView_previewFullComments.json index 9ab620ecd..f45459e24 100644 --- a/pkg/cmd/issue/view/fixtures/issueView_previewFullComments.json +++ b/pkg/cmd/issue/view/fixtures/issueView_previewFullComments.json @@ -1,7 +1,6 @@ { "data": { - "repository": { - "issue": { + "node": { "comments": { "nodes": [ { @@ -315,6 +314,5 @@ "totalCount": 6 } } - } } } diff --git a/pkg/cmd/issue/view/http.go b/pkg/cmd/issue/view/http.go new file mode 100644 index 000000000..c4f87d677 --- /dev/null +++ b/pkg/cmd/issue/view/http.go @@ -0,0 +1,50 @@ +package view + +import ( + "context" + "net/http" + + "github.com/cli/cli/api" + "github.com/cli/cli/internal/ghinstance" + "github.com/cli/cli/internal/ghrepo" + "github.com/shurcooL/githubv4" + "github.com/shurcooL/graphql" +) + +func preloadIssueComments(client *http.Client, repo ghrepo.Interface, issue *api.Issue) error { + type response struct { + Node struct { + Issue struct { + Comments api.Comments `graphql:"comments(first: 100, after: $endCursor)"` + } `graphql:"...on Issue"` + } `graphql:"node(id: $id)"` + } + + variables := map[string]interface{}{ + "id": githubv4.ID(issue.ID), + "endCursor": (*githubv4.String)(nil), + } + if issue.Comments.PageInfo.HasNextPage { + variables["endCursor"] = githubv4.String(issue.Comments.PageInfo.EndCursor) + } else { + issue.Comments.Nodes = issue.Comments.Nodes[0:0] + } + + gql := graphql.NewClient(ghinstance.GraphQLEndpoint(repo.RepoHost()), client) + for { + var query response + err := gql.QueryNamed(context.Background(), "CommentsForIssue", &query, variables) + if err != nil { + return err + } + + issue.Comments.Nodes = append(issue.Comments.Nodes, query.Node.Issue.Comments.Nodes...) + if !query.Node.Issue.Comments.PageInfo.HasNextPage { + break + } + variables["endCursor"] = githubv4.String(query.Node.Issue.Comments.PageInfo.EndCursor) + } + + issue.Comments.PageInfo.HasNextPage = false + return nil +} diff --git a/pkg/cmd/issue/view/view.go b/pkg/cmd/issue/view/view.go index 6888cc791..c24733c13 100644 --- a/pkg/cmd/issue/view/view.go +++ b/pkg/cmd/issue/view/view.go @@ -16,6 +16,7 @@ import ( "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/pkg/markdown" + "github.com/cli/cli/pkg/set" "github.com/cli/cli/utils" "github.com/spf13/cobra" ) @@ -82,9 +83,17 @@ func viewRun(opts *ViewOptions) error { if err != nil { return err } - apiClient := api.NewClientFromHTTP(httpClient) - issue, repo, err := issueShared.IssueFromArg(apiClient, opts.BaseRepo, opts.SelectorArg) + loadComments := opts.Comments + if !loadComments && opts.Exporter != nil { + fields := set.NewStringSet() + fields.AddValues(opts.Exporter.Fields()) + loadComments = fields.Contains("comments") + } + + opts.IO.StartProgressIndicator() + issue, err := findIssue(httpClient, opts.BaseRepo, opts.SelectorArg, loadComments) + opts.IO.StopProgressIndicator() if err != nil { return err } @@ -97,21 +106,9 @@ func viewRun(opts *ViewOptions) error { return opts.Browser.Browse(openURL) } - if opts.Comments { - opts.IO.StartProgressIndicator() - comments, err := api.CommentsForIssue(apiClient, repo, issue) - opts.IO.StopProgressIndicator() - if err != nil { - return err - } - issue.Comments = *comments - } - opts.IO.DetectTerminalTheme() - - err = opts.IO.StartPager() - if err != nil { - return err + if err := opts.IO.StartPager(); err != nil { + fmt.Fprintf(opts.IO.ErrOut, "error starting pager: %v", err) } defer opts.IO.StopPager() @@ -131,6 +128,19 @@ func viewRun(opts *ViewOptions) error { return printRawIssuePreview(opts.IO.Out, issue) } +func findIssue(client *http.Client, baseRepoFn func() (ghrepo.Interface, error), selector string, loadComments bool) (*api.Issue, error) { + apiClient := api.NewClientFromHTTP(client) + issue, repo, err := issueShared.IssueFromArg(apiClient, baseRepoFn, selector) + if err != nil { + return issue, err + } + + if loadComments { + err = preloadIssueComments(client, repo, issue) + } + return issue, err +} + func printRawIssuePreview(out io.Writer, issue *api.Issue) error { assignees := issueAssigneeList(*issue) labels := shared.IssueLabelList(*issue) diff --git a/pkg/cmd/pr/checks/checks.go b/pkg/cmd/pr/checks/checks.go index e97d1e46f..b21631820 100644 --- a/pkg/cmd/pr/checks/checks.go +++ b/pkg/cmd/pr/checks/checks.go @@ -92,11 +92,11 @@ func checksRun(opts *ChecksOptions) error { return opts.Browser.Browse(openURL) } - if len(pr.Commits.Nodes) == 0 { + if len(pr.StatusCheckRollup.Nodes) == 0 { return fmt.Errorf("no commit found on the pull request") } - rollup := pr.Commits.Nodes[0].Commit.StatusCheckRollup.Contexts.Nodes + rollup := pr.StatusCheckRollup.Nodes[0].Commit.StatusCheckRollup.Contexts.Nodes if len(rollup) == 0 { return fmt.Errorf("no checks reported on the '%s' branch", pr.BaseRefName) } @@ -118,7 +118,7 @@ func checksRun(opts *ChecksOptions) error { outputs := []output{} - for _, c := range pr.Commits.Nodes[0].Commit.StatusCheckRollup.Contexts.Nodes { + for _, c := range pr.StatusCheckRollup.Nodes[0].Commit.StatusCheckRollup.Contexts.Nodes { mark := "✓" bucket := "pass" state := c.State diff --git a/pkg/cmd/pr/checks/checks_test.go b/pkg/cmd/pr/checks/checks_test.go index 6cedd6d32..0a189bd4d 100644 --- a/pkg/cmd/pr/checks/checks_test.go +++ b/pkg/cmd/pr/checks/checks_test.go @@ -83,7 +83,7 @@ func Test_checksRun(t *testing.T) { }, { name: "no checks", - prJSON: `{ "number": 123, "commits": { "nodes": [{"commit": {"oid": "abc"}}]}, "baseRefName": "master" }`, + prJSON: `{ "number": 123, "statusCheckRollup": { "nodes": [{"commit": {"oid": "abc"}}]}, "baseRefName": "master" }`, wantOut: "", wantErr: "no checks reported on the 'master' branch", }, @@ -114,7 +114,7 @@ func Test_checksRun(t *testing.T) { { name: "no checks", nontty: true, - prJSON: `{ "number": 123, "commits": { "nodes": [{"commit": {"oid": "abc"}}]}, "baseRefName": "master" }`, + prJSON: `{ "number": 123, "statusCheckRollup": { "nodes": [{"commit": {"oid": "abc"}}]}, "baseRefName": "master" }`, wantOut: "", wantErr: "no checks reported on the 'master' branch", }, diff --git a/pkg/cmd/pr/checks/fixtures/allPassing.json b/pkg/cmd/pr/checks/fixtures/allPassing.json index 5116b5916..8d1f33510 100644 --- a/pkg/cmd/pr/checks/fixtures/allPassing.json +++ b/pkg/cmd/pr/checks/fixtures/allPassing.json @@ -1,6 +1,6 @@ { "number": 123, - "commits": { + "statusCheckRollup": { "nodes": [ { "commit": { diff --git a/pkg/cmd/pr/checks/fixtures/someFailing.json b/pkg/cmd/pr/checks/fixtures/someFailing.json index 887e22ab3..f407cd4b5 100644 --- a/pkg/cmd/pr/checks/fixtures/someFailing.json +++ b/pkg/cmd/pr/checks/fixtures/someFailing.json @@ -1,6 +1,6 @@ { "number": 123, - "commits": { + "statusCheckRollup": { "nodes": [ { "commit": { diff --git a/pkg/cmd/pr/checks/fixtures/somePending.json b/pkg/cmd/pr/checks/fixtures/somePending.json index 5214a7930..2d558f39e 100644 --- a/pkg/cmd/pr/checks/fixtures/somePending.json +++ b/pkg/cmd/pr/checks/fixtures/somePending.json @@ -1,6 +1,6 @@ { "number": 123, - "commits": { + "statusCheckRollup": { "nodes": [ { "commit": { diff --git a/pkg/cmd/pr/checks/fixtures/withStatuses.json b/pkg/cmd/pr/checks/fixtures/withStatuses.json index 2b4a808c7..ddc7374ba 100644 --- a/pkg/cmd/pr/checks/fixtures/withStatuses.json +++ b/pkg/cmd/pr/checks/fixtures/withStatuses.json @@ -1,6 +1,6 @@ { "number": 123, - "commits": { + "statusCheckRollup": { "nodes": [ { "commit": { diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index 298d59b7f..f25732a7b 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -184,7 +184,7 @@ func mergeRun(opts *MergeOptions) error { if opts.SelectorArg == "" && len(pr.Commits.Nodes) > 0 { if localBranchLastCommit, err := git.LastCommit(); err == nil { - if localBranchLastCommit.Sha != pr.Commits.Nodes[0].Commit.Oid { + if localBranchLastCommit.Sha != pr.Commits.Nodes[len(pr.Commits.Nodes)-1].Commit.OID { fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d (%s) has diverged from local branch\n", cs.Yellow("!"), pr.Number, pr.Title) } diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index 4abb88aca..abd0d9f55 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -205,7 +205,7 @@ func baseRepo(owner, repo, branch string) ghrepo.Interface { func stubCommit(pr *api.PullRequest, oid string) { pr.Commits.Nodes = append(pr.Commits.Nodes, api.PullRequestCommit{ - Commit: api.PullRequestCommitCommit{Oid: oid}, + Commit: api.PullRequestCommitCommit{OID: oid}, }) } diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index 52b2a436a..1ad253be2 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -1,6 +1,7 @@ package shared import ( + "context" "errors" "fmt" "net/http" @@ -11,11 +12,15 @@ import ( "strings" "github.com/cli/cli/api" - "github.com/cli/cli/context" + remotes "github.com/cli/cli/context" "github.com/cli/cli/git" + "github.com/cli/cli/internal/ghinstance" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/set" + "github.com/shurcooL/githubv4" + "github.com/shurcooL/graphql" + "golang.org/x/sync/errgroup" ) type PRFinder interface { @@ -30,7 +35,7 @@ type progressIndicator interface { type finder struct { baseRepoFn func() (ghrepo.Interface, error) branchFn func() (string, error) - remotesFn func() (context.Remotes, error) + remotesFn func() (remotes.Remotes, error) httpClient func() (*http.Client, error) branchConfig func(string) git.BranchConfig progress progressIndicator @@ -120,20 +125,43 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err defer f.progress.StopProgressIndicator() } + fields := set.NewStringSet() + fields.AddValues(opts.Fields) + numberFieldOnly := fields.Len() == 1 && fields.Contains("number") + fields.Add("id") // for additional preload queries below + + var pr *api.PullRequest if f.prNumber > 0 { - if len(opts.Fields) == 1 && opts.Fields[0] == "number" { + if numberFieldOnly { // avoid hitting the API if we already have all the information return &api.PullRequest{Number: f.prNumber}, f.repo, nil } - pr, err := findByNumber(httpClient, f.repo, f.prNumber, opts.Fields) + pr, err = findByNumber(httpClient, f.repo, f.prNumber, fields.ToSlice()) + } else { + pr, err = findForBranch(httpClient, f.repo, opts.BaseBranch, f.branchName, opts.States, fields.ToSlice()) + } + if err != nil { return pr, f.repo, err } - pr, err := findForBranch(httpClient, f.repo, opts.BaseBranch, f.branchName, opts.States, opts.Fields) + g, _ := errgroup.WithContext(context.Background()) + if fields.Contains("reviews") { + g.Go(func() error { + return preloadPrReviews(httpClient, f.repo, pr) + }) + } + if fields.Contains("comments") { + g.Go(func() error { + return preloadPrComments(httpClient, f.repo, pr) + }) + } + if fields.Contains("statusCheckRollup") { + g.Go(func() error { + return preloadPrChecks(httpClient, f.repo, pr) + }) + } - // TODO: preload view: api.ReviewsForPullRequest, api.CommentsForPullRequest - // TODO: preload checks: get all checks - return pr, f.repo, err + return pr, f.repo, g.Wait() } var pullURLRE = regexp.MustCompile(`^/([^/]+)/([^/]+)/pull/(\d+)`) @@ -291,6 +319,139 @@ func findForBranch(httpClient *http.Client, repo ghrepo.Interface, baseBranch, h return nil, &NotFoundError{fmt.Errorf("no pull requests found for branch %q", headBranch)} } +func preloadPrReviews(httpClient *http.Client, repo ghrepo.Interface, pr *api.PullRequest) error { + if !pr.Reviews.PageInfo.HasNextPage { + return nil + } + + type response struct { + Node struct { + PullRequest struct { + Reviews api.PullRequestReviews `graphql:"reviews(first: 100, after: $endCursor)"` + } `graphql:"...on PullRequest"` + } `graphql:"node(id: $id)"` + } + + variables := map[string]interface{}{ + "id": githubv4.ID(pr.ID), + "endCursor": githubv4.String(pr.Reviews.PageInfo.EndCursor), + } + + gql := graphql.NewClient(ghinstance.GraphQLEndpoint(repo.RepoHost()), httpClient) + + for { + var query response + err := gql.QueryNamed(context.Background(), "ReviewsForPullRequest", &query, variables) + if err != nil { + return err + } + + pr.Reviews.Nodes = append(pr.Reviews.Nodes, query.Node.PullRequest.Reviews.Nodes...) + pr.Reviews.TotalCount = len(pr.Reviews.Nodes) + + if !query.Node.PullRequest.Reviews.PageInfo.HasNextPage { + break + } + variables["endCursor"] = githubv4.String(query.Node.PullRequest.Reviews.PageInfo.EndCursor) + } + + pr.Reviews.PageInfo.HasNextPage = false + return nil +} + +func preloadPrComments(client *http.Client, repo ghrepo.Interface, pr *api.PullRequest) error { + if !pr.Comments.PageInfo.HasNextPage { + return nil + } + + type response struct { + Node struct { + PullRequest struct { + Comments api.Comments `graphql:"comments(first: 100, after: $endCursor)"` + } `graphql:"...on PullRequest"` + } `graphql:"node(id: $id)"` + } + + variables := map[string]interface{}{ + "id": githubv4.ID(pr.ID), + "endCursor": githubv4.String(pr.Comments.PageInfo.EndCursor), + } + + gql := graphql.NewClient(ghinstance.GraphQLEndpoint(repo.RepoHost()), client) + + for { + var query response + err := gql.QueryNamed(context.Background(), "CommentsForPullRequest", &query, variables) + if err != nil { + return err + } + + pr.Comments.Nodes = append(pr.Comments.Nodes, query.Node.PullRequest.Comments.Nodes...) + pr.Comments.TotalCount = len(pr.Comments.Nodes) + + if !query.Node.PullRequest.Comments.PageInfo.HasNextPage { + break + } + variables["endCursor"] = githubv4.String(query.Node.PullRequest.Comments.PageInfo.EndCursor) + } + + pr.Comments.PageInfo.HasNextPage = false + return nil +} + +func preloadPrChecks(client *http.Client, repo ghrepo.Interface, pr *api.PullRequest) error { + if len(pr.StatusCheckRollup.Nodes) == 0 { + return nil + } + statusCheckRollup := &pr.StatusCheckRollup.Nodes[0].Commit.StatusCheckRollup.Contexts + if !statusCheckRollup.PageInfo.HasNextPage { + return nil + } + + endCursor := statusCheckRollup.PageInfo.EndCursor + + type response struct { + Node *api.PullRequest + } + + query := fmt.Sprintf(` + query PullRequestStatusChecks($id: ID!, $endCursor: String!) { + node(id: $id) { + ...on PullRequest { + %s + } + } + }`, api.StatusCheckRollupGraphQL("$endCursor")) + + variables := map[string]interface{}{ + "id": pr.ID, + } + + apiClient := api.NewClientFromHTTP(client) + for { + variables["endCursor"] = endCursor + var resp response + err := apiClient.GraphQL(repo.RepoHost(), query, variables, &resp) + if err != nil { + return err + } + + result := resp.Node.StatusCheckRollup.Nodes[0].Commit.StatusCheckRollup.Contexts + statusCheckRollup.Nodes = append( + statusCheckRollup.Nodes, + result.Nodes..., + ) + + if !result.PageInfo.HasNextPage { + break + } + endCursor = result.PageInfo.EndCursor + } + + statusCheckRollup.PageInfo.HasNextPage = false + return nil +} + type NotFoundError struct { error } diff --git a/pkg/cmd/pr/status/fixtures/prStatusChecks.json b/pkg/cmd/pr/status/fixtures/prStatusChecks.json index 55035ae36..dd1605b1d 100644 --- a/pkg/cmd/pr/status/fixtures/prStatusChecks.json +++ b/pkg/cmd/pr/status/fixtures/prStatusChecks.json @@ -17,7 +17,7 @@ "url": "https://github.com/cli/cli/pull/8", "headRefName": "strawberries", "reviewDecision": "CHANGES_REQUESTED", - "commits": { + "statusCheckRollup": { "nodes": [ { "commit": { @@ -44,7 +44,7 @@ "url": "https://github.com/cli/cli/pull/7", "headRefName": "banananana", "reviewDecision": "APPROVED", - "commits": { + "statusCheckRollup": { "nodes": [ { "commit": { @@ -72,7 +72,7 @@ "url": "https://github.com/cli/cli/pull/6", "headRefName": "avo", "reviewDecision": "REVIEW_REQUIRED", - "commits": { + "statusCheckRollup": { "nodes": [ { "commit": { diff --git a/pkg/cmd/pr/view/view.go b/pkg/cmd/pr/view/view.go index fd924387c..951ce7953 100644 --- a/pkg/cmd/pr/view/view.go +++ b/pkg/cmd/pr/view/view.go @@ -78,11 +78,10 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman var defaultFields = []string{ "url", "number", "title", "state", "body", "author", - "isDraft", "maintainerCanModify", "mergeable", "additions", "deletions", + "isDraft", "maintainerCanModify", "mergeable", "additions", "deletions", "commitsCount", "baseRefName", "headRefName", "headRepositoryOwner", "headRepository", "isCrossRepository", "reviewRequests", "reviews", "assignees", "labels", "projectCards", "milestone", - "comments", // TODO: fetch only 1 last comment unless `opts.Comments` was set - "reactionGroups", + "comments", "reactionGroups", } func viewRun(opts *ViewOptions) error { From 42155c7d2de352a667c99a64c9091c4b177eee38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 18 May 2021 18:19:28 +0200 Subject: [PATCH 5/6] Export more IDs in issue/pr JSON payload --- api/export_pr.go | 14 +------------- api/export_pr_test.go | 10 ++++++++-- api/queries_issue.go | 22 +++++++++++++++------- api/queries_pr.go | 7 ++++--- api/query_builder.go | 16 ++++++++-------- api/query_builder_test.go | 2 +- pkg/cmd/issue/edit/edit.go | 4 +++- pkg/cmd/issue/view/view.go | 8 ++++++-- pkg/cmd/pr/checkout/checkout_test.go | 2 +- pkg/cmd/pr/close/close_test.go | 2 +- pkg/cmd/pr/edit/edit.go | 4 +++- pkg/cmd/pr/view/view.go | 8 ++++++-- 12 files changed, 57 insertions(+), 42 deletions(-) diff --git a/api/export_pr.go b/api/export_pr.go index 96411aead..4bd0aabc7 100644 --- a/api/export_pr.go +++ b/api/export_pr.go @@ -11,12 +11,6 @@ func (issue *Issue) ExportData(fields []string) *map[string]interface{} { for _, f := range fields { switch f { - case "milestone": - if issue.Milestone.Title != "" { - data[f] = map[string]string{"title": issue.Milestone.Title} - } else { - data[f] = nil - } case "comments": data[f] = issue.Comments.Nodes case "assignees": @@ -41,13 +35,7 @@ func (pr *PullRequest) ExportData(fields []string) *map[string]interface{} { for _, f := range fields { switch f { case "headRepository": - data[f] = map[string]string{"name": pr.HeadRepository.Name} - case "milestone": - if pr.Milestone.Title != "" { - data[f] = map[string]string{"title": pr.Milestone.Title} - } else { - data[f] = nil - } + data[f] = pr.HeadRepository case "statusCheckRollup": if n := pr.StatusCheckRollup.Nodes; len(n) > 0 { data[f] = n[0].Commit.StatusCheckRollup.Contexts.Nodes diff --git a/api/export_pr_test.go b/api/export_pr_test.go index 9243e6a8d..dde730884 100644 --- a/api/export_pr_test.go +++ b/api/export_pr_test.go @@ -40,7 +40,10 @@ func TestIssue_ExportData(t *testing.T) { outputJSON: heredoc.Doc(` { "milestone": { - "title": "The next big thing" + "number": 0, + "title": "The next big thing", + "description": "", + "dueOn": null }, "number": 2345 } @@ -119,7 +122,10 @@ func TestPullRequest_ExportData(t *testing.T) { outputJSON: heredoc.Doc(` { "milestone": { - "title": "The next big thing" + "number": 0, + "title": "The next big thing", + "description": "", + "dueOn": null }, "number": 2345 } diff --git a/api/queries_issue.go b/api/queries_issue.go index 18f2c83c7..1c4f122ec 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -36,14 +36,12 @@ type Issue struct { Assignees Assignees Labels Labels ProjectCards ProjectCards - Milestone Milestone + Milestone *Milestone ReactionGroups ReactionGroups } type Assignees struct { - Nodes []struct { - Login string `json:"login"` - } + Nodes []GitHubUser TotalCount int } @@ -56,9 +54,7 @@ func (a Assignees) Logins() []string { } type Labels struct { - Nodes []struct { - Name string `json:"name"` - } + Nodes []IssueLabel TotalCount int } @@ -102,10 +98,14 @@ type IssuesDisabledError struct { } type Owner struct { + ID string `json:"id,omitempty"` + Name string `json:"name,omitempty"` Login string `json:"login"` } type Author struct { + ID string `json:"id,omitempty"` + Name string `json:"name,omitempty"` Login string `json:"login"` } @@ -273,13 +273,18 @@ func IssueByNumber(client *Client, repo ghrepo.Interface, number int) (*Issue, e createdAt assignees(first: 100) { nodes { + id + name login } totalCount } labels(first: 100) { nodes { + id name + description + color } totalCount } @@ -295,7 +300,10 @@ func IssueByNumber(client *Client, repo ghrepo.Interface, number int) (*Issue, e totalCount } milestone { + number title + description + dueOn } reactionGroups { content diff --git a/api/queries_pr.go b/api/queries_pr.go index eefbcdd49..8a1d0e421 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -58,7 +58,7 @@ type PullRequest struct { Author Author MergedBy *Author HeadRepositoryOwner Owner - HeadRepository PRRepository + HeadRepository *PRRepository IsCrossRepository bool IsDraft bool MaintainerCanModify bool @@ -105,7 +105,7 @@ type PullRequest struct { Assignees Assignees Labels Labels ProjectCards ProjectCards - Milestone Milestone + Milestone *Milestone Comments Comments ReactionGroups ReactionGroups Reviews PullRequestReviews @@ -113,7 +113,8 @@ type PullRequest struct { } type PRRepository struct { - Name string + ID string `json:"id"` + Name string `json:"name"` } // Commit loads just the commit SHA and nothing else diff --git a/api/query_builder.go b/api/query_builder.go index a97d5eee9..04d681d60 100644 --- a/api/query_builder.go +++ b/api/query_builder.go @@ -21,7 +21,7 @@ func shortenQuery(q string) string { var issueComments = shortenQuery(` comments(first: 100) { nodes { - author{login}, + author{login,...on User{id,name}}, authorAssociation, body, createdAt, @@ -177,21 +177,21 @@ func PullRequestGraphQL(fields []string) string { for _, field := range fields { switch field { case "author": - q = append(q, `author{login}`) + q = append(q, `author{login,...on User{id,name}}`) case "mergedBy": - q = append(q, `mergedBy{login}`) + q = append(q, `mergedBy{login,,...on User{id,name}}`) case "headRepositoryOwner": - q = append(q, `headRepositoryOwner{login}`) + q = append(q, `headRepositoryOwner{id,login,,...on User{name}}`) case "headRepository": - q = append(q, `headRepository{name}`) + q = append(q, `headRepository{id,name}`) case "assignees": - q = append(q, `assignees(first:100){nodes{login},totalCount}`) + q = append(q, `assignees(first:100){nodes{id,login,name},totalCount}`) case "labels": - q = append(q, `labels(first:100){nodes{name},totalCount}`) + q = append(q, `labels(first:100){nodes{id,name,description,color},totalCount}`) case "projectCards": q = append(q, `projectCards(first:100){nodes{project{name}column{name}},totalCount}`) case "milestone": - q = append(q, `milestone{title}`) + q = append(q, `milestone{number,title,description,dueOn}`) case "reactionGroups": q = append(q, `reactionGroups{content,users{totalCount}}`) case "mergeCommit": diff --git a/api/query_builder_test.go b/api/query_builder_test.go index e8d48a10e..3c510d6da 100644 --- a/api/query_builder_test.go +++ b/api/query_builder_test.go @@ -21,7 +21,7 @@ func TestPullRequestGraphQL(t *testing.T) { { name: "fields with nested structures", fields: []string{"author", "assignees"}, - want: "author{login},assignees(first:100){nodes{login},totalCount}", + want: "author{login,...on User{id,name}},assignees(first:100){nodes{id,login,name},totalCount}", }, { name: "compressed query", diff --git a/pkg/cmd/issue/edit/edit.go b/pkg/cmd/issue/edit/edit.go index b3f85a36b..5e6f0583e 100644 --- a/pkg/cmd/issue/edit/edit.go +++ b/pkg/cmd/issue/edit/edit.go @@ -149,7 +149,9 @@ func editRun(opts *EditOptions) error { editable.Assignees.Default = issue.Assignees.Logins() editable.Labels.Default = issue.Labels.Names() editable.Projects.Default = issue.ProjectCards.ProjectNames() - editable.Milestone.Default = issue.Milestone.Title + if issue.Milestone != nil { + editable.Milestone.Default = issue.Milestone.Title + } if opts.Interactive { err = opts.FieldsToEditSurvey(&editable) diff --git a/pkg/cmd/issue/view/view.go b/pkg/cmd/issue/view/view.go index c24733c13..b6b42ff9f 100644 --- a/pkg/cmd/issue/view/view.go +++ b/pkg/cmd/issue/view/view.go @@ -155,7 +155,11 @@ func printRawIssuePreview(out io.Writer, issue *api.Issue) error { fmt.Fprintf(out, "comments:\t%d\n", issue.Comments.TotalCount) fmt.Fprintf(out, "assignees:\t%s\n", assignees) fmt.Fprintf(out, "projects:\t%s\n", projects) - fmt.Fprintf(out, "milestone:\t%s\n", issue.Milestone.Title) + var milestoneTitle string + if issue.Milestone != nil { + milestoneTitle = issue.Milestone.Title + } + fmt.Fprintf(out, "milestone:\t%s\n", milestoneTitle) fmt.Fprintln(out, "--") fmt.Fprintln(out, issue.Body) return nil @@ -196,7 +200,7 @@ func printHumanIssuePreview(opts *ViewOptions, issue *api.Issue) error { fmt.Fprint(out, cs.Bold("Projects: ")) fmt.Fprintln(out, projects) } - if issue.Milestone.Title != "" { + if issue.Milestone != nil { fmt.Fprint(out, cs.Bold("Milestone: ")) fmt.Fprintln(out, issue.Milestone.Title) } diff --git a/pkg/cmd/pr/checkout/checkout_test.go b/pkg/cmd/pr/checkout/checkout_test.go index 583602628..932db4778 100644 --- a/pkg/cmd/pr/checkout/checkout_test.go +++ b/pkg/cmd/pr/checkout/checkout_test.go @@ -53,7 +53,7 @@ func stubPR(repo, prHead string) (ghrepo.Interface, *api.PullRequest) { Number: 123, HeadRefName: headRefName, HeadRepositoryOwner: api.Owner{Login: headRepo.RepoOwner()}, - HeadRepository: api.PRRepository{Name: headRepo.RepoName()}, + HeadRepository: &api.PRRepository{Name: headRepo.RepoName()}, IsCrossRepository: !ghrepo.IsSame(baseRepo, headRepo), MaintainerCanModify: false, } diff --git a/pkg/cmd/pr/close/close_test.go b/pkg/cmd/pr/close/close_test.go index 4a02dc13d..c94fe83f3 100644 --- a/pkg/cmd/pr/close/close_test.go +++ b/pkg/cmd/pr/close/close_test.go @@ -53,7 +53,7 @@ func stubPR(repo, prHead string) (ghrepo.Interface, *api.PullRequest) { State: "OPEN", HeadRefName: headRefName, HeadRepositoryOwner: api.Owner{Login: headRepo.RepoOwner()}, - HeadRepository: api.PRRepository{Name: headRepo.RepoName()}, + HeadRepository: &api.PRRepository{Name: headRepo.RepoName()}, IsCrossRepository: !ghrepo.IsSame(baseRepo, headRepo), } } diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index ea1b73562..13481bcba 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -167,7 +167,9 @@ func editRun(opts *EditOptions) error { editable.Assignees.Default = pr.Assignees.Logins() editable.Labels.Default = pr.Labels.Names() editable.Projects.Default = pr.ProjectCards.ProjectNames() - editable.Milestone.Default = pr.Milestone.Title + if pr.Milestone != nil { + editable.Milestone.Default = pr.Milestone.Title + } if opts.Interactive { err = opts.Surveyor.FieldsToEdit(&editable) diff --git a/pkg/cmd/pr/view/view.go b/pkg/cmd/pr/view/view.go index 951ce7953..91087d74b 100644 --- a/pkg/cmd/pr/view/view.go +++ b/pkg/cmd/pr/view/view.go @@ -149,7 +149,11 @@ func printRawPrPreview(io *iostreams.IOStreams, pr *api.PullRequest) error { 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) + var milestoneTitle string + if pr.Milestone != nil { + milestoneTitle = pr.Milestone.Title + } + fmt.Fprintf(out, "milestone:\t%s\n", milestoneTitle) fmt.Fprintf(out, "number:\t%d\n", pr.Number) fmt.Fprintf(out, "url:\t%s\n", pr.URL) fmt.Fprintf(out, "additions:\t%s\n", cs.Green(strconv.Itoa(pr.Additions))) @@ -201,7 +205,7 @@ func printHumanPrPreview(opts *ViewOptions, pr *api.PullRequest) error { fmt.Fprint(out, cs.Bold("Projects: ")) fmt.Fprintln(out, projects) } - if pr.Milestone.Title != "" { + if pr.Milestone != nil { fmt.Fprint(out, cs.Bold("Milestone: ")) fmt.Fprintln(out, pr.Milestone.Title) } From 1440fd81a1e76fa6a32b1c5cc6e97affbed420e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 18 May 2021 18:35:34 +0200 Subject: [PATCH 6/6] Fix broken GraphQL queries due to editing Author struct --- api/queries_issue.go | 5 +++-- api/query_builder.go | 8 ++++---- api/query_builder_test.go | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/api/queries_issue.go b/api/queries_issue.go index 1c4f122ec..c67ad3bcc 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -104,8 +104,9 @@ type Owner struct { } type Author struct { - ID string `json:"id,omitempty"` - Name string `json:"name,omitempty"` + // adding these breaks generated GraphQL requests + //ID string `json:"id,omitempty"` + //Name string `json:"name,omitempty"` Login string `json:"login"` } diff --git a/api/query_builder.go b/api/query_builder.go index 04d681d60..3bdbb8c9b 100644 --- a/api/query_builder.go +++ b/api/query_builder.go @@ -21,7 +21,7 @@ func shortenQuery(q string) string { var issueComments = shortenQuery(` comments(first: 100) { nodes { - author{login,...on User{id,name}}, + author{login}, authorAssociation, body, createdAt, @@ -177,11 +177,11 @@ func PullRequestGraphQL(fields []string) string { for _, field := range fields { switch field { case "author": - q = append(q, `author{login,...on User{id,name}}`) + q = append(q, `author{login}`) case "mergedBy": - q = append(q, `mergedBy{login,,...on User{id,name}}`) + q = append(q, `mergedBy{login}`) case "headRepositoryOwner": - q = append(q, `headRepositoryOwner{id,login,,...on User{name}}`) + q = append(q, `headRepositoryOwner{id,login,...on User{name}}`) case "headRepository": q = append(q, `headRepository{id,name}`) case "assignees": diff --git a/api/query_builder_test.go b/api/query_builder_test.go index 3c510d6da..7806f2d05 100644 --- a/api/query_builder_test.go +++ b/api/query_builder_test.go @@ -21,7 +21,7 @@ func TestPullRequestGraphQL(t *testing.T) { { name: "fields with nested structures", fields: []string{"author", "assignees"}, - want: "author{login,...on User{id,name}},assignees(first:100){nodes{id,login,name},totalCount}", + want: "author{login},assignees(first:100){nodes{id,login,name},totalCount}", }, { name: "compressed query",