🌱 convert CI-Tests check to probes (ossf#3621)

* 🌱 convert CITest check to probes Signed-off-by: AdamKorcz <[email protected]> * fix lint issues Signed-off-by: Adam Korczynski <[email protected]> * debug failing integration test Signed-off-by: Adam Korczynski <[email protected]> * Add negative outcome to test Signed-off-by: Adam Korczynski <[email protected]> * remove 'totalTested' and 'totalMerged' values from findings Signed-off-by: Adam Korczynski <[email protected]> * Log at debug level Signed-off-by: Adam Korczynski <[email protected]> --------- Signed-off-by: AdamKorcz <[email protected]> Signed-off-by: Adam Korczynski <[email protected]>
kgangerlm · Dec 11, 2023 · 30ef6b1 · 30ef6b1
1 parent 5dc03b7
commit 30ef6b1
Show file tree

Hide file tree

Showing 7 changed files with 806 additions and 503 deletions.
diff --git a/checks/ci_tests.go b/checks/ci_tests.go
@@ -19,9 +19,10 @@ import (
 	"github.com/ossf/scorecard/v4/checks/evaluation"
 	"github.com/ossf/scorecard/v4/checks/raw"
 	sce "github.com/ossf/scorecard/v4/errors"
+	"github.com/ossf/scorecard/v4/probes"
+	"github.com/ossf/scorecard/v4/probes/zrunner"
 )
 
-// CheckCodeReview is the registered name for DoesCodeReview.
 const CheckCITests = "CI-Tests"
 
 //nolint:gochecknoinits
@@ -35,19 +36,22 @@ func init() {
 	}
 }
 
-// CodeReview will check if the maintainers perform code review.
 func CITests(c *checker.CheckRequest) checker.CheckResult {
 	rawData, err := raw.CITests(c.RepoClient)
 	if err != nil {
 		e := sce.WithMessage(sce.ErrScorecardInternal, err.Error())
 		return checker.CreateRuntimeErrorResult(CheckCITests, e)
 	}
 
-	// Return raw results.
-	if c.RawResults != nil {
-		c.RawResults.CITestResults = rawData
+	pRawResults := getRawResults(c)
+	pRawResults.CITestResults = rawData
+
+	// Evaluate the probes.
+	findings, err := zrunner.Run(pRawResults, probes.CITests)
+	if err != nil {
+		e := sce.WithMessage(sce.ErrScorecardInternal, err.Error())
+		return checker.CreateRuntimeErrorResult(CheckCITests, e)
 	}
 
-	// Return the score evaluation.
-	return evaluation.CITests(CheckCITests, &rawData, c.Dlogger)
+	return evaluation.CITests(CheckCITests, findings, c.Dlogger)
 }
diff --git a/checks/evaluation/ci_tests.go b/checks/evaluation/ci_tests.go
@@ -16,122 +16,72 @@ package evaluation
 
 import (
 	"fmt"
-	"strings"
 
 	"github.com/ossf/scorecard/v4/checker"
+	sce "github.com/ossf/scorecard/v4/errors"
 	"github.com/ossf/scorecard/v4/finding"
+	"github.com/ossf/scorecard/v4/probes/testsRunInCI"
 )
 
-const (
-	// CheckCITests is the registered name for CITests.
-	CheckCITests = "CI-Tests"
-	success      = "success"
-)
-
-func CITests(_ string, c *checker.CITestData, dl checker.DetailLogger) checker.CheckResult {
-	totalMerged := 0
-	totalTested := 0
-	for i := range c.CIInfo {
-		r := c.CIInfo[i]
-		totalMerged++
-
-		var foundCI bool
-
-		// GitHub Statuses.
-		prSuccessStatus, err := prHasSuccessStatus(r, dl)
-		if err != nil {
-			return checker.CreateRuntimeErrorResult(CheckCITests, err)
-		}
-		if prSuccessStatus {
-			totalTested++
-			foundCI = true
-			continue
-		}
+const CheckCITests = "CI-Tests"
 
-		// GitHub Check Runs.
-		prCheckSuccessful, err := prHasSuccessfulCheck(r, dl)
-		if err != nil {
-			return checker.CreateRuntimeErrorResult(CheckCITests, err)
-		}
-		if prCheckSuccessful {
-			totalTested++
-			foundCI = true
-		}
-
-		if !foundCI {
-			// Log message says commit, but really we only care about PRs, and
-			// use only one commit (branch HEAD) to refer to all commits in a PR
+func CITests(name string,
+	findings []finding.Finding,
+	dl checker.DetailLogger,
+) checker.CheckResult {
+	expectedProbes := []string{
+		testsRunInCI.Probe,
+	}
+	if !finding.UniqueProbesEqual(findings, expectedProbes) {
+		e := sce.WithMessage(sce.ErrScorecardInternal, "invalid probe results")
+		return checker.CreateRuntimeErrorResult(name, e)
+	}
 
+	// Debug PRs that were merged without CI tests
+	for i := range findings {
+		f := &findings[i]
+		if f.Outcome == finding.OutcomeNegative || f.Outcome == finding.OutcomePositive {
 			dl.Debug(&checker.LogMessage{
-				Text: fmt.Sprintf("merged PR %d without CI test at HEAD: %s", r.PullRequestNumber, r.HeadSHA),
+				Text: f.Message,
 			})
 		}
 	}
 
-	if totalMerged == 0 {
+	// check that the project has pull requests
+	if noPullRequestsFound(findings) {
 		return checker.CreateInconclusiveResult(CheckCITests, "no pull request found")
 	}
 
+	totalMerged, totalTested := getMergedAndTested(findings)
+
+	if totalMerged < totalTested || len(findings) < totalTested {
+		e := sce.WithMessage(sce.ErrScorecardInternal, "invalid finding values")
+		return checker.CreateRuntimeErrorResult(name, e)
+	}
+
 	reason := fmt.Sprintf("%d out of %d merged PRs checked by a CI test", totalTested, totalMerged)
 	return checker.CreateProportionalScoreResult(CheckCITests, reason, totalTested, totalMerged)
 }
 
-// PR has a status marked 'success' and a CI-related context.
-//
-//nolint:unparam
-func prHasSuccessStatus(r checker.RevisionCIInfo, dl checker.DetailLogger) (bool, error) {
-	for _, status := range r.Statuses {
-		if status.State != success {
-			continue
-		}
-		if isTest(status.Context) || isTest(status.TargetURL) {
-			dl.Debug(&checker.LogMessage{
-				Path: status.URL,
-				Type: finding.FileTypeURL,
-				Text: fmt.Sprintf("CI test found: pr: %s, context: %s", r.HeadSHA,
-					status.Context),
-			})
-			return true, nil
-		}
-	}
-	return false, nil
-}
+func getMergedAndTested(findings []finding.Finding) (int, int) {
+	totalMerged := 0
+	totalTested := 0
 
-// PR has a successful CI-related check.
-//
-//nolint:unparam
-func prHasSuccessfulCheck(r checker.RevisionCIInfo, dl checker.DetailLogger) (bool, error) {
-	for _, cr := range r.CheckRuns {
-		if cr.Status != "completed" {
-			continue
-		}
-		if cr.Conclusion != success {
-			continue
-		}
-		if isTest(cr.App.Slug) {
-			dl.Debug(&checker.LogMessage{
-				Path: cr.URL,
-				Type: finding.FileTypeURL,
-				Text: fmt.Sprintf("CI test found: pr: %d, context: %s", r.PullRequestNumber,
-					cr.App.Slug),
-			})
-			return true, nil
+	for i := range findings {
+		f := &findings[i]
+		totalMerged++
+		if f.Outcome == finding.OutcomePositive {
+			totalTested++
 		}
 	}
-	return false, nil
-}
 
-// isTest returns true if the given string is a CI test.
-func isTest(s string) bool {
-	l := strings.ToLower(s)
+	return totalMerged, totalTested
+}
 
-	// Add more patterns here!
-	for _, pattern := range []string{
-		"appveyor", "buildkite", "circleci", "e2e", "github-actions", "jenkins",
-		"mergeable", "packit-as-a-service", "semaphoreci", "test", "travis-ci",
-		"flutter-dashboard", "Cirrus CI", "azure-pipelines",
-	} {
-		if strings.Contains(l, pattern) {
+func noPullRequestsFound(findings []finding.Finding) bool {
+	for i := range findings {
+		f := &findings[i]
+		if f.Outcome == finding.OutcomeNotApplicable {
 			return true
 		}
 	}