amadan
Fix round: add a test that decouples seq from id in newestSnapshot
TestNewestSnapshotPrefersHighestSeqThenHighestID writes rows in seq order, so its expected winner is simultaneously the highest-seq and highest-id row -- an id-only ordering would pass it too. Add TestNewestSnapshotPrefersHigherSeqOverHigherID, which writes a higher-seq row first (giving it the lower id) so only an implementation that genuinely orders by seq first can pass. Verified by temporarily switching newestSnapshot's Order clause to id DESC alone: the new test failed as expected while the original still passed, confirming the gap the review found and that the new test closes it. Order clause restored before committing.
1 file changed,
+34
−0
internal/docs/snapshots_test.go+34 −0
diff --git a/internal/docs/snapshots_test.go b/internal/docs/snapshots_test.go| index a985606..187f750 100644 |
| --- a/internal/docs/snapshots_test.go |
| +++ b/internal/docs/snapshots_test.go |
| @@ -34,6 +34,40 @@ func TestNewestSnapshotPrefersHighestSeqThenHighestID(t *testing.T) { |
| } |
| } |
| +// TestNewestSnapshotPrefersHigherSeqOverHigherID pins seq as the |
| +// primary sort key, not just id. TestNewestSnapshotPrefersHighestSeqThenHighestID |
| +// above writes rows in seq order, so its highest-seq row is also its |
| +// highest-id row -- an implementation that ordered by id DESC alone, |
| +// ignoring seq entirely, would pass it too. Here the higher-seq row |
| +// is written first and so gets the lower id, decoupling the two: only |
| +// an implementation that actually orders by seq first returns it. |
| +func TestNewestSnapshotPrefersHigherSeqOverHigherID(t *testing.T) { |
| + app, err := NewForTest(t) |
| + if err != nil { |
| + t.Fatalf("NewForTest: %v", err) |
| + } |
| + ctx := context.Background() |
| + owner := app.TestPerson(t, "owner@example.com") |
| + doc, _ := app.Store().Create(ctx, owner, "Seq before id") |
| + |
| + write := func(seq int64, state string) { |
| + row := &DocSnapshot{DocID: doc.ID, Seq: seq, State: []byte(state), ABIVersion: coreABIVersion, CreatedAt: time.Now().UTC()} |
| + if err := app.Store().db.WithContext(ctx).Create(row).Error; err != nil { |
| + t.Fatalf("insert snapshot: %v", err) |
| + } |
| + } |
| + write(30, "high-seq-low-id") |
| + write(10, "low-seq-high-id") |
| + |
| + got, err := app.Store().newestSnapshot(ctx, doc.ID) |
| + if err != nil { |
| + t.Fatalf("newestSnapshot: %v", err) |
| + } |
| + if got == nil || string(got.State) != "high-seq-low-id" { |
| + t.Fatalf("newestSnapshot returned %+v, want the seq 30 row despite its lower id", got) |
| + } |
| +} |
| + |
| func TestNewestSnapshotIgnoresAnotherABIsRows(t *testing.T) { |
| app, _ := NewForTest(t) |
| ctx := context.Background() |