From 856419158a11cf2a087fd7a5ba222c6b34e08190 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Wed, 10 Apr 2024 15:43:12 -0700 Subject: [PATCH] :seedling: migrate code review check to probes (#3979) * initial conversion Signed-off-by: Spencer Schrock * appease the linter Signed-off-by: Spencer Schrock * cleanup outcomes from positive/negative to true/false conversion Signed-off-by: Spencer Schrock --------- Signed-off-by: Spencer Schrock --- checks/code_review.go | 19 ++- checks/code_review_test.go | 12 +- checks/evaluation/code_review.go | 104 +++++---------- checks/evaluation/code_review_test.go | 181 +++++--------------------- e2e/code_review_test.go | 6 +- probes/codeApproved/impl.go | 22 ++-- probes/codeApproved/impl_test.go | 8 +- probes/entries.go | 2 +- 8 files changed, 93 insertions(+), 261 deletions(-) diff --git a/checks/code_review.go b/checks/code_review.go index 70634e7361e..dbbf458d048 100644 --- 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 index e6b094e08b7..1a19fe9577e 100644 --- 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 index edd59ded537..b1610d6b344 100644 --- 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") } diff --git a/checks/evaluation/code_review_test.go b/checks/evaluation/code_review_test.go index b916224509d..297a286b33c 100644 --- a/checks/evaluation/code_review_test.go +++ b/checks/evaluation/code_review_test.go @@ -18,108 +18,54 @@ import ( "testing" "github.com/ossf/scorecard/v4/checker" - "github.com/ossf/scorecard/v4/clients" sce "github.com/ossf/scorecard/v4/errors" + "github.com/ossf/scorecard/v4/finding" + "github.com/ossf/scorecard/v4/probes/codeApproved" 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 + findings []finding.Finding expected scut.TestReturn - rawData *checker.CodeReviewData }{ { - name: "NullRawData", + name: "no findings is an error", expected: scut.TestReturn{ Error: sce.ErrScorecardInternal, Score: checker.InconclusiveResultScore, }, - rawData: nil, + findings: nil, }, { - name: "NoCommits", + name: "no changesets", expected: scut.TestReturn{ Score: checker.InconclusiveResultScore, }, - rawData: &checker.CodeReviewData{}, - }, - { - name: "NoReviews", - expected: scut.TestReturn{ - Score: checker.MinResultScore, - NumberOfDebug: 2, - }, - rawData: &checker.CodeReviewData{ - DefaultBranchChangesets: []checker.Changeset{ - { - ReviewPlatform: checker.ReviewPlatformUnknown, - Commits: []clients.Commit{{SHA: "a"}}, - }, - { - ReviewPlatform: checker.ReviewPlatformUnknown, - Commits: []clients.Commit{{SHA: "a"}}, - }, + findings: []finding.Finding{ + { + Probe: codeApproved.Probe, + Outcome: finding.OutcomeNotApplicable, + Message: "no changesets detected", }, }, }, { - name: "Unreviewed human and bot changes", + name: "unreviewed changes result in minimum score", expected: scut.TestReturn{ Score: checker.MinResultScore, - NumberOfDebug: 2, - }, - rawData: &checker.CodeReviewData{ - DefaultBranchChangesets: []checker.Changeset{ - { - ReviewPlatform: checker.ReviewPlatformUnknown, - Commits: []clients.Commit{{SHA: "a", Committer: clients.User{IsBot: true}}}, - }, - { - ReviewPlatform: checker.ReviewPlatformUnknown, - Commits: []clients.Commit{{SHA: "b"}}, - }, - }, + NumberOfDebug: 0, // TODO }, - }, - { - name: "all human changesets reviewed, missing review on bot changeset", - expected: scut.TestReturn{ - Score: 5, - NumberOfDebug: 1, - }, - rawData: &checker.CodeReviewData{ - DefaultBranchChangesets: []checker.Changeset{ - { - Author: clients.User{Login: "alice"}, - ReviewPlatform: checker.ReviewPlatformGitHub, - RevisionID: "1", - Reviews: []clients.Review{ - { - Author: &clients.User{}, - State: "APPROVED", - }, - }, - Commits: []clients.Commit{ - { - Committer: clients.User{Login: "bob"}, - SHA: "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}, - SHA: "b", - }, - }, + findings: []finding.Finding{ + { + Probe: codeApproved.Probe, + Outcome: finding.OutcomeFalse, + Message: "Found 0/2 approved changesets", + Values: map[string]string{ + codeApproved.NumApprovedKey: "0", + codeApproved.NumTotalKey: "2", }, }, }, @@ -129,80 +75,14 @@ func TestCodeReview(t *testing.T) { expected: scut.TestReturn{ Score: checker.MaxResultScore, }, - rawData: &checker.CodeReviewData{ - DefaultBranchChangesets: []checker.Changeset{ - { - Author: clients.User{Login: "alice"}, - ReviewPlatform: checker.ReviewPlatformGitHub, - RevisionID: "1", - Reviews: []clients.Review{ - { - Author: &clients.User{}, - State: "APPROVED", - }, - }, - Commits: []clients.Commit{ - { - Committer: clients.User{Login: "bob"}, - SHA: "b", - }, - }, - }, - }, - }, - }, - - { - name: "bot commits only", - expected: scut.TestReturn{ - Score: checker.InconclusiveResultScore, - NumberOfDebug: 1, - }, - rawData: &checker.CodeReviewData{ - DefaultBranchChangesets: []checker.Changeset{ - { - Author: clients.User{Login: "alice-the-bot[bot]", IsBot: true}, - ReviewPlatform: checker.ReviewPlatformGitHub, - RevisionID: "1", - Reviews: []clients.Review{ - { - Author: &clients.User{}, - State: "APPROVED", - }, - }, - Commits: []clients.Commit{ - { - Committer: clients.User{Login: "alice-the-bot[bot]", IsBot: true}, - SHA: "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}, - SHA: "b", - }, - }, - }, - }, - }, - }, - - { - name: "all changesets reviewed outside github", - expected: scut.TestReturn{ - Score: checker.MaxResultScore, - }, - rawData: &checker.CodeReviewData{ - DefaultBranchChangesets: []checker.Changeset{ - { - ReviewPlatform: checker.ReviewPlatformGerrit, - RevisionID: "1", - Commits: []clients.Commit{{SHA: "a"}}, + findings: []finding.Finding{ + { + Probe: codeApproved.Probe, + Outcome: finding.OutcomeTrue, + Message: "All changesets approved", + Values: map[string]string{ + codeApproved.NumApprovedKey: "2", + codeApproved.NumTotalKey: "2", }, }, }, @@ -213,9 +93,8 @@ func TestCodeReview(t *testing.T) { 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) + res := CodeReview(tt.name, tt.findings, dl) scut.ValidateTestReturn(t, tt.name, &tt.expected, &res, dl) }) } diff --git a/e2e/code_review_test.go b/e2e/code_review_test.go index 38560e14080..c5ae0edcfb8 100644 --- a/e2e/code_review_test.go +++ b/e2e/code_review_test.go @@ -93,8 +93,7 @@ var _ = Describe("E2E TEST:"+checks.CheckCodeReview, func() { Dlogger: &dl, } expected := scut.TestReturn{ - Score: 0, - NumberOfDebug: 18, + Score: 0, } result := checks.CodeReview(&req) scut.ValidateTestReturn(GinkgoTB(), "use code reviews", &expected, &result, &dl) @@ -115,8 +114,7 @@ var _ = Describe("E2E TEST:"+checks.CheckCodeReview, func() { Dlogger: &dl, } expected := scut.TestReturn{ - Score: 1, - NumberOfDebug: 10, + Score: 1, } result := checks.CodeReview(&req) scut.ValidateTestReturn(GinkgoTB(), "use code reviews", &expected, &result, &dl) diff --git a/probes/codeApproved/impl.go b/probes/codeApproved/impl.go index 00271564d9c..df9c739de5c 100644 --- a/probes/codeApproved/impl.go +++ b/probes/codeApproved/impl.go @@ -17,7 +17,6 @@ package codeApproved import ( "embed" - "errors" "fmt" "strconv" @@ -31,13 +30,8 @@ func init() { probes.MustRegister(Probe, Run, []probes.CheckName{probes.CodeReview}) } -var ( - //go:embed *.yml - fs embed.FS - - errNoAuthor = errors.New("could not retrieve changeset author") - errNoReviewer = errors.New("could not retrieve the changeset reviewer") -) +//go:embed *.yml +var fs embed.FS const ( Probe = "codeApproved" @@ -120,13 +114,15 @@ func approvedRun(reviewData *checker.CodeReviewData, fs embed.FS, probeID string } func approved(c *checker.Changeset) (bool, error) { - if c.Author.Login == "" { - return false, errNoAuthor + switch c.ReviewPlatform { + // reviewed outside GitHub / GitLab + case checker.ReviewPlatformProw, + checker.ReviewPlatformGerrit, + checker.ReviewPlatformPhabricator, + checker.ReviewPlatformPiper: + return true, nil } for _, review := range c.Reviews { - if review.Author.Login == "" { - return false, errNoReviewer - } if review.State == "APPROVED" && review.Author.Login != c.Author.Login { return true, nil } diff --git a/probes/codeApproved/impl_test.go b/probes/codeApproved/impl_test.go index 20732f2bbaf..ed3c9de474f 100644 --- a/probes/codeApproved/impl_test.go +++ b/probes/codeApproved/impl_test.go @@ -44,7 +44,7 @@ func TestProbeCodeApproved(t *testing.T) { }, }, { - name: "no changesets no authors", + name: "changesets no reviews", rawResults: &checker.RawResults{ CodeReviewResults: checker.CodeReviewData{ DefaultBranchChangesets: []checker.Changeset{ @@ -60,7 +60,7 @@ func TestProbeCodeApproved(t *testing.T) { }, }, expectedOutcomes: []finding.Outcome{ - finding.OutcomeError, + finding.OutcomeFalse, }, }, { @@ -100,7 +100,7 @@ func TestProbeCodeApproved(t *testing.T) { }, }, { - name: "no changesets no review authors", + name: "changesets no review authors", rawResults: &checker.RawResults{ CodeReviewResults: checker.CodeReviewData{ DefaultBranchChangesets: []checker.Changeset{ @@ -120,7 +120,7 @@ func TestProbeCodeApproved(t *testing.T) { }, }, expectedOutcomes: []finding.Outcome{ - finding.OutcomeError, + finding.OutcomeFalse, }, }, { diff --git a/probes/entries.go b/probes/entries.go index bdd5aed42c0..1b6be369625 100644 --- a/probes/entries.go +++ b/probes/entries.go @@ -99,7 +99,6 @@ var ( } CodeReview = []ProbeImpl{ codeApproved.Run, - codeReviewOneReviewers.Run, } SAST = []ProbeImpl{ sastToolConfigured.Run, @@ -156,6 +155,7 @@ var ( // Probes which aren't included by any checks. // These still need to be listed so they can be called with --probes. Uncategorized = []ProbeImpl{ + codeReviewOneReviewers.Run, hasBinaryArtifacts.Run, } )