test(engine): classify every Engine method in the contract-case registry - #1247
Closed
Kiran01bm wants to merge 3 commits into
Closed
test(engine): classify every Engine method in the contract-case registry#1247Kiran01bm wants to merge 3 commits into
Kiran01bm wants to merge 3 commits into
Conversation
Restructure the enginetest suite around a contract-case registry that binds each case to its Harness fixture and the engine.Engine methods it pins, with a documented exclusion list for methods whose behavior is engine-specific. A DB-free completeness test holds the registry, the Harness fixture fields, and the Engine method set in lockstep, so a new engine capability or fixture cannot land without a conformance decision.
Review found the registry's name/harnessField columns were inert: run funcs hardcoded their own Case and fixture, so a miswired row stayed green. Runners now resolve both from their registry row, optional capability interfaces and Case constants join the ratchet, and a DB-free execution test pins that every registered case actually runs.
Record engine-method invocations per registered case so declared engineMethods lists and fixture signatures are proven rather than trusted, and document the declaration shapes the AST ratchets can and cannot see.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The refactor is localized to test infrastructure and the only noted issue is a minor robustness improvement (avoiding a potential nil-function panic in a completeness test).
Pull request overview
Refactors the pkg/engine/enginetest contract suite to use a registry-driven structure with DB-free completeness tests, ensuring every engine.Engine method and every Harness fixture is explicitly classified (pinned by a case or documented as excluded).
Changes:
- Introduces a
contractCasesregistry tying each contractCaseto exactly oneHarnessfixture field, its pinnedengine.Enginemethods, and a dedicated runner function. - Adds
engineMethodExclusionsplus coverage tests that enforce: no unclaimed Harness fixtures, no unclassified Engine methods, and no stale exclusions. - Updates engine documentation to point readers to
pkg/engine/enginetestas the conformance decision source.
File summaries
| File | Description |
|---|---|
| pkg/engine/README.md | Documents that pkg/engine/enginetest classifies/pins the conformance decision for each engine.Engine method. |
| pkg/engine/enginetest/enginetest.go | Replaces hand-enumerated subtests with a contractCases registry and adds method-exclusion documentation. |
| pkg/engine/enginetest/enginetest_test.go | Adds DB-free completeness tests for registry uniqueness, Harness fixture consumption, Engine method classification, and run-function execution fidelity. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| require.False(t, claimed, "Harness fixture %q is consumed by both case %q and case %q", c.harnessField, prev, c.name) | ||
| claimedFields[c.harnessField] = c.name | ||
|
|
||
| distinctRunFuncPointers[reflect.ValueOf(c.run).Pointer()] = true |
Collaborator
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Restructures the engine contract suite around a case registry with a completeness test, so every
engine.Enginemethod is either pinned by a contract case or carries a documented exclusion.Why
The enginetest suite hand-enumerated its subtests, so nothing connected the suite to the
engine.Engineinterface: a new engine method could land with no conformance decision, and a new Harness fixture could sit unconsumed without any test noticing. The storage conformance suite already solved this with a registry plus a reflection-driven completeness test; this applies the same treatment to the engine layer.What
contractCasesregistry inpkg/engine/enginetest: each case names its Harness fixture field, theengine.Enginemethods whose cross-engine contract it pins, and its run function.Runiterates the registry.engineMethodExclusions: documented reasons for the methods the suite deliberately does not pin (engine-specific behavior each engine's own tests cover).TestContractCaseCoverage(DB-free): registry rows are unique; every claimed fixture field exists, is a function, and is claimed exactly once; each case runs the function named for it; everyengine.Enginemethod is pinned or excluded (both at once fails); exclusions have non-empty reasons and match real methods; no Harness fixture goes unconsumed.Verified non-vacuous by mutation: excluding a pinned method, adding a stale exclusion, dropping an exclusion, deleting a registry row, and wiring the wrong run function each fail the test.
Before / after