From 7477bdb69026887a98cf2b673cc253661f8cd7e1 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 11 Mar 2026 15:21:17 -0600 Subject: [PATCH] refactor(pr status): remove ChecksStatus slow path All supported GHES versions (3.16 through 3.20) support the checkRunCountsByState and statusContextCountsByState fields on StatusCheckRollupContextConnection. The slow path that iterated individual CheckContext nodes in ChecksStatus() is dead code. This commit: - Removes the slow path from ChecksStatus(), keeping only the aggregated counts-by-state path - Removes parseCheckStatusFromCheckConclusionState (no callers remain) - Removes CheckRunAndStatusContextCounts from PullRequestFeatures and its introspection detection - Consolidates the two feature detection introspection queries into one (PullRequest + WorkflowRun fits within the platform limit of two __type expressions) - Removes the errgroup dependency from feature detection - Always uses statusCheckRollupWithCountByState in pr status queries - Updates pr view fixtures to include counts-by-state fields Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- api/pull_request_test.go | 522 ++++-------------- api/queries_pr.go | 98 +--- .../featuredetection/feature_detection.go | 43 +- .../feature_detection_test.go | 67 +-- pkg/cmd/pr/status/http.go | 12 +- pkg/cmd/pr/status/status.go | 14 - pkg/cmd/pr/status/status_test.go | 49 +- .../prViewPreviewWithAllChecksFailing.json | 9 + .../prViewPreviewWithAllChecksPassing.json | 9 + .../prViewPreviewWithSomeChecksFailing.json | 13 + .../prViewPreviewWithSomeChecksPending.json | 13 + 11 files changed, 182 insertions(+), 667 deletions(-) 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",