From 39464a527bcb8affef8d8a225708e31c1d0715e3 Mon Sep 17 00:00:00 2001
From: Cameron Moore <moorereason@gmail.com>
Date: Tue, 22 Dec 2020 20:01:44 -0600
Subject: [PATCH] Refactor CI-Tests to show negative results

Add negative check results to the CI-Tests output.

Assuming that a repo will only support one CI system, GithubStatuses and
GithubCheckRuns are merged into a single CITests function.  Since both
GithubStatuses and GithubCheckRuns were essentially validating the same
PRs, it makes more sense to keep all of that state together in a single
check.

Additionaly, a single check can reduce the number of API queries once we
detect the CI system in use.

Fixes #96
Updates #95
---
 checks/ci_tests.go | 117 ++++++++++++++++++++++++---------------------
 1 file changed, 63 insertions(+), 54 deletions(-)

diff --git a/checks/ci_tests.go b/checks/ci_tests.go
index b9c8f55c2bc..b3a052fcf1b 100644
--- a/checks/ci_tests.go
+++ b/checks/ci_tests.go
@@ -22,10 +22,10 @@ import (
 )
 
 func init() {
-	registerCheck("CI-Tests", checker.MultiCheck(GithubStatuses, GithubCheckRuns))
+	registerCheck("CI-Tests", CITests)
 }
 
-func GithubStatuses(c checker.Checker) checker.CheckResult {
+func CITests(c checker.Checker) checker.CheckResult {
 	prs, _, err := c.Client.PullRequests.List(c.Ctx, c.Owner, c.Repo, &github.PullRequestListOptions{
 		State: "closed",
 	})
@@ -33,6 +33,14 @@ func GithubStatuses(c checker.Checker) checker.CheckResult {
 		return checker.RetryResult(err)
 	}
 
+	const (
+		// States for which CI system is in use.
+		unknown = iota
+		githubStatuses
+		githubCheckRuns
+	)
+
+	usedSystem := unknown
 	totalMerged := 0
 	totalTested := 0
 	for _, pr := range prs {
@@ -40,24 +48,64 @@ func GithubStatuses(c checker.Checker) checker.CheckResult {
 			continue
 		}
 		totalMerged++
-		statuses, _, err := c.Client.Repositories.ListStatuses(c.Ctx, c.Owner, c.Repo, pr.GetHead().GetSHA(), &github.ListOptions{})
-		if err != nil {
-			return checker.RetryResult(err)
-		}
-		for _, status := range statuses {
-			if status.GetState() != "success" {
+
+		var foundCI bool
+
+		// Github Statuses
+		if usedSystem <= githubStatuses {
+			statuses, _, err := c.Client.Repositories.ListStatuses(c.Ctx, c.Owner, c.Repo, pr.GetHead().GetSHA(), &github.ListOptions{})
+			if err != nil {
+				return checker.RetryResult(err)
+			}
+
+			for _, status := range statuses {
+				if status.GetState() != "success" {
+					continue
+				}
+				if isTest(status.GetContext()) {
+					c.Logf("CI test found: pr: %d, context: %s, url: %s", pr.GetNumber(), status.GetContext(), status.GetURL())
+					totalTested++
+					foundCI = true
+					usedSystem = githubStatuses
+					break
+				}
+			}
+
+			if foundCI {
 				continue
 			}
-			if isTest(status.GetContext()) {
-				c.Logf("CI test found: context: %s, url: %s", status.GetContext(), status.GetURL())
-				totalTested++
-				break
+		}
+
+		// Github Check Runs
+		if usedSystem == githubCheckRuns || usedSystem == unknown {
+			crs, _, err := c.Client.Checks.ListCheckRunsForRef(c.Ctx, c.Owner, c.Repo, pr.GetHead().GetSHA(), &github.ListCheckRunsOptions{})
+			if err != nil || crs == nil {
+				return checker.RetryResult(err)
+			}
+
+			for _, cr := range crs.CheckRuns {
+				if cr.GetStatus() != "completed" {
+					continue
+				}
+				if cr.GetConclusion() != "success" {
+					continue
+				}
+				if isTest(cr.GetApp().GetSlug()) {
+					c.Logf("CI test found: pr: %d, context: %s, url: %s", pr.GetNumber(), cr.GetApp().GetSlug(), cr.GetURL())
+					totalTested++
+					foundCI = true
+					usedSystem = githubCheckRuns
+					break
+				}
 			}
 		}
+
+		if !foundCI {
+			c.Logf("!! found merged PR without CI test: %d", pr.GetNumber())
+		}
 	}
-	if totalTested == 0 {
-		return checker.InconclusiveResult
-	}
+
+	c.Logf("found CI tests for %d of %d merged PRs", totalTested, totalMerged)
 	return checker.ProportionalResult(totalTested, totalMerged, .75)
 }
 
@@ -72,42 +120,3 @@ func isTest(s string) bool {
 	}
 	return false
 }
-
-func GithubCheckRuns(c checker.Checker) checker.CheckResult {
-	prs, _, err := c.Client.PullRequests.List(c.Ctx, c.Owner, c.Repo, &github.PullRequestListOptions{
-		State: "closed",
-	})
-	if err != nil {
-		return checker.RetryResult(err)
-	}
-
-	totalMerged := 0
-	totalTested := 0
-	for _, pr := range prs {
-		if pr.MergedAt == nil {
-			continue
-		}
-		totalMerged++
-		crs, _, err := c.Client.Checks.ListCheckRunsForRef(c.Ctx, c.Owner, c.Repo, pr.GetHead().GetSHA(), &github.ListCheckRunsOptions{})
-		if err != nil || crs == nil {
-			return checker.RetryResult(err)
-		}
-		for _, cr := range crs.CheckRuns {
-			if cr.GetStatus() != "completed" {
-				continue
-			}
-			if cr.GetConclusion() != "success" {
-				continue
-			}
-			if isTest(cr.GetApp().GetSlug()) {
-				c.Logf("CI test found: context: %s, url: %s", cr.GetApp().GetSlug(), cr.GetURL())
-				totalTested++
-				break
-			}
-		}
-	}
-	if totalTested == 0 {
-		return checker.InconclusiveResult
-	}
-	return checker.ProportionalResult(totalTested, totalMerged, .75)
-}