Skip to content

Commit

Permalink
⚠️ Removing the error field from result
Browse files Browse the repository at this point in the history 
- Removing the error field from result
- #1393

Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
  • Loading branch information
@naveensrinivasan
naveensrinivasan committed Apr 22, 2022
1 parent 72e2486 commit 720be15
Show file tree
Hide file tree
Showing 26 changed files with 54 additions and 87 deletions.
12 changes: 4 additions & 8 deletions checker/check_result.go
View file
Expand Up @@ -79,7 +79,6 @@ const (
// nolint:govet
type CheckResult struct {
// TODO(#1393): Remove old structure after deprecation.
Error error `json:"-"`
Name string
Details []string
Confidence int
Expand All @@ -88,7 +87,7 @@ type CheckResult struct {
// UPGRADEv2: New structure. Omitting unchanged Name field
// for simplicity.
Version int `json:"-"` // Default value of 0 indicates old structure.
Error2 error `json:"-"` // Runtime error indicate a filure to run the check.
Error error `json:"-"` // Runtime error indicate a filure to run the check.
Details2 []CheckDetail `json:"-"` // Details of tests and sub-checks
Score int `json:"-"` // {[-1,0...10], -1 = Inconclusive}
Reason string `json:"-"` // A sentence describing the check result (score, etc)
Expand Down Expand Up @@ -161,12 +160,11 @@ func CreateResultWithScore(name, reason string, score int) CheckResult {
return CheckResult{
Name: name,
// Old structure.
Error: nil,
Confidence: MaxResultScore,
Pass: pass,
// New structure.
Version: 2,
Error2: nil,
Error: nil,
Score: score,
Reason: reason,
}
Expand All @@ -186,12 +184,11 @@ func CreateProportionalScoreResult(name, reason string, b, t int) CheckResult {
return CheckResult{
Name: name,
// Old structure.
Error: nil,
Confidence: MaxResultConfidence,
Pass: pass,
// New structure.
Version: 2,
Error2: nil,
Error: nil,
Score: score,
Reason: NormalizeReason(reason, score),
}
Expand Down Expand Up @@ -232,12 +229,11 @@ func CreateRuntimeErrorResult(name string, e error) CheckResult {
return CheckResult{
Name: name,
// Old structure.
Error: e,
Confidence: 0,
Pass: false,
// New structure.
Version: 2,
Error2: e,
Error: e,
Score: InconclusiveResultScore,
Reason: e.Error(), // Note: message already accessible by caller thru `Error`.
}
Expand Down
6 changes: 3 additions & 3 deletions checker/check_runner.go
View file
Expand Up @@ -77,7 +77,7 @@ func logStats(ctx context.Context, startTime time.Time, result *CheckResult) err
opencensusstats.Record(ctx, stats.CheckRuntimeInSec.M(runTimeInSecs))

if result.Error != nil {
ctx, err := tag.New(ctx, tag.Upsert(stats.ErrorName, sce.GetName(result.Error2)))
ctx, err := tag.New(ctx, tag.Upsert(stats.ErrorName, sce.GetName(result.Error)))
if err != nil {
return sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("tag.New: %v", err))
}
Expand Down Expand Up @@ -109,9 +109,9 @@ func (r *Runner) Run(ctx context.Context, c Check) CheckResult {
checkRequest.Ctx = ctx
checkRequest.Dlogger = l
res = c.Fn(&checkRequest)
if res.Error2 != nil && errors.Is(res.Error2, sce.ErrRepoUnreachable) {
if res.Error != nil && errors.Is(res.Error, sce.ErrRepoUnreachable) {
checkRequest.Dlogger.Warn(&LogMessage{
Text: fmt.Sprintf("%v", res.Error2),
Text: fmt.Sprintf("%v", res.Error),
})
continue
}
Expand Down
2 changes: 1 addition & 1 deletion checks/code_review_test.go
View file
Expand Up @@ -207,7 +207,7 @@ func TestCodereview(t *testing.T) {
res := CodeReview(&req)

if tt.err != nil {
if res.Error2 == nil {
if res.Error == nil {
t.Errorf("Expected error %v, got nil", tt.err)
}
// return as we don't need to check the rest of the fields.
Expand Down
2 changes: 1 addition & 1 deletion checks/contributors_test.go
View file
Expand Up @@ -175,7 +175,7 @@ func TestContributors(t *testing.T) {
res := Contributors(&req)

if tt.err != nil {
if res.Error2 == nil {
if res.Error == nil {
t.Errorf("Expected error %v, got nil", tt.err)
}
// return as we don't need to check the rest of the fields.
Expand Down
4 changes: 2 additions & 2 deletions checks/evaluation/binary_artifacts_test.go
View file
Expand Up @@ -277,8 +277,8 @@ func TestBinaryArtifacts(t *testing.T) {
t.Parallel()
got := BinaryArtifacts(tt.args.name, tt.args.dl, tt.args.r)
if tt.wantErr {
if got.Error2 == nil {
t.Errorf("BinaryArtifacts() error = %v, wantErr %v", got.Error2, tt.wantErr)
if got.Error == nil {
t.Errorf("BinaryArtifacts() error = %v, wantErr %v", got.Error, tt.wantErr)
}
} else {
if got.Score != tt.want.Score {
Expand Down
20 changes: 10 additions & 10 deletions checks/evaluation/dependency_update_tool_test.go
View file
Expand Up @@ -70,8 +70,8 @@ func TestDependencyUpdateTool(t *testing.T) {
},
},
want: checker.CheckResult{
Score: 0,
Error2: nil,
Score: 0,
Error: nil,
},
err: false,
expected: scut.TestReturn{
Expand Down Expand Up @@ -104,8 +104,8 @@ func TestDependencyUpdateTool(t *testing.T) {
},
},
want: checker.CheckResult{
Score: 10,
Error2: nil,
Score: 10,
Error: nil,
},
expected: scut.TestReturn{
Error: nil,
Expand All @@ -131,8 +131,8 @@ func TestDependencyUpdateTool(t *testing.T) {
},
},
want: checker.CheckResult{
Score: -1,
Error2: nil,
Score: -1,
Error: nil,
},
expected: scut.TestReturn{
Error: sce.ErrScorecardInternal,
Expand All @@ -147,8 +147,8 @@ func TestDependencyUpdateTool(t *testing.T) {
dl: &scut.TestDetailLogger{},
},
want: checker.CheckResult{
Score: -1,
Error2: nil,
Score: -1,
Error: nil,
},
expected: scut.TestReturn{
Error: sce.ErrScorecardInternal,
Expand All @@ -167,8 +167,8 @@ func TestDependencyUpdateTool(t *testing.T) {
if tt.want.Score != got.Score {
t.Errorf("DependencyUpdateTool() got Score = %v, want %v for %v", got.Score, tt.want.Score, tt.name)
}
if tt.err && got.Error2 == nil {
t.Errorf("DependencyUpdateTool() error = %v, want %v for %v", got.Error2, tt.want.Error2, tt.name)
if tt.err && got.Error == nil {
t.Errorf("DependencyUpdateTool() error = %v, want %v for %v", got.Error, tt.want.Error, tt.name)
return
}

Expand Down
4 changes: 2 additions & 2 deletions checks/evaluation/webhooks_test.go
View file
Expand Up @@ -139,8 +139,8 @@ func TestWebhooks(t *testing.T) {
t.Parallel()
got := Webhooks(tt.args.name, tt.args.dl, tt.args.r)
if tt.wantErr {
if got.Error2 == nil {
t.Errorf("Webhooks() error = %v, wantErr %v", got.Error2, tt.wantErr)
if got.Error == nil {
t.Errorf("Webhooks() error = %v, wantErr %v", got.Error, tt.wantErr)
}
} else {
if got.Score != tt.want.Score {
Expand Down
4 changes: 2 additions & 2 deletions checks/fuzzing_test.go
View file
Expand Up @@ -295,8 +295,8 @@ func TestFuzzing(t *testing.T) {
}

result := Fuzzing(&req)
if (result.Error2 != nil) != tt.wantErr {
t.Errorf("Fuzzing() error = %v, wantErr %v", result.Error2, tt.wantErr)
if (result.Error != nil) != tt.wantErr {
t.Errorf("Fuzzing() error = %v, wantErr %v", result.Error, tt.wantErr)
return
}

Expand Down
2 changes: 1 addition & 1 deletion checks/maintained_test.go
View file
Expand Up @@ -345,7 +345,7 @@ func Test_Maintained(t *testing.T) {
res := Maintained(&req)

if tt.err != nil {
if res.Error2 == nil {
if res.Error == nil {
t.Errorf("Expected error %v, got nil", tt.err)
}
// return as we don't need to check the rest of the fields.
Expand Down
40 changes: 20 additions & 20 deletions checks/pinned_dependencies_test.go
View file
Expand Up @@ -128,8 +128,8 @@ func TestGithubWorkflowPinning(t *testing.T) {

s, e := testIsGitHubActionsWorkflowPinned(p, content, &dl)
actual := checker.CheckResult{
Score: s,
Error2: e,
Score: s,
Error: e,
}
if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) {
t.Fail()
Expand Down Expand Up @@ -221,8 +221,8 @@ func TestNonGithubWorkflowPinning(t *testing.T) {

s, e := testIsGitHubActionsWorkflowPinned(p, content, &dl)
actual := checker.CheckResult{
Score: s,
Error2: e,
Score: s,
Error: e,
}
if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) {
t.Fail()
Expand Down Expand Up @@ -268,8 +268,8 @@ func TestGithubWorkflowPkgManagerPinning(t *testing.T) {

s, e := testValidateGitHubWorkflowScriptFreeOfInsecureDownloads(p, content, &dl)
actual := checker.CheckResult{
Score: s,
Error2: e,
Score: s,
Error: e,
}

if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) {
Expand Down Expand Up @@ -392,8 +392,8 @@ func TestDockerfilePinning(t *testing.T) {
dl := scut.TestDetailLogger{}
s, e := testValidateDockerfileIsPinned(tt.filename, content, &dl)
actual := checker.CheckResult{
Score: s,
Error2: e,
Score: s,
Error: e,
}
if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) {
t.Fail()
Expand Down Expand Up @@ -781,8 +781,8 @@ func TestDockerfilePinningWihoutHash(t *testing.T) {
dl := scut.TestDetailLogger{}
s, e := testValidateDockerfileIsPinned(tt.filename, content, &dl)
actual := checker.CheckResult{
Score: s,
Error2: e,
Score: s,
Error: e,
}
if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) {
t.Fail()
Expand Down Expand Up @@ -970,8 +970,8 @@ func TestDockerfileScriptDownload(t *testing.T) {
dl := scut.TestDetailLogger{}
s, e := testValidateDockerfileIsFreeOfInsecureDownloads(tt.filename, content, &dl)
actual := checker.CheckResult{
Score: s,
Error2: e,
Score: s,
Error: e,
}
if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) {
t.Fail()
Expand Down Expand Up @@ -1013,8 +1013,8 @@ func TestDockerfileScriptDownloadInfo(t *testing.T) {
dl := scut.TestDetailLogger{}
s, e := testValidateDockerfileIsFreeOfInsecureDownloads(tt.filename, content, &dl)
actual := checker.CheckResult{
Score: s,
Error2: e,
Score: s,
Error: e,
}
if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) {
t.Fail()
Expand Down Expand Up @@ -1123,8 +1123,8 @@ func TestShellScriptDownload(t *testing.T) {
dl := scut.TestDetailLogger{}
s, e := testValidateShellScriptIsFreeOfInsecureDownloads(tt.filename, content, &dl)
actual := checker.CheckResult{
Score: s,
Error2: e,
Score: s,
Error: e,
}
if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) {
t.Fail()
Expand Down Expand Up @@ -1178,8 +1178,8 @@ func TestShellScriptDownloadPinned(t *testing.T) {
dl := scut.TestDetailLogger{}
s, e := testValidateShellScriptIsFreeOfInsecureDownloads(tt.filename, content, &dl)
actual := checker.CheckResult{
Score: s,
Error2: e,
Score: s,
Error: e,
}
if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) {
t.Fail()
Expand Down Expand Up @@ -1257,8 +1257,8 @@ func TestGitHubWorflowRunDownload(t *testing.T) {

s, e := testValidateGitHubWorkflowScriptFreeOfInsecureDownloads(p, content, &dl)
actual := checker.CheckResult{
Score: s,
Error2: e,
Score: s,
Error: e,
}
if !scut.ValidateTestReturn(t, tt.name, &tt.expected, &actual, &dl) {
t.Fail()
Expand Down
8 changes: 4 additions & 4 deletions checks/signed_releases_test.go
View file
Expand Up @@ -359,9 +359,9 @@ func TestSignedRelease(t *testing.T) {
name: "Error getting releases",
err: errors.New("Error getting releases"),
expected: checker.CheckResult{
Pass: false,
Score: -1,
Error2: errors.New("Error getting releases"),
Pass: false,
Score: -1,
Error: errors.New("Error getting releases"),
},
},
}
Expand Down Expand Up @@ -390,7 +390,7 @@ func TestSignedRelease(t *testing.T) {
res := SignedReleases(&req)

if tt.err != nil {
if res.Error2 == nil {
if res.Error == nil {
t.Errorf("Expected error %v, got nil", tt.err)
}
// return as we don't need to check the rest of the fields.
Expand Down
2 changes: 1 addition & 1 deletion checks/webhook_test.go
View file
Expand Up @@ -123,7 +123,7 @@ func TestWebhooks(t *testing.T) {
}
res := WebHooks(&req)
if tt.err != nil {
if res.Error2 == nil {
if res.Error == nil {
t.Errorf("Expected error %v, got nil", tt.err)
}
// return as we don't need to check the rest of the fields.
Expand Down
4 changes: 2 additions & 2 deletions cron/worker/main.go
View file
Expand Up @@ -118,10 +118,10 @@ func processRequest(ctx context.Context,
}
for checkIndex := range result.Checks {
check := &result.Checks[checkIndex]
if !errors.Is(check.Error2, sce.ErrScorecardInternal) {
if !errors.Is(check.Error, sce.ErrScorecardInternal) {
continue
}
errorMsg := fmt.Sprintf("check %s has a runtime error: %v", check.Name, check.Error2)
errorMsg := fmt.Sprintf("check %s has a runtime error: %v", check.Name, check.Error)
if !(*ignoreRuntimeErrors) {
// nolint: goerr113
return errors.New(errorMsg)
Expand Down
2 changes: 0 additions & 2 deletions e2e/binary_artifacts_test.go
View file
Expand Up @@ -86,7 +86,6 @@ var _ = Describe("E2E TEST:"+checks.CheckBinaryArtifacts, func() {
result := checks.BinaryArtifacts(&req)
// UPGRADEv2: to remove.
// Old version.
Expect(result.Error).Should(BeNil())
Expect(result.Pass).Should(BeFalse())
// New version.
Expect(scut.ValidateTestReturn(nil, "binary artifacts", &expected, &result, &dl)).Should(BeTrue())
Expand Down Expand Up @@ -117,7 +116,6 @@ var _ = Describe("E2E TEST:"+checks.CheckBinaryArtifacts, func() {
result := checks.BinaryArtifacts(&req)
// UPGRADEv2: to remove.
// Old version.
Expect(result.Error).Should(BeNil())
Expect(result.Pass).Should(BeFalse())
// New version.
Expect(scut.ValidateTestReturn(nil, "binary artifacts", &expected, &result, &dl)).Should(BeTrue())
Expand Down
5 changes: 1 addition & 4 deletions e2e/branch_protection_test.go
View file
Expand Up @@ -54,7 +54,6 @@ var _ = Describe("E2E TEST PAT:"+checks.CheckBranchProtection, func() {
result := checks.BranchProtection(&req)
// UPGRADEv2: to remove.
// Old version.
Expect(result.Error).Should(BeNil())
Expect(result.Pass).Should(BeFalse())

// New version.
Expand Down Expand Up @@ -86,7 +85,6 @@ var _ = Describe("E2E TEST PAT:"+checks.CheckBranchProtection, func() {
result := checks.BranchProtection(&req)
// UPGRADEv2: to remove.
// Old version.
Expect(result.Error).Should(BeNil())
Expect(result.Pass).Should(BeFalse())

// New version.
Expand Down Expand Up @@ -118,7 +116,6 @@ var _ = Describe("E2E TEST PAT:"+checks.CheckBranchProtection, func() {
result := checks.BranchProtection(&req)
// UPGRADEv2: to remove.
// Old version.
Expect(result.Error).Should(BeNil())
Expect(result.Pass).Should(BeFalse())

// New version.
Expand Down Expand Up @@ -146,9 +143,9 @@ var _ = Describe("E2E TEST GITHUB_TOKEN:"+checks.CheckBranchProtection, func() {
}

result := checks.BranchProtection(&req)
Expect(result.Error).ShouldNot(BeNil())
// There should be an error with the GITHUB_TOKEN, until it's supported
// byt GitHub.
Expect(result.Error).ShouldNot(BeNil())
Expect(repoClient.Close()).Should(BeNil())
})
})
Expand Down

0 comments on commit 720be15

Please sign in to comment.