From dd081f89f393b4a62e223811ede209e8cb8b0195 Mon Sep 17 00:00:00 2001 From: Dominik Richter Date: Thu, 25 Jul 2024 11:11:36 -0700 Subject: [PATCH] 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 --- policy/score_calculator.go | 29 ++++++- policy/score_calculator_test.go | 133 +++++++++++++++++++++++++++++++- 2 files changed, 155 insertions(+), 7 deletions(-) diff --git a/policy/score_calculator.go b/policy/score_calculator.go index 4223d2da..647cba39 100644 --- 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 index bb5bc40a..4238b6f1 100644 --- 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() +}