From ba4fb1b94b72026a696ce556d1665dd12fb965a3 Mon Sep 17 00:00:00 2001
From: Spencer Schrock <sschrock@google.com>
Date: Mon, 8 Apr 2024 09:31:39 -0700
Subject: [PATCH] :warning: Switch `Outcome` type to string (#4006)

* convert outcome constants to strings

Originally, these were introduced as ints to enable ordering between them.
Today, I don't see the value in doing that, and it makes the output less readable.

Signed-off-by: Spencer Schrock <sschrock@google.com>

* explicitly mention negative outcome for some tests

previously, OutcomeNegative had the integer value of 0. So some tests
didnt specify the outcome and happened to pass due to the zero value.
This also fixes the tests names while I was here.

Signed-off-by: Spencer Schrock <sschrock@google.com>

* match expected probe output with new string values

this change demonstrates the reason for this PR.
Human readable outcomes are good!

Signed-off-by: Spencer Schrock <sschrock@google.com>

---------

Signed-off-by: Spencer Schrock <sschrock@google.com>
---
 finding/finding.go                           | 32 +++++---------------
 pkg/scorecard_test.go                        |  1 +
 pkg/testdata/probe1.json                     |  4 +--
 probes/sastToolRunsOnAllCommits/impl_test.go |  5 +--
 4 files changed, 13 insertions(+), 29 deletions(-)

diff --git a/finding/finding.go b/finding/finding.go
index 10f906d1858..50594a3216e 100644
--- a/finding/finding.go
+++ b/finding/finding.go
@@ -54,43 +54,25 @@ type Location struct {
 }
 
 // Outcome is the result of a finding.
-type Outcome int
+type Outcome string
 
 // TODO(#2928): re-visit the finding definitions.
 const (
-	// NOTE: The additional '_' are intended for future use.
-	// This allows adding outcomes without breaking the values
-	// of existing outcomes.
 	// OutcomeNegative indicates a negative outcome.
-	OutcomeNegative Outcome = iota
-	_
-	_
-	_
+	OutcomeNegative Outcome = "Negative"
 	// OutcomeNotAvailable indicates an unavailable outcome,
 	// typically because an API call did not return an answer.
-	OutcomeNotAvailable
-	_
-	_
-	_
+	OutcomeNotAvailable Outcome = "NotAvailable"
 	// OutcomeError indicates an errors while running.
 	// The results could not be determined.
-	OutcomeError
-	_
-	_
-	_
+	OutcomeError Outcome = "Error"
 	// OutcomePositive indicates a positive outcome.
-	OutcomePositive
-	_
-	_
-	_
+	OutcomePositive Outcome = "Positive"
 	// OutcomeNotSupported indicates a non-supported outcome.
-	OutcomeNotSupported
-	_
-	_
-	_
+	OutcomeNotSupported Outcome = "NotSupported"
 	// OutcomeNotApplicable indicates if a finding should not
 	// be considered in evaluation.
-	OutcomeNotApplicable
+	OutcomeNotApplicable Outcome = "NotApplicable"
 )
 
 // Finding represents a finding.
diff --git a/pkg/scorecard_test.go b/pkg/scorecard_test.go
index 836bb81b437..4b4ad1b22f0 100644
--- a/pkg/scorecard_test.go
+++ b/pkg/scorecard_test.go
@@ -235,6 +235,7 @@ func TestExperimentalRunProbes(t *testing.T) {
 				Findings: []finding.Finding{
 					{
 						Probe:   fuzzed.Probe,
+						Outcome: finding.OutcomeNegative,
 						Message: "no fuzzer integrations found",
 						Remediation: &probe.Remediation{
 							Effort: probe.RemediationEffortHigh,
diff --git a/pkg/testdata/probe1.json b/pkg/testdata/probe1.json
index 8666ab81a83..09c9a7a501d 100644
--- a/pkg/testdata/probe1.json
+++ b/pkg/testdata/probe1.json
@@ -16,12 +16,12 @@
             },
             "probe": "check for X",
             "message": "found X",
-            "outcome": 12
+            "outcome": "Positive"
         },
         {
             "probe": "check for Y",
             "message": "did not find Y",
-            "outcome": 0
+            "outcome": "Negative"
         }
     ]
 }
diff --git a/probes/sastToolRunsOnAllCommits/impl_test.go b/probes/sastToolRunsOnAllCommits/impl_test.go
index 46e023b7f63..050de598be6 100644
--- a/probes/sastToolRunsOnAllCommits/impl_test.go
+++ b/probes/sastToolRunsOnAllCommits/impl_test.go
@@ -36,7 +36,7 @@ func Test_Run(t *testing.T) {
 		expectedFindings []finding.Finding
 	}{
 		{
-			name: "sonar present",
+			name: "any unchecked commits leads to negative outcome",
 			err:  nil,
 			raw: &checker.RawResults{
 				SASTResults: checker.SASTData{
@@ -57,6 +57,7 @@ func Test_Run(t *testing.T) {
 				{
 					Probe:   Probe,
 					Message: "1 commits out of 2 are checked with a SAST tool",
+					Outcome: finding.OutcomeNegative,
 					Values: map[string]string{
 						AnalyzedPRsKey: "1",
 						TotalPRsKey:    "2",
@@ -65,7 +66,7 @@ func Test_Run(t *testing.T) {
 			},
 		},
 		{
-			name: "sonar present",
+			name: "all commits checked is positive outcome",
 			err:  nil,
 			raw: &checker.RawResults{
 				SASTResults: checker.SASTData{