🐛 Code Review: Use proportional scoring

checker/raw_result.go

@@ -176,6 +176,7 @@ const (
 	ReviewPlatformGerrit      ReviewPlatform = "Gerrit"
 	ReviewPlatformPhabricator ReviewPlatform = "Phabricator"
 	ReviewPlatformPiper       ReviewPlatform = "Piper"
+	ReviewPlatformUnknown     ReviewPlatform = "Unknown"
 )
 
 type Changeset struct {

checks/code_review_test.go

@@ -186,7 +186,7 @@ func TestCodereview(t *testing.T) {
 				},
 			},
 			expected: checker.CheckResult{
-				Score: 3,
+				Score: 5,
 			},
 		},
 		{
@@ -211,7 +211,7 @@ func TestCodereview(t *testing.T) {
 				},
 			},
 			expected: checker.CheckResult{
-				Score: 3,
+				Score: 5,
 			},
 		},
 		{

checks/evaluation/code_review.go

@@ -16,7 +16,7 @@ package evaluation
 
 import (
 	"fmt"
-	"strings"
+	"math"
 
 	"github.com/ossf/scorecard/v4/checker"
 	sce "github.com/ossf/scorecard/v4/errors"
@@ -39,89 +39,60 @@ func CodeReview(name string, dl checker.DetailLogger, r *checker.CodeReviewData)
 		return checker.CreateRuntimeErrorResult(name, e)
 	}
 
-	if len(r.DefaultBranchChangesets) == 0 {
+	N := len(r.DefaultBranchChangesets)
+	if N == 0 {
 		return checker.CreateInconclusiveResult(name, "no commits found")
 	}
 
-	foundHumanReviewActivity := false
-	foundBotReviewActivity := false
-	nUnreviewedHumanChanges := 0
-	nUnreviewedBotChanges := 0
+	nUnreviewedChanges := 0
+	nChanges := 0
+	foundHumanActivity := false
 
-	var unreviewedHumanRevisionIDs, unreviewedBotRevisionIDs []string
 	for i := range r.DefaultBranchChangesets {
 		cs := &r.DefaultBranchChangesets[i]
-		isReviewed := reviewScoreForChangeset(cs) >= changesReviewed
-		isBotCommit := cs.Author.IsBot
-
-		switch {
-		case isReviewed && isBotCommit:
-			foundBotReviewActivity = true
-		case isReviewed && !isBotCommit:
-			foundHumanReviewActivity = true
-		case !isReviewed && isBotCommit:
-			nUnreviewedBotChanges += 1
-			unreviewedBotRevisionIDs = append(unreviewedBotRevisionIDs, cs.RevisionID)
-		case !isReviewed && !isBotCommit:
-			nUnreviewedHumanChanges += 1
-			unreviewedHumanRevisionIDs = append(unreviewedHumanRevisionIDs, cs.RevisionID)
+		isReviewed := reviewScoreForChangeset(cs, dl) >= changesReviewed
+		if isReviewed && cs.Author.IsBot {
+			continue // ignore reviewed bot commits
 		}
-	}
 
-	// Let's include non-compliant revision IDs in details
-	if len(unreviewedHumanRevisionIDs) > 0 {
-		dl.Debug(&checker.LogMessage{
-			Text: fmt.Sprintf("List of revision IDs not reviewed by humans:%s", strings.Join(unreviewedHumanRevisionIDs, ",")),
-		})
-	}
+		nChanges += 1
 
-	if len(unreviewedBotRevisionIDs) > 0 {
-		dl.Debug(&checker.LogMessage{
-			Text: fmt.Sprintf("List of bot revision IDs not reviewed by humans:%s", strings.Join(unreviewedBotRevisionIDs, ",")),
-		})
-	}
+		if !cs.Author.IsBot {
+			foundHumanActivity = true
+		}
 
-	if foundBotReviewActivity && !foundHumanReviewActivity {
-		reason := fmt.Sprintf("found no human review activity in the last %v changesets", len(r.DefaultBranchChangesets))
-		return checker.CreateInconclusiveResult(name, reason)
+		if !isReviewed {
+			nUnreviewedChanges += 1
+		}
 	}
 
 	switch {
-	case nUnreviewedBotChanges > 0 && nUnreviewedHumanChanges > 0:
-		return checker.CreateMinScoreResult(
-			name,
-			fmt.Sprintf("found unreviewed changesets (%v human %v bot)", nUnreviewedHumanChanges, nUnreviewedBotChanges),
-		)
-	case nUnreviewedBotChanges > 0:
-		return checker.CreateResultWithScore(
-			name,
-			fmt.Sprintf("all human changesets reviewed, found %v unreviewed bot changesets", nUnreviewedBotChanges),
-			7,
-		)
-	case nUnreviewedHumanChanges > 0:
-		score := 3
-		if len(r.DefaultBranchChangesets) == 1 || nUnreviewedHumanChanges > 1 {
-			score = 0
-		}
-		return checker.CreateResultWithScore(
+	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 %v unreviewed human changesets (%d total)",
-				nUnreviewedHumanChanges, len(r.DefaultBranchChangesets),
+				"found %d unreviewed changesets out of %d", nUnreviewedChanges,
+				nChanges,
 			),
-			score,
+			int(math.Max(float64(nChanges-nUnreviewedChanges), 0)),
+			nChanges,
 		)
 	}
 
 	return checker.CreateMaxScoreResult(name, "all changesets reviewed")
 }
 
-func reviewScoreForChangeset(changeset *checker.Changeset) (score reviewScore) {
-	if changeset.ReviewPlatform != "" && changeset.ReviewPlatform != checker.ReviewPlatformGitHub {
+func reviewScoreForChangeset(changeset *checker.Changeset, dl checker.DetailLogger) (score reviewScore) {
+	plat := changeset.ReviewPlatform
+	if plat != checker.ReviewPlatformUnknown &&
+		plat != checker.ReviewPlatformGitHub {
 		return reviewedOutsideGithub
 	}
 
-	if changeset.ReviewPlatform == checker.ReviewPlatformGitHub {
+	if plat == checker.ReviewPlatformGitHub {
 		for i := range changeset.Reviews {
 			review := changeset.Reviews[i]
 			if review.State == "APPROVED" && review.Author.Login != changeset.Author.Login {
@@ -130,5 +101,13 @@ func reviewScoreForChangeset(changeset *checker.Changeset) (score reviewScore) {
 		}
 	}
 
+	dl.Debug(
+		&checker.LogMessage{
+			Text: fmt.Sprintf(
+				"couldn't find approvals for revision: %s platform: %s",
+				changeset.RevisionID, plat,
+			),
+		},
+	)
 	return noReview
 }

checks/evaluation/code_review_test.go

@@ -51,15 +51,17 @@ func TestCodeReview(t *testing.T) {
 			name: "NoReviews",
 			expected: scut.TestReturn{
 				Score:         checker.MinResultScore,
-				NumberOfDebug: 1,
+				NumberOfDebug: 2,
 			},
 			rawData: &checker.CodeReviewData{
 				DefaultBranchChangesets: []checker.Changeset{
 					{
-						Commits: []clients.Commit{{SHA: "a"}},
+						ReviewPlatform: checker.ReviewPlatformUnknown,
+						Commits:        []clients.Commit{{SHA: "a"}},
 					},
 					{
-						Commits: []clients.Commit{{SHA: "a"}},
+						ReviewPlatform: checker.ReviewPlatformUnknown,
+						Commits:        []clients.Commit{{SHA: "a"}},
 					},
 				},
 			},
@@ -68,23 +70,25 @@ func TestCodeReview(t *testing.T) {
 			name: "Unreviewed human and bot changes",
 			expected: scut.TestReturn{
 				Score:         checker.MinResultScore,
-				NumberOfDebug: 1,
+				NumberOfDebug: 2,
 			},
 			rawData: &checker.CodeReviewData{
 				DefaultBranchChangesets: []checker.Changeset{
 					{
-						Commits: []clients.Commit{{SHA: "a", Committer: clients.User{IsBot: true}}},
+						ReviewPlatform: checker.ReviewPlatformUnknown,
+						Commits:        []clients.Commit{{SHA: "a", Committer: clients.User{IsBot: true}}},
 					},
 					{
-						Commits: []clients.Commit{{SHA: "b"}},
+						ReviewPlatform: checker.ReviewPlatformUnknown,
+						Commits:        []clients.Commit{{SHA: "b"}},
 					},
 				},
 			},
 		},
 		{
 			name: "all human changesets reviewed, missing review on bot changeset",
 			expected: scut.TestReturn{
-				Score:         7,
+				Score:         5,
 				NumberOfDebug: 1,
 			},
 			rawData: &checker.CodeReviewData{
@@ -107,8 +111,9 @@ func TestCodeReview(t *testing.T) {
 						},
 					},
 					{
-						Author:     clients.User{Login: "alice-the-bot[bot]", IsBot: true},
-						RevisionID: "b",
+						Author:         clients.User{Login: "alice-the-bot[bot]", IsBot: true},
+						ReviewPlatform: checker.ReviewPlatformUnknown,
+						RevisionID:     "b",
 						Commits: []clients.Commit{
 							{
 								Committer: clients.User{Login: "alice-the-bot[bot]", IsBot: true},
@@ -173,7 +178,9 @@ func TestCodeReview(t *testing.T) {
 						},
 					},
 					{
-						RevisionID: "b",
+						Author:         clients.User{Login: "alice-the-bot[bot]", IsBot: true},
+						ReviewPlatform: checker.ReviewPlatformUnknown,
+						RevisionID:     "b",
 						Commits: []clients.Commit{
 							{
 								Committer: clients.User{Login: "alice-the-bot[bot]", IsBot: true},

checks/raw/code_review.go

@@ -141,7 +141,7 @@ func detectCommitRevisionInfo(c *clients.Commit) revisionInfo {
 		return revisionInfo{checker.ReviewPlatformPiper, revisionID}
 	}
 
-	return revisionInfo{}
+	return revisionInfo{checker.ReviewPlatformUnknown, ""}
 }
 
 // Group commits by the changeset they belong to

e2e/code_review_test.go

@@ -32,34 +32,9 @@ import (
 // TODO: use dedicated repo that don't change.
 var _ = Describe("E2E TEST:"+checks.CheckCodeReview, func() {
 	Context("E2E TEST:Validating use of code reviews", func() {
-		It("Should return use of code reviews", func() {
-			dl := scut.TestDetailLogger{}
-			repo, err := githubrepo.MakeGithubRepo("ossf-tests/airflow")
-			Expect(err).Should(BeNil())
-			repoClient := githubrepo.CreateGithubRepoClient(context.Background(), logger)
-			err = repoClient.InitRepo(repo, clients.HeadSHA, 0)
-			Expect(err).Should(BeNil())
-
-			req := checker.CheckRequest{
-				Ctx:        context.Background(),
-				RepoClient: repoClient,
-				Repo:       repo,
-				Dlogger:    &dl,
-			}
-			expected := scut.TestReturn{
-				Error:         nil,
-				Score:         checker.MinResultScore,
-				NumberOfWarn:  0,
-				NumberOfInfo:  0,
-				NumberOfDebug: 1,
-			}
-			result := checks.CodeReview(&req)
-			Expect(scut.ValidateTestReturn(nil, "use code reviews", &expected, &result, &dl)).Should(BeTrue())
-			Expect(repoClient.Close()).Should(BeNil())
-		})
 		It("Should return use of code reviews at commit", func() {
 			dl := scut.TestDetailLogger{}
-			repo, err := githubrepo.MakeGithubRepo("ossf-tests/airflow")
+			repo, err := githubrepo.MakeGithubRepo("apache/airflow")
 			Expect(err).Should(BeNil())
 			repoClient := githubrepo.CreateGithubRepoClient(context.Background(), logger)
 			err = repoClient.InitRepo(repo, "0a6850647e531b08f68118ff8ca20577a5b4062c", 0)
@@ -73,10 +48,10 @@ var _ = Describe("E2E TEST:"+checks.CheckCodeReview, func() {
 			}
 			expected := scut.TestReturn{
 				Error:         nil,
-				Score:         checker.MinResultScore,
+				Score:         10,
 				NumberOfWarn:  0,
 				NumberOfInfo:  0,
-				NumberOfDebug: 1,
+				NumberOfDebug: 0,
 			}
 			result := checks.CodeReview(&req)
 			Expect(scut.ValidateTestReturn(nil, "use code reviews", &expected, &result, &dl)).Should(BeTrue())
@@ -103,7 +78,7 @@ var _ = Describe("E2E TEST:"+checks.CheckCodeReview, func() {
 
 			Expect(repoClient.Close()).Should(BeNil())
 		})
-		It("Should return inconclusive results for a single-maintainer project with only self- or bot changesets", func() {
+		It("Should return min score for a single-maintainer project with only self- or bot changesets", func() {
 			dl := scut.TestDetailLogger{}
 			repo, err := githubrepo.MakeGithubRepo("Kromey/fast_poisson")
 			Expect(err).Should(BeNil())
@@ -118,14 +93,14 @@ var _ = Describe("E2E TEST:"+checks.CheckCodeReview, func() {
 				Dlogger:    &dl,
 			}
 			expected := scut.TestReturn{
-				Score:         checker.InconclusiveResultScore,
-				NumberOfDebug: 1,
+				Score:         0,
+				NumberOfDebug: 18,
 			}
 			result := checks.CodeReview(&req)
 			Expect(scut.ValidateTestReturn(nil, "use code reviews", &expected, &result, &dl)).Should(BeTrue())
 			Expect(repoClient.Close()).Should(BeNil())
 		})
-		It("Should return minimum score for a single-maintainer project with some unreviewed human changesets", func() {
+		It("Should return partial score for a single-maintainer project with some unreviewed human changesets", func() {
 			dl := scut.TestDetailLogger{}
 			repo, err := githubrepo.MakeGithubRepo("Kromey/fast_poisson")
 			Expect(err).Should(BeNil())
@@ -140,8 +115,8 @@ var _ = Describe("E2E TEST:"+checks.CheckCodeReview, func() {
 				Dlogger:    &dl,
 			}
 			expected := scut.TestReturn{
-				Score:         checker.MinResultScore,
-				NumberOfDebug: 1,
+				Score:         1,
+				NumberOfDebug: 10,
 			}
 			result := checks.CodeReview(&req)
 			Expect(scut.ValidateTestReturn(nil, "use code reviews", &expected, &result, &dl)).Should(BeTrue())