From 55b6134be1c2affe48587d80a40684df91d59fbf Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Thu, 11 Mar 2021 18:01:19 +0300 Subject: [PATCH] Improves UTs coverage in shared/aggregation (#8593) * improves shared/aggregation test suite * improves coverage in shared/aggregation/attestations --- shared/aggregation/aggregation.go | 6 -- .../attestations/attestations_test.go | 64 +++++++++++++++++++ shared/aggregation/maxcover.go | 11 ---- shared/aggregation/maxcover_test.go | 35 ++++++++++ 4 files changed, 99 insertions(+), 17 deletions(-) diff --git a/shared/aggregation/aggregation.go b/shared/aggregation/aggregation.go index 29f68578f7af..f36db62c2a25 100644 --- a/shared/aggregation/aggregation.go +++ b/shared/aggregation/aggregation.go @@ -3,7 +3,6 @@ package aggregation import ( "errors" - "fmt" "github.com/prysmaticlabs/go-bitfield" "github.com/sirupsen/logrus" @@ -29,8 +28,3 @@ type Aggregation struct { // Keys is a list of candidate keys in the best available solution. Keys []int } - -// String provides string representation of a solution. -func (ag *Aggregation) String() string { - return fmt.Sprintf("{coverage: %#b, keys: %v}", ag.Coverage, ag.Keys) -} diff --git a/shared/aggregation/attestations/attestations_test.go b/shared/aggregation/attestations/attestations_test.go index eccbba699ce5..26c3405800c2 100644 --- a/shared/aggregation/attestations/attestations_test.go +++ b/shared/aggregation/attestations/attestations_test.go @@ -248,6 +248,70 @@ func TestAggregateAttestations_Aggregate(t *testing.T) { runner() }) } + + t.Run("invalid strategy", func(t *testing.T) { + resetCfg := featureconfig.InitWithReset(&featureconfig.Flags{ + AttestationAggregationStrategy: "foobar", + }) + defer resetCfg() + _, err := Aggregate(aggtesting.MakeAttestationsFromBitlists([]bitfield.Bitlist{})) + assert.ErrorContains(t, "\"foobar\": invalid aggregation strategy", err) + }) + + t.Run("broken attestation bitset", func(t *testing.T) { + wantErr := "bitlist cannot be nil or empty: invalid max_cover problem" + t.Run(string(MaxCoverAggregation), func(t *testing.T) { + resetCfg := featureconfig.InitWithReset(&featureconfig.Flags{ + AttestationAggregationStrategy: string(MaxCoverAggregation), + }) + defer resetCfg() + _, err := Aggregate(aggtesting.MakeAttestationsFromBitlists([]bitfield.Bitlist{ + {0b00000011, 0b0}, + {0b00000111, 0b100}, + {0b00000100, 0b1}, + })) + assert.ErrorContains(t, wantErr, err) + }) + t.Run(string(OptMaxCoverAggregation), func(t *testing.T) { + resetCfg := featureconfig.InitWithReset(&featureconfig.Flags{ + AttestationAggregationStrategy: string(OptMaxCoverAggregation), + }) + defer resetCfg() + _, err := Aggregate(aggtesting.MakeAttestationsFromBitlists([]bitfield.Bitlist{ + {0b00000011, 0b0}, + {0b00000111, 0b100}, + {0b00000100, 0b1}, + })) + assert.ErrorContains(t, wantErr, err) + }) + }) + + t.Run("candidate swapping when aggregating", func(t *testing.T) { + // The first item cannot be aggregated, and should be pushed down the list, + // by two swaps with aggregated items (aggregation is done in-place, so the very same + // underlying array is used for storing both aggregated and non-aggregated items). + resetCfg := featureconfig.InitWithReset(&featureconfig.Flags{ + AttestationAggregationStrategy: string(OptMaxCoverAggregation), + }) + defer resetCfg() + got, err := Aggregate(aggtesting.MakeAttestationsFromBitlists([]bitfield.Bitlist{ + {0b10000000, 0b1}, + {0b11000101, 0b1}, + {0b00011000, 0b1}, + {0b01010100, 0b1}, + {0b10001010, 0b1}, + })) + want := []bitfield.Bitlist{ + {0b11011101, 0b1}, + {0b11011110, 0b1}, + {0b10000000, 0b1}, + } + assert.NoError(t, err) + assert.Equal(t, len(want), len(got)) + for i, w := range want { + assert.DeepEqual(t, w.Bytes(), got[i].AggregationBits.Bytes()) + } + }) } func TestAggregateAttestations_PerformanceComparison(t *testing.T) { diff --git a/shared/aggregation/maxcover.go b/shared/aggregation/maxcover.go index 2b0a92e89275..dec029778cf7 100644 --- a/shared/aggregation/maxcover.go +++ b/shared/aggregation/maxcover.go @@ -1,7 +1,6 @@ package aggregation import ( - "fmt" "sort" "github.com/pkg/errors" @@ -79,11 +78,6 @@ func (mc *MaxCoverProblem) Cover(k int, allowOverlaps bool) (*Aggregation, error break } if !candidate.processed { - if !allowOverlaps && solution.Coverage.Overlaps(*candidate.bits) { - // Overlapping candidates violate non-intersection invariant. - candidate.processed = true - continue - } solution.Coverage = solution.Coverage.Or(*candidate.bits) solution.Keys = append(solution.Keys, candidate.key) remainingBits = remainingBits.And(candidate.bits.Not()) @@ -240,8 +234,3 @@ func union(candidates []*bitfield.Bitlist64) *bitfield.Bitlist64 { } return ret } - -// String provides string representation of a candidate. -func (c *MaxCoverCandidate) String() string { - return fmt.Sprintf("{%v, %#b, s%d, %t}", c.key, c.bits, c.score, c.processed) -} diff --git a/shared/aggregation/maxcover_test.go b/shared/aggregation/maxcover_test.go index 4f2e0d5b41bb..89aa52b9fa83 100644 --- a/shared/aggregation/maxcover_test.go +++ b/shared/aggregation/maxcover_test.go @@ -475,6 +475,32 @@ func TestMaxCover_MaxCoverProblem_Cover(t *testing.T) { Keys: []int{0, 3, 1, 2, 6}, }, }, + { + name: "empty bitlists", + args: args{k: 5, allowOverlaps: true, candidates: MaxCoverCandidates{ + {1, &bitfield.Bitlist{0b0}, 0, false}, + {0, &bitfield.Bitlist{0b00000000, 0b00000000, 0b00000000, 0b1}, 0, false}, + {2, &bitfield.Bitlist{0b00000000, 0b00000000, 0b00000000, 0b1}, 0, false}, + }}, + wantedErr: "empty bitlists: invalid max_cover problem", + }, + { + name: "overlapping solution dropped", + args: args{k: 5, allowOverlaps: false, candidates: MaxCoverCandidates{ + {0, &bitfield.Bitlist{0b11111111, 0b11000111, 0b11111111, 0b1}, 0, false}, + // All remaining bitlists will overlap, so will be dropped. + {1, &bitfield.Bitlist{0b11111111, 0b00001100, 0b11111111, 0b1}, 0, false}, + {2, &bitfield.Bitlist{0b00000000, 0b01110000, 0b01110000, 0b1}, 0, false}, + {3, &bitfield.Bitlist{0b00000111, 0b10000001, 0b10000000, 0b1}, 0, false}, + {4, &bitfield.Bitlist{0b00000000, 0b00000110, 0b00000110, 0b1}, 0, false}, + {5, &bitfield.Bitlist{0b00000000, 0b00000001, 0b01100010, 0b1}, 0, false}, + {6, &bitfield.Bitlist{0b00001000, 0b00001000, 0b10000010, 0b1}, 0, false}, + }}, + want: &Aggregation{ + Coverage: bitfield.Bitlist{0xff, 0b11000111, 0xff, 0b1}, + Keys: []int{0}, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -625,6 +651,15 @@ func TestMaxCover_MaxCover(t *testing.T) { Keys: []int{0, 1, 2, 3, 6}, }, }, + { + name: "empty bitlists", + args: args{k: 5, allowOverlaps: true, candidates: []*bitfield.Bitlist64{ + bitfield.NewBitlist64From([]uint64{}), + bitfield.NewBitlist64From([]uint64{0b00000000, 0b00001110, 0b00001110}), + bitfield.NewBitlist64From([]uint64{0b00000000, 0b01110000, 0b01110000}), + }}, + wantedErr: "empty bitlists: invalid max_cover problem", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {