feat(provider-tck): emit a conformance report whose results are Cucumber Messages - #413
Draft
aepfli wants to merge 6 commits into
Draft
feat(provider-tck): emit a conformance report whose results are Cucumber Messages#413aepfli wants to merge 6 commits into
aepfli wants to merge 6 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). Unset means no report, which is the default and is not an error. An environment variable rather than a TckConfig field, so that emitting 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. Several suites in one pytest session each write their own file, so flagd's two resolvers would not collide. The load-bearing part is the per-scenario list. Appendix F requires that a scenario skipped for an undeclared capability is reported as skipped with the reason and never as passed, and nothing downstream can check that against a summary line. Recording every scenario's outcome individually makes the rule checkable by the consumer instead of dependent on the runner. It is also required to be complete, because a document that quietly dropped what it skipped would satisfy the letter of the rule and still mislead whoever read it. pytest, unlike godog, reports a skip honestly -- so the interesting divergence here is elsewhere. The one scenario the Python SDK cannot satisfy is marked xfail, so the run finishes green; the provider still did not satisfy it, and the document says failed with the reason. An expected failure is a recorded deviation, not an excused one. Scenarios are therefore enumerated at collection and resolved at the end of the session rather than as fixtures run, which is also what keeps a scenario skipped by a marker -- whose fixtures never run at all -- from vanishing from the document. Identity comes from spec_revision.json, generated by hatch_build_sync.py beside the copied assets and force-included into the wheel. It has to be captured at build time: the submodule that knows the answer is not in the distribution, so an installed copy has nothing left to ask. A build that cannot reach git -- an unpacked sdist -- warns and records "unknown" rather than inventing a commit. Both the commit and the tree hash are recorded, the tree because it identifies the assets alone: unchanged by unrelated edits elsewhere in the specification, so two runs of identical assets 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 TckConfig.name recorded as the configuration, because TckConfig.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 off an optional control_api property rather than added to the BackendControl protocol, so that adding it leaves every existing control complete and one that stays quiet simply omits the field. The tests assert the two properties a consumer is entitled to assume -- that no scenario the capability gate stopped is ever reported as passed, and that every collected scenario appears exactly once, counted against pytest's own collection rather than against a number written down beside it. 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: trueComment |
A report entry was identified by feature and name. Every row of a Scenario Outline shares one name, so the eleven rows of the type-mismatch matrix in errors.feature produced eleven entries nothing could tell apart -- and in the Python run one of the eleven fails while ten pass, which is exactly the case the report could not express. A consumer keying on feature and name kept whichever row it happened to see last. Each entry from an outline now carries the row it came from, as the Examples parameters keyed by column header, matching the "example" property added to the schema. Values are the cell contents verbatim as strings: Gherkin has no types, so "1" stays "1" rather than becoming a number the table never mentioned. pytest-bdd parametrizes the generated test over one dict per row, keyed by the header, so the row is read back off the node's callspec -- available at collection, which is what lets a row the capability gate skipped be identified as precisely as one that ran. This removes the workaround that appended pytest's own id for the row to the scenario name. It was the wrong shape twice over. The name is the feature file's name, and qualifying it made Python disagree with Go and JavaScript about a scenario all three ran, which defeats the cross-language comparison the report exists for. And a name format would be normative text -- a separator, an ordering, an escaping rule -- that four languages have to reproduce byte for byte, where drift is invisible until two reports silently fail to line up. The parameters are the identity, and they come from the feature file rather than from any runner. The uniqueness test now keys on feature, name and example together, which is the property this change exists to establish. The examples the report emits are checked against the Examples tables read out of the Gherkin by hand, rather than against pytest-bdd's parser, which is what produced them. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
…ng untested ones
Two defects in the capability rollup, mirroring the fix already made in Go
(go-sdk-contrib#944).
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 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 ever reaches that branch. The test now 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.
An undeclared capability is still reported with its reason whether or not any
scenario carries it, because that is a fact about the provider rather than about
the suite.
Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
Gherkin lets an Examples block carry its own tags, so two rows of one Scenario Outline can differ in which capability gates them. The capability gate already handled that correctly -- pytest-bdd attaches an Examples block's tags as marks on that block's parameter sets, and the gate reads the node's markers -- but the report did not. A scenario's tags were read from the scenario, the feature and the rule, which is everywhere those tags are not. The consequence was a misreport of exactly the kind the format exists to rule out. A row skipped because its Examples block was tagged with an undeclared capability appeared with no tags at all, so it was classified not-applicable rather than not-declared -- the run had a reason not to execute it, said the report, when the reason was a capability the provider does not have. The capability rollup did not count it either. The row's tags are now resolved by intersecting the tags the scenario's Examples blocks declare with the markers pytest put on the node. That names this row's blocks without having to work out which block a row came from, and admits nothing that is not a Gherkin tag of this scenario. No canonical feature file uses per-Examples tags today, so this is latent. It was found while checking a defect the Go implementation hit in the same area, where per-scenario bookkeeping keyed by scenario name let one gated row suppress the accounting for every row of its outline. Nothing here is keyed by name -- the collector, the durations and the records are all keyed by pytest node id, which is unique per row -- and the test added here confirms that every row of an outline is still reported when one of them is gated. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
The emitter defined its own per-scenario result list: a four-value outcome enum, a tag list, a reason, and a field naming which Scenario Outline row an entry came from. All of it already exists in Cucumber Messages, which is maintained, cross-language, schema'd, and emitted natively by cucumber-jvm. The report schema was reshaped to reference a Messages payload rather than define one (open-feature/spec#425); this follows it. A run now writes two files per suite: <name>.json, the envelope, and <name>.ndjson, the results it points at, with results.digest over the exact bytes written. Deleted, because Messages carries them: scenarios[] - now TestCase/TestCaseStarted/TestStepFinished/TestCaseFinished. the outcome enum - Cucumber's own seven statuses. The declared/not-applicable distinction was never a property of the run: it follows from the declaration and the scenario's tags, so it is stated once in the envelope instead of once per scenario. example - a pickle's astNodeIds are [scenario id, table row id], and the row id resolves in the GherkinDocument to the cells the feature file wrote. Four implementations were each reinventing this field by hand. tck.assetsTree - the payload carries the executed feature Source verbatim, which answers "did two runs ask the same questions" directly rather than by proxy. Two things Messages cannot carry, so they stay. The declaration is an input to reading the results, not a summary of them. And no standard results format has a slot for the tested subject: Messages records the runtime and the OS, not what was being asked about. pytest-bdd emits no Messages -- it ships the legacy Cucumber JSON format -- so messages.py assembles the stream. Two dependencies, each doing the half it owns: cucumber-messages, the official Python types from the protocol's own repository, for the execution messages; gherkin-official, already a transitive dependency of pytest-bdd, for the gherkinDocument and pickle payloads, which are used as it produces them rather than round-tripped through another representation. The feature files are parsed again because pytest-bdd's own dataclasses drop the AST node ids a pickle refers to. Step results come from pytest-bdd's step hooks rather than from the scenario's verdict, because a stream that marked all eight steps of a scenario failed would be saying something untrue about the seven that passed and the ones never reached. Each test case also carries a before- and after-hook TestStep: pytest runs three phases and only the middle one executes steps, so that is where a capability skip's reason and a teardown failure belong. A verdict no step accounts for -- a strict xfail that passes -- is attached to the after-hook, so it survives a consumer computing the test case's status as the worst of its steps. An expected failure is still a failure in the payload. The acknowledgement moved to the envelope's knownDeviations, declared by TckConfig.known_deviations, where it records the gap without softening the result. TckConfig also gains not_applicable, for a capability that cannot hold rather than one the provider declines. Verified locally; CI does not run on this branch, which targets the report branch rather than main. Both suites' envelopes validate against the reshaped schema with a Draft 2020-12 validator and their digests match; both streams validate clean against the Cucumber Messages JSON schema at v34.2.0 (661 messages each, zero errors). The stream accounts for all 29 collected scenarios; the five the capability gate stopped are SKIPPED for every step, none PASSED, and the one row the SDK fails is FAILED while pytest exits zero. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
…he stream The envelope named the results format but not its version, and Messages is versioned. This implementation is on 34.2.0 while the Go TCK builds against v21 and cucumber-jvm ships a different release again, so a consumer holding two reports cannot assume one schema validates both. Guessing is worse than not validating. A later schema accepts messages this producer could not have emitted, and an earlier one rejects messages that are perfectly valid, so a check against the wrong version reports a result that has nothing to do with the stream. It reuses the function that already computes the stream's own Meta protocolVersion rather than adding a second source, so the envelope and the stream cannot disagree about which release produced it. That function reads the version from the installed distribution rather than declaring it, so a dependency bump cannot leave the report claiming the old one. 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.
Stacked on #409 — base is
feat/provider-tck, so the diff here is only the report emitter. Part of open-feature/spec#424; the schema is open-feature/spec#425.This PR was reworked after review. It originally defined a per-scenario result format in the report schema. It no longer does: the results are carried in Cucumber Messages, the ndjson protocol, and the schema is an envelope that references them. The rest of this description describes what that means and what was deleted.
Setting
PROVIDER_TCK_REPORT_DIRmakes each suite write two files: an envelope at<dir>/<name>.jsonand the results it points at at<dir>/<name>.ndjson.The envelope:
{ "schemaVersion": "1", "provider": { "name": "In-Memory Provider", "language": "python", "configuration": "in-memory" }, "sdk": { "name": "openfeature-sdk", "version": "0.8.4" }, "tck": { "implementation": "python-sdk-contrib/tools/openfeature-provider-tck", "version": "0.1.0", "specRevision": "dfa16586d91ca020ef1b3b82a7c972d833ff8f29" }, "declaration": { "declared": ["@events", "@object", "@strict-numeric-typing"] }, "results": { "format": "cucumber-messages", "location": "in-memory.ndjson", "digest": "sha256:c7e12a28e86c8a715331ce134b9787732f9dc04d2f89423910e99e71e3771db6" }, "backend": { "description": "the Python SDK's InMemoryProvider, rebuilt per scenario" }, "knownDeviations": [ { "issue": "https://github.com/open-feature/python-sdk/issues/619", "summary": "python-sdk: a boolean satisfies an Integer request. …" } ] }Why the results are not our format
Per-scenario outcomes, tags, Scenario Outline row identity and the executed feature source are all already specified by Cucumber Messages, which is maintained, cross-language, schema'd, and emitted natively by cucumber-jvm. Defining them again in the report schema created a second format to maintain and version, and two places for the same fact to disagree.
What was deleted
scenarios[]. NowTestCase/TestCaseStarted/TestStepStarted/TestStepFinished/TestCaseFinished, keyed by ids the protocol defines.The four-value outcome enum. Cucumber's own seven statuses replace it. The
not-declared/not-applicablesplit it existed for was never a property of the run: it follows from the envelope'sdeclarationand the scenario's tags, both of which are present, so it is stated once instead of once per scenario.declaration.notApplicablekeeps the distinction the enum was drawing, where it belongs — as an input to reading the results rather than a per-scenario fact.example. This is the clearest case. It was added days ago so an outline row could be identified, and four implementations each reinvented it — one of them, this one, by reverse-engineering how its runner maps a pickle back to a table row. Messages has carried that identity all along: a pickle'sastNodeIdsare[scenario id, table row id], and the row id resolves in theGherkinDocumentto exactly the cells the feature file wrote.tck.assetsTree. The stream carries the executed featureSourceverbatim, so a consumer can read the questions that were actually asked rather than trusting a hash to stand for them. That is strictly stronger than what the tree hash was carrying, and it is one fewer thing for the build to get right. The generatedspec_revision.jsonis now just{"specRevision": "…"}.What stays OpenFeature-specific, and why
The declaration is an input, not a summary. A skipped scenario in the payload says the question was not put to this provider; only the declaration says whether that is because the provider declines the capability. It cannot be derived from the results, which is exactly why it has to be stated.
No standard results format identifies the tested subject. Messages'
Metarecords the runtime, the OS and the CPU — what produced the answers, not what was being asked about. Soprovider,sdk,tckandbackendstay in the envelope.knownDeviations. A gap the provider acknowledges is a claim about the provider, not a result. The Python suite marks one scenarioxfail(strict=True)against python-sdk#619; the payload still reports that scenario asFAILED, and the envelope carries the acknowledgement beside it. An adoption declares it withTckConfig.known_deviations. Recording it in the envelope is what lets a consumer tell a tracked gap from a surprise without the result itself being weakened, which is the trade the previous version got wrong by carrying the issue reference as prose inside areason.Dependencies added
pytest-bdd emits no Cucumber Messages. It ships
cucumber_json.py, the legacy Cucumber JSON format, and nothing for the ndjson protocol — somessages.pyassembles the stream. Two libraries, each doing the half it owns:cucumber-messages(34.2.0, new) — the official Python types, published fromcucumber/messagesitself. Zero dependencies,requires-python >=3.9, actively released. Used for the execution messages:Meta,TestCase,TestCaseStarted,TestStepFinished,TestStepResult,TestRunStarted/Finished,Hook.gherkin-official(>=29, promoted from a transitive dependency of pytest-bdd to a direct one) — the reference Gherkin parser. It produces thegherkinDocumentandpicklepayloads, which are Messages: emitting Messages ndjson is what that library exists for, so its output is used as it comes rather than round-tripped through another representation that could quietly drop a field it does not model.uv.lockis regenerated, anduv sync --frozenverified against it. Onlycucumber-messagesis genuinely new to the lock.The feature files are parsed a second time, by this package. pytest-bdd parses them with
gherkin-officialtoo but converts the result into dataclasses of its own that do not carry the AST node ids — and those ids are what a pickle refers to and what makes one outline row distinguishable from another. Four small files, once per session.Truthful skips, and per-step results
Appendix F requires that a scenario skipped for an undeclared capability is reported as skipped with the reason and never as passed. pytest, unlike godog, already reports a skip honestly, and Cucumber's
SKIPPEDsays the same thing, so this is the easy part in Python — but it is checked explicitly rather than assumed.Step results come from pytest-bdd's step hooks (
pytest_bdd_after_step,pytest_bdd_step_error,pytest_bdd_step_func_lookup_error) rather than from the scenario's verdict. Marking all eight steps of a scenarioFAILEDbecause the scenario failed would be saying something untrue about the seven that passed and the ones never reached. The failing row of the type-mismatch matrix comes out as:with the assertion carried as a
TestStepResult.exceptionof typeAssertionError, which is what that field is for.Every test case also carries a before- and after-hook
TestStep, with aHookmessage declaring each. pytest runs a scenario in three phases and only the middle one executes Gherkin steps: the capability gate skips during setup, and a provider that fails to shut down fails during teardown. Neither has a pickle step to attach a result to, so without hooks a gated skip would have to borrow the first step's result and a teardown failure would be invisible behind a row of passed steps. Cucumber models exactly this.That also closes a gap worth naming. A consumer derives a test case's outcome as the worst of its steps. A verdict no step accounts for — a strict
xfailthat passes, which pytest fails while every step passed — would be lost on the way out, so it is attached to the after-hook. There is a unit test for it.Scenarios are enumerated at collection and resolved only at the end of the session, unchanged from before and still load-bearing: a scenario skipped by a marker never runs a fixture, so an emitter that learned of a scenario when its fixtures ran would leave it out of the stream entirely.
TckConfigalso gainsnot_applicable={Capability.X: "why"}for a capability that cannot hold rather than one the provider declines. The suite treats the two identically — the scenarios are skipped either way — but the report keeps them apart, and a configuration that puts a capability in both, or gives no reason, is rejected at construction.Verification
Run on WSL Ubuntu, Python 3.10 (the repo's pinned version), against the schema as it stands on
feat/provider-tck-report-schemaat46ebc218. CI does not run on this PR:build.ymltriggers only on pull requests targetingmain, and this one targetsfeat/provider-tck. Everything below is local.Package test suite.
94 passed, 9 skipped, 2 xfailed.ruff check,ruff format --checkandmypy(whose scope includestests) are all clean.Envelope validation. Both envelopes validate with
jsonschemaas Draft 2020-12 againstconformance-report.schema.json, zero errors, andresults.digestmatches a SHA-256 recomputed over the ndjson bytes:in-memorycontrollable-in-memoryPayload validation. Both streams validate clean — 661 of 661 messages, zero errors — against the published Cucumber Messages JSON schema at tag
v34.2.0(jsonschema/src/*.schema.jsonfromcucumber/messages, resolved locally through areferencingregistry so the relative$refs work). Message census, identical for both suites:Every scenario accounted for. 29
TestCasemessages against 29 scenarios from pytest's own--collect-only, in both suites, with(uri, scenario name, Examples row)distinct across all 29.test_every_collected_scenario_appears_exactly_oncechecks the total against--collect-onlyrather than a number written down beside it, so adding a scenario to the specification cannot leave it passing while the payload loses one.Outcome counts, computed the way a consumer must — worst step status per test case:
in-memorycontrollable-in-memoryPer step, for
in-memory: 220PASSED, 45SKIPPED, 1FAILED. Of the 45, 42 belong to the five gated scenarios (their before-hooks included, which is where the reason is) and three are the steps of the failing scenario that were never reached.Gated skips are truthful. Every scenario carrying a tag absent from
declaration.declaredisSKIPPED, at every step, in both suites — none is reported as passed. Checked against the reason the gate actually gave, so the derivation from the two documents agrees with reality rather than merely being possible:And the distinction survives: the scenario the generated test suite skips with a plain marker is also
SKIPPED, but carries no undeclared tag, so it is not attributable to a capability.The payload disagrees with the runner, deliberately. pytest exits zero — the one scenario the SDK fails is
xfailed — and the stream reports itFAILED. That is asserted, not just observed.The eleven rows.
Requesting the wrong type returns the code defaultproduces eleven test cases sharing one scenario name, told apart by the second entry of the pickle'sastNodeIds(the id of theTableRowit was compiled from), which resolves in theGherkinDocumentto the cells the feature file wrote:25is theScenarionode all eleven share, which is right: the scenario is one scenario. The rows the tests recover from the stream this way are compared against the three Examples tables read out of the Gherkin by hand, rather than against the parser that produced them.The same holds for rows the capability gate skipped, since row identity comes from the pickle rather than from the run: with
@objectundeclared, all four rows ofRequesting a structured flag as a scalar returns the code defaultare present,SKIPPED, and individually identified.Tags on an Examples block. Still covered. Gherkin lets an Examples block carry its own tags, so two rows of one outline can differ in which capability gates them, and those tags are on neither the scenario, the feature nor the rule. No canonical feature file does this yet, so the test writes its own: with one row of a three-row outline gated, all three appear, the gated one is
SKIPPEDand carries@object, and its two siblings pass.The executed source is present and correct. Every
Sourcemessage'sdatacompares equal to the feature file on disk, and the set ofSourceuris equals the set of uris the test cases name.Cross-platform.
pathliband list-argvsubprocessthroughout. Feature uris are normalised to forward slashes: pytest-bdd builds its relative filename withos.path.join, so on Windows it arrives backslash-separated, and the same string has to appear in theSource, theGherkinDocumentand everyPickleor nothing ties them together — a report emitted on Windows would otherwise not be comparable with one emitted on Linux. There is a test for it. The emitter itself was developed and verified on Linux; the Windows path of that normalisation is covered by the unit test rather than by a Windows run.Packaging.
uv buildproduces a wheel carryingspec_revision.jsonalongside the four feature files, the canonical flag set andcontrol-api.yaml, and declaring the two new requirements.Failure to write still fails the run loudly, verified by pointing
PROVIDER_TCK_REPORT_DIRat a path under a regular file. A run that asked for a report and silently did not get one is how a publishing pipeline ends up serving a stale result forever. A scenario that ran and matches no pickle fails the session the same way and names the scenario, because that is the one failure mode this format exists to rule out.Things worth raising, rather than glossing
The capability rollup is gone entirely, not moved. The previous version summarised a verdict per capability. Both defects raised against it — a failed capability with no reason, and a declared-but-untested capability reported green — disappear with it, but so does the summary. A consumer wanting "did
@objectpass" now computes it from the payload: the scenarios carrying that tag, and their statuses. That is more work for the consumer and one fewer thing to disagree with the results. Whether the schema should offer a derived summary is worth deciding deliberately rather than by inheritance.There is still no overall verdict in the envelope.
TestRunFinished.successin the payload isfalsewhen any scenario failed, which is more than the previous version had, but a consumer must open the payload to see it. Worth deciding whether the envelope wants a top-level verdict.The stream is assembled by this package, not by the runner. If pytest-bdd ever emits Messages natively,
messages.pyshould shrink to a shim. Two places where that assembly makes a judgement a native emitter would not have to: the join from a pytest node to a pickle goes through(uri, scenario name, Examples row), because pytest-bdd's parse and this one are separate; and test-case/step ids are synthesised (test-case-7-3) rather than being ids the runner already owns. Both are internally consistent and neither is observable across runs, but they are not the ids cucumber-js would emit.Meta.protocolVersionis read from the installedcucumber-messagesversion, not pinned in code, so it cannot go stale after a dependency bump. It reports the library version rather than a protocol version negotiated with anything.Not done here: CI does not upload the reports as artifacts. That belongs with whatever consumes them, and can follow.