🌱 migrate code review check to probes (#3979)

* initial conversion Signed-off-by: Spencer Schrock <[email protected]> * appease the linter Signed-off-by: Spencer Schrock <[email protected]> * cleanup outcomes from positive/negative to true/false conversion Signed-off-by: Spencer Schrock <[email protected]> --------- Signed-off-by: Spencer Schrock <[email protected]>
ossf · Apr 10, 2024 · 8564191 · 8564191
1 parent 99a6dc4
commit 8564191
Show file tree

Hide file tree

Showing 8 changed files with 93 additions and 261 deletions.
diff --git a/checks/code_review.go b/checks/code_review.go
@@ -19,6 +19,8 @@ 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.
@@ -43,12 +45,19 @@ func CodeReview(c *checker.CheckRequest) checker.CheckResult {
 		return checker.CreateRuntimeErrorResult(CheckCodeReview, e)
 	}
 
-	// Return raw results.
-	if c.RawResults != nil {
-		c.RawResults.CodeReviewResults = rawData
+	// Set the raw results.
+	pRawResults := getRawResults(c)
+	pRawResults.CodeReviewResults = rawData
+
+	// Evaluate the probes.
+	findings, err := zrunner.Run(pRawResults, probes.CodeReview)
+	if err != nil {
+		e := sce.WithMessage(sce.ErrScorecardInternal, err.Error())
+		return checker.CreateRuntimeErrorResult(CheckCodeReview, e)
 	}
 
 	// Return the score evaluation.
-	// TODO(#3049)
-	return evaluation.CodeReview(CheckCodeReview, c.Dlogger, &rawData)
+	ret := evaluation.CodeReview(CheckCodeReview, findings, c.Dlogger)
+	ret.Findings = findings
+	return ret
 }
diff --git a/checks/code_review_test.go b/checks/code_review_test.go
@@ -140,8 +140,7 @@ func TestCodereview(t *testing.T) {
 				},
 			},
 			expected: scut.TestReturn{
-				Score:         0,
-				NumberOfDebug: 1, // one per un-reviewed change
+				Score: 0,
 			},
 		},
 		{
@@ -171,8 +170,7 @@ func TestCodereview(t *testing.T) {
 				},
 			},
 			expected: scut.TestReturn{
-				Score:         5,
-				NumberOfDebug: 1, // one per un-reviewed change
+				Score: 5,
 			},
 		},
 		{
@@ -197,8 +195,7 @@ func TestCodereview(t *testing.T) {
 				},
 			},
 			expected: scut.TestReturn{
-				Score:         5,
-				NumberOfDebug: 1, // one per un-reviewed change
+				Score: 5,
 			},
 		},
 		{
@@ -228,8 +225,7 @@ func TestCodereview(t *testing.T) {
 				},
 			},
 			expected: scut.TestReturn{
-				Score:         0,
-				NumberOfDebug: 1, // one per un-reviewed change
+				Score: 0,
 			},
 		},
 		{

diff --git a/checks/evaluation/code_review.go b/checks/evaluation/code_review.go
@@ -15,95 +15,49 @@
 package evaluation
 
 import (
-	"fmt"
+	"strconv"
 
 	"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/codeApproved"
 )
 
-type reviewScore int
-
 // TODO(raghavkaul) More partial credit? E.g. approval from non-contributor, discussion liveness,
 // number of resolved comments, number of approvers (more eyes on a project).
-const (
-	noReview              reviewScore = 0 // No approving review before merge
-	changesReviewed       reviewScore = 1 // Changes were reviewed
-	reviewedOutsideGithub reviewScore = 1 // Full marks until we can check review platforms outside of GitHub
-)
 
 // CodeReview applies the score policy for the Code-Review check.
-func CodeReview(name string, dl checker.DetailLogger, r *checker.CodeReviewData) checker.CheckResult {
-	if r == nil {
-		e := sce.WithMessage(sce.ErrScorecardInternal, "empty raw data")
-		return checker.CreateRuntimeErrorResult(name, e)
+func CodeReview(name string, findings []finding.Finding, dl checker.DetailLogger) checker.CheckResult {
+	expectedProbes := []string{
+		codeApproved.Probe,
 	}
 
-	N := len(r.DefaultBranchChangesets)
-	if N == 0 {
-		return checker.CreateInconclusiveResult(name, "no commits found")
-	}
-
-	nUnreviewedChanges := 0
-	nChanges := 0
-	foundHumanActivity := false
-
-	for i := range r.DefaultBranchChangesets {
-		cs := &r.DefaultBranchChangesets[i]
-		isReviewed := reviewScoreForChangeset(cs, dl) >= changesReviewed
-		if isReviewed && cs.Author.IsBot {
-			continue // ignore reviewed bot commits (https://github.com/ossf/scorecard/issues/2450)
-		}
-
-		nChanges += 1
-
-		if !cs.Author.IsBot {
-			foundHumanActivity = true
-		}
-
-		if !isReviewed {
-			nUnreviewedChanges += 1
-		}
-	}
-
-	switch {
-	case nChanges == 0 || !foundHumanActivity:
-		reason := fmt.Sprintf("found no human review activity in the last %v changesets", N)
-		return checker.CreateInconclusiveResult(name, reason)
-	case nUnreviewedChanges > 0:
-		return checker.CreateProportionalScoreResult(
-			name,
-			fmt.Sprintf("found %d unreviewed changesets out of %d", nUnreviewedChanges, nChanges),
-			max(nChanges-nUnreviewedChanges, 0),
-			nChanges,
-		)
-	}
-
-	return checker.CreateMaxScoreResult(name, "all changesets reviewed")
-}
-
-func reviewScoreForChangeset(changeset *checker.Changeset, dl checker.DetailLogger) (score reviewScore) {
-	plat := changeset.ReviewPlatform
-	if plat != checker.ReviewPlatformUnknown &&
-		plat != checker.ReviewPlatformGitHub {
-		return reviewedOutsideGithub
+	if !finding.UniqueProbesEqual(findings, expectedProbes) {
+		e := sce.WithMessage(sce.ErrScorecardInternal, "invalid probe results")
+		return checker.CreateRuntimeErrorResult(name, e)
 	}
 
-	if plat == checker.ReviewPlatformGitHub {
-		for i := range changeset.Reviews {
-			review := changeset.Reviews[i]
-			if review.State == "APPROVED" && review.Author.Login != changeset.Author.Login {
-				return changesReviewed
+	for _, f := range findings {
+		switch f.Outcome {
+		case finding.OutcomeNotApplicable:
+			return checker.CreateInconclusiveResult(name, f.Message)
+		case finding.OutcomeTrue:
+			return checker.CreateMaxScoreResult(name, "all changesets reviewed")
+		case finding.OutcomeError:
+			return checker.CreateRuntimeErrorResult(name, sce.WithMessage(sce.ErrScorecardInternal, f.Message))
+		default:
+			approved, err := strconv.Atoi(f.Values[codeApproved.NumApprovedKey])
+			if err != nil {
+				err = sce.WithMessage(sce.ErrScorecardInternal, "converting approved count: %v")
+				return checker.CreateRuntimeErrorResult(name, err)
+			}
+			total, err := strconv.Atoi(f.Values[codeApproved.NumTotalKey])
+			if err != nil {
+				err = sce.WithMessage(sce.ErrScorecardInternal, "converting total count: %v")
+				return checker.CreateRuntimeErrorResult(name, err)
 			}
+			return checker.CreateProportionalScoreResult(name, f.Message, approved, total)
 		}
 	}
-
-	dl.Debug(
-		&checker.LogMessage{
-			Text: fmt.Sprintf(
-				"couldn't find approvals for revision: %s platform: %s",
-				changeset.RevisionID, plat,
-			),
-		},
-	)
-	return noReview
+	return checker.CreateMaxScoreResult(name, "all changesets reviewed")
 }