test(storage): add cross-dialect storage parity suite

[Kiran01bm]

Summary

Adds pkg/storage/storagetest — a behavioral parity suite that any storage.Storage implementation must pass. The suite asserts on the storage interface contract (typed results, typed errors, ordering, state transitions), never on SQL text or dialect-specific behavior, so it can validate a future non-MySQL state store slice by slice as it lands.

What

• New storagetest.Harness interface: implementations supply a clean bootstrapped store (NewStorage) and a store on an unusable connection (NewUnreachableStorage) for error-path coverage.
• Single storagetest.Run(t, h) entry point running every parity family, so an implementation cannot silently opt out of part of the contract; per-family entry points remain exported for bring-up. Shared interface-level fixture helpers (CreateLock, CreateApply*) back both the suite and mysqlstore's own tests, which delegate to them so the two can't drift.
• The settings and apply-log test families move from mysqlstore into the suite; mysqlstore runs them through a thin harness adapter over its existing testcontainer fixture (parity_test.go).
• The orderings the suite asserts are now normative in the interface docs: apply-log reads return created_at ascending with ties broken by ascending id (added the missing tie-break to GetByApply), and settings List returns keys ascending. New parity coverage: GetByApply and the apply-log DB-error family.

Why

The suite is the oracle for the upcoming Postgres state store: each storage sub-interface implementation lands validated against exactly the behaviors the MySQL store already guarantees. Remaining test families migrate incrementally in follow-up PRs, ahead of or alongside the store slices they validate.

                      ┌───────────────────────────┐
                      │  pkg/storage/storagetest  │
                      │  Run(t, h)                │
                      │  ├─ TestSettings(t, h)    │
                      │  └─ TestApplyLogs(t, h)   │
                      └────────────┬──────────────┘
                          runs against Harness
                     ┌─────────────┴─────────────┐
          ┌──────────┴──────────┐     ┌──────────┴──────────┐
          │ mysqlstore harness  │     │ postgres harness    │
          │ (this PR)           │     │ (future)            │
          └─────────────────────┘     └─────────────────────┘

[Copilot]

Pull request overview

Adds a new pkg/storage/storagetest package that defines a cross-dialect behavioral parity suite for storage.Storage implementations, and migrates existing MySQL storage integration tests (settings + apply logs) to run through that suite via a MySQL harness adapter.

Changes:

• Introduce storagetest.Harness plus shared test fixtures (CreateLock, CreateApply*) to standardize parity testing across storage implementations.
• Add parity suites for SettingsStore and ApplyLogStore (storagetest.TestSettings, storagetest.TestApplyLogs).
• Replace MySQL-specific settings/apply-log integration tests with a single parity_test.go that runs the shared suite against mysqlstore.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
pkg/storage/storagetest/storagetest.go	Defines the harness interface and shared fixture helpers for parity tests.
pkg/storage/storagetest/settings.go	Adds the SettingsStore parity suite (CRUD + unreachable-DB error paths).
pkg/storage/storagetest/apply_logs.go	Adds the ApplyLogStore parity suite (bounded newest-window ordering + filtering).
pkg/storage/mysqlstore/parity_test.go	Wires MySQL’s existing integration fixture into the parity suite via a harness adapter.
pkg/storage/mysqlstore/settings_test.go	Removed in favor of running the shared parity suite.
pkg/storage/mysqlstore/apply_logs_test.go	Removed in favor of running the shared parity suite.

Suppressed comments (1)

pkg/storage/storagetest/settings.go:101

• Similar to the Set test, Get can return (nil, nil) if the Set didn't persist the value. Accessing setting.Value would panic and make the failure harder to diagnose; add require.NotNil before dereferencing.

		setting, err := store.Settings().Get(ctx, "rate_limits")
		require.NoError(t, err)
		require.Equal(t, jsonValue, setting.Value)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Adversarial correctness review, requested by Armand and performed by his agent. Reviewed at head dc3120f.

Verdict: clean — no fix-before-merge findings. The moved coverage is a strict superset of what it replaces, and the one production change is exactly as narrow as advertised.

The core risk in a test-move PR like this is silent coverage loss — a behavior the old package-local tests pinned that the new suite no longer exercises. I audited both moved families against the deleted files at the merge base. Settings is 1:1 (all eight behaviors: Get/Set/List/Delete/JSONValues plus the three DB-error paths), and its List test got stronger — keys are now seeded out of key order, so sorted output can no longer pass by insertion order alone. Apply logs gains coverage: a new GetByApply family (the reader whose ordering this PR fixes) and four DB-error tests that didn't exist before. Nothing was weakened in the move.

Follow-up, not blocking
Rebase coordination	#934 also edits pkg/storage/mysqlstore/applies_test.go (different hunks — it adds claim tests mid-file; this PR rewrites the fixture helpers at the bottom). Whichever lands second needs a trivial rebase.
Unused env parameter	createTestLock/createTestLockWithPR keep a discarded env parameter for signature compatibility. A later mechanical sweep could drop it from the call sites and the signatures together.

Verified solid: the production change is confined to GetByApply gaining the id ASC tie-break that GetRecentByApply and List already had — I checked all three readers at head, and both bounded readers correctly select newest-first with the tie-break and reverse to chronological order; the interface docs now make the ordering contract and Delete → ErrSettingNotFound normative, so a future implementation can't legally diverge from what the suite asserts; the storagetest fixture helpers are byte-equivalent to the deleted mysqlstore ones (same staging/pending defaults, same Lock/Apply row shape), the delegation leaves no stale callers, and the NewUnreachableStorage harness reproduces the old DB-error setup exactly (open-then-close on the test DSN); the suite runs sequentially through a single Run(t, h) entry, matching the package's shared-container convention, so an implementation can't silently opt out of a family; all three Copilot comments are already addressed at head (nil guards, out-of-order seeding, interface docs); locally at dc3120f the build passes and the full mysqlstore integration suite — including TestStorageParity — is green with -race; CI is green across all 32 checks.

This review was generated by Claude Code (claude-fable-5).

[aparajon]

🤖 Approving on Armand's behalf after the adversarial correctness review above (no fix-before-merge findings). This stamp was left by Claude Code (claude-fable-5).