diff --git a/api/pull_request_test.go b/api/pull_request_test.go index dc736bd75..5ab67161a 100644 --- a/api/pull_request_test.go +++ b/api/pull_request_test.go @@ -2,7 +2,6 @@ package api import ( "encoding/json" - "fmt" "reflect" "testing" "time" @@ -14,8 +13,8 @@ func TestChecksStatus_NoCheckRunsOrStatusContexts(t *testing.T) { t.Parallel() payload := ` - { "statusCheckRollup": { "nodes": [] } } - ` +{ "statusCheckRollup": { "nodes": [] } } +` var pr PullRequest require.NoError(t, json.Unmarshal([]byte(payload), &pr)) @@ -28,306 +27,107 @@ func TestChecksStatus_NoCheckRunsOrStatusContexts(t *testing.T) { require.Equal(t, expectedChecksStatus, pr.ChecksStatus()) } -func TestChecksStatus_SummarisingCheckRuns(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - payload string - expectedChecksStatus PullRequestChecksStatus - }{ - { - name: "QUEUED is treated as Pending", - payload: singleCheckRunWithStatus("QUEUED"), - expectedChecksStatus: PullRequestChecksStatus{Pending: 1, Total: 1}, - }, - { - name: "IN_PROGRESS is treated as Pending", - payload: singleCheckRunWithStatus("IN_PROGRESS"), - expectedChecksStatus: PullRequestChecksStatus{Pending: 1, Total: 1}, - }, - { - name: "WAITING is treated as Pending", - payload: singleCheckRunWithStatus("WAITING"), - expectedChecksStatus: PullRequestChecksStatus{Pending: 1, Total: 1}, - }, - { - name: "PENDING is treated as Pending", - payload: singleCheckRunWithStatus("PENDING"), - expectedChecksStatus: PullRequestChecksStatus{Pending: 1, Total: 1}, - }, - { - name: "REQUESTED is treated as Pending", - payload: singleCheckRunWithStatus("REQUESTED"), - expectedChecksStatus: PullRequestChecksStatus{Pending: 1, Total: 1}, - }, - { - name: "COMPLETED with no conclusion is treated as Pending", - payload: singleCheckRunWithStatus("COMPLETED"), - expectedChecksStatus: PullRequestChecksStatus{Pending: 1, Total: 1}, - }, - { - name: "COMPLETED / STARTUP_FAILURE is treated as Pending", - payload: singleCompletedCheckRunWithConclusion("STARTUP_FAILURE"), - expectedChecksStatus: PullRequestChecksStatus{Pending: 1, Total: 1}, - }, - { - name: "COMPLETED / STALE is treated as Pending", - payload: singleCompletedCheckRunWithConclusion("STALE"), - expectedChecksStatus: PullRequestChecksStatus{Pending: 1, Total: 1}, - }, - { - name: "COMPLETED / SUCCESS is treated as Passing", - payload: singleCompletedCheckRunWithConclusion("SUCCESS"), - expectedChecksStatus: PullRequestChecksStatus{Passing: 1, Total: 1}, - }, - { - name: "COMPLETED / NEUTRAL is treated as Passing", - payload: singleCompletedCheckRunWithConclusion("NEUTRAL"), - expectedChecksStatus: PullRequestChecksStatus{Passing: 1, Total: 1}, - }, - { - name: "COMPLETED / SKIPPED is treated as Passing", - payload: singleCompletedCheckRunWithConclusion("SKIPPED"), - expectedChecksStatus: PullRequestChecksStatus{Passing: 1, Total: 1}, - }, - { - name: "COMPLETED / ACTION_REQUIRED is treated as Failing", - payload: singleCompletedCheckRunWithConclusion("ACTION_REQUIRED"), - expectedChecksStatus: PullRequestChecksStatus{Failing: 1, Total: 1}, - }, - { - name: "COMPLETED / TIMED_OUT is treated as Failing", - payload: singleCompletedCheckRunWithConclusion("TIMED_OUT"), - expectedChecksStatus: PullRequestChecksStatus{Failing: 1, Total: 1}, - }, - { - name: "COMPLETED / CANCELLED is excluded from counts", - payload: singleCompletedCheckRunWithConclusion("CANCELLED"), - expectedChecksStatus: PullRequestChecksStatus{}, - }, - { - name: "COMPLETED / CANCELLED is excluded from counts (duplicate)", - payload: singleCompletedCheckRunWithConclusion("CANCELLED"), - expectedChecksStatus: PullRequestChecksStatus{}, - }, - { - name: "COMPLETED / FAILURE is treated as Failing", - payload: singleCompletedCheckRunWithConclusion("FAILURE"), - expectedChecksStatus: PullRequestChecksStatus{Failing: 1, Total: 1}, - }, - { - name: "Unrecognized Status are treated as Pending", - payload: singleCheckRunWithStatus("AnUnrecognizedStatusJustForThisTest"), - expectedChecksStatus: PullRequestChecksStatus{Pending: 1, Total: 1}, - }, - { - name: "Unrecognized Conclusions are treated as Pending", - payload: singleCompletedCheckRunWithConclusion("AnUnrecognizedConclusionJustForThisTest"), - expectedChecksStatus: PullRequestChecksStatus{Pending: 1, Total: 1}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - var pr PullRequest - require.NoError(t, json.Unmarshal([]byte(tt.payload), &pr)) - - require.Equal(t, tt.expectedChecksStatus, pr.ChecksStatus()) - }) - } -} - -func TestChecksStatus_SummarisingStatusContexts(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - payload string - expectedChecksStatus PullRequestChecksStatus - }{ - { - name: "EXPECTED is treated as Pending", - payload: singleStatusContextWithState("EXPECTED"), - expectedChecksStatus: PullRequestChecksStatus{Pending: 1, Total: 1}, - }, - { - name: "PENDING is treated as Pending", - payload: singleStatusContextWithState("PENDING"), - expectedChecksStatus: PullRequestChecksStatus{Pending: 1, Total: 1}, - }, - { - name: "SUCCESS is treated as Passing", - payload: singleStatusContextWithState("SUCCESS"), - expectedChecksStatus: PullRequestChecksStatus{Passing: 1, Total: 1}, - }, - { - name: "ERROR is treated as Failing", - payload: singleStatusContextWithState("ERROR"), - expectedChecksStatus: PullRequestChecksStatus{Failing: 1, Total: 1}, - }, - { - name: "FAILURE is treated as Failing", - payload: singleStatusContextWithState("FAILURE"), - expectedChecksStatus: PullRequestChecksStatus{Failing: 1, Total: 1}, - }, - { - name: "Unrecognized States are treated as Pending", - payload: singleStatusContextWithState("AnUnrecognizedStateJustForThisTest"), - expectedChecksStatus: PullRequestChecksStatus{Pending: 1, Total: 1}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - var pr PullRequest - require.NoError(t, json.Unmarshal([]byte(tt.payload), &pr)) - - require.Equal(t, tt.expectedChecksStatus, pr.ChecksStatus()) - }) - } -} - -func TestChecksStatus_SummarisingCheckRunsAndStatusContexts(t *testing.T) { - t.Parallel() - - // This might look a bit intimidating, but we're just inserting three nodes - // into the rollup, two completed check run nodes and one status context node. - payload := fmt.Sprintf(` - { "statusCheckRollup": { "nodes": [{ "commit": { - "statusCheckRollup": { - "contexts": { - "nodes": [ - %s, - %s, - %s - ] - } - } - } }] } } - `, - completedCheckRunNodeWithName("build", "SUCCESS"), - statusContextNodeWithName("ci/deploy", "PENDING"), - completedCheckRunNodeWithName("lint", "FAILURE"), - ) - - var pr PullRequest - require.NoError(t, json.Unmarshal([]byte(payload), &pr)) - - expectedChecksStatus := PullRequestChecksStatus{ - Pending: 1, - Failing: 1, - Passing: 1, - Total: 3, - } - require.Equal(t, expectedChecksStatus, pr.ChecksStatus()) -} - func TestChecksStatus_SummarisingCheckRunAndStatusContextCountsByState(t *testing.T) { t.Parallel() payload := ` - { "statusCheckRollup": { "nodes": [{ "commit": { - "statusCheckRollup": { - "contexts": { - "checkRunCount": 14, - "checkRunCountsByState": [ - { - "state": "ACTION_REQUIRED", - "count": 1 - }, - { - "state": "CANCELLED", - "count": 1 - }, - { - "state": "COMPLETED", - "count": 1 - }, - { - "state": "FAILURE", - "count": 1 - }, - { - "state": "IN_PROGRESS", - "count": 1 - }, - { - "state": "NEUTRAL", - "count": 1 - }, - { - "state": "PENDING", - "count": 1 - }, - { - "state": "QUEUED", - "count": 1 - }, - { - "state": "SKIPPED", - "count": 1 - }, - { - "state": "STALE", - "count": 1 - }, - { - "state": "STARTUP_FAILURE", - "count": 1 - }, - { - "state": "SUCCESS", - "count": 1 - }, - { - "state": "TIMED_OUT", - "count": 1 - }, - { - "state": "WAITING", - "count": 1 - }, - { - "state": "AnUnrecognizedStateJustForThisTest", - "count": 1 - } - ], - "statusContextCount": 6, - "statusContextCountsByState": [ - { - "state": "EXPECTED", - "count": 1 - }, - { - "state": "ERROR", - "count": 1 - }, - { - "state": "FAILURE", - "count": 1 - }, - { - "state": "PENDING", - "count": 1 - }, - { - "state": "SUCCESS", - "count": 1 - }, - { - "state": "AnUnrecognizedStateJustForThisTest", - "count": 1 - } - ] - } - } - } }] } } - ` +{ "statusCheckRollup": { "nodes": [{ "commit": { +"statusCheckRollup": { +"contexts": { +"checkRunCount": 14, +"checkRunCountsByState": [ +{ +"state": "ACTION_REQUIRED", +"count": 1 +}, +{ +"state": "CANCELLED", +"count": 1 +}, +{ +"state": "COMPLETED", +"count": 1 +}, +{ +"state": "FAILURE", +"count": 1 +}, +{ +"state": "IN_PROGRESS", +"count": 1 +}, +{ +"state": "NEUTRAL", +"count": 1 +}, +{ +"state": "PENDING", +"count": 1 +}, +{ +"state": "QUEUED", +"count": 1 +}, +{ +"state": "SKIPPED", +"count": 1 +}, +{ +"state": "STALE", +"count": 1 +}, +{ +"state": "STARTUP_FAILURE", +"count": 1 +}, +{ +"state": "SUCCESS", +"count": 1 +}, +{ +"state": "TIMED_OUT", +"count": 1 +}, +{ +"state": "WAITING", +"count": 1 +}, +{ +"state": "AnUnrecognizedStateJustForThisTest", +"count": 1 +} +], +"statusContextCount": 6, +"statusContextCountsByState": [ +{ +"state": "EXPECTED", +"count": 1 +}, +{ +"state": "ERROR", +"count": 1 +}, +{ +"state": "FAILURE", +"count": 1 +}, +{ +"state": "PENDING", +"count": 1 +}, +{ +"state": "SUCCESS", +"count": 1 +}, +{ +"state": "AnUnrecognizedStateJustForThisTest", +"count": 1 +} +] +} +} +} }] } } +` var pr PullRequest require.NoError(t, json.Unmarshal([]byte(payload), &pr)) @@ -341,130 +141,6 @@ func TestChecksStatus_SummarisingCheckRunAndStatusContextCountsByState(t *testin require.Equal(t, expectedChecksStatus, pr.ChecksStatus()) } -// Note that it would be incorrect to provide a status of COMPLETED here -// as the conclusion is always set to null. If you want a COMPLETED status, -// use `singleCompletedCheckRunWithConclusion`. -func singleCheckRunWithStatus(status string) string { - return fmt.Sprintf(` - { "statusCheckRollup": { "nodes": [{ "commit": { - "statusCheckRollup": { - "contexts": { - "nodes": [ - { - "__typename": "CheckRun", - "status": "%s", - "conclusion": null - } - ] - } - } - } }] } } - `, status) -} - -func singleCompletedCheckRunWithConclusion(conclusion string) string { - return fmt.Sprintf(` - { "statusCheckRollup": { "nodes": [{ "commit": { - "statusCheckRollup": { - "contexts": { - "nodes": [ - { - "__typename": "CheckRun", - "status": "COMPLETED", - "conclusion": "%s" - } - ] - } - } - } }] } } - `, conclusion) -} - -func singleStatusContextWithState(state string) string { - return fmt.Sprintf(` - { "statusCheckRollup": { "nodes": [{ "commit": { - "statusCheckRollup": { - "contexts": { - "nodes": [ - { - "__typename": "StatusContext", - "state": "%s" - } - ] - } - } - } }] } } - `, state) -} - -func completedCheckRunNodeWithName(name, conclusion string) string { - return fmt.Sprintf(` - { - "__typename": "CheckRun", - "name": "%s", - "status": "COMPLETED", - "conclusion": "%s" - }`, name, conclusion) -} - -func statusContextNodeWithName(context, state string) string { - return fmt.Sprintf(` - { - "__typename": "StatusContext", - "context": "%s", - "state": "%s" - }`, context, state) -} - -func TestChecksStatus_DuplicateCheckRunsAreDeduplicated(t *testing.T) { - t.Parallel() - - // Simulate cancel-in-progress: a cancelled run followed by a newer successful run - // for the same check name. Only the newer (successful) run should be counted. - pr := PullRequest{} - pr.StatusCheckRollup.Nodes = []StatusCheckRollupNode{ - { - Commit: StatusCheckRollupCommit{ - StatusCheckRollup: CommitStatusCheckRollup{ - Contexts: CheckContexts{ - Nodes: []CheckContext{ - { - TypeName: "CheckRun", - Name: "Prevent Merging", - Status: "COMPLETED", - Conclusion: CheckConclusionStateCancelled, - StartedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), - }, - { - TypeName: "CheckRun", - Name: "Prevent Merging", - Status: "COMPLETED", - Conclusion: CheckConclusionStateSuccess, - StartedAt: time.Date(2024, 1, 1, 1, 0, 0, 0, time.UTC), - }, - { - TypeName: "CheckRun", - Name: "Build", - Status: "COMPLETED", - Conclusion: CheckConclusionStateSuccess, - StartedAt: time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC), - }, - }, - }, - }, - }, - }, - } - - status := pr.ChecksStatus() - require.Equal(t, PullRequestChecksStatus{ - Passing: 2, - Failing: 0, - Pending: 0, - Total: 2, - }, status) -} - func TestEliminateDuplicateChecks(t *testing.T) { t.Parallel() diff --git a/api/queries_pr.go b/api/queries_pr.go index 523213abc..d39807123 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -339,71 +339,29 @@ func (pr *PullRequest) ChecksStatus() PullRequestChecksStatus { contexts := pr.StatusCheckRollup.Nodes[0].Commit.StatusCheckRollup.Contexts - // If this commit has counts by state then we can summarise check status from those - if len(contexts.CheckRunCountsByState) != 0 && len(contexts.StatusContextCountsByState) != 0 { - summary.Total = contexts.CheckRunCount + contexts.StatusContextCount - for _, countByState := range contexts.CheckRunCountsByState { - switch parseCheckStatusFromCheckRunState(countByState.State) { - case passing: - summary.Passing += countByState.Count - case failing: - summary.Failing += countByState.Count - case cancelled: - summary.Total -= countByState.Count - default: - summary.Pending += countByState.Count - } + summary.Total = contexts.CheckRunCount + contexts.StatusContextCount + for _, countByState := range contexts.CheckRunCountsByState { + switch parseCheckStatusFromCheckRunState(countByState.State) { + case passing: + summary.Passing += countByState.Count + case failing: + summary.Failing += countByState.Count + case cancelled: + summary.Total -= countByState.Count + default: + summary.Pending += countByState.Count } - - for _, countByState := range contexts.StatusContextCountsByState { - switch parseCheckStatusFromStatusState(countByState.State) { - case passing: - summary.Passing += countByState.Count - case failing: - summary.Failing += countByState.Count - default: - summary.Pending += countByState.Count - } - } - - return summary } - // If we don't have the counts by state, then we'll need to summarise by looking at the more detailed contexts - for _, c := range EliminateDuplicateChecks(contexts.Nodes) { - // Nodes are a discriminated union of CheckRun or StatusContext and we can match on - // the TypeName to narrow the type. - if c.TypeName == "CheckRun" { - // https://docs.github.com/en/graphql/reference/enums#checkstatusstate - // If the status is completed then we can check the conclusion field - if c.Status == "COMPLETED" { - switch parseCheckStatusFromCheckConclusionState(c.Conclusion) { - case passing: - summary.Passing++ - case failing: - summary.Failing++ - case cancelled: - continue - default: - summary.Pending++ - } - // otherwise we're in some form of pending state: - // "COMPLETED", "IN_PROGRESS", "PENDING", "QUEUED", "REQUESTED", "WAITING" or otherwise unknown - } else { - summary.Pending++ - } - - } else { // c.TypeName == StatusContext - switch parseCheckStatusFromStatusState(c.State) { - case passing: - summary.Passing++ - case failing: - summary.Failing++ - default: - summary.Pending++ - } + for _, countByState := range contexts.StatusContextCountsByState { + switch parseCheckStatusFromStatusState(countByState.State) { + case passing: + summary.Passing += countByState.Count + case failing: + summary.Failing += countByState.Count + default: + summary.Pending += countByState.Count } - summary.Total++ } return summary @@ -453,24 +411,6 @@ func parseCheckStatusFromCheckRunState(state CheckRunState) checkStatus { } } -func parseCheckStatusFromCheckConclusionState(state CheckConclusionState) checkStatus { - switch state { - case CheckConclusionStateNeutral, CheckConclusionStateSkipped, CheckConclusionStateSuccess: - return passing - case CheckConclusionStateActionRequired, CheckConclusionStateFailure, CheckConclusionStateTimedOut: - return failing - case CheckConclusionStateCancelled: - return cancelled - case CheckConclusionStateStale, CheckConclusionStateStartupFailure: - return pending - // Currently, we treat anything unknown as pending, which includes any future unknown - // states we might get back from the API. It might be interesting to do some work to add an additional - // unknown state. - default: - return pending - } -} - // EliminateDuplicateChecks filters a set of checks to only the most recent ones if the set // includes repeated runs. It deduplicates CheckRun entries by name/workflow/event and // StatusContext entries by context name, keeping the most recently started entry in each case. diff --git a/internal/featuredetection/feature_detection.go b/internal/featuredetection/feature_detection.go index 9af4c5aec..d7b1b92bb 100644 --- a/internal/featuredetection/feature_detection.go +++ b/internal/featuredetection/feature_detection.go @@ -6,7 +6,6 @@ import ( "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/gh" "github.com/hashicorp/go-version" - "golang.org/x/sync/errgroup" ghauth "github.com/cli/go-gh/v2/pkg/auth" ) @@ -31,18 +30,13 @@ var allIssueFeatures = IssueFeatures{ } type PullRequestFeatures struct { - MergeQueue bool - // CheckRunAndStatusContextCounts indicates whether the API supports - // the checkRunCount, checkRunCountsByState, statusContextCount and statusContextCountsByState - // fields on the StatusCheckRollupContextConnection - CheckRunAndStatusContextCounts bool - CheckRunEvent bool + MergeQueue bool + CheckRunEvent bool } var allPullRequestFeatures = PullRequestFeatures{ - MergeQueue: true, - CheckRunAndStatusContextCounts: true, - CheckRunEvent: true, + MergeQueue: true, + CheckRunEvent: true, } type RepositoryFeatures struct { @@ -154,16 +148,6 @@ func (d *detector) PullRequestFeatures() (PullRequestFeatures, error) { Name string } `graphql:"fields(includeDeprecated: true)"` } `graphql:"PullRequest: __type(name: \"PullRequest\")"` - StatusCheckRollupContextConnection struct { - Fields []struct { - Name string - } `graphql:"fields(includeDeprecated: true)"` - } `graphql:"StatusCheckRollupContextConnection: __type(name: \"StatusCheckRollupContextConnection\")"` - } - - // Break feature detection down into two separate queries because the platform - // only supports two `__type` expressions in one query. - var pullRequestFeatureDetection2 struct { WorkflowRun struct { Fields []struct { Name string @@ -173,14 +157,7 @@ func (d *detector) PullRequestFeatures() (PullRequestFeatures, error) { gql := api.NewClientFromHTTP(d.httpClient) - var wg errgroup.Group - wg.Go(func() error { - return gql.Query(d.host, "PullRequest_fields", &pullRequestFeatureDetection, nil) - }) - wg.Go(func() error { - return gql.Query(d.host, "PullRequest_fields2", &pullRequestFeatureDetection2, nil) - }) - if err := wg.Wait(); err != nil { + if err := gql.Query(d.host, "PullRequest_fields", &pullRequestFeatureDetection, nil); err != nil { return PullRequestFeatures{}, err } @@ -192,15 +169,7 @@ func (d *detector) PullRequestFeatures() (PullRequestFeatures, error) { } } - for _, field := range pullRequestFeatureDetection.StatusCheckRollupContextConnection.Fields { - // We only check for checkRunCount here but it, checkRunCountsByState, statusContextCount and statusContextCountsByState - // were all introduced in the same version of the API. - if field.Name == "checkRunCount" { - features.CheckRunAndStatusContextCounts = true - } - } - - for _, field := range pullRequestFeatureDetection2.WorkflowRun.Fields { + for _, field := range pullRequestFeatureDetection.WorkflowRun.Fields { if field.Name == "event" { features.CheckRunEvent = true } diff --git a/internal/featuredetection/feature_detection_test.go b/internal/featuredetection/feature_detection_test.go index 82132ab83..12d8a9bef 100644 --- a/internal/featuredetection/feature_detection_test.go +++ b/internal/featuredetection/feature_detection_test.go @@ -87,19 +87,6 @@ func TestPullRequestFeatures(t *testing.T) { {"name": "isMergeQueueEnabled"} ] }, - "StatusCheckRollupContextConnection": { - "fields": [ - {"name": "checkRunCount"}, - {"name": "checkRunCountsByState"}, - {"name": "statusContextCount"}, - {"name": "statusContextCountsByState"} - ] - } - } - }`), - `query PullRequest_fields2\b`: heredoc.Doc(` - { - "data": { "WorkflowRun": { "fields": [ {"name": "event"} @@ -109,9 +96,8 @@ func TestPullRequestFeatures(t *testing.T) { }`), }, wantFeatures: PullRequestFeatures{ - MergeQueue: true, - CheckRunAndStatusContextCounts: true, - CheckRunEvent: true, + MergeQueue: true, + CheckRunEvent: true, }, wantErr: false, }, @@ -125,19 +111,6 @@ func TestPullRequestFeatures(t *testing.T) { "PullRequest": { "fields": [] }, - "StatusCheckRollupContextConnection": { - "fields": [ - {"name": "checkRunCount"}, - {"name": "checkRunCountsByState"}, - {"name": "statusContextCount"}, - {"name": "statusContextCountsByState"} - ] - } - } - }`), - `query PullRequest_fields2\b`: heredoc.Doc(` - { - "data": { "WorkflowRun": { "fields": [ {"name": "event"} @@ -147,9 +120,8 @@ func TestPullRequestFeatures(t *testing.T) { }`), }, wantFeatures: PullRequestFeatures{ - MergeQueue: false, - CheckRunAndStatusContextCounts: true, - CheckRunEvent: true, + MergeQueue: false, + CheckRunEvent: true, }, wantErr: false, }, @@ -166,19 +138,6 @@ func TestPullRequestFeatures(t *testing.T) { {"name": "isMergeQueueEnabled"} ] }, - "StatusCheckRollupContextConnection": { - "fields": [ - {"name": "checkRunCount"}, - {"name": "checkRunCountsByState"}, - {"name": "statusContextCount"}, - {"name": "statusContextCountsByState"} - ] - } - } - }`), - `query PullRequest_fields2\b`: heredoc.Doc(` - { - "data": { "WorkflowRun": { "fields": [ {"name": "event"} @@ -188,9 +147,8 @@ func TestPullRequestFeatures(t *testing.T) { }`), }, wantFeatures: PullRequestFeatures{ - MergeQueue: true, - CheckRunAndStatusContextCounts: true, - CheckRunEvent: true, + MergeQueue: true, + CheckRunEvent: true, }, wantErr: false, }, @@ -204,14 +162,6 @@ func TestPullRequestFeatures(t *testing.T) { "PullRequest": { "fields": [] }, - "StatusCheckRollupContextConnection": { - "fields": [] - } - } - }`), - `query PullRequest_fields2\b`: heredoc.Doc(` - { - "data": { "WorkflowRun": { "fields": [] } @@ -219,9 +169,8 @@ func TestPullRequestFeatures(t *testing.T) { }`), }, wantFeatures: PullRequestFeatures{ - MergeQueue: false, - CheckRunAndStatusContextCounts: false, - CheckRunEvent: false, + MergeQueue: false, + CheckRunEvent: false, }, wantErr: false, }, diff --git a/pkg/cmd/pr/status/http.go b/pkg/cmd/pr/status/http.go index b3fbbac26..067483955 100644 --- a/pkg/cmd/pr/status/http.go +++ b/pkg/cmd/pr/status/http.go @@ -18,8 +18,6 @@ type requestOptions struct { Username string Fields []string ConflictStatus bool - - CheckRunAndStatusContextCountsSupported bool } type pullRequestsPayload struct { @@ -60,7 +58,7 @@ func pullRequestStatus(httpClient *http.Client, repo ghrepo.Interface, options r fragments = fmt.Sprintf("fragment pr on PullRequest{%s}fragment prWithReviews on PullRequest{...pr}", gr) } else { var err error - fragments, err = pullRequestFragment(options.ConflictStatus, options.CheckRunAndStatusContextCountsSupported) + fragments, err = pullRequestFragment(options.ConflictStatus) if err != nil { return nil, err } @@ -197,7 +195,7 @@ func getCurrentUsername(username string, hostname string, apiClient *api.Client) return username, nil } -func pullRequestFragment(conflictStatus bool, statusCheckRollupWithCountByState bool) (string, error) { +func pullRequestFragment(conflictStatus bool) (string, error) { fields := []string{ "number", "title", "state", "url", "isDraft", "isCrossRepository", "headRefName", "headRepositoryOwner", "mergeStateStatus", @@ -208,11 +206,7 @@ func pullRequestFragment(conflictStatus bool, statusCheckRollupWithCountByState fields = append(fields, "mergeable") } - if statusCheckRollupWithCountByState { - fields = append(fields, "statusCheckRollupWithCountByState") - } else { - fields = append(fields, "statusCheckRollup") - } + fields = append(fields, "statusCheckRollupWithCountByState") reviewFields := []string{"reviewDecision", "latestReviews"} fragments := fmt.Sprintf(` diff --git a/pkg/cmd/pr/status/status.go b/pkg/cmd/pr/status/status.go index 60202594f..e13b410b4 100644 --- a/pkg/cmd/pr/status/status.go +++ b/pkg/cmd/pr/status/status.go @@ -7,13 +7,11 @@ import ( "net/http" "regexp" "strconv" - "time" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" ghContext "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/git" - fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/text" @@ -35,8 +33,6 @@ type StatusOptions struct { HasRepoOverride bool Exporter cmdutil.Exporter ConflictStatus bool - - Detector fd.Detector } func NewCmdStatus(f *cmdutil.Factory, runF func(*StatusOptions) error) *cobra.Command { @@ -143,16 +139,6 @@ func statusRun(opts *StatusOptions) error { options.Fields = opts.Exporter.Fields() } - if opts.Detector == nil { - cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24) - opts.Detector = fd.NewDetector(cachedClient, baseRefRepo.RepoHost()) - } - prFeatures, err := opts.Detector.PullRequestFeatures() - if err != nil { - return err - } - options.CheckRunAndStatusContextCountsSupported = prFeatures.CheckRunAndStatusContextCounts - prPayload, err := pullRequestStatus(httpClient, baseRefRepo, options) if err != nil { return err diff --git a/pkg/cmd/pr/status/status_test.go b/pkg/cmd/pr/status/status_test.go index 41c01e915..ee3b92e1f 100644 --- a/pkg/cmd/pr/status/status_test.go +++ b/pkg/cmd/pr/status/status_test.go @@ -11,7 +11,6 @@ import ( "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/config" - fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/run" @@ -24,10 +23,6 @@ import ( ) func runCommand(rt http.RoundTripper, branch string, isTTY bool, cli string) (*test.CmdOut, error) { - return runCommandWithDetector(rt, branch, isTTY, cli, &fd.DisabledDetectorMock{}) -} - -func runCommandWithDetector(rt http.RoundTripper, branch string, isTTY bool, cli string, detector fd.Detector) (*test.CmdOut, error) { ios, _, stdout, stderr := iostreams.Test() ios.SetStdoutTTY(isTTY) ios.SetStdinTTY(isTTY) @@ -61,12 +56,7 @@ func runCommandWithDetector(rt http.RoundTripper, branch string, isTTY bool, cli GitClient: &git.Client{GitPath: "some/path/git"}, } - withProvidedDetector := func(opts *StatusOptions) error { - opts.Detector = detector - return statusRun(opts) - } - - cmd := NewCmdStatus(factory, withProvidedDetector) + cmd := NewCmdStatus(factory, nil) cmd.PersistentFlags().StringP("repo", "R", "", "") argv, err := shlex.Split(cli) @@ -125,40 +115,7 @@ func TestPRStatus(t *testing.T) { func TestPRStatus_reviewsAndChecks(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) - // status,conclusion matches the old StatusContextRollup query - http.Register(httpmock.GraphQL(`status,conclusion`), httpmock.FileResponse("./fixtures/prStatusChecks.json")) - - // stub successful git command - rs, cleanup := run.Stub() - defer cleanup(t) - rs.Register(`git config --get-regexp \^branch\\.`, 0, "") - rs.Register(`git config remote.pushDefault`, 0, "") - rs.Register(`git rev-parse --symbolic-full-name blueberries@{push}`, 128, "") - rs.Register(`git config push.default`, 1, "") - - output, err := runCommand(http, "blueberries", true, "") - if err != nil { - t.Errorf("error running command `pr status`: %v", err) - } - - expected := []string{ - "✓ Checks passing + Changes requested ! Merge conflict status unknown", - "- Checks pending ✓ 2 Approved", - "× 1/3 checks failing - Review required ✓ No merge conflicts", - "✓ Checks passing × Merge conflicts", - } - - for _, line := range expected { - if !strings.Contains(output.String(), line) { - t.Errorf("output did not contain %q: %q", line, output.String()) - } - } -} - -func TestPRStatus_reviewsAndChecksWithStatesByCount(t *testing.T) { - http := initFakeHTTP() - defer http.Verify(t) - // checkRunCount,checkRunCountsByState matches the new StatusContextRollup query + // checkRunCount,checkRunCountsByState matches the StatusContextRollup query http.Register(httpmock.GraphQL(`checkRunCount,checkRunCountsByState`), httpmock.FileResponse("./fixtures/prStatusChecksWithStatesByCount.json")) // stub successful git command @@ -169,7 +126,7 @@ func TestPRStatus_reviewsAndChecksWithStatesByCount(t *testing.T) { rs.Register(`git rev-parse --symbolic-full-name blueberries@{push}`, 128, "") rs.Register(`git config push.default`, 1, "") - output, err := runCommandWithDetector(http, "blueberries", true, "", &fd.EnabledDetectorMock{}) + output, err := runCommand(http, "blueberries", true, "") if err != nil { t.Errorf("error running command `pr status`: %v", err) } diff --git a/pkg/cmd/pr/view/fixtures/prViewPreviewWithAllChecksFailing.json b/pkg/cmd/pr/view/fixtures/prViewPreviewWithAllChecksFailing.json index 48484f1c4..70864fe9e 100644 --- a/pkg/cmd/pr/view/fixtures/prViewPreviewWithAllChecksFailing.json +++ b/pkg/cmd/pr/view/fixtures/prViewPreviewWithAllChecksFailing.json @@ -44,6 +44,15 @@ "oid": "abc", "statusCheckRollup": { "contexts": { + "checkRunCount": 2, + "checkRunCountsByState": [ + { + "state": "FAILURE", + "count": 2 + } + ], + "statusContextCount": 0, + "statusContextCountsByState": [], "nodes": [ { "__typename": "CheckRun", diff --git a/pkg/cmd/pr/view/fixtures/prViewPreviewWithAllChecksPassing.json b/pkg/cmd/pr/view/fixtures/prViewPreviewWithAllChecksPassing.json index 87152d692..82e1277ae 100644 --- a/pkg/cmd/pr/view/fixtures/prViewPreviewWithAllChecksPassing.json +++ b/pkg/cmd/pr/view/fixtures/prViewPreviewWithAllChecksPassing.json @@ -44,6 +44,15 @@ "oid": "abc", "statusCheckRollup": { "contexts": { + "checkRunCount": 3, + "checkRunCountsByState": [ + { + "state": "SUCCESS", + "count": 3 + } + ], + "statusContextCount": 0, + "statusContextCountsByState": [], "nodes": [ { "__typename": "CheckRun", diff --git a/pkg/cmd/pr/view/fixtures/prViewPreviewWithSomeChecksFailing.json b/pkg/cmd/pr/view/fixtures/prViewPreviewWithSomeChecksFailing.json index adb15e7c3..1852364e7 100644 --- a/pkg/cmd/pr/view/fixtures/prViewPreviewWithSomeChecksFailing.json +++ b/pkg/cmd/pr/view/fixtures/prViewPreviewWithSomeChecksFailing.json @@ -44,6 +44,19 @@ "oid": "abc", "statusCheckRollup": { "contexts": { + "checkRunCount": 2, + "checkRunCountsByState": [ + { + "state": "SUCCESS", + "count": 1 + }, + { + "state": "FAILURE", + "count": 1 + } + ], + "statusContextCount": 0, + "statusContextCountsByState": [], "nodes": [ { "__typename": "CheckRun", diff --git a/pkg/cmd/pr/view/fixtures/prViewPreviewWithSomeChecksPending.json b/pkg/cmd/pr/view/fixtures/prViewPreviewWithSomeChecksPending.json index 4a011e5c1..30e40464b 100644 --- a/pkg/cmd/pr/view/fixtures/prViewPreviewWithSomeChecksPending.json +++ b/pkg/cmd/pr/view/fixtures/prViewPreviewWithSomeChecksPending.json @@ -44,6 +44,19 @@ "oid": "abc", "statusCheckRollup": { "contexts": { + "checkRunCount": 2, + "checkRunCountsByState": [ + { + "state": "WAITING", + "count": 1 + }, + { + "state": "SUCCESS", + "count": 1 + } + ], + "statusContextCount": 0, + "statusContextCountsByState": [], "nodes": [ { "__typename": "CheckRun",