From cd8547b227774e34df0101ce92da9702a29075f2 Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 16 May 2023 19:35:10 +0200 Subject: [PATCH 1/3] Add some further test coverage around PR ChecksStatus --- api/pull_request_test.go | 301 +++++++++++++++++++++++++++++++++++---- 1 file changed, 277 insertions(+), 24 deletions(-) diff --git a/api/pull_request_test.go b/api/pull_request_test.go index 2e4fa73b1..67ea20cb5 100644 --- a/api/pull_request_test.go +++ b/api/pull_request_test.go @@ -2,42 +2,295 @@ package api import ( "encoding/json" + "fmt" "testing" - "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -func TestPullRequest_ChecksStatus(t *testing.T) { - pr := PullRequest{} +func TestChecksStatus_NoCheckRunsOrStatusContexts(t *testing.T) { + t.Parallel() + payload := ` + { "statusCheckRollup": { "nodes": [] } } + ` + var pr PullRequest + require.NoError(t, json.Unmarshal([]byte(payload), &pr)) + + expectedChecksStatus := PullRequestChecksStatus{ + Pending: 0, + Failing: 0, + Passing: 0, + Total: 0, + } + 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 / 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 treated as Failing", + payload: singleCompletedCheckRunWithConclusion("CANCELLED"), + expectedChecksStatus: PullRequestChecksStatus{Failing: 1, Total: 1}, + }, + { + name: "COMPLETED / CANCELLED is treated as Failing", + payload: singleCompletedCheckRunWithConclusion("CANCELLED"), + expectedChecksStatus: PullRequestChecksStatus{Failing: 1, Total: 1}, + }, + { + 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 { + tt := tt + 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 { + tt := tt + 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": [ - { "state": "SUCCESS" }, - { "state": "PENDING" }, - { "state": "FAILURE" }, - { "status": "IN_PROGRESS", - "conclusion": null }, - { "status": "COMPLETED", - "conclusion": "SUCCESS" }, - { "status": "COMPLETED", - "conclusion": "FAILURE" }, - { "status": "COMPLETED", - "conclusion": "ACTION_REQUIRED" }, - { "status": "COMPLETED", - "conclusion": "STALE" } + %s, + %s, + %s ] } } } }] } } - ` - err := json.Unmarshal([]byte(payload), &pr) - assert.NoError(t, err) + `, + completedCheckRunNode("SUCCESS"), + statusContextNode("PENDING"), + completedCheckRunNode("FAILURE"), + ) - checks := pr.ChecksStatus() - assert.Equal(t, 8, checks.Total) - assert.Equal(t, 3, checks.Pending) - assert.Equal(t, 3, checks.Failing) - assert.Equal(t, 2, checks.Passing) + 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()) +} + +// 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 completedCheckRunNode(conclusion string) string { + return fmt.Sprintf(` + { + "__typename": "CheckRun", + "status": "COMPLETED", + "conclusion": "%s" + }`, conclusion) +} + +func statusContextNode(state string) string { + return fmt.Sprintf(` + { + "__typename": "StatusContext", + "state": "%s" + }`, state) } From c4bb344dddc7b0cd805c28cdc5eb47404a8c2cb1 Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 16 May 2023 19:41:59 +0200 Subject: [PATCH 2/3] Add some comments to PR ChecksStatus --- api/queries_pr.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index ae8f7c0c6..5ae5cbb8f 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -265,11 +265,17 @@ func (pr *PullRequest) ChecksStatus() (summary PullRequestChecksStatus) { if len(pr.StatusCheckRollup.Nodes) == 0 { return } + commit := pr.StatusCheckRollup.Nodes[0].Commit for _, c := range commit.StatusCheckRollup.Contexts.Nodes { - state := c.State // StatusContext + // Nodes are a discriminated union of CheckRun or StatusContext, + // but we can use the State field to disambiguate, rather than using the TypeName. + // First we try to get the State, as if we have a StatusContext + state := c.State + // But if the State is empty, then we assume we actually have a CheckRun if state == "" { - // CheckRun + // If the Status of a CheckRun is COMPLETED, then we want to look more closely + // at the Conclusion, as that contains the relevant information. if c.Status == "COMPLETED" { state = c.Conclusion } else { @@ -281,7 +287,9 @@ func (pr *PullRequest) ChecksStatus() (summary PullRequestChecksStatus) { summary.Passing++ case "ERROR", "FAILURE", "CANCELLED", "TIMED_OUT", "ACTION_REQUIRED": summary.Failing++ - default: // "EXPECTED", "REQUESTED", "WAITING", "QUEUED", "PENDING", "IN_PROGRESS", "STALE" + // We treat anything that isn't Passing or Failing as Pending, which included any future unknown states + // we might get back from the API. + default: // "EXPECTED", "REQUESTED", "WAITING", "QUEUED", "PENDING", "IN_PROGRESS", "STALE", "STARTUP_FAILURE" summary.Pending++ } summary.Total++ From bfb5e8f1d6bef0f161271a26578fba7baf22b05f Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 16 May 2023 19:43:07 +0200 Subject: [PATCH 3/3] Avoid using named return in PR ChecksStatus See: https://dave.cheney.net/practical-go/presentations/gophercon-israel.html#_avoid_named_return_values --- api/queries_pr.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 5ae5cbb8f..6f121caab 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -261,9 +261,11 @@ type PullRequestChecksStatus struct { Total int } -func (pr *PullRequest) ChecksStatus() (summary PullRequestChecksStatus) { +func (pr *PullRequest) ChecksStatus() PullRequestChecksStatus { + var summary PullRequestChecksStatus + if len(pr.StatusCheckRollup.Nodes) == 0 { - return + return summary } commit := pr.StatusCheckRollup.Nodes[0].Commit @@ -295,7 +297,7 @@ func (pr *PullRequest) ChecksStatus() (summary PullRequestChecksStatus) { summary.Total++ } - return + return summary } func (pr *PullRequest) DisplayableReviews() PullRequestReviews {