Skip to content

test(storage): enforce conformance suite completeness - #1204

Draft
Kiran01bm wants to merge 2 commits into
mainfrom
kiran01bm/db15i-storagetest-coverage
Draft

test(storage): enforce conformance suite completeness#1204
Kiran01bm wants to merge 2 commits into
mainfrom
kiran01bm/db15i-storagetest-coverage

Conversation

@Kiran01bm

Copy link
Copy Markdown
Collaborator

The storage conformance suite now proves it covers every storage sub-interface, so a backend cannot silently opt out of a suite.

Why

storagetest.Run invoked each conformance suite by hand. Nothing tied that list to the storage.Storage interface, so adding a new sub-interface without a suite — or dropping a suite invocation — passed silently for every backend.

What

The suites move into a single authoritative registry that Run iterates, and a reflection test asserts the registry covers each storage.Storage sub-interface accessor exactly once. The completeness check runs as a plain unit test, without a database.

Before / after

Before
storagetest.Run ──▶ hand-written calls: suiteA(t); suiteB(t); ...
                    (nothing checks the list is complete)

After
storage.Storage sub-interfaces ◀── reflection test ──▶ suite registry
                                                          │
storagetest.Run ──────────────────────────────▶ iterates registry

A reflection check pins the storagetest suite registry to every
storage.Storage sub-interface so a backend cannot silently skip one.
Copilot AI lite review requested due to automatic review settings August 29, 2026 04:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes the storage conformance suite self-validating by centralizing parity suite registration and adding a unit-testable completeness check against the storage.Storage interface, preventing backends from silently skipping new sub-interface suites.

Changes:

  • Introduced an authoritative parityFamilies registry and updated storagetest.Run to iterate it.
  • Added a reflection-based coverage assertion to ensure the registry covers every storage.Storage sub-interface accessor exactly once.
  • Added a standalone unit test to run the coverage assertion without requiring a database/harness.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
pkg/storage/storagetest/storagetest.go Adds parity family registry and reflection-based completeness assertion; Run now iterates the registry.
pkg/storage/storagetest/storagetest_test.go Adds a unit test that exercises the completeness assertion without DB setup.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread pkg/storage/storagetest/storagetest.go
The coverage check enumerates all storage.Storage methods against the
registry plus an explicit Ping/Close allowlist, ties each family's
suite to its method by function name, and runs only in the unit test.
@Kiran01bm

Copy link
Copy Markdown
Collaborator Author

Review response from Kiran's (@Kiran01bm) code review assessment agent (Amp / Claude Opus 4.5)

Four findings fixed in a follow-up commit (the completeness check is now inverted and non-vacuous); the enginetest sibling gets the same treatment as a tracked follow-up.

# Finding Status Explanation
1 Accessor-shape filter lets differently-shaped methods silently opt out of coverage fixed Inverted: every storage.Storage method must be in the registry or an explicit Ping/Close allowlist, and the allowlist itself is validated against the interface. Non-vacuity proven: removing the Checks row fails naming it.
2 Registry test function never tied to its storageMethod (mismatched row passes) fixed Each family's suite function name (via runtime.FuncForPC) must end Test<storageMethod>.
4 assertParityFamilyCoverage inside Run is redundant and its FailNow goexits the caller fixed Removed from Run; the DB-free unit test is the sole home of the assertion.
3 name field redundant with storageMethod, nothing asserts they agree fixed Field dropped; storageMethod names the subtests.
5 enginetest.go hand-lists suites with the same gap deferred Tracked as an internal follow-up to apply the same registry + allowlist + reflection-test treatment.

Source: adversarial review of #1204 by Claude Code (claude-opus-5), delivered as an offline review doc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants