From 1c274a8a56532724ae1e0ec10181575dc969b004 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Wed, 11 Mar 2026 12:33:31 -0600 Subject: [PATCH] fix(pr status): don't count cancelled checks as failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a GitHub Actions workflow uses concurrency with cancel-in-progress, cancelled runs were counted as failures in `gh pr status` and `gh pr view`, even when a newer run for the same check name succeeded. The GitHub web UI and `gh pr checks` both handle this correctly. Three changes fix this: 1. Add a `cancelled` check status category. Cancelled runs are now excluded from all summary counts (passing/failing/pending) and subtracted from the total, matching the web UI behavior. 2. Move `eliminateDuplicates` from pkg/cmd/pr/checks to `api.EliminateDuplicateChecks` (exported). The function operates entirely on `api.CheckContext` and is now shared by both `pr checks` and `ChecksStatus()` (used by `pr status` and `pr view`). 3. Apply deduplication in the `ChecksStatus()` slow path, keeping only the most recent run per check name — consistent with `pr checks`. Fixes #12895 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- api/pull_request_test.go | 248 +++++++++++++++++- api/queries_pr.go | 47 +++- pkg/cmd/pr/checks/aggregate.go | 31 +-- pkg/cmd/pr/checks/checks_test.go | 2 +- .../pr/status/fixtures/prStatusChecks.json | 18 +- 5 files changed, 296 insertions(+), 50 deletions(-) diff --git a/api/pull_request_test.go b/api/pull_request_test.go index 8ccfa976f..a04e0a934 100644 --- a/api/pull_request_test.go +++ b/api/pull_request_test.go @@ -3,7 +3,9 @@ package api import ( "encoding/json" "fmt" + "reflect" "testing" + "time" "github.com/stretchr/testify/require" ) @@ -100,14 +102,14 @@ func TestChecksStatus_SummarisingCheckRuns(t *testing.T) { expectedChecksStatus: PullRequestChecksStatus{Failing: 1, Total: 1}, }, { - name: "COMPLETED / CANCELLED is treated as Failing", + name: "COMPLETED / CANCELLED is excluded from counts", payload: singleCompletedCheckRunWithConclusion("CANCELLED"), - expectedChecksStatus: PullRequestChecksStatus{Failing: 1, Total: 1}, + expectedChecksStatus: PullRequestChecksStatus{}, }, { - name: "COMPLETED / CANCELLED is treated as Failing", + name: "COMPLETED / CANCELLED is excluded from counts (duplicate)", payload: singleCompletedCheckRunWithConclusion("CANCELLED"), - expectedChecksStatus: PullRequestChecksStatus{Failing: 1, Total: 1}, + expectedChecksStatus: PullRequestChecksStatus{}, }, { name: "COMPLETED / FAILURE is treated as Failing", @@ -208,9 +210,9 @@ func TestChecksStatus_SummarisingCheckRunsAndStatusContexts(t *testing.T) { } } }] } } `, - completedCheckRunNode("SUCCESS"), - statusContextNode("PENDING"), - completedCheckRunNode("FAILURE"), + completedCheckRunNodeWithName("build", "SUCCESS"), + statusContextNodeWithName("ci/deploy", "PENDING"), + completedCheckRunNodeWithName("lint", "FAILURE"), ) var pr PullRequest @@ -332,9 +334,9 @@ func TestChecksStatus_SummarisingCheckRunAndStatusContextCountsByState(t *testin expectedChecksStatus := PullRequestChecksStatus{ Pending: 11, - Failing: 6, + Failing: 5, Passing: 4, - Total: 20, + Total: 19, } require.Equal(t, expectedChecksStatus, pr.ChecksStatus()) } @@ -404,6 +406,16 @@ func completedCheckRunNode(conclusion string) string { }`, conclusion) } +func completedCheckRunNodeWithName(name, conclusion string) string { + return fmt.Sprintf(` + { + "__typename": "CheckRun", + "name": "%s", + "status": "COMPLETED", + "conclusion": "%s" + }`, name, conclusion) +} + func statusContextNode(state string) string { return fmt.Sprintf(` { @@ -411,3 +423,221 @@ func statusContextNode(state string) string { "state": "%s" }`, state) } + +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() + + tests := []struct { + name string + checkContexts []CheckContext + want []CheckContext + }{ + { + name: "duplicate CheckRun keeps most recent", + checkContexts: []CheckContext{ + { + TypeName: "CheckRun", + Name: "lint", + Status: "COMPLETED", + Conclusion: "FAILURE", + StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC), + }, + { + TypeName: "CheckRun", + Name: "lint", + Status: "COMPLETED", + Conclusion: "SUCCESS", + StartedAt: time.Date(2022, 2, 2, 2, 2, 2, 2, time.UTC), + }, + { + TypeName: "CheckRun", + Name: "build", + Status: "COMPLETED", + Conclusion: "SUCCESS", + StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC), + }, + }, + want: []CheckContext{ + { + TypeName: "CheckRun", + Name: "lint", + Status: "COMPLETED", + Conclusion: "SUCCESS", + StartedAt: time.Date(2022, 2, 2, 2, 2, 2, 2, time.UTC), + }, + { + TypeName: "CheckRun", + Name: "build", + Status: "COMPLETED", + Conclusion: "SUCCESS", + StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC), + }, + }, + }, + { + name: "duplicate StatusContext keeps most recent", + checkContexts: []CheckContext{ + { + TypeName: "StatusContext", + Context: "ci/test", + State: "FAILURE", + StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC), + }, + { + TypeName: "StatusContext", + Context: "ci/test", + State: "SUCCESS", + StartedAt: time.Date(2022, 2, 2, 2, 2, 2, 2, time.UTC), + }, + }, + want: []CheckContext{ + { + TypeName: "StatusContext", + Context: "ci/test", + State: "SUCCESS", + StartedAt: time.Date(2022, 2, 2, 2, 2, 2, 2, time.UTC), + }, + }, + }, + { + name: "unique checks are preserved", + checkContexts: []CheckContext{ + { + TypeName: "CheckRun", + Name: "build", + Status: "COMPLETED", + Conclusion: "SUCCESS", + StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC), + }, + { + TypeName: "StatusContext", + Context: "ci/test", + State: "SUCCESS", + StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC), + }, + }, + want: []CheckContext{ + { + TypeName: "CheckRun", + Name: "build", + Status: "COMPLETED", + Conclusion: "SUCCESS", + StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC), + }, + { + TypeName: "StatusContext", + Context: "ci/test", + State: "SUCCESS", + StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC), + }, + }, + }, + { + name: "different workflow names are not deduplicated", + checkContexts: []CheckContext{ + { + TypeName: "CheckRun", + Name: "build", + Status: "COMPLETED", + Conclusion: "SUCCESS", + StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC), + CheckSuite: CheckSuite{WorkflowRun: WorkflowRun{Event: "push", Workflow: Workflow{Name: "CI"}}}, + }, + { + TypeName: "CheckRun", + Name: "build", + Status: "COMPLETED", + Conclusion: "SUCCESS", + StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC), + CheckSuite: CheckSuite{WorkflowRun: WorkflowRun{Event: "push", Workflow: Workflow{Name: "Release"}}}, + }, + }, + want: []CheckContext{ + { + TypeName: "CheckRun", + Name: "build", + Status: "COMPLETED", + Conclusion: "SUCCESS", + StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC), + CheckSuite: CheckSuite{WorkflowRun: WorkflowRun{Event: "push", Workflow: Workflow{Name: "CI"}}}, + }, + { + TypeName: "CheckRun", + Name: "build", + Status: "COMPLETED", + Conclusion: "SUCCESS", + StartedAt: time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC), + CheckSuite: CheckSuite{WorkflowRun: WorkflowRun{Event: "push", Workflow: Workflow{Name: "Release"}}}, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := EliminateDuplicateChecks(tt.checkContexts) + if !reflect.DeepEqual(tt.want, got) { + t.Errorf("got EliminateDuplicateChecks %+v, want %+v", got, tt.want) + } + }) + } +} diff --git a/api/queries_pr.go b/api/queries_pr.go index d0488c64b..05275d6c5 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -4,6 +4,7 @@ import ( "fmt" "net/http" "net/url" + "sort" "time" "github.com/cli/cli/v2/internal/ghrepo" @@ -347,6 +348,8 @@ func (pr *PullRequest) ChecksStatus() PullRequestChecksStatus { summary.Passing += countByState.Count case failing: summary.Failing += countByState.Count + case cancelled: + summary.Total -= countByState.Count default: summary.Pending += countByState.Count } @@ -367,7 +370,7 @@ func (pr *PullRequest) ChecksStatus() PullRequestChecksStatus { } // 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 contexts.Nodes { + 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" { @@ -379,6 +382,8 @@ func (pr *PullRequest) ChecksStatus() PullRequestChecksStatus { summary.Passing++ case failing: summary.Failing++ + case cancelled: + continue default: summary.Pending++ } @@ -407,9 +412,10 @@ func (pr *PullRequest) ChecksStatus() PullRequestChecksStatus { type checkStatus int const ( - passing checkStatus = iota + passing checkStatus = iota failing pending + cancelled ) func parseCheckStatusFromStatusState(state StatusState) checkStatus { @@ -432,8 +438,10 @@ func parseCheckStatusFromCheckRunState(state CheckRunState) checkStatus { switch state { case CheckRunStateNeutral, CheckRunStateSkipped, CheckRunStateSuccess: return passing - case CheckRunStateActionRequired, CheckRunStateCancelled, CheckRunStateFailure, CheckRunStateTimedOut: + case CheckRunStateActionRequired, CheckRunStateFailure, CheckRunStateTimedOut: return failing + case CheckRunStateCancelled: + return cancelled case CheckRunStateCompleted, CheckRunStateInProgress, CheckRunStatePending, CheckRunStateQueued, CheckRunStateStale, CheckRunStateStartupFailure, CheckRunStateWaiting: return pending @@ -449,8 +457,10 @@ func parseCheckStatusFromCheckConclusionState(state CheckConclusionState) checkS switch state { case CheckConclusionStateNeutral, CheckConclusionStateSkipped, CheckConclusionStateSuccess: return passing - case CheckConclusionStateActionRequired, CheckConclusionStateCancelled, CheckConclusionStateFailure, CheckConclusionStateTimedOut: + 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 @@ -461,6 +471,35 @@ func parseCheckStatusFromCheckConclusionState(state CheckConclusionState) checkS } } +// 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. +func EliminateDuplicateChecks(checkContexts []CheckContext) []CheckContext { + sort.Slice(checkContexts, func(i, j int) bool { return checkContexts[i].StartedAt.After(checkContexts[j].StartedAt) }) + + mapChecks := make(map[string]struct{}) + mapContexts := make(map[string]struct{}) + unique := make([]CheckContext, 0, len(checkContexts)) + + for _, ctx := range checkContexts { + if ctx.Context != "" { + if _, exists := mapContexts[ctx.Context]; exists { + continue + } + mapContexts[ctx.Context] = struct{}{} + } else { + key := fmt.Sprintf("%s/%s/%s", ctx.Name, ctx.CheckSuite.WorkflowRun.Workflow.Name, ctx.CheckSuite.WorkflowRun.Event) + if _, exists := mapChecks[key]; exists { + continue + } + mapChecks[key] = struct{}{} + } + unique = append(unique, ctx) + } + + return unique +} + // CreatePullRequest creates a pull request in a GitHub repository func CreatePullRequest(client *Client, repo *Repository, params map[string]interface{}) (*PullRequest, error) { query := ` diff --git a/pkg/cmd/pr/checks/aggregate.go b/pkg/cmd/pr/checks/aggregate.go index 91cec4335..0270cdd79 100644 --- a/pkg/cmd/pr/checks/aggregate.go +++ b/pkg/cmd/pr/checks/aggregate.go @@ -1,8 +1,6 @@ package checks import ( - "fmt" - "sort" "time" "github.com/cli/cli/v2/api" @@ -34,7 +32,7 @@ func (ch *check) ExportData(fields []string) map[string]interface{} { } func aggregateChecks(checkContexts []api.CheckContext, requiredChecks bool) (checks []check, counts checkCounts) { - for _, c := range eliminateDuplicates(checkContexts) { + for _, c := range api.EliminateDuplicateChecks(checkContexts) { if requiredChecks && !c.IsRequired { continue } @@ -91,30 +89,3 @@ func aggregateChecks(checkContexts []api.CheckContext, requiredChecks bool) (che } return } - -// eliminateDuplicates filters a set of checks to only the most recent ones if the set includes repeated runs -func eliminateDuplicates(checkContexts []api.CheckContext) []api.CheckContext { - sort.Slice(checkContexts, func(i, j int) bool { return checkContexts[i].StartedAt.After(checkContexts[j].StartedAt) }) - - mapChecks := make(map[string]struct{}) - mapContexts := make(map[string]struct{}) - unique := make([]api.CheckContext, 0, len(checkContexts)) - - for _, ctx := range checkContexts { - if ctx.Context != "" { - if _, exists := mapContexts[ctx.Context]; exists { - continue - } - mapContexts[ctx.Context] = struct{}{} - } else { - key := fmt.Sprintf("%s/%s/%s", ctx.Name, ctx.CheckSuite.WorkflowRun.Workflow.Name, ctx.CheckSuite.WorkflowRun.Event) - if _, exists := mapChecks[key]; exists { - continue - } - mapChecks[key] = struct{}{} - } - unique = append(unique, ctx) - } - - return unique -} diff --git a/pkg/cmd/pr/checks/checks_test.go b/pkg/cmd/pr/checks/checks_test.go index cbb8ddc7f..460ff90ca 100644 --- a/pkg/cmd/pr/checks/checks_test.go +++ b/pkg/cmd/pr/checks/checks_test.go @@ -1018,7 +1018,7 @@ func TestEliminateDuplicates(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := eliminateDuplicates(tt.checkContexts) + got := api.EliminateDuplicateChecks(tt.checkContexts) if !reflect.DeepEqual(tt.want, got) { t.Errorf("got eliminateDuplicates %+v, want %+v\n", got, tt.want) } diff --git a/pkg/cmd/pr/status/fixtures/prStatusChecks.json b/pkg/cmd/pr/status/fixtures/prStatusChecks.json index 0f5d88902..8feb8f3ac 100644 --- a/pkg/cmd/pr/status/fixtures/prStatusChecks.json +++ b/pkg/cmd/pr/status/fixtures/prStatusChecks.json @@ -26,7 +26,8 @@ "contexts": { "nodes": [ { - "__typeName": "StatusContext", + "__typename": "StatusContext", + "context": "ci/build", "state": "SUCCESS" } ] @@ -81,7 +82,8 @@ "contexts": { "nodes": [ { - "__typeName": "CheckRun", + "__typename": "CheckRun", + "name": "build", "status": "IN_PROGRESS", "conclusion": "" } @@ -111,16 +113,19 @@ "contexts": { "nodes": [ { - "__typeName": "CheckRun", + "__typename": "CheckRun", + "name": "build", "status": "IN_PROGRESS", "conclusion": "" }, { - "__typeName": "StatusContext", + "__typename": "StatusContext", + "context": "ci/deploy", "state": "FAILURE" }, { - "__typeName": "CheckRun", + "__typename": "CheckRun", + "name": "lint", "status": "COMPLETED", "conclusion": "NEUTRAL" } @@ -149,7 +154,8 @@ "contexts": { "nodes": [ { - "__typeName": "StatusContext", + "__typename": "StatusContext", + "context": "ci/build", "state": "SUCCESS" } ]