From 11c94ea05126982f2400a7645070b45d1b0cd9b2 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Thu, 22 Feb 2024 11:44:52 -0800 Subject: [PATCH 01/14] tidy probe documentation Signed-off-by: Spencer Schrock --- probes/codeApproved/def.yml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/probes/codeApproved/def.yml b/probes/codeApproved/def.yml index 7bd82284a04..bcc95d0adb0 100644 --- a/probes/codeApproved/def.yml +++ b/probes/codeApproved/def.yml @@ -19,10 +19,11 @@ motivation: > To ensure that the review process works, the proposed changes should have a minimum number of approvals. implementation: > - This probe looks for whether all changes over the last `--commit-depth` commits have been approved before merge. Commits are grouped by the Pull Request they were introduced in, and each Pull Request must have at least one approval. + This probe looks for whether all changes over the last `--commit-depth` commits have been approved before merge. + Commits are grouped by the changeset they were introduced in, and each changesets must have at least one approval. outcome: - - If all commits were approved, the probe returns OutcomePositive (1) - - If any commit was not approved, the prove returns OutcomeNegative (0) + - If all commits were approved, the probe returns OutcomePositive + - If any commits were not approved, the probe returns OutcomeNegative remediation: effort: Low text: From eccbeb91b73609dec2002de855607cc2e02ea76f Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Thu, 22 Feb 2024 11:45:56 -0800 Subject: [PATCH 02/14] export probe name Signed-off-by: Spencer Schrock --- probes/codeApproved/impl.go | 4 ++-- probes/codeApproved/impl_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/probes/codeApproved/impl.go b/probes/codeApproved/impl.go index 631be68d1fb..f7a2edf6e16 100644 --- a/probes/codeApproved/impl.go +++ b/probes/codeApproved/impl.go @@ -27,11 +27,11 @@ import ( //go:embed *.yml var fs embed.FS -const probe = "codeApproved" +const Probe = "codeApproved" func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { rawReviewData := &raw.CodeReviewResults - return approvedRun(rawReviewData, fs, probe, finding.OutcomePositive, finding.OutcomeNegative) + return approvedRun(rawReviewData, fs, Probe, finding.OutcomePositive, finding.OutcomeNegative) } // Looks through the data and validates that each changeset has been approved at least once. diff --git a/probes/codeApproved/impl_test.go b/probes/codeApproved/impl_test.go index 567851f2963..78122c3468c 100644 --- a/probes/codeApproved/impl_test.go +++ b/probes/codeApproved/impl_test.go @@ -331,7 +331,7 @@ func TestProbeCodeApproved(t *testing.T) { t.Errorf("Expected error %v, got nil", tt.err) case res == nil && err == nil: t.Errorf("Probe returned nil for both finding and error") - case probeID != probe: + case probeID != Probe: t.Errorf("Probe returned the wrong probe ID") default: for i := range tt.expectedFindings { From c7a46ae364528401525a9f6f73b53effc9653130 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Thu, 22 Feb 2024 11:49:19 -0800 Subject: [PATCH 03/14] check for no raw data Signed-off-by: Spencer Schrock --- probes/codeApproved/impl.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/probes/codeApproved/impl.go b/probes/codeApproved/impl.go index f7a2edf6e16..06072aa4ad9 100644 --- a/probes/codeApproved/impl.go +++ b/probes/codeApproved/impl.go @@ -21,6 +21,7 @@ import ( "github.com/ossf/scorecard/v4/checker" "github.com/ossf/scorecard/v4/finding" + "github.com/ossf/scorecard/v4/probes/internal/utils/uerror" "github.com/ossf/scorecard/v4/probes/utils" ) @@ -30,6 +31,9 @@ var fs embed.FS const Probe = "codeApproved" func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { + if raw == nil { + return nil, "", fmt.Errorf("%w: raw", uerror.ErrNil) + } rawReviewData := &raw.CodeReviewResults return approvedRun(rawReviewData, fs, Probe, finding.OutcomePositive, finding.OutcomeNegative) } From 03877014fdfa253b28f2a0e4783d632df5a7a54c Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Thu, 22 Feb 2024 11:55:32 -0800 Subject: [PATCH 04/14] return OutcomeNotApplicable when no changesets are present Signed-off-by: Spencer Schrock --- probes/codeApproved/impl.go | 17 +++++++++++++---- probes/codeApproved/impl_test.go | 11 ++++++----- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/probes/codeApproved/impl.go b/probes/codeApproved/impl.go index 06072aa4ad9..8f25e09c458 100644 --- a/probes/codeApproved/impl.go +++ b/probes/codeApproved/impl.go @@ -22,7 +22,6 @@ import ( "github.com/ossf/scorecard/v4/checker" "github.com/ossf/scorecard/v4/finding" "github.com/ossf/scorecard/v4/probes/internal/utils/uerror" - "github.com/ossf/scorecard/v4/probes/utils" ) //go:embed *.yml @@ -34,6 +33,18 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { if raw == nil { return nil, "", fmt.Errorf("%w: raw", uerror.ErrNil) } + var findings []finding.Finding + changesets := raw.CodeReviewResults.DefaultBranchChangesets + + if len(changesets) == 0 { + f, err := finding.NewWith(fs, Probe, "no changesets detected", nil, finding.OutcomeNotApplicable) + if err != nil { + return nil, Probe, fmt.Errorf("create finding: %w", err) + } + findings = append(findings, *f) + return findings, Probe, nil + } + rawReviewData := &raw.CodeReviewResults return approvedRun(rawReviewData, fs, Probe, finding.OutcomePositive, finding.OutcomeNegative) } @@ -50,9 +61,7 @@ func approvedRun(reviewData *checker.CodeReviewData, fs embed.FS, probeID string nChangesets := len(changesets) nChanges := 0 nUnapprovedChangesets := 0 - if nChangesets == 0 { - return nil, probeID, utils.ErrNoChangesets - } + for x := range changesets { data := &changesets[x] if data.Author.Login == "" { diff --git a/probes/codeApproved/impl_test.go b/probes/codeApproved/impl_test.go index 78122c3468c..f939ddd0032 100644 --- a/probes/codeApproved/impl_test.go +++ b/probes/codeApproved/impl_test.go @@ -16,7 +16,6 @@ package codeApproved import ( - "errors" "testing" "github.com/ossf/scorecard/v4/checker" @@ -24,8 +23,6 @@ import ( "github.com/ossf/scorecard/v4/finding" ) -var errProbeReturned = errors.New("probe run failure") - func TestProbeCodeApproved(t *testing.T) { t.Parallel() probeTests := []struct { @@ -41,8 +38,12 @@ func TestProbeCodeApproved(t *testing.T) { DefaultBranchChangesets: []checker.Changeset{}, }, }, - err: errProbeReturned, - expectedFindings: nil, + expectedFindings: []finding.Finding{ + { + Probe: Probe, + Outcome: finding.OutcomeNotApplicable, + }, + }, }, { name: "no changesets no authors", From fdbd4d55311907267ca1f5dfaba4327272641f93 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Thu, 22 Feb 2024 14:57:30 -0800 Subject: [PATCH 05/14] extract approved logic and return errors as OutcomeError Signed-off-by: Spencer Schrock --- probes/codeApproved/impl.go | 39 ++++++++++++++++---------------- probes/codeApproved/impl_test.go | 8 +++---- 2 files changed, 24 insertions(+), 23 deletions(-) diff --git a/probes/codeApproved/impl.go b/probes/codeApproved/impl.go index 8f25e09c458..5a360a7fe91 100644 --- a/probes/codeApproved/impl.go +++ b/probes/codeApproved/impl.go @@ -50,8 +50,6 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { } // Looks through the data and validates that each changeset has been approved at least once. - -//nolint:gocognit func approvedRun(reviewData *checker.CodeReviewData, fs embed.FS, probeID string, positiveOutcome, negativeOutcome finding.Outcome, ) ([]finding.Finding, string, error) { @@ -64,29 +62,15 @@ func approvedRun(reviewData *checker.CodeReviewData, fs embed.FS, probeID string for x := range changesets { data := &changesets[x] - if data.Author.Login == "" { - f, err := finding.NewNotAvailable(fs, probeID, "Could not retrieve the author of a changeset.", nil) + approvedChangeset, err := approved(data) + if err != nil { + f, err := finding.NewWith(fs, probeID, err.Error(), nil, finding.OutcomeError) if err != nil { return nil, probeID, fmt.Errorf("create finding: %w", err) } findings = append(findings, *f) return findings, probeID, nil } - approvedChangeset := false - for y := range data.Reviews { - if data.Reviews[y].Author.Login == "" { - f, err := finding.NewNotAvailable(fs, probeID, "Could not retrieve the reviewer of a changeset.", nil) - if err != nil { - return nil, probeID, fmt.Errorf("create finding: %w", err) - } - findings = append(findings, *f) - return findings, probeID, nil - } - if data.Reviews[y].State == "APPROVED" && data.Reviews[y].Author.Login != data.Author.Login { - approvedChangeset = true - break - } - } if approvedChangeset && data.Author.IsBot { continue } @@ -127,3 +111,20 @@ func approvedRun(reviewData *checker.CodeReviewData, fs embed.FS, probeID string } return findings, probeID, nil } + +func approved(c *checker.Changeset) (bool, error) { + if c.Author.Login == "" { + //nolint:goerr113 // TODO revisit + return false, fmt.Errorf("could not retrieve changeset author") + } + for _, review := range c.Reviews { + if review.Author.Login == "" { + //nolint:goerr113 // TODO revisit + return false, fmt.Errorf("could not retrieve the changeset reviewer") + } + if review.State == "APPROVED" && review.Author.Login != c.Author.Login { + return true, nil + } + } + return false, nil +} diff --git a/probes/codeApproved/impl_test.go b/probes/codeApproved/impl_test.go index f939ddd0032..fec4f561d9a 100644 --- a/probes/codeApproved/impl_test.go +++ b/probes/codeApproved/impl_test.go @@ -63,8 +63,8 @@ func TestProbeCodeApproved(t *testing.T) { }, expectedFindings: []finding.Finding{ { - Probe: "codeApproved", - Outcome: finding.OutcomeNotAvailable, + Probe: Probe, + Outcome: finding.OutcomeError, }, }, }, @@ -129,8 +129,8 @@ func TestProbeCodeApproved(t *testing.T) { }, expectedFindings: []finding.Finding{ { - Probe: "codeApproved", - Outcome: finding.OutcomeNotAvailable, + Probe: Probe, + Outcome: finding.OutcomeError, }, }, }, From 44db60ae932d6af985a7e71f0341bf612c1f989b Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Thu, 22 Feb 2024 15:42:31 -0800 Subject: [PATCH 06/14] simplify finding creation Signed-off-by: Spencer Schrock --- probes/codeApproved/impl.go | 41 ++++++++++++++----------------------- 1 file changed, 15 insertions(+), 26 deletions(-) diff --git a/probes/codeApproved/impl.go b/probes/codeApproved/impl.go index 5a360a7fe91..1b3a9e9d0b6 100644 --- a/probes/codeApproved/impl.go +++ b/probes/codeApproved/impl.go @@ -46,13 +46,11 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { } rawReviewData := &raw.CodeReviewResults - return approvedRun(rawReviewData, fs, Probe, finding.OutcomePositive, finding.OutcomeNegative) + return approvedRun(rawReviewData, fs, Probe) } // Looks through the data and validates that each changeset has been approved at least once. -func approvedRun(reviewData *checker.CodeReviewData, fs embed.FS, probeID string, - positiveOutcome, negativeOutcome finding.Outcome, -) ([]finding.Finding, string, error) { +func approvedRun(reviewData *checker.CodeReviewData, fs embed.FS, probeID string) ([]finding.Finding, string, error) { changesets := reviewData.DefaultBranchChangesets var findings []finding.Finding foundHumanActivity := false @@ -82,33 +80,24 @@ func approvedRun(reviewData *checker.CodeReviewData, fs embed.FS, probeID string nUnapprovedChangesets += 1 } } + var outcome finding.Outcome + var reason string switch { case !foundHumanActivity: - // returns a NotAvailable outcome if all changesets were authored by bots - f, err := finding.NewNotAvailable(fs, probeID, fmt.Sprintf("Found no human activity "+ - "in the last %d changesets", nChangesets), nil) - if err != nil { - return nil, probeID, fmt.Errorf("create finding: %w", err) - } - findings = append(findings, *f) - return findings, probeID, nil + outcome = finding.OutcomeNotAvailable + reason = fmt.Sprintf("Found no human activity in the last %d changesets", nChangesets) case nUnapprovedChangesets > 0: - // returns NegativeOutcome if not all changesets were approved - f, err := finding.NewWith(fs, probeID, fmt.Sprintf("Not all changesets approved. "+ - "Found %d unapproved changesets of %d.", nUnapprovedChangesets, nChanges), nil, negativeOutcome) - if err != nil { - return nil, probeID, fmt.Errorf("create finding: %w", err) - } - findings = append(findings, *f) + outcome = finding.OutcomeNegative + reason = fmt.Sprintf("Found %d/%d unapproved changesets", nUnapprovedChangesets, nChanges) default: - // returns PositiveOutcome if all changesets have been approved - f, err := finding.NewWith(fs, probeID, fmt.Sprintf("All %d changesets approved.", - nChangesets), nil, positiveOutcome) - if err != nil { - return nil, probeID, fmt.Errorf("create finding: %w", err) - } - findings = append(findings, *f) + outcome = finding.OutcomePositive + reason = "All changesets approved" + } + f, err := finding.NewWith(fs, probeID, reason, nil, outcome) + if err != nil { + return nil, probeID, fmt.Errorf("create finding: %w", err) } + findings = append(findings, *f) return findings, probeID, nil } From b79d45256ea56522be7c9b006fbfb71b105cf9b2 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Thu, 22 Feb 2024 15:43:01 -0800 Subject: [PATCH 07/14] add clarifying comment for skipping bot changes Signed-off-by: Spencer Schrock --- probes/codeApproved/impl.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/probes/codeApproved/impl.go b/probes/codeApproved/impl.go index 1b3a9e9d0b6..5d93116e31c 100644 --- a/probes/codeApproved/impl.go +++ b/probes/codeApproved/impl.go @@ -69,6 +69,8 @@ func approvedRun(reviewData *checker.CodeReviewData, fs embed.FS, probeID string findings = append(findings, *f) return findings, probeID, nil } + // skip bot authored changesets, which can skew single maintainer projects which otherwise dont code review + // https://github.com/ossf/scorecard/issues/2450 if approvedChangeset && data.Author.IsBot { continue } From ecc9d176b12b4cecae876ea8d90d1dd77272d2bd Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Thu, 22 Feb 2024 15:44:00 -0800 Subject: [PATCH 08/14] only bot commits results in OutcomeNotApplicable Signed-off-by: Spencer Schrock --- probes/codeApproved/impl.go | 2 +- probes/codeApproved/impl_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/probes/codeApproved/impl.go b/probes/codeApproved/impl.go index 5d93116e31c..61c1c60e801 100644 --- a/probes/codeApproved/impl.go +++ b/probes/codeApproved/impl.go @@ -86,7 +86,7 @@ func approvedRun(reviewData *checker.CodeReviewData, fs embed.FS, probeID string var reason string switch { case !foundHumanActivity: - outcome = finding.OutcomeNotAvailable + outcome = finding.OutcomeNotApplicable reason = fmt.Sprintf("Found no human activity in the last %d changesets", nChangesets) case nUnapprovedChangesets > 0: outcome = finding.OutcomeNegative diff --git a/probes/codeApproved/impl_test.go b/probes/codeApproved/impl_test.go index fec4f561d9a..1846b306dac 100644 --- a/probes/codeApproved/impl_test.go +++ b/probes/codeApproved/impl_test.go @@ -103,7 +103,7 @@ func TestProbeCodeApproved(t *testing.T) { expectedFindings: []finding.Finding{ { Probe: "codeApproved", - Outcome: finding.OutcomeNotAvailable, + Outcome: finding.OutcomeNotApplicable, }, }, }, @@ -203,7 +203,7 @@ func TestProbeCodeApproved(t *testing.T) { expectedFindings: []finding.Finding{ { Probe: "codeApproved", - Outcome: finding.OutcomeNotAvailable, + Outcome: finding.OutcomeNotApplicable, }, }, }, From cd291167540df88d19c8a8d3b1d56061255f70b0 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Mon, 26 Feb 2024 10:07:59 -0800 Subject: [PATCH 09/14] move no changeset code back to where it was originally Signed-off-by: Spencer Schrock --- probes/codeApproved/impl.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/probes/codeApproved/impl.go b/probes/codeApproved/impl.go index 61c1c60e801..b0e70afccca 100644 --- a/probes/codeApproved/impl.go +++ b/probes/codeApproved/impl.go @@ -33,8 +33,14 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { if raw == nil { return nil, "", fmt.Errorf("%w: raw", uerror.ErrNil) } + rawReviewData := &raw.CodeReviewResults + return approvedRun(rawReviewData, fs, Probe) +} + +// Looks through the data and validates that each changeset has been approved at least once. +func approvedRun(reviewData *checker.CodeReviewData, fs embed.FS, probeID string) ([]finding.Finding, string, error) { + changesets := reviewData.DefaultBranchChangesets var findings []finding.Finding - changesets := raw.CodeReviewResults.DefaultBranchChangesets if len(changesets) == 0 { f, err := finding.NewWith(fs, Probe, "no changesets detected", nil, finding.OutcomeNotApplicable) @@ -45,14 +51,6 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { return findings, Probe, nil } - rawReviewData := &raw.CodeReviewResults - return approvedRun(rawReviewData, fs, Probe) -} - -// Looks through the data and validates that each changeset has been approved at least once. -func approvedRun(reviewData *checker.CodeReviewData, fs embed.FS, probeID string) ([]finding.Finding, string, error) { - changesets := reviewData.DefaultBranchChangesets - var findings []finding.Finding foundHumanActivity := false nChangesets := len(changesets) nChanges := 0 From c0e616e3ca43df6b4fe1496e09561abde268a0d0 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Mon, 26 Feb 2024 10:18:24 -0800 Subject: [PATCH 10/14] include ratio of approved/total as values count the number of approved vs unapproved changesets Signed-off-by: Spencer Schrock --- probes/codeApproved/impl.go | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/probes/codeApproved/impl.go b/probes/codeApproved/impl.go index b0e70afccca..a9776a4f701 100644 --- a/probes/codeApproved/impl.go +++ b/probes/codeApproved/impl.go @@ -18,6 +18,7 @@ package codeApproved import ( "embed" "fmt" + "strconv" "github.com/ossf/scorecard/v4/checker" "github.com/ossf/scorecard/v4/finding" @@ -27,7 +28,11 @@ import ( //go:embed *.yml var fs embed.FS -const Probe = "codeApproved" +const ( + Probe = "codeApproved" + NumApprovedKey = "approvedChangesets" + NumTotalKey = "totalChangesets" +) func Run(raw *checker.RawResults) ([]finding.Finding, string, error) { if raw == nil { @@ -54,7 +59,7 @@ func approvedRun(reviewData *checker.CodeReviewData, fs embed.FS, probeID string foundHumanActivity := false nChangesets := len(changesets) nChanges := 0 - nUnapprovedChangesets := 0 + nApproved := 0 for x := range changesets { data := &changesets[x] @@ -76,8 +81,8 @@ func approvedRun(reviewData *checker.CodeReviewData, fs embed.FS, probeID string if !data.Author.IsBot { foundHumanActivity = true } - if !approvedChangeset { - nUnapprovedChangesets += 1 + if approvedChangeset { + nApproved += 1 } } var outcome finding.Outcome @@ -86,9 +91,9 @@ func approvedRun(reviewData *checker.CodeReviewData, fs embed.FS, probeID string case !foundHumanActivity: outcome = finding.OutcomeNotApplicable reason = fmt.Sprintf("Found no human activity in the last %d changesets", nChangesets) - case nUnapprovedChangesets > 0: + case nApproved != nChanges: outcome = finding.OutcomeNegative - reason = fmt.Sprintf("Found %d/%d unapproved changesets", nUnapprovedChangesets, nChanges) + reason = fmt.Sprintf("Found %d/%d approved changesets", nApproved, nChanges) default: outcome = finding.OutcomePositive reason = "All changesets approved" @@ -97,6 +102,8 @@ func approvedRun(reviewData *checker.CodeReviewData, fs embed.FS, probeID string if err != nil { return nil, probeID, fmt.Errorf("create finding: %w", err) } + f.WithValue(NumApprovedKey, strconv.Itoa(nApproved)) + f.WithValue(NumTotalKey, strconv.Itoa(nChanges)) findings = append(findings, *f) return findings, probeID, nil } From 8e6951dab8ad1dc41fb7bd5836a59327ecb87fe6 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Mon, 26 Feb 2024 10:28:37 -0800 Subject: [PATCH 11/14] ensure unreviewed bot PRs always give negative outcome Signed-off-by: Spencer Schrock --- probes/codeApproved/impl.go | 6 ++--- probes/codeApproved/impl_test.go | 46 +++++++++++++++++++++++++++++--- 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/probes/codeApproved/impl.go b/probes/codeApproved/impl.go index a9776a4f701..cd82aee1452 100644 --- a/probes/codeApproved/impl.go +++ b/probes/codeApproved/impl.go @@ -88,12 +88,12 @@ func approvedRun(reviewData *checker.CodeReviewData, fs embed.FS, probeID string var outcome finding.Outcome var reason string switch { - case !foundHumanActivity: - outcome = finding.OutcomeNotApplicable - reason = fmt.Sprintf("Found no human activity in the last %d changesets", nChangesets) case nApproved != nChanges: outcome = finding.OutcomeNegative reason = fmt.Sprintf("Found %d/%d approved changesets", nApproved, nChanges) + case !foundHumanActivity: + outcome = finding.OutcomeNotApplicable + reason = fmt.Sprintf("Found no human activity in the last %d changesets", nChangesets) default: outcome = finding.OutcomePositive reason = "All changesets approved" diff --git a/probes/codeApproved/impl_test.go b/probes/codeApproved/impl_test.go index 1846b306dac..99633aeddde 100644 --- a/probes/codeApproved/impl_test.go +++ b/probes/codeApproved/impl_test.go @@ -158,7 +158,7 @@ func TestProbeCodeApproved(t *testing.T) { }, }, { - name: "all authors are bots", + name: "only approved bot PRs gives not applicable outcome", rawResults: &checker.RawResults{ CodeReviewResults: checker.CodeReviewData{ DefaultBranchChangesets: []checker.Changeset{ @@ -174,7 +174,12 @@ func TestProbeCodeApproved(t *testing.T) { Message: "Title\nPiperOrigin-RevId: 444529962", }, }, - Reviews: []clients.Review{}, + Reviews: []clients.Review{ + { + Author: &clients.User{Login: "baldur"}, + State: "APPROVED", + }, + }, Author: clients.User{ Login: "bot", IsBot: true, @@ -191,7 +196,12 @@ func TestProbeCodeApproved(t *testing.T) { }, }, }, - Reviews: []clients.Review{}, + Reviews: []clients.Review{ + { + Author: &clients.User{Login: "baldur"}, + State: "APPROVED", + }, + }, Author: clients.User{ Login: "bot", IsBot: true, @@ -318,6 +328,36 @@ func TestProbeCodeApproved(t *testing.T) { }, }, }, + { + name: "only unreviewed bot changesets gives negative outcome", + rawResults: &checker.RawResults{ + CodeReviewResults: checker.CodeReviewData{ + DefaultBranchChangesets: []checker.Changeset{ + { + ReviewPlatform: checker.ReviewPlatformGitHub, + Commits: []clients.Commit{ + { + SHA: "sha", + Committer: clients.User{Login: "dependabot"}, + Message: "foo", + }, + }, + Reviews: []clients.Review{}, + Author: clients.User{ + IsBot: true, + Login: "dependabot", + }, + }, + }, + }, + }, + expectedFindings: []finding.Finding{ + { + Probe: Probe, + Outcome: finding.OutcomeNegative, + }, + }, + }, } for _, tt := range probeTests { From 72941fa342a2b6277eebed4b07821e5b4fb47575 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Mon, 26 Feb 2024 12:31:27 -0800 Subject: [PATCH 12/14] use common outcome test code Signed-off-by: Spencer Schrock --- probes/codeApproved/impl_test.go | 80 +++++++++----------------------- 1 file changed, 23 insertions(+), 57 deletions(-) diff --git a/probes/codeApproved/impl_test.go b/probes/codeApproved/impl_test.go index 99633aeddde..7defde3747a 100644 --- a/probes/codeApproved/impl_test.go +++ b/probes/codeApproved/impl_test.go @@ -21,6 +21,7 @@ import ( "github.com/ossf/scorecard/v4/checker" "github.com/ossf/scorecard/v4/clients" "github.com/ossf/scorecard/v4/finding" + "github.com/ossf/scorecard/v4/probes/internal/utils/test" ) func TestProbeCodeApproved(t *testing.T) { @@ -29,7 +30,7 @@ func TestProbeCodeApproved(t *testing.T) { name string rawResults *checker.RawResults err error - expectedFindings []finding.Finding + expectedOutcomes []finding.Outcome }{ { name: "no changesets", @@ -38,11 +39,8 @@ func TestProbeCodeApproved(t *testing.T) { DefaultBranchChangesets: []checker.Changeset{}, }, }, - expectedFindings: []finding.Finding{ - { - Probe: Probe, - Outcome: finding.OutcomeNotApplicable, - }, + expectedOutcomes: []finding.Outcome{ + finding.OutcomeNotApplicable, }, }, { @@ -61,11 +59,8 @@ func TestProbeCodeApproved(t *testing.T) { }, }, }, - expectedFindings: []finding.Finding{ - { - Probe: Probe, - Outcome: finding.OutcomeError, - }, + expectedOutcomes: []finding.Outcome{ + finding.OutcomeError, }, }, { @@ -100,11 +95,8 @@ func TestProbeCodeApproved(t *testing.T) { }, }, }, - expectedFindings: []finding.Finding{ - { - Probe: "codeApproved", - Outcome: finding.OutcomeNotApplicable, - }, + expectedOutcomes: []finding.Outcome{ + finding.OutcomeNotApplicable, }, }, { @@ -127,11 +119,8 @@ func TestProbeCodeApproved(t *testing.T) { }, }, }, - expectedFindings: []finding.Finding{ - { - Probe: Probe, - Outcome: finding.OutcomeError, - }, + expectedOutcomes: []finding.Outcome{ + finding.OutcomeError, }, }, { @@ -150,11 +139,8 @@ func TestProbeCodeApproved(t *testing.T) { }, }, }, - expectedFindings: []finding.Finding{ - { - Probe: "codeApproved", - Outcome: finding.OutcomeNegative, - }, + expectedOutcomes: []finding.Outcome{ + finding.OutcomeNegative, }, }, { @@ -210,11 +196,8 @@ func TestProbeCodeApproved(t *testing.T) { }, }, }, - expectedFindings: []finding.Finding{ - { - Probe: "codeApproved", - Outcome: finding.OutcomeNotApplicable, - }, + expectedOutcomes: []finding.Outcome{ + finding.OutcomeNotApplicable, }, }, { @@ -241,11 +224,8 @@ func TestProbeCodeApproved(t *testing.T) { }, }, }, - expectedFindings: []finding.Finding{ - { - Probe: "codeApproved", - Outcome: finding.OutcomeNegative, - }, + expectedOutcomes: []finding.Outcome{ + finding.OutcomeNegative, }, }, { @@ -285,11 +265,8 @@ func TestProbeCodeApproved(t *testing.T) { }, }, }, - expectedFindings: []finding.Finding{ - { - Probe: "codeApproved", - Outcome: finding.OutcomePositive, - }, + expectedOutcomes: []finding.Outcome{ + finding.OutcomePositive, }, }, { @@ -321,11 +298,8 @@ func TestProbeCodeApproved(t *testing.T) { }, }, }, - expectedFindings: []finding.Finding{ - { - Probe: "codeApproved", - Outcome: finding.OutcomePositive, - }, + expectedOutcomes: []finding.Outcome{ + finding.OutcomePositive, }, }, { @@ -351,11 +325,8 @@ func TestProbeCodeApproved(t *testing.T) { }, }, }, - expectedFindings: []finding.Finding{ - { - Probe: Probe, - Outcome: finding.OutcomeNegative, - }, + expectedOutcomes: []finding.Outcome{ + finding.OutcomeNegative, }, }, } @@ -375,12 +346,7 @@ func TestProbeCodeApproved(t *testing.T) { case probeID != Probe: t.Errorf("Probe returned the wrong probe ID") default: - for i := range tt.expectedFindings { - if tt.expectedFindings[i].Outcome != res[i].Outcome { - t.Errorf("Code-review probe: %v error: test name: \"%v\", wanted outcome %v, got %v", - res[i].Probe, tt.name, tt.expectedFindings[i].Outcome, res[i].Outcome) - } - } + test.AssertOutcomes(t, res, tt.expectedOutcomes) } }) } From e2072e7e2d8ea361f79bcc303031414b3e8e4722 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Mon, 26 Feb 2024 12:35:58 -0800 Subject: [PATCH 13/14] fix linter Signed-off-by: Spencer Schrock --- probes/codeApproved/impl.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/probes/codeApproved/impl.go b/probes/codeApproved/impl.go index cd82aee1452..c2d74789a41 100644 --- a/probes/codeApproved/impl.go +++ b/probes/codeApproved/impl.go @@ -17,6 +17,7 @@ package codeApproved import ( "embed" + "errors" "fmt" "strconv" @@ -25,8 +26,13 @@ import ( "github.com/ossf/scorecard/v4/probes/internal/utils/uerror" ) -//go:embed *.yml -var fs embed.FS +var ( + //go:embed *.yml + fs embed.FS + + errNoAuthor = errors.New("could not retrieve changeset author") + errNoReviewer = errors.New("could not retrieve the changeset reviewer") +) const ( Probe = "codeApproved" @@ -110,13 +116,11 @@ func approvedRun(reviewData *checker.CodeReviewData, fs embed.FS, probeID string func approved(c *checker.Changeset) (bool, error) { if c.Author.Login == "" { - //nolint:goerr113 // TODO revisit - return false, fmt.Errorf("could not retrieve changeset author") + return false, errNoAuthor } for _, review := range c.Reviews { if review.Author.Login == "" { - //nolint:goerr113 // TODO revisit - return false, fmt.Errorf("could not retrieve the changeset reviewer") + return false, errNoReviewer } if review.State == "APPROVED" && review.Author.Login != c.Author.Login { return true, nil From 130534cd603ff6ddc01705ff80d104a6ed2eb4f0 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Mon, 26 Feb 2024 12:53:17 -0800 Subject: [PATCH 14/14] mention dependabot in probe description Signed-off-by: Spencer Schrock --- probes/codeApproved/def.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/probes/codeApproved/def.yml b/probes/codeApproved/def.yml index bcc95d0adb0..2ba1154bb97 100644 --- a/probes/codeApproved/def.yml +++ b/probes/codeApproved/def.yml @@ -21,9 +21,11 @@ motivation: > implementation: > This probe looks for whether all changes over the last `--commit-depth` commits have been approved before merge. Commits are grouped by the changeset they were introduced in, and each changesets must have at least one approval. + Reviewed, bot authored changesets (e.g. dependabot) are not counted. outcome: - If all commits were approved, the probe returns OutcomePositive - If any commits were not approved, the probe returns OutcomeNegative + - If there are no changes, the probe returns OutcomeNotApplicable remediation: effort: Low text: