STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Merge pull request #3547 from cli/pr-lookup-refactor

Browse source 
Eliminate API overfetching in `pr` commands
This commit is contained in:
Mislav Marohnić 2021-05-18 18:55:00 +02:00 • committed by GitHub
commit f30afce5da
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
55 changed files with 2089 additions and 2909 deletions
Download patch file Download diff file Expand all files Collapse all files

39
api/export_pr.go
View file

@ -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,19 +35,36 @@ 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.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":

12
api/export_pr_test.go
View file

@ -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
}
@ -129,7 +135,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",

2
api/pull_request_test.go
View file

@ -10,7 +10,7 @@ import (
func TestPullRequest_ChecksStatus(t *testing.T) {
pr := PullRequest{}
payload := `
{ "commits": { "nodes": [{ "commit": {
{ "statusCheckRollup": { "nodes": [{ "commit": {
"statusCheckRollup": {
"contexts": {
"nodes": [

101
api/queries_comments.go
View file

@ -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
@ -135,24 +60,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
}

27
api/queries_issue.go
View file

@ -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
}
@ -101,7 +97,16 @@ type IssuesDisabledError struct {
error
}
type Owner struct {
ID string `json:"id,omitempty"`
Name string `json:"name,omitempty"`
Login string `json:"login"`
}
type Author struct {
// adding these breaks generated GraphQL requests
//ID string `json:"id,omitempty"`
//Name string `json:"name,omitempty"`
Login string `json:"login"`
}
@ -269,13 +274,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
}
@ -291,7 +301,10 @@ func IssueByNumber(client *Client, repo ghrepo.Interface, number int) (*Issue, e
totalCount
}
milestone {
number
title
description
dueOn
}
reactionGroups {
content

332
api/queries_pr.go
View file

@ -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,12 +57,8 @@ type PullRequest struct {
Author Author
MergedBy *Author
HeadRepositoryOwner struct {
Login string `json:"login"`
}
HeadRepository struct {
Name string
}
HeadRepositoryOwner Owner
HeadRepository *PRRepository
IsCrossRepository bool
IsDraft bool
MaintainerCanModify bool
@ -77,9 +73,11 @@ type PullRequest struct {
Commits struct {
TotalCount int
Nodes []struct {
Nodes []PullRequestCommit
}
StatusCheckRollup struct {
Nodes []struct {
Commit struct {
Oid string
StatusCheckRollup struct {
Contexts struct {
Nodes []struct {
@ -94,25 +92,56 @@ type PullRequest struct {
DetailsURL string `json:"detailsUrl"`
TargetURL string `json:"targetUrl,omitempty"`
}
PageInfo struct {
HasNextPage bool
EndCursor string
}
}
}
}
}
}
Assignees Assignees
Labels Labels
ProjectCards ProjectCards
Milestone Milestone
Milestone *Milestone
Comments Comments
ReactionGroups ReactionGroups
Reviews PullRequestReviews
ReviewRequests ReviewRequests
}
type PRRepository struct {
ID string `json:"id"`
Name string `json:"name"`
}
// Commit loads just the commit SHA and nothing else
type Commit struct {
OID string `json:"oid"`
}
type PullRequestCommit struct {
Commit PullRequestCommitCommit
}
// PullRequestCommitCommit contains full information about a commit
type PullRequestCommitCommit struct {
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 {
Path string `json:"path"`
Additions int `json:"additions"`
@ -127,7 +156,6 @@ type ReviewRequests struct {
Name string `json:"name"`
}
}
TotalCount int
}
func (r ReviewRequests) Logins() []string {
@ -138,14 +166,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)
@ -192,10 +212,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 == "" {
@ -247,7 +267,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 +580,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 := `

43
api/queries_pr_review.go
View file

@ -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
}

29
api/queries_pr_test.go
View file

@ -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")
}
}

68
api/query_builder.go
View file

@ -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",
@ -153,17 +181,17 @@ func PullRequestGraphQL(fields []string) string {
case "mergedBy":
q = append(q, `mergedBy{login}`)
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":
@ -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)
}

2
api/query_builder_test.go
View file

@ -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},assignees(first:100){nodes{id,login,name},totalCount}",
},
{
name: "compressed query",

9
api/reaction_groups.go
View file

@ -57,12 +57,3 @@ var reactionEmoji = map[string]string{
"ROCKET": "\U0001f680",
"EYES": "\U0001f440",
}
func reactionGroupsFragment() string {
return `reactionGroups {
content
users {
totalCount
}
}`
}

4
pkg/cmd/issue/edit/edit.go
View file

@ -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)

4
pkg/cmd/issue/view/fixtures/issueView_previewFullComments.json
View file

@ -1,7 +1,6 @@
{
"data": {
"repository": {
"issue": {
"node": {
"comments": {
"nodes": [
{
@ -315,6 +314,5 @@
"totalCount": 6
}
}
}
}
}

50
pkg/cmd/issue/view/http.go Normal file
View file

@ -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
}

50
pkg/cmd/issue/view/view.go
View file

@ -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)
@ -145,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
@ -186,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)
}

33
pkg/cmd/pr/checkout/checkout.go
View file

@ -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,11 @@ 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,
Fields: []string{"number", "headRefName", "headRepository", "headRepositoryOwner"},
}
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 +83,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 +109,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

367
pkg/cmd/pr/checkout/checkout_test.go
View file

@ -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())
}

60
pkg/cmd/pr/checks/checks.go
View file

@ -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,25 +70,17 @@ func NewCmdChecks(f *cmdutil.Factory, runF func(*ChecksOptions) error) *cobra.Co
}
func checksRun(opts *ChecksOptions) error {
httpClient, err := opts.HttpClient()
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
}
apiClient := api.NewClientFromHTTP(httpClient)
pr, baseRepo, err := shared.PRFromArgs(apiClient, opts.BaseRepo, opts.Branch, opts.Remotes, opts.SelectorArg)
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()
@ -111,6 +92,15 @@ func checksRun(opts *ChecksOptions) error {
return opts.Browser.Browse(openURL)
}
if len(pr.StatusCheckRollup.Nodes) == 0 {
return fmt.Errorf("no commit found on the pull request")
}
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)
}
passing := 0
failing := 0
pending := 0
@ -128,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

102
pkg/cmd/pr/checks/checks_test.go
View file

@ -2,16 +2,20 @@ package checks
import (
"bytes"
"net/http"
"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) {
@ -66,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, "statusCheckRollup": { "nodes": [{"commit": {"oid": "abc"}}]}, "baseRefName": "master" }`,
wantOut: "",
wantErr: "no checks reported on the 'master' branch",
},
@ -124,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, "statusCheckRollup": { "nodes": [{"commit": {"oid": "abc"}}]}, "baseRefName": "master" }`,
wantOut: "",
wantErr: "no checks reported on the 'master' branch",
},
@ -170,28 +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,
BaseRepo: func() (ghrepo.Interface, error) {
return ghrepo.New("OWNER", "REPO"), nil
},
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))
}
opts.HttpClient = func() (*http.Client, error) {
return &http.Client{Transport: reg}, nil
Finder: shared.NewMockFinder("123", response, ghrepo.New("OWNER", "REPO")),
}
err := checksRun(opts)
@ -232,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)
@ -246,21 +220,15 @@ 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",
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)
})
}

9
pkg/cmd/pr/checks/fixtures/allPassing.json
View file

@ -1,6 +1,6 @@
{ "data": { "repository": { "pullRequest": {
{
"number": 123,
"commits": {
"statusCheckRollup": {
"nodes": [
{
"commit": {
@ -37,5 +37,6 @@
}
}
}
]}
} } } }
]
}
}

9
pkg/cmd/pr/checks/fixtures/someFailing.json
View file

@ -1,6 +1,6 @@
{ "data": { "repository": { "pullRequest": {
{
"number": 123,
"commits": {
"statusCheckRollup": {
"nodes": [
{
"commit": {
@ -37,5 +37,6 @@
}
}
}
]}
} } } }
]
}
}

9
pkg/cmd/pr/checks/fixtures/somePending.json
View file

@ -1,6 +1,6 @@
{ "data": { "repository": { "pullRequest": {
{
"number": 123,
"commits": {
"statusCheckRollup": {
"nodes": [
{
"commit": {
@ -37,5 +37,6 @@
}
}
}
]}
} } } }
]
}
}

9
pkg/cmd/pr/checks/fixtures/withStatuses.json
View file

@ -1,6 +1,6 @@
{ "data": { "repository": { "pullRequest": {
{
"number": 123,
"commits": {
"statusCheckRollup": {
"nodes": [
{
"commit": {
@ -34,5 +34,6 @@
}
}
}
]}
} } } }
]
}
}

53
pkg/cmd/pr/close/close.go
View file

@ -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,25 +58,29 @@ 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,
Fields: []string{"state", "number", "title", "isCrossRepository", "headRefName"},
}
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
}
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
}
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)
@ -88,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
@ -113,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)
}
}
@ -125,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 Skipped 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)

195
pkg/cmd/pr/close/close_test.go
View file

@ -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)
! Skipped 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())
}

32
pkg/cmd/pr/comment/comment.go
View file

@ -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
}
}

33
pkg/cmd/pr/comment/comment_test.go
View file

@ -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`),

13
pkg/cmd/pr/create/create.go
View file

@ -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, &notFound) {
return fmt.Errorf("error checking for existing pull request: %w", err)
}

74
pkg/cmd/pr/create/create_test.go
View file

@ -16,6 +16,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"
@ -246,12 +247,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")

26
pkg/cmd/pr/diff/diff.go
View file

@ -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)

92
pkg/cmd/pr/diff/diff_test.go
View file

@ -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)
}

30
pkg/cmd/pr/edit/edit.go
View file

@ -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,11 @@ 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,
Fields: []string{"id", "url", "title", "body", "baseRefName", "reviewRequests", "assignees", "labels", "projectCards", "milestone"},
}
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
}
@ -175,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)
@ -184,6 +178,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()

48
pkg/cmd/pr/edit/edit_test.go
View file

@ -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)
},
@ -450,11 +458,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)
@ -465,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`),
@ -551,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{}

32
pkg/cmd/pr/merge/merge.go
View file

@ -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 {
@ -186,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)
}

494
pkg/cmd/pr/merge/merge_test.go
View file

@ -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,20 @@ 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 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)
@ -208,24 +220,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 +253,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 +291,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 +326,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 +364,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 +391,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 +410,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 +452,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",
}
stubCommit(pr, "COMMITSHA1")
shared.RunCommandFinder(
"",
pr,
baseRepo("OWNER", "REPO", "master"),
)
http.Register(
httpmock.GraphQL(`mutation PullRequestMerge\b`),
httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) {
@ -504,7 +482,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 +496,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",
}
stubCommit(pr, "COMMITSHA1")
shared.RunCommandFinder(
"",
pr,
baseRepo("OWNER", "REPO", "master"),
)
http.Register(
httpmock.GraphQL(`mutation PullRequestMerge\b`),
httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) {
@ -549,7 +523,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 +538,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 +558,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 +574,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 +612,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 +649,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)
@ -707,23 +674,25 @@ 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) {
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 +709,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 +737,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 +759,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 +793,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 +822,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 +863,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 +894,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 +933,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 +951,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 +970,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 +986,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)

35
pkg/cmd/pr/ready/ready.go
View file

@ -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,9 @@ 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
SelectorArg string
}
@ -31,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{
@ -47,8 +39,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,25 +62,29 @@ 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,
Fields: []string{"id", "number", "state", "isDraft"},
}
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
}
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
}
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)

105
pkg/cmd/pr/ready/ready_test.go
View file

@ -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())
}

30
pkg/cmd/pr/reopen/reopen.go
View file

@ -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,27 +50,31 @@ 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,
Fields: []string{"id", "number", "state", "title"},
}
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
}
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
}
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)

125
pkg/cmd/pr/reopen/reopen_test.go
View file

@ -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())
}

28
pkg/cmd/pr/review/review.go
View file

@ -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,11 @@ 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,
Fields: []string{"id", "number"},
}
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 +175,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)

310
pkg/cmd/pr/review/review_test.go
View file

@ -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())
}

502
pkg/cmd/pr/shared/finder.go Normal file
View file

@ -0,0 +1,502 @@
package shared
import (
"context"
"errors"
"fmt"
"net/http"
"net/url"
"regexp"
"sort"
"strconv"
"strings"
"github.com/cli/cli/api"
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 {
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() (remotes.Remotes, error)
httpClient func() (*http.Client, error)
branchConfig func(string) git.BranchConfig
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,
branchConfig: git.ReadBranchConfig,
}
}
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 `<owner>:` 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")
}
if repo, prNumber, err := f.parseURL(opts.Selector); err == nil {
f.prNumber = prNumber
f.repo = repo
}
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 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 {
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()
}
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 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, 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
}
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)
})
}
return pr, f.repo, g.Wait()
}
var pullURLRE = regexp.MustCompile(`^/([^/]+)/([^/]+)/pull/(\d+)`)
func (f *finder) parseURL(prURL string) (ghrepo.Interface, int, error) {
if prURL == "" {
return nil, 0, fmt.Errorf("invalid URL: %q", prURL)
}
u, err := url.Parse(prURL)
if err != nil {
return nil, 0, err
}
if u.Scheme != "https" && u.Scheme != "http" {
return nil, 0, fmt.Errorf("invalid scheme: %s", u.Scheme)
}
m := pullURLRE.FindStringSubmatch(u.Path)
if m == nil {
return nil, 0, fmt.Errorf("not a pull request URL: %s", prURL)
}
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() (string, int, error) {
prHeadRef, err := f.branchFn()
if err != nil {
return "", 0, err
}
branchConfig := f.branchConfig(prHeadRef)
// the branch is configured to merge a special PR head ref
if m := prHeadRE.FindStringSubmatch(branchConfig.MergeRef); m != nil {
prNumber, _ := strconv.Atoi(m[1])
return "", prNumber, 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)
}
}
return prHeadRef, 0, 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)}
}
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
}
func (err *NotFoundError) Unwrap() error {
return err.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,
}
}
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
}

394
pkg/cmd/pr/shared/finder_test.go Normal file
View file

@ -0,0 +1,394 @@
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)
branchConfig func(string) git.BranchConfig
remotesFn func() (context.Remotes, error)
selector string
fields []string
baseBranch string
}
tests := []struct {
name string
args args
httpStub func(*httpmock.Registry)
wantPR int
wantRepo string
wantErr bool
}{
{
name: "number argument",
args: args{
selector: "13",
fields: []string{"id", "number"},
baseRepoFn: func() (ghrepo.Interface, error) {
return ghrepo.FromFullName("OWNER/REPO")
},
},
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",
},
{
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{
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")
},
},
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",
},
{
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) {
r.Register(
httpmock.GraphQL(`query PullRequestByNumber\b`),
httpmock.StringResponse(`{"data":{"repository":{
"pullRequest":{"number":13}
}}}`))
},
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) {
reg := &httpmock.Registry{}
defer reg.Verify(t)
if tt.httpStub != nil {
tt.httpStub(reg)
}
f := finder{
httpClient: func() (*http.Client, error) {
return &http.Client{Transport: reg}, nil
},
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,
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)
}
repoURL := ghrepo.GenerateRepoURL(repo, "")
if repoURL != tt.wantRepo {
t.Errorf("want repo %s, got %s", tt.wantRepo, repoURL)
}
})
}
}

121
pkg/cmd/pr/shared/lookup.go
View file

@ -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)
}

108
pkg/cmd/pr/shared/lookup_test.go
View file

@ -1,108 +0,0 @@
package shared
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) {
type args struct {
baseRepoFn func() (ghrepo.Interface, error)
branchFn func() (string, error)
remotesFn func() (context.Remotes, error)
selector string
}
tests := []struct {
name string
args args
httpStub func(*httpmock.Registry)
wantPR int
wantRepo string
wantErr bool
}{
{
name: "number argument",
args: args{
selector: "13",
baseRepoFn: func() (ghrepo.Interface, error) {
return ghrepo.FromFullName("OWNER/REPO")
},
},
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",
},
{
name: "number with hash argument",
args: args{
selector: "#13",
baseRepoFn: func() (ghrepo.Interface, error) {
return ghrepo.FromFullName("OWNER/REPO")
},
},
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",
},
{
name: "URL argument",
args: args{
selector: "https://example.org/OWNER/REPO/pull/13/files",
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":{
"pullRequest":{"number":13}
}}}`))
},
wantPR: 13,
wantRepo: "https://example.org/OWNER/REPO",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
reg := &httpmock.Registry{}
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)
if (err != nil) != tt.wantErr {
t.Errorf("IssueFromArg() error = %v, wantErr %v", err, tt.wantErr)
return
}
if pr.Number != tt.wantPR {
t.Errorf("want issue #%d, got #%d", tt.wantPR, pr.Number)
}
repoURL := ghrepo.GenerateRepoURL(repo, "")
if repoURL != tt.wantRepo {
t.Errorf("want repo %s, got %s", tt.wantRepo, repoURL)
}
})
}
}

6
pkg/cmd/pr/status/fixtures/prStatusChecks.json
View file

@ -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": {

54
pkg/cmd/pr/view/fixtures/prView.json
View file

@ -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
}
]
}
}
}
}

54
pkg/cmd/pr/view/fixtures/prViewPreviewDraftStatebyBranch.json
View file

@ -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
}
]
}
}
}
}

1
pkg/cmd/pr/view/fixtures/prViewPreviewNoReviews.json
View file

@ -1 +0,0 @@
{ "data": { "repository": { "pullRequest": { "reviews": { } } } } }

139
pkg/cmd/pr/view/fixtures/prViewPreviewWithMetadataByBranch.json
View file

@ -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
}
]
}
}
}
}

52
pkg/cmd/pr/view/fixtures/prView_EmptyBody.json
View file

@ -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
}
]
}
}
}
}

15
pkg/cmd/pr/view/fixtures/prView_NoActiveBranch.json
View file

@ -1,15 +0,0 @@
{"data":{
"repository": {
"pullRequests": {
"edges": []
}
},
"viewerCreated": {
"edges": [],
"pageInfo": { "hasNextPage": false }
},
"reviewRequested": {
"edges": [],
"pageInfo": { "hasNextPage": false }
}
}}

103
pkg/cmd/pr/view/view.go
View file

@ -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,25 @@ 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", "commitsCount",
"baseRefName", "headRefName", "headRepositoryOwner", "headRepository", "isCrossRepository",
"reviewRequests", "reviews", "assignees", "labels", "projectCards", "milestone",
"comments", "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
}
@ -148,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)))
@ -200,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)
}
@ -412,51 +417,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
}

319
pkg/cmd/pr/view/view_test.go
View file

@ -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)

5
pkg/cmd/run/shared/shared.go
View file

@ -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, &notFound) {
var httpError api.HTTPError
if errors.As(err, &httpError) && httpError.StatusCode == 404 {
return []Annotation{}, nil
}
return nil, err
}