Fix for repos which do not squash PR commits (#1637)

Co-authored-by: Azeem Shaikh <azeems@google.com>

Author: azeemshaikh38

checks/evaluation/code_review.go

@@ -36,7 +36,10 @@ func CodeReview(name string, dl checker.DetailLogger,
 		return checker.CreateRuntimeErrorResult(name, e)
 	}
 
-	totalCommits := 0
+	if len(r.DefaultBranchCommits) == 0 {
+		return checker.CreateInconclusiveResult(name, "no commits found")
+	}
+
 	totalReviewed := map[string]int{
 		// The 3 platforms we support.
 		reviewPlatformGitHub: 0,
@@ -47,37 +50,25 @@ func CodeReview(name string, dl checker.DetailLogger,
 	for i := range r.DefaultBranchCommits {
 		commit := r.DefaultBranchCommits[i]
 
-		// New commit to consider.
-		totalCommits++
-
 		rs := getApprovedReviewSystem(&commit, dl)
-		// No commits.
 		if rs == "" {
+			dl.Warn(&checker.LogMessage{
+				Text:    fmt.Sprintf("no reviews found for commit: %s", commit.SHA),
+				Version: 3,
+			})
 			continue
 		}
 
 		totalReviewed[rs]++
 	}
 
-	if totalCommits == 0 {
-		return checker.CreateInconclusiveResult(name, "no commits found")
-	}
-
 	if totalReviewed[reviewPlatformGitHub] == 0 &&
 		totalReviewed[reviewPlatformGerrit] == 0 &&
 		totalReviewed[reviewPlatformProw] == 0 {
-		// Only show all warnings if all fail.
-		// We should not show warning if at least one succeeds, as this is confusing.
-		for k := range totalReviewed {
-			dl.Warn(&checker.LogMessage{
-				Text:    fmt.Sprintf("no %s reviews found", k),
-				Version: 3,
-			})
-		}
-
 		return checker.CreateMinScoreResult(name, "no reviews found")
 	}
 
+	totalCommits := len(r.DefaultBranchCommits)
 	// Consider a single review system.
 	nbReviews, reviewSystem := computeReviews(totalReviewed)
 	if nbReviews == totalCommits {
@@ -134,8 +125,8 @@ func isReviewedOnGitHub(c *checker.DefaultBranchCommit, dl checker.DetailLogger)
 	for _, r := range mr.Reviews {
 		if r.State == "APPROVED" {
 			dl.Debug(&checker.LogMessage{
-				Text: fmt.Sprintf("%s #%d merge request approved",
-					reviewPlatformGitHub, mr.Number),
+				Text: fmt.Sprintf("commit %s was reviewed through %s #%d approved merge request",
+					c.SHA, reviewPlatformGitHub, mr.Number),
 				Version: 3,
 			})
 			return true
@@ -148,8 +139,8 @@ func isReviewedOnGitHub(c *checker.DefaultBranchCommit, dl checker.DetailLogger)
 	if c.Committer.Login != "" &&
 		c.Committer.Login != mr.Author.Login {
 		dl.Debug(&checker.LogMessage{
-			Text: fmt.Sprintf("%s #%d merge request approved",
-				reviewPlatformGitHub, mr.Number),
+			Text: fmt.Sprintf("commit %s was reviewed through %s #%d merge request",
+				c.SHA, reviewPlatformGitHub, mr.Number),
 			Version: 3,
 		})
 		return true
@@ -161,7 +152,7 @@ func isReviewedOnGitHub(c *checker.DefaultBranchCommit, dl checker.DetailLogger)
 func isReviewedOnProw(c *checker.DefaultBranchCommit, dl checker.DetailLogger) bool {
 	if isBot(c.Committer.Login) {
 		dl.Debug(&checker.LogMessage{
-			Text:    fmt.Sprintf("skip commit from bot account: %s", c.Committer.Login),
+			Text:    fmt.Sprintf("skip commit %s from bot account: %s", c.SHA, c.Committer.Login),
 			Version: 3,
 		})
 		return true
@@ -171,8 +162,8 @@ func isReviewedOnProw(c *checker.DefaultBranchCommit, dl checker.DetailLogger) b
 		for _, l := range c.MergeRequest.Labels {
 			if l == "lgtm" || l == "approved" {
 				dl.Debug(&checker.LogMessage{
-					Text: fmt.Sprintf("%s #%d merge request approved",
-						reviewPlatformProw, c.MergeRequest.Number),
+					Text: fmt.Sprintf("commit %s review was through %s #%d approved merge request",
+						c.SHA, reviewPlatformProw, c.MergeRequest.Number),
 					Version: 3,
 				})
 				return true
@@ -185,7 +176,7 @@ func isReviewedOnProw(c *checker.DefaultBranchCommit, dl checker.DetailLogger) b
 func isReviewedOnGerrit(c *checker.DefaultBranchCommit, dl checker.DetailLogger) bool {
 	if isBot(c.Committer.Login) {
 		dl.Debug(&checker.LogMessage{
-			Text:    fmt.Sprintf("skip commit from bot account: %s", c.Committer.Login),
+			Text:    fmt.Sprintf("skip commit %s from bot account: %s", c.SHA, c.Committer.Login),
 			Version: 3,
 		})
 		return true
@@ -195,7 +186,7 @@ func isReviewedOnGerrit(c *checker.DefaultBranchCommit, dl checker.DetailLogger)
 	if strings.Contains(m, "\nReviewed-on: ") &&
 		strings.Contains(m, "\nReviewed-by: ") {
 		dl.Debug(&checker.LogMessage{
-			Text:    fmt.Sprintf("%s commit approved", reviewPlatformGerrit),
+			Text:    fmt.Sprintf("commit %s was approved through %s", c.SHA, reviewPlatformGerrit),
 			Version: 3,
 		})
 		return true

checks/evaluation/code_review_test.go

@@ -0,0 +1,80 @@
+// Copyright 2021 Security Scorecard Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package evaluation
+
+import (
+	"testing"
+
+	"github.com/ossf/scorecard/v4/checker"
+	sce "github.com/ossf/scorecard/v4/errors"
+	scut "github.com/ossf/scorecard/v4/utests"
+)
+
+func TestCodeReview(t *testing.T) {
+	t.Parallel()
+
+	// nolint:govet // ignore since this is a test.
+	tests := []struct {
+		name     string
+		expected scut.TestReturn
+		rawData  *checker.CodeReviewData
+	}{
+		{
+			name: "NullRawData",
+			expected: scut.TestReturn{
+				Error: sce.ErrScorecardInternal,
+				Score: checker.InconclusiveResultScore,
+			},
+			rawData: nil,
+		},
+		{
+			name: "NoCommits",
+			expected: scut.TestReturn{
+				Score: checker.InconclusiveResultScore,
+			},
+			rawData: &checker.CodeReviewData{},
+		},
+		{
+			name: "NoReviews",
+			expected: scut.TestReturn{
+				Score:        checker.MinResultScore,
+				NumberOfWarn: 2,
+			},
+			rawData: &checker.CodeReviewData{
+				DefaultBranchCommits: []checker.DefaultBranchCommit{
+					{
+						SHA: "1",
+					},
+					{
+						SHA: "2",
+					},
+				},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt // Re-initializing variable so it is not changed while executing the closure below
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			dl := &scut.TestDetailLogger{}
+			res := CodeReview(tt.name, dl, tt.rawData)
+			if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &res, dl) {
+				t.Error()
+			}
+		})
+	}
+}

clients/githubrepo/graphql.go

@@ -69,15 +69,10 @@ type graphqlData struct {
 								Author struct {
 									Login githubv4.String
 								}
-								Number      githubv4.Int
-								HeadRefOid  githubv4.String
-								MergedAt    githubv4.DateTime
-								MergeCommit struct {
-									// NOTE: only used for sanity check.
-									// Use original commit oid instead.
-									Oid githubv4.GitObjectID
-								}
-								Labels struct {
+								Number     githubv4.Int
+								HeadRefOid githubv4.String
+								MergedAt   githubv4.DateTime
+								Labels     struct {
 									Nodes []struct {
 										Name githubv4.String
 									}
@@ -205,8 +200,10 @@ func commitsFrom(data *graphqlData, repoOwner, repoName string) ([]clients.Commi
 		var associatedPR clients.PullRequest
 		for i := range commit.AssociatedPullRequests.Nodes {
 			pr := commit.AssociatedPullRequests.Nodes[i]
-			if pr.MergeCommit.Oid != commit.Oid ||
-				string(pr.Repository.Owner.Login) != repoOwner ||
+			// NOTE: PR mergeCommit may not match commit.SHA in case repositories
+			// have `enableSquashCommit` disabled. So we accept any associatedPR
+			// to handle this case.
+			if string(pr.Repository.Owner.Login) != repoOwner ||
 				string(pr.Repository.Name) != repoName {
 				continue
 			}

e2e/code_review_test.go

@@ -48,7 +48,7 @@ var _ = Describe("E2E TEST:CodeReview", func() {
 			expected := scut.TestReturn{
 				Error:         nil,
 				Score:         checker.MinResultScore,
-				NumberOfWarn:  3,
+				NumberOfWarn:  30,
 				NumberOfInfo:  0,
 				NumberOfDebug: 0,
 			}
@@ -73,7 +73,7 @@ var _ = Describe("E2E TEST:CodeReview", func() {
 			expected := scut.TestReturn{
 				Error:         nil,
 				Score:         checker.MinResultScore,
-				NumberOfWarn:  3,
+				NumberOfWarn:  30,
 				NumberOfInfo:  0,
 				NumberOfDebug: 0,
 			}