Skip to content

Commit fbbc5a6

Browse files
authored
attribute: make TestHashKVs linear-time (#8204)
Unblocks #8166 Per #8166 (comment) ## Why This updates `TestHashKVsEquality` to avoid the quadratic pairwise comparison that was causing slow runs and timeouts in CI. Below are the results from my machine. Old: ``` $ go test -run=TestHashKVs -count=1 PASS ok go.opentelemetry.io/otel/attribute 2.063s ``` New: ``` $ go test -run=TestHashKVs -count=1 PASS ok go.opentelemetry.io/otel/attribute 0.024s ``` Instead of collecting every generated testcase and comparing each hash against every other hash, the test now checks uniqueness as each case is generated by storing previously seen hashes in a map. Note that it also does not use the equality operator of `KeyValue` to determine if the hash should be equal or different. ## What - Rename the test from `TestHashKVsEquality` to `TestHashKVs` - Replace the O(n^2) post-processing loop with an O(n) streaming uniqueness check - Simplify failure reporting by removing the dedicated `msg` helper Side note: for me it also makes the test more readable (however, this is opinionated).
1 parent 3ee1d5a commit fbbc5a6

1 file changed

Lines changed: 24 additions & 45 deletions

File tree

attribute/hash_test.go

Lines changed: 24 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -41,82 +41,61 @@ var keyVals = []func(string) KeyValue{
4141
func(k string) KeyValue { return KeyValue{Key: Key(k)} }, // Empty value.
4242
}
4343

44-
func TestHashKVsEquality(t *testing.T) {
44+
func TestHashKVs(t *testing.T) {
4545
type testcase struct {
46+
num int
4647
hash uint64
4748
kvs []KeyValue
4849
}
4950

5051
keys := []string{"k0", "k1"}
5152

52-
// Test all combinations up to length 3.
53-
n := len(keyVals)
54-
result := make([]testcase, 0, 1+len(keys)*(n+(n*n)+(n*n*n)))
53+
// Track hashes as we generate them so collision detection stays linear.
54+
i := 0
55+
seen := make(map[uint64]testcase)
56+
assertUniqueHash := func(kvs []KeyValue) {
57+
hash := hashKVs(kvs)
58+
tc := testcase{num: i, hash: hash, kvs: kvs}
59+
i++
60+
61+
if prev, ok := seen[hash]; ok {
62+
t.Errorf("hashes equal: (%d: %d)%s == (%d: %d)%s",
63+
prev.num, prev.hash, slice(prev.kvs), tc.num, tc.hash, slice(tc.kvs))
64+
return
65+
}
66+
67+
seen[hash] = tc
68+
}
5569

56-
result = append(result, testcase{hashKVs(nil), nil})
70+
// Test empty slice.
71+
assertUniqueHash(nil)
5772

73+
// Test all combinations of 1, 2, and 3 attributes with different keys and values.
5874
for _, key := range keys {
5975
for i := range keyVals {
6076
kvs := []KeyValue{keyVals[i](key)}
61-
hash := hashKVs(kvs)
62-
result = append(result, testcase{hash, kvs})
77+
assertUniqueHash(kvs)
6378

6479
for j := range keyVals {
6580
kvs := []KeyValue{
6681
keyVals[i](key),
6782
keyVals[j](key),
6883
}
69-
hash := hashKVs(kvs)
70-
result = append(result, testcase{hash, kvs})
84+
assertUniqueHash(kvs)
7185

7286
for k := range keyVals {
7387
kvs := []KeyValue{
7488
keyVals[i](key),
7589
keyVals[j](key),
7690
keyVals[k](key),
7791
}
78-
hash := hashKVs(kvs)
79-
result = append(result, testcase{hash, kvs})
80-
}
81-
}
82-
}
83-
}
84-
85-
for i := 0; i < len(result); i++ {
86-
hI, kvI := result[i].hash, result[i].kvs
87-
for j := 0; j < len(result); j++ {
88-
hJ, kvJ := result[j].hash, result[j].kvs
89-
m := msg{i: i, j: j, hI: hI, hJ: hJ, kvI: kvI, kvJ: kvJ}
90-
if i == j {
91-
m.cmp = "=="
92-
if hI != hJ {
93-
t.Errorf("hashes not equal: %s", m)
94-
}
95-
} else {
96-
m.cmp = "!="
97-
if hI == hJ {
98-
// Do not use testify/assert here. It is slow.
99-
t.Errorf("hashes equal: %s", m)
92+
assertUniqueHash(kvs)
10093
}
10194
}
10295
}
10396
}
10497
}
10598

106-
type msg struct {
107-
cmp string
108-
i, j int
109-
hI, hJ uint64
110-
kvI, kvJ []KeyValue
111-
}
112-
113-
func (m msg) String() string {
114-
return fmt.Sprintf(
115-
"(%d: %d)%s %s (%d: %d)%s",
116-
m.i, m.hI, slice(m.kvI), m.cmp, m.j, m.hJ, slice(m.kvJ),
117-
)
118-
}
119-
12099
func slice(kvs []KeyValue) string {
121100
if len(kvs) == 0 {
122101
return "[]"

0 commit comments

Comments
 (0)