✨ [migration to score] 1: create errors and new functions (#712)

* details-1

* comment

* doc

* nits

* typo

* commments

* nit

* linter

CONTRIBUTING.md

@@ -12,6 +12,7 @@ project. This document describes the contribution guidelines for the project.
   * [Getting started](#getting-started)
   * [Environment Setup](#environment-setup)
 * [Contributing steps](#contributing-steps)
+* [Error handling](#error-handling)
 * [How to build scorecard locally](#how-to-build-scorecard-locally)
 * [PR Process](#pr-process)
 * [What to do before submitting a pull request](#what-to-do-before-submitting-a-pull-request)
@@ -52,6 +53,10 @@ You must install these tools:
 1.  Fork the desired repo, develop and test your code changes.
 1.  Submit a pull request.
 
+## Error handling
+
+See [errors/errors.md].
+
 ## How to build scorecard locally
 
 Note that, by building the scorecard from the source code we are allowed to test
@@ -140,22 +145,7 @@ Submit a PR for this file and scorecard would start scanning in subsequent runs.
 
 ## Adding New Checks
 
-Each check is currently just a function of type `CheckFn`. The signature is:
-
-```golang
-type CheckFn func(*c.Checker) CheckResult
-```
-
-Checks are registered in an init function:
-
-```golang
-const codeReviewStr = "Code-Review"
-
-//nolint:gochecknoinits
-func init() {
-	registerCheck(codeReviewStr, DoesCodeReview)
-}
-```
+See [checks/write.md](checks/write.md).
 
 ### Updating Docs
 

checker/check_request.go

@@ -30,6 +30,8 @@ type CheckRequest struct {
 	GraphClient *githubv4.Client
 	HTTPClient  *http.Client
 	RepoClient  clients.RepoClient
+	// UPGRADEv2: Logf will be removed.
 	Logf        func(s string, f ...interface{})
+	Dlogger     DetailLogger
 	Owner, Repo string
 }

checker/check_result.go

@@ -16,30 +16,199 @@ package checker
 
 import (
 	"errors"
+	"fmt"
+	"math"
 
 	scorecarderrors "github.com/ossf/scorecard/errors"
 )
 
-const MaxResultConfidence = 10
+// UPGRADEv2: to remove.
+const (
+	MaxResultConfidence  = 10
+	HalfResultConfidence = 5
+	MinResultConfidence  = 0
+)
+
+// UPGRADEv2: to remove.
+const migrationThresholdPassValue = 8
 
 // ErrorDemoninatorZero indicates the denominator for a proportional result is 0.
+// UPGRADEv2: to remove.
 var ErrorDemoninatorZero = errors.New("internal error: denominator is 0")
 
+// Types of details.
+type DetailType int
+
+const (
+	DetailInfo DetailType = iota
+	DetailWarn
+	DetailDebug
+)
+
+// CheckDetail contains information for each detail.
+//nolint:govet
+type CheckDetail struct {
+	Type DetailType // Any of DetailWarn, DetailInfo, DetailDebug.
+	Msg  string     // A short string explaining why the details was recorded/logged..
+}
+
+type DetailLogger interface {
+	Info(desc string, args ...interface{})
+	Warn(desc string, args ...interface{})
+	Debug(desc string, args ...interface{})
+}
+
+//nolint
+const (
+	MaxResultScore          = 10
+	MinResultScore          = 0
+	InconclusiveResultScore = -1
+)
+
+//nolint
 type CheckResult struct {
+	// Old structure
 	Error       error `json:"-"`
 	Name        string
 	Details     []string
 	Confidence  int
 	Pass        bool
 	ShouldRetry bool `json:"-"`
+
+	// 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.
+	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)
+}
+
+// CreateResultWithScore is used when
+// the check runs without runtime errors and we want to assign a
+// specific score.
+func CreateResultWithScore(name, reason string, score int) CheckResult {
+	pass := true
+	if score < migrationThresholdPassValue {
+		pass = false
+	}
+	return CheckResult{
+		Name: name,
+		// Old structure.
+		Error:       nil,
+		Confidence:  MaxResultScore,
+		Pass:        pass,
+		ShouldRetry: false,
+		// New structure.
+		//nolint
+		Version: 2,
+		Error2:  nil,
+		Score:   score,
+		Reason:  reason,
+	}
+}
+
+// CreateProportionalScoreResult is used when
+// the check runs without runtime errors and we assign a
+// proportional score. This may be used if a check contains
+// multiple tests and we want to assign a score proportional
+// the the number of tests that succeeded.
+func CreateProportionalScoreResult(name, reason string, b, t int) CheckResult {
+	pass := true
+	score := int(math.Min(float64(MaxResultScore*b/t), float64(MaxResultScore)))
+	if score < migrationThresholdPassValue {
+		pass = false
+	}
+	return CheckResult{
+		Name: name,
+		// Old structure.
+		Error:       nil,
+		Confidence:  MaxResultConfidence,
+		Pass:        pass,
+		ShouldRetry: false,
+		// New structure.
+		//nolint
+		Version: 2,
+		Error2:  nil,
+		Score:   score,
+		Reason:  fmt.Sprintf("%v -- score normalized to %d", reason, score),
+	}
+}
+
+// CreateMaxScoreResult is used when
+// the check runs without runtime errors and we can assign a
+// maximum score to the result.
+func CreateMaxScoreResult(name, reason string) CheckResult {
+	return CreateResultWithScore(name, reason, MaxResultScore)
+}
+
+// CreateMinScoreResult is used when
+// the check runs without runtime errors and we can assign a
+// minimum score to the result.
+func CreateMinScoreResult(name, reason string) CheckResult {
+	return CreateResultWithScore(name, reason, MinResultScore)
+}
+
+// CreateInconclusiveResult is used when
+// the check runs without runtime errors, but we don't
+// have enough evidence to set a score.
+func CreateInconclusiveResult(name, reason string) CheckResult {
+	return CheckResult{
+		Name: name,
+		// Old structure.
+		Confidence:  0,
+		Pass:        false,
+		ShouldRetry: false,
+		// New structure.
+		//nolint
+		Version: 2,
+		Score:   InconclusiveResultScore,
+		Reason:  reason,
+	}
+}
+
+// CreateRuntimeErrorResult is used when the check fails to run because of a runtime error.
+func CreateRuntimeErrorResult(name string, e error) CheckResult {
+	return CheckResult{
+		Name: name,
+		// Old structure.
+		Error:       e,
+		Confidence:  0,
+		Pass:        false,
+		ShouldRetry: false,
+		// New structure.
+		//nolint
+		Version: 2,
+		Error2:  e,
+		Score:   InconclusiveResultScore,
+		Reason:  e.Error(), // Note: message already accessible by caller thru `Error`.
+	}
+}
+
+// UPGRADEv2: will be renaall functions belowed will be removed.
+func MakeAndResult2(checks ...CheckResult) CheckResult {
+	if len(checks) == 0 {
+		// That should never happen.
+		panic("MakeResult called with no checks")
+	}
+
+	worseResult := checks[0]
+	// UPGRADEv2: will go away after old struct is removed.
+	//nolint
+	for _, result := range checks[1:] {
+		if result.Score < worseResult.Score {
+			worseResult = result
+		}
+	}
+	return worseResult
 }
 
 func MakeInconclusiveResult(name string, err error) CheckResult {
 	return CheckResult{
 		Name:       name,
 		Pass:       false,
 		Confidence: 0,
-		Error:      scorecarderrors.MakeZeroConfidenceError(err),
+		Error:      scorecarderrors.MakeLowConfidenceError(err),
 	}
 }
 
@@ -48,6 +217,7 @@ func MakePassResult(name string) CheckResult {
 		Name:       name,
 		Pass:       true,
 		Confidence: MaxResultConfidence,
+		Error:      nil,
 	}
 }
 
@@ -98,6 +268,7 @@ func MakeProportionalResult(name string, numerator int, denominator int,
 }
 
 // Given a min result, check if another result is worse.
+//nolint
 func isMinResult(result, min CheckResult) bool {
 	if Bool2int(result.Pass) < Bool2int(min.Pass) {
 		return true
@@ -117,7 +288,8 @@ func MakeAndResult(checks ...CheckResult) CheckResult {
 		Pass:       true,
 		Confidence: MaxResultConfidence,
 	}
-
+	// UPGRADEv2: will go away after old struct is removed.
+	//nolint
 	for _, result := range checks {
 		if minResult.Name == "" {
 			minResult.Name = result.Name

checker/check_runner.go

@@ -39,15 +39,34 @@ type CheckFn func(*CheckRequest) CheckResult
 
 type CheckNameToFnMap map[string]CheckFn
 
+// UPGRADEv2: messages2 will ultimately
+// be renamed to messages.
 type logger struct {
-	messages []string
+	messages  []string
+	messages2 []CheckDetail
 }
 
+func (l *logger) Info(desc string, args ...interface{}) {
+	cd := CheckDetail{Type: DetailInfo, Msg: fmt.Sprintf(desc, args...)}
+	l.messages2 = append(l.messages2, cd)
+}
+
+func (l *logger) Warn(desc string, args ...interface{}) {
+	cd := CheckDetail{Type: DetailWarn, Msg: fmt.Sprintf(desc, args...)}
+	l.messages2 = append(l.messages2, cd)
+}
+
+func (l *logger) Debug(desc string, args ...interface{}) {
+	cd := CheckDetail{Type: DetailDebug, Msg: fmt.Sprintf(desc, args...)}
+	l.messages2 = append(l.messages2, cd)
+}
+
+// UPGRADEv2: to remove.
 func (l *logger) Logf(s string, f ...interface{}) {
 	l.messages = append(l.messages, fmt.Sprintf(s, f...))
 }
 
-func logStats(ctx context.Context, startTime time.Time, result CheckResult) error {
+func logStats(ctx context.Context, startTime time.Time, result *CheckResult) error {
 	runTimeInSecs := time.Now().Unix() - startTime.Unix()
 	opencensusstats.Record(ctx, stats.CheckRuntimeInSec.M(runTimeInSecs))
 
@@ -74,17 +93,22 @@ func (r *Runner) Run(ctx context.Context, f CheckFn) CheckResult {
 		checkRequest := r.CheckRequest
 		checkRequest.Ctx = ctx
 		l = logger{}
+		// UPGRADEv2: to remove.
 		checkRequest.Logf = l.Logf
+		checkRequest.Dlogger = &l
 		res = f(&checkRequest)
+		// UPGRADEv2: to fix using proper error check.
 		if res.ShouldRetry && !strings.Contains(res.Error.Error(), "invalid header field value") {
 			checkRequest.Logf("error, retrying: %s", res.Error)
 			continue
 		}
 		break
 	}
+	// UPGRADEv2: to remove.
 	res.Details = l.messages
+	res.Details2 = l.messages2
 
-	if err := logStats(ctx, startTime, res); err != nil {
+	if err := logStats(ctx, startTime, &res); err != nil {
 		panic(err)
 	}
 	return res
@@ -97,6 +121,39 @@ func Bool2int(b bool) int {
 	return 0
 }
 
+// UPGRADEv2: will be renamed.
+func MultiCheckOr2(fns ...CheckFn) CheckFn {
+	return func(c *CheckRequest) CheckResult {
+		//nolint
+		maxResult := CheckResult{Version: 2}
+
+		for _, fn := range fns {
+			result := fn(c)
+
+			if result.Score > maxResult.Score {
+				maxResult = result
+			}
+
+			if maxResult.Score >= MaxResultScore {
+				break
+			}
+		}
+		return maxResult
+	}
+}
+
+func MultiCheckAnd2(fns ...CheckFn) CheckFn {
+	return func(c *CheckRequest) CheckResult {
+		var checks []CheckResult
+		for _, fn := range fns {
+			res := fn(c)
+			checks = append(checks, res)
+		}
+		return MakeAndResult2(checks...)
+	}
+}
+
+// UPGRADEv2: will be removed.
 // MultiCheckOr returns the best check result out of several ones performed.
 func MultiCheckOr(fns ...CheckFn) CheckFn {
 	return func(c *CheckRequest) CheckResult {

checker/check_test.go

@@ -27,8 +27,8 @@ func TestMakeCheckAnd(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
 		name   string
-		checks []CheckResult
 		want   CheckResult
+		checks []CheckResult
 	}{
 		{
 			name: "Multiple passing",

checks/branch_protected_test.go

@@ -88,6 +88,7 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { //nolint:tparallel // mock
 	rel1 := "release/v.1"
 	sha := "8fb3cb86082b17144a80402f5367ae65f06083bd"
 	main := "main"
+	//nolint
 	tests := []struct {
 		name          string
 		branches      []*string