🌱 Feature: Group commits into changesets (ossf#2260)

* Group raw commits into changesets Signed-off-by: Raghav Kaul <[email protected]> * Add tests, fix golint Signed-off-by: Raghav Kaul <[email protected]> * Fix lint Signed-off-by: Raghav Kaul <[email protected]> * Address PR comments Signed-off-by: Raghav Kaul <[email protected]> * Fix test failures, remove unneeded fields from raw results Signed-off-by: Raghav Kaul <[email protected]> * Fix lint Signed-off-by: Raghav Kaul <[email protected]> * Fix tests * Handle randomized order * e2e Signed-off-by: Raghav Kaul <[email protected]> * Accept code reviews on any commit, not just HEAD Signed-off-by: Raghav Kaul <[email protected]> * Address PR comments Signed-off-by: Raghav Kaul <[email protected]> Signed-off-by: Raghav Kaul <[email protected]> Co-authored-by: Azeem Shaikh <[email protected]> Signed-off-by: nathaniel.wert <[email protected]>
N8BWert · Nov 28, 2022 · bbd557e · bbd557e
1 parent 5070698
commit bbd557e
Show file tree

Hide file tree

Showing 11 changed files with 620 additions and 332 deletions.
diff --git a/checker/raw_result.go b/checker/raw_result.go
@@ -122,7 +122,22 @@ type LicenseData struct {
 // CodeReviewData contains the raw results
 // for the Code-Review check.
 type CodeReviewData struct {
-	DefaultBranchCommits []clients.Commit
+	DefaultBranchChangesets []Changeset
+}
+type ReviewPlatform = string
+
+const (
+	ReviewPlatformGitHub      ReviewPlatform = "GitHub"
+	ReviewPlatformProw        ReviewPlatform = "Prow"
+	ReviewPlatformGerrit      ReviewPlatform = "Gerrit"
+	ReviewPlatformPhabricator ReviewPlatform = "Phabricator"
+	ReviewPlatformPiper       ReviewPlatform = "Piper"
+)
+
+type Changeset struct {
+	ReviewPlatform string
+	RevisionID     string
+	Commits        []clients.Commit
 }
 
 // ContributorsData represents contributor information.

diff --git a/checks/code_review_test.go b/checks/code_review_test.go
@@ -229,7 +229,7 @@ func TestCodereview(t *testing.T) {
 				},
 			},
 			expected: checker.CheckResult{
-				Score: 0,
+				Score: checker.MaxResultScore,
 			},
 		},
 		{

diff --git a/checks/evaluation/code_review.go b/checks/evaluation/code_review.go
@@ -16,210 +16,59 @@ package evaluation
 
 import (
 	"fmt"
-	"strings"
 
 	"github.com/ossf/scorecard/v4/checker"
-	"github.com/ossf/scorecard/v4/clients"
 	sce "github.com/ossf/scorecard/v4/errors"
 )
 
+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 (
-	reviewPlatformGitHub      = "GitHub"
-	reviewPlatformProw        = "Prow"
-	reviewPlatformGerrit      = "Gerrit"
-	reviewPlatformPhabricator = "Phabricator"
-	reviewPlatformPiper       = "Piper"
+	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 {
+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)
 	}
 
-	if len(r.DefaultBranchCommits) == 0 {
+	if len(r.DefaultBranchChangesets) == 0 {
 		return checker.CreateInconclusiveResult(name, "no commits found")
 	}
 
-	totalReviewed := map[string]int{
-		reviewPlatformGitHub:      0,
-		reviewPlatformProw:        0,
-		reviewPlatformGerrit:      0,
-		reviewPlatformPhabricator: 0,
-		reviewPlatformPiper:       0,
-	}
-
-	for i := range r.DefaultBranchCommits {
-		commit := r.DefaultBranchCommits[i]
-
-		rs := getApprovedReviewSystem(&commit, dl)
-		if rs == "" {
-			dl.Warn(&checker.LogMessage{
-				Text: fmt.Sprintf("no reviews found for commit: %s", commit.SHA),
-			})
-			continue
+	score := 0
+	numReviewed := 0
+	for i := range r.DefaultBranchChangesets {
+		score += reviewScoreForChangeset(&r.DefaultBranchChangesets[i])
+		if score >= changesReviewed {
+			numReviewed += 1
 		}
-
-		totalReviewed[rs]++
 	}
+	reason := fmt.Sprintf(
+		"%v out of last %v changesets reviewed before merge", numReviewed, len(r.DefaultBranchChangesets),
+	)
 
-	if totalReviewed[reviewPlatformGitHub] == 0 &&
-		totalReviewed[reviewPlatformGerrit] == 0 &&
-		totalReviewed[reviewPlatformProw] == 0 &&
-		totalReviewed[reviewPlatformPhabricator] == 0 && totalReviewed[reviewPlatformPiper] == 0 {
-		return checker.CreateMinScoreResult(name, "no reviews found")
-	}
-
-	totalCommits := len(r.DefaultBranchCommits)
-	// Consider a single review system.
-	nbReviews, reviewSystem := computeReviews(totalReviewed)
-	if nbReviews == totalCommits {
-		return checker.CreateMaxScoreResult(name,
-			fmt.Sprintf("all last %v commits are reviewed through %s", totalCommits, reviewSystem))
-	}
-
-	reason := fmt.Sprintf("%s code reviews found for %v commits out of the last %v", reviewSystem, nbReviews, totalCommits)
-	return checker.CreateProportionalScoreResult(name, reason, nbReviews, totalCommits)
+	return checker.CreateProportionalScoreResult(name, reason, score, len(r.DefaultBranchChangesets))
 }
 
-func computeReviews(m map[string]int) (int, string) {
-	n := 0
-	s := ""
-	for k, v := range m {
-		if v > n {
-			n = v
-			s = k
-		}
+func reviewScoreForChangeset(changeset *checker.Changeset) (score reviewScore) {
+	if changeset.ReviewPlatform != "" && changeset.ReviewPlatform != checker.ReviewPlatformGitHub {
+		return reviewedOutsideGithub
 	}
-	return n, s
-}
 
-func isBot(name string) bool {
-	for _, substring := range []string{"bot", "gardener"} {
-		if strings.Contains(name, substring) {
-			return true
-		}
-	}
-	return false
-}
-
-func getApprovedReviewSystem(c *clients.Commit, dl checker.DetailLogger) string {
-	switch {
-	case isReviewedOnGitHub(c, dl):
-		return reviewPlatformGitHub
-	case isReviewedOnProw(c, dl):
-		return reviewPlatformProw
-	case isReviewedOnGerrit(c, dl):
-		return reviewPlatformGerrit
-	case isReviewedOnPhabricator(c, dl):
-		return reviewPlatformPhabricator
-	case isReviewedOnPiper(c, dl):
-		return reviewPlatformPiper
-	}
-	return ""
-}
-
-func isReviewedOnGitHub(c *clients.Commit, dl checker.DetailLogger) bool {
-	mr := c.AssociatedMergeRequest
-	if mr.MergedAt.IsZero() {
-		return false
-	}
-
-	for _, r := range mr.Reviews {
-		if r.State == "APPROVED" {
-			dl.Debug(&checker.LogMessage{
-				Text: fmt.Sprintf("commit %s was reviewed through %s #%d approved merge request",
-					c.SHA, reviewPlatformGitHub, mr.Number),
-			})
-			return true
-		}
-	}
-
-	// Check if the merge request is committed by someone other than author. This is kind
-	// of equivalent to a review and is done several times on small prs to save
-	// time on clicking the approve button.
-	if c.Committer.Login != "" &&
-		c.Committer.Login != mr.Author.Login {
-		dl.Debug(&checker.LogMessage{
-			Text: fmt.Sprintf("commit %s was reviewed through %s #%d merge request",
-				c.SHA, reviewPlatformGitHub, mr.Number),
-		})
-		return true
-	}
-
-	return false
-}
-
-func isReviewedOnProw(c *clients.Commit, dl checker.DetailLogger) bool {
-	if isBot(c.Committer.Login) {
-		dl.Debug(&checker.LogMessage{
-			Text: fmt.Sprintf("skip commit %s from bot account: %s", c.SHA, c.Committer.Login),
-		})
-		return true
-	}
-
-	if !c.AssociatedMergeRequest.MergedAt.IsZero() {
-		for _, l := range c.AssociatedMergeRequest.Labels {
-			if l.Name == "lgtm" || l.Name == "approved" {
-				dl.Debug(&checker.LogMessage{
-					Text: fmt.Sprintf("commit %s review was through %s #%d approved merge request",
-						c.SHA, reviewPlatformProw, c.AssociatedMergeRequest.Number),
-				})
-				return true
+	for i := range changeset.Commits {
+		for _, review := range changeset.Commits[i].AssociatedMergeRequest.Reviews {
+			if review.State == "APPROVED" {
+				return changesReviewed
 			}
 		}
 	}
-	return false
-}
-
-func isReviewedOnGerrit(c *clients.Commit, dl checker.DetailLogger) bool {
-	if isBot(c.Committer.Login) {
-		dl.Debug(&checker.LogMessage{
-			Text: fmt.Sprintf("skip commit %s from bot account: %s", c.SHA, c.Committer.Login),
-		})
-		return true
-	}
-
-	m := c.Message
-	if strings.Contains(m, "\nReviewed-on: ") &&
-		strings.Contains(m, "\nReviewed-by: ") {
-		dl.Debug(&checker.LogMessage{
-			Text: fmt.Sprintf("commit %s was approved through %s", c.SHA, reviewPlatformGerrit),
-		})
-		return true
-	}
-	return false
-}
-
-func isReviewedOnPhabricator(c *clients.Commit, dl checker.DetailLogger) bool {
-	if isBot(c.Committer.Login) {
-		dl.Debug(&checker.LogMessage{
-			Text: fmt.Sprintf("skip commit %s from bot account: %s", c.SHA, c.Committer.Login),
-		})
-		return true
-	}
 
-	m := c.Message
-	if strings.Contains(m, "\nDifferential Revision: ") &&
-		strings.Contains(m, "\nReviewed By: ") {
-		dl.Debug(&checker.LogMessage{
-			Text: fmt.Sprintf("commit %s was approved through %s", c.SHA, reviewPlatformPhabricator),
-		})
-		return true
-	}
-	return false
-}
-
-func isReviewedOnPiper(c *clients.Commit, dl checker.DetailLogger) bool {
-	m := c.Message
-	if strings.Contains(m, "\nPiperOrigin-RevId: ") {
-		dl.Debug(&checker.LogMessage{
-			Text: fmt.Sprintf("commit %s was approved through %s", c.SHA, reviewPlatformPiper),
-		})
-		return true
-	}
-	return false
+	return noReview
 }
diff --git a/checks/evaluation/code_review_test.go b/checks/evaluation/code_review_test.go
@@ -50,16 +50,15 @@ func TestCodeReview(t *testing.T) {
 		{
 			name: "NoReviews",
 			expected: scut.TestReturn{
-				Score:        checker.MinResultScore,
-				NumberOfWarn: 2,
+				Score: checker.MinResultScore,
 			},
 			rawData: &checker.CodeReviewData{
-				DefaultBranchCommits: []clients.Commit{
+				DefaultBranchChangesets: []checker.Changeset{
 					{
-						SHA: "1",
+						Commits: []clients.Commit{{SHA: "1"}},
 					},
 					{
-						SHA: "2",
+						Commits: []clients.Commit{{SHA: "1"}},
 					},
 				},
 			},
-Original file line number
+Diff line change
@@ Expand Up / @@ -229,7 +229,7 @@ func TestCodereview(t *testing.T) { @@
     				},
     			},
     			expected: checker.CheckResult{
-    				Score: 0,
+    				Score: checker.MaxResultScore,
     			},
     		},
     		{
@@ Expand Down @@