critical: fix banded scoring on variation within bands (#1385)

Variations within bands were inverted and led to incorrect results on the edges of those variations i.e. if all scores within a band were failing or only one was failing. The tests reflect have been added to test out this behavior of variation within bands and will be further expanded in the future. Signed-off-by: Dominik Richter <dominik.richter@gmail.com>
mondoohq · Jul 25, 2024 · dd081f8 · dd081f8
1 parent 59fcc6d
commit dd081f8
Show file tree

Hide file tree

Showing 2 changed files with 155 additions and 7 deletions.
diff --git a/policy/score_calculator.go b/policy/score_calculator.go
@@ -17,6 +17,7 @@ type ScoreCalculator interface {
 	Add(score *Score, impact *explorer.Impact)
 	Calculate() *Score
 	Init()
+	String() string
 }
 
 type averageScoreCalculator struct {
@@ -32,6 +33,10 @@ type averageScoreCalculator struct {
 	featureFlagFailErrors bool
 }
 
+func (c *averageScoreCalculator) String() string {
+	return "Average"
+}
+
 func (c *averageScoreCalculator) Init() {
 	c.value = 0
 	c.weight = 0
@@ -216,6 +221,10 @@ type weightedScoreCalculator struct {
 	featureFlagFailErrors bool
 }
 
+func (c *weightedScoreCalculator) String() string {
+	return "Weighted Average"
+}
+
 func (c *weightedScoreCalculator) Init() {
 	c.value = 0
 	c.weight = 0
@@ -327,6 +336,10 @@ type worstScoreCalculator struct {
 	featureFlagFailErrors bool
 }
 
+func (c *worstScoreCalculator) String() string {
+	return "Highest Impact"
+}
+
 func (c *worstScoreCalculator) Init() {
 	c.value = 100
 	c.weight = 0
@@ -450,6 +463,10 @@ type bandedScoreCalculator struct {
 	featureFlagFailErrors bool
 }
 
+func (c *bandedScoreCalculator) String() string {
+	return "Banded"
+}
+
 func (c *bandedScoreCalculator) Init() {
 	c.minscore = 100
 	c.value = 100
@@ -570,21 +587,21 @@ func (c *bandedScoreCalculator) Calculate() *Score {
 
 		pcrLow := float64(1)
 		if c.lowMax != 0 {
-			pcrLow = float64(c.low) / float64(c.lowMax)
+			pcrLow = float64(c.lowMax-c.low) / float64(c.lowMax)
 		}
 		fMid := float64(1)
 		if c.midMax != 0 {
-			fMid = float64(c.mid) / float64(c.midMax)
+			fMid = float64(c.midMax-c.mid) / float64(c.midMax)
 		}
 		pcrMid := (3 + pcrLow) / 4 * fMid
 		fHigh := float64(1)
 		if c.highMax != 0 {
-			fHigh = float64(c.high) / float64(c.highMax)
+			fHigh = float64(c.highMax-c.high) / float64(c.highMax)
 		}
 		pcrHigh := (1 + pcrMid) / 2 * fHigh
 		fCrit := float64(1)
 		if c.critMax != 0 {
-			fCrit = float64(c.crit) / float64(c.critMax)
+			fCrit = float64(c.critMax-c.crit) / float64(c.critMax)
 		}
 		pcrCrit := (1 + 4*pcrHigh) / 5 * fCrit
 
@@ -622,6 +639,10 @@ type decayedScoreCalculator struct {
 	featureFlagFailErrors bool
 }
 
+func (c *decayedScoreCalculator) String() string {
+	return "Decayed"
+}
+
 var gravity float64 = 10
 
 func (c *decayedScoreCalculator) Init() {

diff --git a/policy/score_calculator_test.go b/policy/score_calculator_test.go
@@ -4,6 +4,7 @@
 package policy
 
 import (
+	"fmt"
 	"strconv"
 	"testing"
 
@@ -222,7 +223,7 @@ func TestBandedScores(t *testing.T) {
 				{Value: &explorer.ImpactValue{Value: 100}},
 				{Action: explorer.Action_IGNORE},
 			},
-			out: &Score{Value: 22, ScoreCompletion: 100, DataCompletion: 66, Weight: 10, Type: ScoreType_Result},
+			out: &Score{Value: 25, ScoreCompletion: 100, DataCompletion: 66, Weight: 10, Type: ScoreType_Result},
 		},
 		{
 			in: []*Score{
@@ -239,7 +240,7 @@ func TestBandedScores(t *testing.T) {
 				// 10 high checks
 				{Value: &explorer.ImpactValue{Value: 80}},
 			},
-			out: &Score{Value: 1, ScoreCompletion: 100, DataCompletion: 66, Weight: 20, Type: ScoreType_Result},
+			out: &Score{Value: 45, ScoreCompletion: 100, DataCompletion: 66, Weight: 20, Type: ScoreType_Result},
 		},
 		{
 			in: []*Score{
@@ -273,7 +274,7 @@ func TestBandedScores(t *testing.T) {
 				// 10 high checks
 				{Value: &explorer.ImpactValue{Value: 80}},
 			},
-			out: &Score{Value: 5, ScoreCompletion: 100, DataCompletion: 66, Weight: 20, Type: ScoreType_Result},
+			out: &Score{Value: 9, ScoreCompletion: 100, DataCompletion: 66, Weight: 20, Type: ScoreType_Result},
 		},
 	})
 }
@@ -404,3 +405,129 @@ func TestImpact(t *testing.T) {
 		require.EqualValues(t, 80, int(s.Value))
 	})
 }
+
+func addMultipleScores(impact uint32, failed int, passed int, calculator ScoreCalculator) {
+	for i := 0; i < failed; i++ {
+		calculator.Add(&Score{
+			Value:           100 - impact,
+			ScoreCompletion: 100,
+			DataCompletion:  100,
+			DataTotal:       1,
+			Weight:          1,
+			Type:            ScoreType_Result,
+		}, &explorer.Impact{
+			Value: &explorer.ImpactValue{
+				Value: int32(impact),
+			},
+		})
+	}
+	for i := 0; i < passed; i++ {
+		calculator.Add(&Score{
+			Value:           100,
+			ScoreCompletion: 100,
+			DataCompletion:  100,
+			DataTotal:       1,
+			Weight:          1,
+			Type:            ScoreType_Result,
+		}, &explorer.Impact{
+			Value: &explorer.ImpactValue{
+				Value: int32(impact),
+			},
+		})
+	}
+}
+
+func addTestResults(critFail, critPass, highFail, highPass, midFail, midPass, lowFail, lowPass int, calculator ScoreCalculator) {
+	addMultipleScores(100, critFail, critPass, calculator)
+	addMultipleScores(80, highFail, highPass, calculator)
+	addMultipleScores(55, midFail, midPass, calculator)
+	addMultipleScores(20, lowFail, lowPass, calculator)
+}
+
+func testEachScoreCalculator(critFail, critCnt, highFail, highCnt, midFail, midCnt, lowFail, lowCnt int, t *testing.T) []*Score {
+	require.LessOrEqual(t, critFail, critCnt)
+	require.LessOrEqual(t, highFail, highCnt)
+	require.LessOrEqual(t, midFail, midCnt)
+	require.LessOrEqual(t, lowFail, lowCnt)
+
+	fmt.Printf("Results: %d/%d CRIT   %d/%d HIGH   %d/%d MID   %d/%d LOW   %d/%d Total\n",
+		critFail, critCnt,
+		highFail, highCnt,
+		midFail, midCnt,
+		lowFail, lowCnt,
+		critFail+highFail+midFail+lowFail, critCnt+highCnt+midCnt+lowCnt,
+	)
+	calculators := []ScoreCalculator{
+		&averageScoreCalculator{},
+		&bandedScoreCalculator{},
+		&decayedScoreCalculator{},
+		&worstScoreCalculator{},
+	}
+
+	var results []*Score
+	for i := range calculators {
+		calculator := calculators[i]
+		calculator.Init()
+
+		addTestResults(critFail, critCnt-critFail, highFail, highCnt-highFail, midFail, midCnt-midFail, lowFail, lowCnt-lowFail, calculator)
+
+		score := calculator.Calculate()
+		results = append(results, score)
+		require.NotNil(t, score)
+		fmt.Printf("    %20s: %3d  (%s)\n", calculator.String(), score.Value, score.Rating().FailureLabel())
+	}
+	fmt.Println("")
+
+	return results
+}
+
+func TestCalculatorBehavior_Banded(t *testing.T) {
+	var res []*Score
+	t.Run("critical band", func(t *testing.T) {
+		res = testEachScoreCalculator(1, 10, 0, 10, 0, 10, 0, 10, t)
+		assert.Equal(t, 45, int(res[1].Value))
+		res = testEachScoreCalculator(5, 10, 0, 10, 0, 10, 0, 10, t)
+		assert.Equal(t, 25, int(res[1].Value))
+		res = testEachScoreCalculator(10, 10, 0, 10, 0, 10, 0, 10, t)
+		assert.Equal(t, 0, int(res[1].Value))
+	})
+	t.Run("critical band with high band variation", func(t *testing.T) {
+		res = testEachScoreCalculator(1, 10, 1, 10, 0, 10, 0, 10, t)
+		assert.Equal(t, 41, int(res[1].Value))
+		res = testEachScoreCalculator(1, 10, 5, 10, 0, 10, 0, 10, t)
+		assert.Equal(t, 27, int(res[1].Value))
+		res = testEachScoreCalculator(1, 10, 10, 10, 0, 10, 0, 10, t)
+		assert.Equal(t, 9, int(res[1].Value))
+	})
+	t.Run("high band", func(t *testing.T) {
+		res = testEachScoreCalculator(0, 10, 1, 10, 0, 10, 0, 10, t)
+		assert.Equal(t, 55, int(res[1].Value))
+		res = testEachScoreCalculator(0, 10, 5, 10, 0, 10, 0, 10, t)
+		assert.Equal(t, 35, int(res[1].Value))
+		res = testEachScoreCalculator(0, 10, 10, 10, 0, 10, 0, 10, t)
+		assert.Equal(t, 10, int(res[1].Value))
+	})
+}
+
+// This is a toolchain that can be used to see what the scoring mechanism would produce under different scenarios
+func TestScoreMechanisms(t *testing.T) {
+	testEachScoreCalculator(1, 2, 1, 2, 2, 3, 1, 3, t)
+
+	testEachScoreCalculator(5, 10, 10, 15, 2, 30, 1, 45, t)
+
+	testEachScoreCalculator(5, 10, 5, 15, 2, 30, 1, 45, t)
+
+	testEachScoreCalculator(2, 10, 4, 15, 2, 30, 1, 45, t)
+
+	testEachScoreCalculator(1, 10, 4, 15, 2, 30, 1, 45, t)
+
+	testEachScoreCalculator(10, 10, 4, 15, 2, 30, 1, 45, t)
+
+	testEachScoreCalculator(0, 1, 1, 9, 0, 23, 0, 0, t)
+
+	testEachScoreCalculator(0, 1, 3, 9, 0, 23, 0, 0, t)
+
+	testEachScoreCalculator(0, 2, 1, 2, 2, 3, 1, 3, t)
+
+	// t.FailNow()
+}