From 1e488a804fa102dffca444e84f7c8ffbedbb3bce Mon Sep 17 00:00:00 2001 From: Azeem Shaikh Date: Mon, 14 Feb 2022 15:33:15 -0800 Subject: [PATCH] Fix for repos which do not squash PR commits (#1637) Co-authored-by: Azeem Shaikh --- checks/evaluation/code_review.go | 45 ++++++--------- checks/evaluation/code_review_test.go | 80 +++++++++++++++++++++++++++ clients/githubrepo/graphql.go | 19 +++---- e2e/code_review_test.go | 4 +- 4 files changed, 108 insertions(+), 40 deletions(-) create mode 100644 checks/evaluation/code_review_test.go diff --git a/checks/evaluation/code_review.go b/checks/evaluation/code_review.go index 71a6e6f9a0d..06975002bf7 100644 --- a/checks/evaluation/code_review.go +++ b/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 diff --git a/checks/evaluation/code_review_test.go b/checks/evaluation/code_review_test.go new file mode 100644 index 00000000000..8c560916adf --- /dev/null +++ b/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() + } + }) + } +} diff --git a/clients/githubrepo/graphql.go b/clients/githubrepo/graphql.go index cb14446e514..da7c8d88076 100644 --- a/clients/githubrepo/graphql.go +++ b/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 } diff --git a/e2e/code_review_test.go b/e2e/code_review_test.go index 955f6a8b930..c9301865262 100644 --- a/e2e/code_review_test.go +++ b/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, }