Skip to content

Commit 0c1b1bc

Browse files
committed
fix(measurements): a zero-value ledger survives a contradiction, not just a record
Reported by @jatmn, and it is a hole in my own previous fix rather than a new defect. That fix made a declared-but-not-constructed Ledger survive Record by initialising the two maps Record touches. It left `raised` nil. Only Conflicts and ConflictsAcrossRuns write that map, and they write it exclusively on the contradiction path — the dedupe that stops the same wrong number being reported twice — so nothing in the Record path could reach it. The test I wrote could not catch this. It asked an AGREEING claim: ledger.Conflicts(Run{}, "TestSomething took 1.25s") // against a recorded 1.25s which returns early with no conflict and never reaches the write. A contradicting claim panics: var ledger Ledger ledger.Record(Run{}, "--- PASS: TestSomething (1.25s)\n") ledger.Conflicts(Run{}, "TestSomething took 99s") -> panic: assignment to entry in nil map Centralised into ensureMaps rather than adding a third `if` beside the other two. The field list now sits beside NewLedger's, so a fourth map added later is a visible omission in one place instead of a panic in whichever entry point forgot it — this is the second time these lists have drifted apart. TestAZeroValueLedgerSurvivesAContradiction covers both conflict entry points and also asserts the dedupe actually works, since that is what `raised` is for. Honest note on the mutation checks: removing `raised` from ensureMaps panics the new test, so the real defect is covered. Removing the ensureMaps call from ConflictsAcrossRuns does NOT fail anything — neither conflict path can write `raised` without data having been recorded first, and Record initialises. Those two calls are defence-in-depth for a future entry point, not independently reachable today.
1 parent af1294a commit 0c1b1bc

2 files changed

Lines changed: 58 additions & 12 deletions

File tree

‎internal/measurements/measurements.go‎

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,33 @@ func NewLedger() *Ledger {
208208
}
209209
}
210210

211+
// ensureMaps makes a zero-value Ledger behave like a constructed one.
212+
//
213+
// ONE PLACE, NOT THREE. The nil receiver is handled separately, but a Ledger
214+
// that was DECLARED rather than built by NewLedger reached the map writes with
215+
// nil maps and panicked. The first attempt at this fixed the two maps Record
216+
// touches and missed `raised`, which only Conflicts and ConflictsAcrossRuns
217+
// write — and they write it exclusively on the contradiction path, so a test
218+
// asking an agreeing claim never reached it. Reported by @jatmn.
219+
//
220+
// Every caller that can write a map calls this, and the field list lives beside
221+
// NewLedger's, so a fourth map added later is a compile-time-visible omission in
222+
// one spot rather than a panic in whichever entry point forgot it.
223+
//
224+
// Callers hold l.mu; this only fills nil fields, so a constructed Ledger pays
225+
// three nil checks and nothing else.
226+
func (l *Ledger) ensureMaps() {
227+
if l.observed == nil {
228+
l.observed = map[string]map[string][]float64{}
229+
}
230+
if l.runs == nil {
231+
l.runs = map[string]Run{}
232+
}
233+
if l.raised == nil {
234+
l.raised = map[raisedKey]bool{}
235+
}
236+
}
237+
211238
// Record reads any timings out of a command's output and remembers them against
212239
// the run that produced it. Returns how many it took, which is what a test
213240
// asserts on.
@@ -226,18 +253,7 @@ func (l *Ledger) Record(run Run, text string) int {
226253
l.mu.Lock()
227254
defer l.mu.Unlock()
228255
key := run.key()
229-
// A LEDGER THAT WAS NEVER CONSTRUCTED STILL HAS TO BEHAVE. The nil receiver
230-
// above is already handled, but a zero-value Ledger got past it and panicked
231-
// with "assignment to entry in nil map" on the first Record — a value that is
232-
// trivially easy to reach by declaring one rather than calling NewLedger.
233-
// Allocating here costs nothing on the NewLedger path, where both maps are
234-
// already non-nil.
235-
if l.observed == nil {
236-
l.observed = map[string]map[string][]float64{}
237-
}
238-
if l.runs == nil {
239-
l.runs = map[string]Run{}
240-
}
256+
l.ensureMaps()
241257
byName := l.observed[key]
242258
if byName == nil {
243259
byName = map[string][]float64{}
@@ -278,6 +294,7 @@ func (l *Ledger) Conflicts(run Run, claim string) []Conflict {
278294
return nil
279295
}
280296
l.mu.Lock()
297+
l.ensureMaps()
281298
defer l.mu.Unlock()
282299

283300
// ONLY THIS RUN'S VALUES. A claim about `go test ./...` is not answered by a
@@ -695,6 +712,7 @@ func (l *Ledger) ConflictsAcrossRuns(claim string) []Conflict {
695712
return nil
696713
}
697714
l.mu.Lock()
715+
l.ensureMaps()
698716
defer l.mu.Unlock()
699717

700718
// Names are gathered across runs first, so a name recorded by two commands

‎internal/measurements/measurements_test.go‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -823,3 +823,31 @@ func TestAZeroValueLedgerRecordsWithoutPanicking(t *testing.T) {
823823
t.Errorf("a zero-value ledger reported a conflict against its own record: %+v", conflicts)
824824
}
825825
}
826+
827+
// THE AGREEING CLAIM ABOVE NEVER REACHES THE MAP THAT PANICS. `raised` is
828+
// written only when a contradiction is actually found — it is the dedupe that
829+
// stops the same wrong number being reported twice — so a test that asks a
830+
// truthful claim returns early and proves nothing about it. The first fix for
831+
// the zero-value Ledger initialised the two maps Record touches and left
832+
// `raised` nil, and this test passed anyway. Reported by @jatmn.
833+
//
834+
// Both conflict entry points are covered: they write different key shapes into
835+
// the same map, so one of them holding says nothing about the other.
836+
func TestAZeroValueLedgerSurvivesAContradiction(t *testing.T) {
837+
var single Ledger
838+
single.Record(Run{}, "--- PASS: TestSomething (1.25s)\n")
839+
conflicts := single.Conflicts(Run{}, "TestSomething took 99s")
840+
if len(conflicts) != 1 || conflicts[0].Claimed != 99 {
841+
t.Errorf("a zero-value ledger did not report the contradiction: %+v", conflicts)
842+
}
843+
// And the dedupe it just wrote actually works, which is what that map is for.
844+
if again := single.Conflicts(Run{}, "TestSomething took 99s"); len(again) != 0 {
845+
t.Errorf("the same wrong number was reported twice: %+v", again)
846+
}
847+
848+
var across Ledger
849+
across.Record(Run{}, "--- PASS: TestSomething (1.25s)\n")
850+
if conflicts := across.ConflictsAcrossRuns("TestSomething took 99s"); len(conflicts) != 1 {
851+
t.Errorf("a zero-value ledger did not report the contradiction across runs: %+v", conflicts)
852+
}
853+
}

0 commit comments

Comments
 (0)