feat(provider-tck): emit a machine-readable conformance report - #944
Draft
aepfli wants to merge 5 commits into
Draft
feat(provider-tck): emit a machine-readable conformance report#944aepfli wants to merge 5 commits into
aepfli wants to merge 5 commits into
Conversation
Setting PROVIDER_TCK_REPORT_DIR makes each suite write its run to <dir>/<name>.json against the report schema in the specification repository (open-feature/spec#425, part of open-feature/spec#424). Go is the language that needs this first. godog counts a capability-gated skip in its passed tally, so a run that skipped five of twenty-nine scenarios prints "29 scenarios (29 passed)". Appendix F is unambiguous that a scenario skipped for an undeclared capability is reported as skipped with the reason and never as passed, and the harness does say so in a separate log line -- but the headline number still says something false, and the number is what gets read. pytest and jest-cucumber report skips correctly, so this is the runner's property rather than the suite's design. The report does not fix godog's summary. It makes the summary stop mattering, by recording every scenario's outcome individually so a consumer can check the rule instead of trusting the runner to have applied it. The same run now reports twenty-four passed and five not-declared, each with its reason. Identity comes from revision.go, generated by sync_assets.go beside the embedded artifacts. Generating both in one command is what keeps them honest: the CI check regenerates and fails on any difference, so a revision disagreeing with the artifacts beside it cannot be committed. The check is widened to cover the generated file, which it would otherwise have missed. Both the commit and the tree hash are recorded, the tree because it identifies the artifacts alone -- unchanged by unrelated edits elsewhere in the specification, so two runs of identical artifacts agree even when pinned to different commits, and checkable because `git rev-parse <commit>:specification/assets/provider-tck` reproduces it. Two smaller decisions. The provider is identified by the name it reports through its own metadata, with Config.Name recorded as the configuration, because Config.Name is chosen to read well in a failure message -- "flagd-rpc" -- and a provider with two materially different modes produces two reports that are not interchangeable. And how the backend was driven is read through an optional interface rather than a new BackendControl method, so that adding it breaks no existing implementation and a control that does not implement it simply omits the field. Emission is opt-in through the environment rather than through Config so that producing a report is a property of the run and not of the code: CI sets it, a local run does not, and no adopter changes a line to publish one. The tests assert the property that motivated the work -- that no scenario the capability gate stopped is ever reported as passed, and that every scenario is accounted for exactly once, since a report that silently omitted what it skipped would satisfy the first half while still misleading a reader. That test earned its place immediately: it caught this emitter recording every skipped scenario twice, the second time as passed, because godog does not deliver the before hook's ErrSkip to the after hook. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ng untested ones
Two defects in the capability rollup, both found by the Java implementation
reviewing this one.
A failed capability was emitted as {"state": "failed"} with no reason. The schema
now requires a reason for any outcome other than passed, so that entry does not
validate -- and it would have appeared only when a provider was actually failing,
which is precisely when the report matters. It now says how many of how many
scenarios carrying the tag failed, and points at the per-scenario results for
which and why.
No test caught it because every self-test suite passes, so nothing that runs end
to end ever reaches that branch. The new internal test drives the report builder
directly with synthetic records, which is the only way to exercise a failure
without breaking a provider on purpose.
A declared capability that no scenario carries was reported as passed. @targeting
is reserved -- it exists in the vocabulary but nothing tests it, because asserting
that an evaluation context reached the backend needs an echo operation the control
API does not have -- so a provider declaring it got a green result for a claim
nothing had examined. That is the vacuous pass the capability vocabulary was
introduced to eliminate, arriving through the report rather than through the
suite.
Such a capability is now omitted. The suite asked no question, so it has no answer
to report, and a consumer sees the tag is absent rather than a pass it cannot
rely on. Omitting is preferred to inventing a fifth outcome: the four in the
schema are about what the provider did, and "the suite does not test this" is a
fact about the suite.
Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
A scenario entry in the conformance report was identified by feature and name. Every row of a Scenario Outline shares one name, so the type-mismatch matrix in errors.feature produced eleven entries that differed only in durationMs. If one row failed and ten passed, the report could not say which failed, and a consumer keying on feature and name kept whichever row it read last. Per the report schema, a scenarioResult now carries `example`: the Examples row it came from, keyed by column header, with the cells verbatim as strings. Gherkin has no types, so "1" stays "1" rather than becoming 1 -- the report says what the table said. It is present only for outline rows and omitted otherwise. godog hands a hook an already-expanded pickle, whose step text has the parameters substituted in and whose row is otherwise gone. What survives is AstNodeIds, whose last entry is the id of the Examples TableRow. The row is therefore recovered by parsing the embedded feature files a second time and indexing every TableRow by that id. Those ids come from a counter godog creates once per run and shares across the files it parses, so reproducing them means reproducing godog's parse -- same files, same order, pickle compilation in between. That coupling is not left to be trusted: a pickle that came from an outline and did not resolve fails the run, because quietly returning to the ambiguity this field exists to remove is worse than a build failure. The capability gate records its outcome before a scenario starts, so it fills the field in too. Four skipped rows of the @object outline are as ambiguous as four failed ones. The gate's own bookkeeping is keyed by pickle id rather than by scenario name for the same reason. Gherkin allows an Examples block to carry its own tags, so two rows of one outline can differ in whether the gate stops them; keyed by name, gating one row suppressed the after hook for every row and the rows that did run would have vanished from the report. gherkin/go/v26 moves from an indirect requirement to a direct one. It is the same module and version godog already builds against, so no dependency is added and no go.sum entry changes. Verified against the schema on open-feature/spec#425 with a Draft 2020-12 validator: every report the self-tests emit validates, the eleven matrix rows carry eleven distinct examples matching the feature file, and (feature, name, example) is unique across every scenario in a report. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
…io ran A capability whose scenarios were all skipped still reported as passed. The in-memory report claimed "@events": "passed" while both scenarios in events.feature had been skipped. Those scenarios carry @events alongside @Stale and @configuration-change, so withholding either one skipped them -- and the rollup counted a skipped scenario's remaining tags as exercising their capabilities. @events was declared, so it missed the not-declared branch, and fell through to passed on the strength of two scenarios that never executed. It is the same vacuous pass as the reserved-capability case fixed alongside it, reached by a different route: the rollup was counting tag presence rather than execution. A scenario now contributes to its capabilities only when its outcome is passed or failed. A declared capability whose every scenario was skipped falls to zero exercised and is omitted, which is the honest answer -- the suite has the question but never got to put it to this provider. Found by the Python implementation, which reached the same rollup semantics and then noticed what they produced. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
…ider @caching is reserved and no scenario carries it, so the known-gaps list said caching was not covered and left it there. That understates the situation. flagd's RPC resolver enables an LRU cache by default and rewrites the reason to CACHED on a hit, and the adoption does not turn it off. So the suite already runs against a caching provider while asserting STATIC everywhere, and passes only because no scenario evaluates the same flag twice in a way that hits the cache. A scenario added later that does will fail against flagd RPC with CACHED, and the failure will look like a provider defect rather than a test-design one. The configuration-change scenario is also already leaning on cache invalidation without saying so: against flagd RPC it reads changing-flag, changes it, and reads again, which only produces the right answer because the change event evicts the entry. A provider whose cache was not invalidated would serve the stale value and fail a scenario that never mentions caching. Writing @caching scenarios is deliberately deferred. Recording the trap is not, because the cost of rediscovering it falls on whoever adds the next scenario. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
aepfli
added a commit
to open-feature/js-sdk-contrib
that referenced
this pull request
Aug 24, 2026
…ng untested ones
Three defects in the capability rollup, all of them ways the report could report
a result the suite never obtained.
A failed capability was emitted as {"state": "failed"} with no reason. The schema
requires a reason for any outcome other than passed, so that entry does not
validate -- and it appears only when a provider is actually failing, which is
precisely when the report matters. It now says how many of how many scenarios
carrying the tag failed, and points at the per-scenario results for which and why.
No test caught it because every self-test suite passes, so nothing that runs end
to end reaches that branch. The new test drives the report builder directly with
synthetic records, which is the only way to exercise a failure without breaking a
provider on purpose.
A declared capability that no scenario carries was reported as passed. @targeting
is reserved -- it is in the vocabulary and nothing tests it, because asserting
that an evaluation context reached the backend needs an echo operation the control
API does not have -- so a provider declaring it got a green result for a claim
nothing had examined.
A declared capability whose every scenario was skipped for a *different* one was
likewise reported as passed, because the rollup counted a capability as exercised
by tag presence rather than by execution. Both scenarios in events.feature carry
@events as well as @Stale or @configuration-change, so the in-memory suite
reported @events as passed while neither scenario ran.
Both are the vacuous pass the capability vocabulary was introduced to eliminate,
arriving through the report rather than through the suite. A scenario now counts
towards a capability only if it actually ran, and a declared capability with
nothing to show is omitted. The suite asked no question of it, so it has no answer
to report, and a consumer sees the tag is absent rather than a pass it cannot rely
on. Omitting is preferred to inventing a fifth outcome: the four in the schema are
about what the provider did, and "this run demonstrated nothing" is a fact about
the run.
An undeclared capability is still reported as not-declared with its reason, and
not-applicable is unaffected -- @strict-numeric-typing being unanswerable in a
language with no integer type is a different statement from a capability nothing
exercised, and both need saying.
Matches the same fix in Go, open-feature/go-sdk-contrib#944.
Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
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.
Makes the TCK emit a machine-readable conformance report.
Stacked on #940, which adds the suite this reports on. Part of open-feature/spec#424; the schema is open-feature/spec#425.
Why Go first
Because Go is the language that needs it most. godog counts a capability-gated skip in its passed tally:
Five of those twenty-nine did not run. Appendix F is unambiguous — a scenario skipped for an undeclared capability is reported as skipped with the reason and never as passed — and the harness does say so in a separate log line. But the headline number still says something false, and the number is what gets read. pytest and jest-cucumber both report skips correctly, so this is a property of the runner, not of the suite's design.
This does not fix godog's summary. It makes the summary stop mattering: every scenario's outcome is recorded individually, so a consumer can check the rule instead of trusting the runner to have applied it. The same run above accounts for all twenty-nine and calls five of them
not-declared, each with its reason.What identifies a report
tck.specRevisionandtck.assetsTreecome from a generatedrevision.go, whichsync_assets.gowrites from the submodule beside the embedded artifacts. Generating both in one command is what keeps them honest —make provider-tck-assets-checkregenerates and fails on any difference, so a revision that disagrees with the artifacts next to it cannot be committed. The check is widened here to cover the generated file, which it would otherwise have missed.Both the commit and the tree hash are recorded. The tree identifies the artifacts alone: unrelated edits elsewhere in the specification leave it untouched, so two runs that executed identical artifacts agree even when pinned to different commits, and it is checkable because
git rev-parse <specRevision>:specification/assets/provider-tckmust reproduce it.What identifies a scenario
featureandnametogether do not. Every row of a Scenario Outline shares one name, and the type-mismatch matrix inerrors.featureis eleven rows, so the report emitted eleven entries that differed only indurationMs:{ "feature": "errors", "name": "Requesting the wrong type returns the code default", "outcome": "passed", "durationMs": 0.21 }If one row failed and ten passed, that report could not say which failed, and a consumer keying on the pair kept whichever row it read last.
A row is identified by its parameters, which the schema now carries in
example— the Examples row it came from, keyed by column header:The values are the cells verbatim, as strings. Gherkin has no types, so
"1"stays"1": coercing it would be this implementation inventing a fact the feature file did not state, and four implementations would each invent a different one. The field is present only for outline rows and omitted otherwise.A field rather than a naming convention, because the parameters are the identity and they come from the feature file rather than from any runner. Mandating a mangled name instead would put a separator, an ordering and an escaping rule into normative text that four languages have to reproduce byte for byte, and drift there is invisible until two reports silently fail to line up. That is not hypothetical: before the field existed, implementations were observed diverging on exactly this — one emitting the bare scenario name for all eleven rows, another appending its runner's example id, a third its runner's expanded title.
A skipped row carries it too. The capability gate records its outcome before the scenario starts, so the four rows of the
@objectoutline would otherwise be fournot-declaredentries differing in nothing — exactly as ambiguous as four failures.How the row is recovered
godog's hooks receive an already-expanded pickle: the parameters are substituted into the step text and the row is otherwise gone. What survives is
AstNodeIds, whose last entry, for a pickle compiled from an outline, is the id of the ExamplesTableRow. So the row is recovered by parsing the embedded feature files a second time and indexing everyTableRowby that id.Those ids are not intrinsic to a document. They come from a counter godog creates once per run and shares across every file it parses, so reproducing them means reproducing godog's parse — the same files in the same order, with pickle compilation in between, because compiling pickles draws from the same counter. That is a coupling to godog's internals, and it is a deliberate one; the alternative is forking the pickle compiler.
It is not left to be trusted. A pickle that came from an outline and did not resolve fails the run, so a godog release that renumbers nodes breaks the build loudly instead of quietly returning to the ambiguity the field exists to remove. Deliberately desynchronising the counter locally produced exactly that failure, which is how the guard was checked.
gherkin/go/v26moves from an indirect requirement to a direct one. It is the same module and version godog already builds against, so no dependency is added and nogo.sumentry changes.A related fix
The capability gate's own bookkeeping was keyed by scenario name and is now keyed by pickle id, for the same reason. Gherkin allows an
Examplesblock to carry its own tags, so two rows of one outline can differ in whether the gate stops them; keyed by name, gating one row suppressed the after hook for every row, and the rows that did run would have vanished from the report entirely. No feature file in the current assets triggers this, so it was latent rather than observed.Smaller decisions
Config. Emitting a report is a property of the run rather than of the code: CI sets the variable, a local run does not, and no adopter changes a line to publish one. Several suites in one binary each write their own file, so flagd's two resolvers do not collide.Config.Namerecorded as theconfiguration.Config.Nameis chosen to read well in a failure message (flagd-rpc), which makes it the configuration; a provider with two materially different modes produces two reports that are not interchangeable.BackendControlmethod, so adding it breaks no existing implementation and a control that does not implement it simply omits the field. (HTTPControllives on the OFREP branch and gains it there.)The tests earned their place immediately
TestReportNeverCallsASkippedScenarioPassedasserts that no gate-stopped scenario is ever reported as passed, and that every scenario is accounted for exactly once — a report that silently omitted what it skipped would satisfy the first half while still misleading a reader.It caught this emitter recording every skipped scenario twice, the second time as passed, because godog does not deliver the before hook's
ErrSkipto the after hook:errarrives nil there, indistinguishable from a scenario that ran and passed.TestOutlineRowsAreDistinguishableasserts the propertyexampleexists to establish:(feature, name, example)is unique across every scenario in a report.TestSkippedOutlineRowsCarryTheirExamplecovers the gate path separately, since it fills the field in from a different place.Verification
go build,go vet,gofmtclean; all self-tests pass.jsonschema4.10.3), including one whose outline rows arenot-declared.exampleobjects, matching the three Examples tables inerrors.featurecell for cell and in table order.exampleentirely; the four skipped@objectrows each carry theirs.make provider-tck-assets-checkpassed on a clean regenerate when the report was first added. Theexamplechange touches no embedded asset and no generated file, so nothing about that check moves.