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

refactor(pr status): remove ChecksStatus slow path

Browse source 
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>
This commit is contained in:
Kynan Ware 2026-03-11 15:21:17 -06:00
parent f7ff8f2079
commit 7477bdb690
11 changed files with 182 additions and 667 deletions
Download patch file Download diff file Expand all files Collapse all files

522
api/pull_request_test.go
View file

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

98
api/queries_pr.go
View file

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

43
internal/featuredetection/feature_detection.go
View file

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

67
internal/featuredetection/feature_detection_test.go
View file

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

12
pkg/cmd/pr/status/http.go
View file

@ -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(`

14
pkg/cmd/pr/status/status.go
View file

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

49
pkg/cmd/pr/status/status_test.go
View file

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

9
pkg/cmd/pr/view/fixtures/prViewPreviewWithAllChecksFailing.json
View file

@ -44,6 +44,15 @@
"oid": "abc",
"statusCheckRollup": {
"contexts": {
"checkRunCount": 2,
"checkRunCountsByState": [
{
"state": "FAILURE",
"count": 2
}
],
"statusContextCount": 0,
"statusContextCountsByState": [],
"nodes": [
{
"__typename": "CheckRun",

9
pkg/cmd/pr/view/fixtures/prViewPreviewWithAllChecksPassing.json
View file

@ -44,6 +44,15 @@
"oid": "abc",
"statusCheckRollup": {
"contexts": {
"checkRunCount": 3,
"checkRunCountsByState": [
{
"state": "SUCCESS",
"count": 3
}
],
"statusContextCount": 0,
"statusContextCountsByState": [],
"nodes": [
{
"__typename": "CheckRun",

13
pkg/cmd/pr/view/fixtures/prViewPreviewWithSomeChecksFailing.json
View file

@ -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",

13
pkg/cmd/pr/view/fixtures/prViewPreviewWithSomeChecksPending.json
View file

@ -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",