From 15546f9d83dfd4cbbb3956745b0f2d20d150e4c6 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 11 Oct 2026 00:49:38 +0200 Subject: [PATCH] messenger: count the store's writes, and name an ask's record when it is the only one held (review of issue 422) The review's two nits. A write of bytes identical to what is stored was invisible to a comparison of content, so the test store counts its writes and the preview's tests read that count: a persist from inside Preview now fails five of them instead of one. And a router holding an ask's record and no condition said it held none, rather than saying what that key is; the branches are the other way round, with a test that holds nothing else. --- .../messenger/cmd/messenger/holder_test.go | 3 +++ modules/messenger/cmd/messenger/preview.go | 7 ++++-- .../messenger/cmd/messenger/preview_test.go | 23 +++++++++++++++---- 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/modules/messenger/cmd/messenger/holder_test.go b/modules/messenger/cmd/messenger/holder_test.go index 68092431..f4be6e3d 100644 --- a/modules/messenger/cmd/messenger/holder_test.go +++ b/modules/messenger/cmd/messenger/holder_test.go @@ -66,6 +66,8 @@ func (f *fakeChannel) Edit(id string, m Message) error { type memStore struct { recs map[string]Record recent []Sent + // puts counts the writes, so a write of bytes identical to what is stored is still seen (issue 422's review). + puts int } func (m *memStore) Put(r Record) error { @@ -73,6 +75,7 @@ func (m *memStore) Put(r Record) error { m.recs = map[string]Record{} } m.recs[r.Key] = r + m.puts++ return nil } func (m *memStore) Delete(k string) error { delete(m.recs, k); return nil } diff --git a/modules/messenger/cmd/messenger/preview.go b/modules/messenger/cmd/messenger/preview.go index 4ab44aa3..caa40e94 100644 --- a/modules/messenger/cmd/messenger/preview.go +++ b/modules/messenger/cmd/messenger/preview.go @@ -68,11 +68,14 @@ func (h *Holder) Preview(key, what string) (map[string]any, error) { if !holding || r.Origin != "condition" { sort.Strings(conditions) switch { - case len(conditions) == 0: - return nil, errorf("no condition %s is open here, and this router holds none: `mesh-controller.conditions` says what the controller holds open", key) + case holding && len(conditions) == 0: + return nil, errorf("%s is not a condition but this router's own %s record, which is said as part of an ask, never on its own; it holds no condition", + key, r.Origin) case holding: return nil, errorf("%s is not a condition but this router's own %s record, which is said as part of an ask, never on its own; it holds the conditions %s", key, r.Origin, strings.Join(conditions, ", ")) + case len(conditions) == 0: + return nil, errorf("no condition %s is open here, and this router holds none: `mesh-controller.conditions` says what the controller holds open", key) default: return nil, errorf("no condition %s is open here; it holds %s", key, strings.Join(conditions, ", ")) } diff --git a/modules/messenger/cmd/messenger/preview_test.go b/modules/messenger/cmd/messenger/preview_test.go index 281b6e7a..ec23f9f7 100644 --- a/modules/messenger/cmd/messenger/preview_test.go +++ b/modules/messenger/cmd/messenger/preview_test.go @@ -33,9 +33,9 @@ func previewHolder(t *testing.T, issues []TrackedIssue) (*Holder, *fakeChannel, // unmoved fails when anything a send would move has moved: the sends, the queue, the history, what is // stored, and the record itself. type unmoved struct { - sends, queued, recent, refusals int - stored Record - held Record + sends, queued, recent, refusals, puts int + stored Record + held Record } func mark(t *testing.T, h *Holder, tg, dt *fakeChannel, st *memStore, key string) unmoved { @@ -47,7 +47,7 @@ func mark(t *testing.T, h *Holder, tg, dt *fakeChannel, st *memStore, key string queued += len(list) } return unmoved{sends: len(tg.sends) + len(dt.sends), queued: queued, recent: len(h.recent), - refusals: len(h.refusals), stored: st.recs[key], held: *h.open[key]} + refusals: len(h.refusals), puts: st.puts, stored: st.recs[key], held: *h.open[key]} } func (was unmoved) check(t *testing.T, h *Holder, tg, dt *fakeChannel, st *memStore, c *clock, key string) { @@ -66,6 +66,9 @@ func (was unmoved) check(t *testing.T, h *Holder, tg, dt *fakeChannel, st *memSt if now.refusals != was.refusals { t.Errorf("a preview recorded %d refusal(s): only a send does that", now.refusals-was.refusals) } + if now.puts != was.puts { + t.Errorf("a preview wrote to the store %d time(s), even if the bytes were the same", now.puts-was.puts) + } if !reflect.DeepEqual(now.stored, was.stored) { t.Errorf("a preview wrote the record: %+v", now.stored) } @@ -204,9 +207,19 @@ func TestAPreviewOfWhatThisRouterDoesNotHoldIsRefusedByName(t *testing.T) { h.open["ask.someone.1"] = &Record{Key: "ask.someone.1", Origin: "ask", Severity: Urgent, Summary: "a question"} h.mu.Unlock() _, err = h.Preview("ask.someone.1", "") - if err == nil || !strings.Contains(err.Error(), "not a condition") { + if err == nil || !strings.Contains(err.Error(), "not a condition") || !strings.Contains(err.Error(), k.Key) { t.Errorf("an ask's record previewed as a condition: %v", err) } + + // A router holding an ask and no condition says what that key is, not that it holds nothing. + bare, _, _, _, _ := newHolder(t) + bare.mu.Lock() + bare.open["ask.someone.2"] = &Record{Key: "ask.someone.2", Origin: "ask", Severity: Urgent, Summary: "a question"} + bare.mu.Unlock() + _, err = bare.Preview("ask.someone.2", "") + if err == nil || !strings.Contains(err.Error(), "not a condition") { + t.Errorf("the only record held is an ask's: %v", err) + } } func TestAPreviewAsksTheTrackerWhenItsWordWentCold(t *testing.T) {