feat(provider-tck): add the Go conformance suite for OpenFeature providers - #940
feat(provider-tck): add the Go conformance suite for OpenFeature providers#940aepfli wants to merge 41 commits into
Conversation
|
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 |
3755b42 to
55e90a3
Compare
|
Hey @aepfli Could we rename Also, could we make it a flat package instead of putting it under |
Hey, Sure thing, this is just me drafting and testing with Claude code, for multiple languages, and I try to stick to how the repo is handling things. The naming is another question. What if there is a tck for other areas (hooks, listeners - bad examples as they are just interfaces currently) should we be ready for this? |
It really depends on the implementation. If |
|
Both make sense — On options vs a struct: agreed, and it lines up with a request from @toddbaert on the spec PR — adopters should be able to add their own scenarios and step definitions that run in the same backend phase, rather than maintaining a parallel harness for provider-specific features. Go has no runtime scanning, so that needs a registration hook: tck.Run(t,
tck.WithProvider(newProvider),
tck.WithControl(control),
tck.WithCapabilities(tck.Events, tck.Object),
tck.WithSteps(func(ctx *godog.ScenarioContext) { /* vendor steps */ }),
tck.WithFeatures(os.DirFS("testdata/tck-extensions")),
)I'd do the rename and the options change in one go so the API only churns once. Happy to fold both into this PR — or split the rename out first if you'd rather review them separately. |
44013f5 to
06a1d5c
Compare
…iders A conformance suite any Go provider can adopt to verify it implements the provider contract of the specification, and the Go implementation of the cross-language suite defined in Appendix F. The adoption surface is one function call and one struct literal. The TCK owns the whole lifecycle: registering the provider under a suite-scoped domain, waiting for readiness, awaiting events, resetting the backend between scenarios and releasing the provider at the end. Each scenario becomes a Go subtest. The canonical Gherkin and flag set are embedded in the module, so adopting it needs no git submodule. They are copies of open-feature/spec's specification/assets/provider-tck/ and are marked as such; a follow-up will source them from a submodule at build time. Capability gating uses godog.ErrSkip from the Before hook, so a scenario whose capability was not declared is skipped rather than failed, and every skip is reported with its reason. A conformance suite that goes green on scenarios it did not run is worse than no suite at all. Three self-tests run against SDK providers with no Docker: memprovider, the TCK's own updatable in-memory provider, and multi.Provider wrapping one child. The multi-provider suite wraps exactly one child on purpose - the correct answer is what the single-provider suite already asserts, so any difference is attributable to delegation alone. Two Go-specific translations of the shared Gherkin, both documented in the README: "no exception should have been thrown" asserts the evaluation did not panic, because a returned error is the normal shape of an errored evaluation in Go; and providers are registered under a suite-scoped rather than per-scenario domain, so each registration shuts the previous provider down instead of leaking one connection per scenario. Finding: the Go SDK's memprovider.InMemoryProvider cannot update its flag set and emits no events, which Appendix A requires of an SDK in-memory provider and which the JS and Java SDKs both implement. The in-memory suite therefore leaves @configuration-change undeclared and reports it as skipped with the reason; tck.ControllableProvider supplies the missing behaviour by wrapping the SDK's provider, so it doubles as a reference for the fix. Part of open-feature/spec#417 Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
The conformance artifacts were vendored under pkg/tck/assets/ as copies of specification/assets/provider-tck/ in open-feature/spec. A copy can be edited in place, and an edited copy forks the definition of conformance -- which is the one thing this suite exists to prevent. Worse, nothing recorded which revision of the specification the copy was taken from, so "conformant" had no version attached to it. Replace the copies with a git submodule of open-feature/spec at tools/provider-tck/pkg/tck/spec, pinned at dfa16586, and point the //go:embed directives straight at paths inside it. Go embeds real files relative to the package directory, so there is no copy step, no generator and nothing to keep in sync: the specification revision this suite conforms to is the submodule pin, and moving the pin is the only way to change it. Adopters are unaffected and still need no submodule of their own -- the assets are compiled into the package. Contributors to this module need --recurse-submodules, and so does CI: the lint job checked out without submodules, which would have failed the embed at compile time, so it gains submodules: recursive alongside the test job that already had it. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
lifecycle.feature was gated by @events, which is wrong in both directions, and the spec revision this module now pins retags it to @lifecycle. Too lax: the Go SDK synthesises PROVIDER_READY for any provider that does not implement openfeature.StateHandler -- "a provider without state handling capability can be assumed to be ready immediately" -- so declaring @events was enough to pass the readiness scenario without demonstrating anything. A NoopProvider passes it identically, which is precisely the silent green this suite exists to make impossible. Too strict: a stateless HTTP provider such as OFREP emits no events of its own and so cannot declare @events, yet the readiness scenario is not really about events. Withholding @events skipped it for the wrong reason, and nothing in the vocabulary let such a provider say what it actually lacked. Add tck.Lifecycle for the distinct claim -- performs an initialisation that reaches its backend, with an observable outcome -- and revisit the three self-test declared sets, which is where the vacuous pass shows up concretely. TestControllableProvider keeps it: tck.ControllableProvider implements StateHandler, so its READY comes out of its own Init, and it is the only self-test that covers the @lifecycle steps without Docker. TestInMemoryProvider and TestMultiProvider drop it: memprovider.InMemoryProvider is not a StateHandler, there is no backend to reach on either side, and both had been passing the readiness scenario on the strength of @events alone. They now report it as skipped with the reason, which is the honest outcome. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
…he submodule Embedding straight from the spec submodule works locally and in CI and then fails for anyone running `go get`. A Go module is distributed as a zip built from the VCS tree, where a submodule appears only as a gitlink, so the module would ship an empty directory and the package would not compile. Verified without publishing anything: $ git archive HEAD tools/provider-tck | tar -t | grep 'pkg/tck/spec' tools/provider-tck/pkg/tck/spec/ $ git archive HEAD tools/provider-tck | tar -t | grep -c '\.feature$' 0 So the submodule stays the source of truth and keeps recording the spec revision, sync_assets.go copies the artifacts into the package, and the copies are committed and embedded. This is a Go-specific concession: a wheel, a JAR and an npm package are all built from a working tree where the submodule is present, so the other three implementations do not need it. A committed generated file is a lie waiting to happen, so `make provider-tck-assets-check` regenerates and fails on any diff, and CI runs it. Hand-editing a feature file is therefore caught, which is the property that matters: the definition of conformance must not be able to drift. Verified with Go 1.25 under WSL: gofmt clean, go vet clean, go test ok. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
…eck platform-safe Two related problems, both found by running things rather than reading them. The sync check would have failed spuriously on Windows. sync_assets.go copies bytes verbatim, so it writes the LF endings the spec repository pins, but the generated directory carried no .gitattributes -- so a fresh clone with core.autocrlf=true, the Windows default, checks the artifacts out as CRLF and the check then reports a diff on a tree nobody touched. It was latent here only because the files had been generated rather than checked out. Placing a .gitattributes beside them by hand does not work either: the generator wipes and rebuilds the whole directory, so the file vanished on the next run. It is now emitted by the generator, which is the honest arrangement -- everything under assets/ is generated, including the rule that keeps it stable across platforms. That also keeps the embedded bytes identical to every other language's copy, which is the property the digest in the conformance report schema exists to check. The generator itself is portable: pure Go, path/filepath, and byte-exact reads and writes with no text-mode translation, so it behaves the same on Windows, Linux and macOS. Verified: go test ok, sync is idempotent, and the check passes on a Windows working tree with autocrlf enabled. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
…rcion The capability vocabulary and the vendored conformance artifacts belong to this PR, so the rename does too. It was written on the report branch, which is a sibling of the flagd and OFREP adoptions rather than an ancestor -- so the adoptions could not see it, and renaming their references there would have broken them against this base. Moving it down is what lets every branch above share one vocabulary. `tck.StrictNumericTyping` becomes `tck.NumericCoercion`, tag `@numeric-coercion`, and the spec submodule moves to dc4d7ae8 so the vendored assets carry the renamed tag. That bump also brings two unrelated spec changes: the lifecycle readiness scenario is renamed, and control-api.yaml gains the requirement that POST /start not return until the seeded state is served. The rule changed as well as the name, and the appendix now frames it honestly. OpenFeature defines one numeric type deliberately -- `number` is "of unspecified type or size" and languages may differentiate "as idioms dictate" -- so no requirement governs coercion between integer and float, and the README said the opposite. The rule tested here is borrowed from flagd's numeric coercion ADR (open-feature/flagd#1996), which is scoped to flagd's own implementations, and a provider behaving differently is not violating the specification. The underlying gap in the provider contract is open-feature/spec#430. The report branch's own files are left to it: revision.go and report_test.go do not exist here, and the README's conformance-report section stays where the feature it documents lives. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
…ration @targeting and @caching are reserved: they are part of the vocabulary but no scenario carries either tag. Appendix F now states plainly that such a tag must not be declared and must not appear in a conformance report's declaration -- nothing carries it, so declaring it cannot be verified, cannot even produce a skip, and tells a reader of the report only that something was claimed and nothing examined. That was a live defect here rather than a hypothetical one. A Java conformance report asserts both tags as declared, not by anyone's decision but because that adoption declares "every capability except X" and collects every reserved tag on the way past. AllCapabilities had the same shape, and it is the default when Config.Capabilities is nil. So the reserved set is named once, beside the constants, and exposed as Capability.IsReserved. AllCapabilities omits it, which also fixes the default. An adopter who names a reserved capability outright is failed rather than warned: it is rejected by newCapabilitySet, which Config.validate calls, so the run stops before any scenario with the same message shape as any other configuration problem. Warning would leave the claim in a published report, which is the outcome the rule exists to prevent, and the fix is to delete one line. Putting the check in newCapabilitySet rather than only in Config.validate is deliberate: the capability set is the only thing declaration.declared is built from, so a reserved tag now has no path into a report at all, and there is no second route that could drift from the rule. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
A provider with behaviour the specification does not describe -- flagd's fractional targeting, a vendor's segment rules -- had no way to run scenarios for it inside this suite. The alternative is a parallel harness that reimplements provider registration, the readiness wait and the per-scenario backend reset, and then drifts from the one here. Two optional Config fields. ExtensionFeatures is any fs.FS; every .feature file in it joins the run. ExtensionSteps is called during scenario initialisation, after the TCK's own registrations, so an adopter's steps see the same context and hooks. ClientFromContext gives those steps the client for the provider under test, which they cannot otherwise reach: the provider is registered under a suite-scoped domain the adopter never names, and a step building its own client would be testing a different provider. Both absent means today's behaviour exactly -- the same fs.FS and the same path handed to godog, not an equivalent one. The two sources are mounted as one filesystem rather than passed through two godog mechanisms. godog 0.15.1 has Options.FeatureContents, which would take the extension features as bytes, but it parses that and Options.Paths in separate calls with separately seeded id generators, so the AST node, pickle and pickle-step ids of the two sets collide. This package keys skip reasons and Messages test cases by pickle id, and the report identifies a Scenario Outline row by AST node id, so a collision is not survivable. One filesystem, one parse, one id space. Canonical features keep their assets/gherkin/ paths and extensions are mounted under extensions/. The partition stops an extension file shadowing a canonical one, and it is what tells the two apart in a conformance report. An extension filesystem holding no .feature file is refused rather than quietly running the canonical suite alone. Java and Python discover extensions by convention -- a classpath scan, a conftest.py. Go has no runtime scanning, so this is configuration; the constraint is that it stays two fields. This is the extension point on its own, which is why it sits on the base branch: nothing here reads a report. Its end-to-end self-test, selftest_extensions_test.go, does -- it establishes what actually ran by reading the conformance report -- so that file travels with the report machinery on feat/provider-tck-report and is not part of this commit. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
… no API The suite works without reporting, and the branch that adds reporting should only add. It does not currently: it exports capabilityForTag, because a reporter outside this package has to tell a capability-gating tag from a merely organisational one -- deciding whether a scenario was skipped legitimately is exactly that question. Exporting it there makes a follow-up widen this PR's public API, which is the one thing a follow-up should not do. Two branches sit on this one and neither should be able to change what the other compiles against. So it is exported here. Nothing in this PR needs it exported; that is the point. The API is complete enough for a reporter to be written against it, and the reporter is somebody else's commit. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
The conformance artifacts reached this package through a git submodule of open-feature/spec, and because a Go module zip carries a submodule only as a gitlink, a generated copy of every artifact was committed beside it and policed by a CI diff. That was three mechanisms -- submodule, generator, sync check -- to express one dependency. open-feature/spec now publishes specification/assets/provider-tck as a nested Go module whose only content is an embed.FS of the artifacts. This package requires that module and reads the files out of it, so: - the submodule, the generator and the committed copies are gone; - the specification revision is the version pinned in go.mod, verified against the Go checksum database on every build, and advanced with go get like any other dependency; - a plain clone builds, so the lint job no longer needs a submodule checkout and there is no sync step to run. Canonical feature URIs lose their "assets/" segment. The artifacts are addressed in the spec module as gherkin/*.feature, so a canonical result is now reported under gherkin/ rather than assets/gherkin/. The partition that tells a canonical scenario from an adopter's extension is unchanged; only the prefix it keys on is shorter. The line-ending pin the generated copies needed is gone with them. The artifacts now arrive as module-zip bytes rather than through a checkout, so no working-tree conversion can reach them on any platform. The pin is a pseudo-version naming the spec commit that introduced the module. It moves to a tag of the form specification/assets/provider-tck/vX.Y.Z once the spec publishes one. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
…e steps
Move the spec-assets pin to 15fe861 (v0.0.0-20260911095834-15fe861170f4),
which adds metadata.feature, the shutdown scenarios in lifecycle.feature,
the falsy-value and integer-precision scenarios in evaluation.feature, the
lossless half of @numeric-coercion, and six flags to the canonical set.
Steps:
- "the error message should be empty" reads ResolutionDetail.ErrorMessage.
- "the provider metadata name should not be empty" reads Metadata().Name
off the provider instance the scenario registered.
- "the provider is shut down" and "the provider is initialized again" call
the provider's own StateHandler directly, never through the SDK, on the
same instance -- replacing it would test the SDK's bookkeeping, and the
SDK's DeepEqual comparison may treat a re-registration as no change.
Each call is timed, run on its own goroutine bounded by ReadyTimeout so
a hanging shutdown fails its step rather than the test binary, and any
panic (or, for Init, error) is recorded on the scenario state.
- "no exception should have been thrown" now inspects those lifecycle
calls before the evaluation, and no longer requires an evaluation when a
lifecycle call was made.
- "the shutdown should have completed within {int}ms" bounds the most
recent shutdown's duration.
Capabilities: add LargeIntegers (@large-integers). Go's accessor is int64,
so every self-test declares it. None of the self-tests declares
NumericCoercion any more: memprovider type-asserts and never converts
between int64 and float64, so it fails the two lossless scenarios the tag
now requires, and only ever passed the lossy one.
CanonicalFlagSet is decoded from the embedded canonical-flags.json instead
of transcribed, through a json.Decoder with UseNumber so that 10 stays an
int64 flag, 10.0 a float64 flag, and 9007199254740991 arrives exactly;
plain encoding/json would have collapsed the first two and made
integral-float-flag pass the lossless scenario without coercing.
TestCanonicalFlagSetMatchesTheFile pins the decoded types.
Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
The canonical flag set renamed its three falsy flags, and the variants
moved with the keys because the scenarios assert the variant as well as
the value:
false-flag/off -> boolean-zero-flag/zero
zero-flag/zero -> integer-zero-flag/zero
empty-string-flag/empty -> string-zero-flag/zero
The new names are the ones Appendix B's SDK suite (evaluation_v2.feature,
test-flags.json) and flagd-testbed (flags/zero-flags.json) already use,
down to the zero/non-zero variants, so this removes a gratuitous
difference between the provider suite and the two things it runs beside
rather than introducing one.
Nothing here seeds those flags. The canonical set is read from the
embedded spec assets module, so moving the pin to ba002ce8 is what adopts
the rename, and the only thing left to follow is what the assertions
expect. TestCanonicalFlagSetMatchesTheFile is the reason that is safe to
say: it failed on the pin bump with one "missing from the canonical flag
set" per renamed key, which is precisely what it is for.
Leaving the assertions alone would have produced the same failure from
the other side. The feature files ask for the new keys, so a fixture
still answering to the old ones fails every falsy scenario with
FLAG_NOT_FOUND instead.
go.sum is what `go mod tidy` produces. It had collected sums for versions
nothing requires any more -- three of gofrs/uuid, two of
go-immutable-radix, and more -- and a plain `go get` would have added the
new spec revision while keeping the old one, leaving two revisions of the
one module this commit exists to move. The indirect block loses
golang.org/x/text and gains go.uber.org/mock for the same reason.
Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
Narrowing Config.Capabilities says a scenario was not run. It cannot say why, and the two reasons are not alike. A provider with no streaming transport declining @configuration-change has made a decision; one declining @numeric-coercion because it narrows 0.5 to 0 with no error code has a bug. In the results they are the same event -- a skip carrying the same reason -- so a consumer comparing providers reads the second as the first unless the provider author says otherwise. The TCK cannot infer which it is. From the outside, a capability withheld by choice and one withheld because it is broken are the same absence, and the only place that knows the difference is the adoption. So the adoption is where it is declared: Config.KnownDeviations, with TrackedDeviation and UntrackedDeviation as the two constructors, mirroring the Java TCK's KnownDeviation.tracked and .untracked so a reader of one recognises the other. Go has no static methods, so they are package-level functions rather than methods on the type. Untracked is a first-class case rather than a degenerate one. A defect with no issue behind it yet is still worth naming, because naming it is what separates it from a capability withheld by choice, and a declaration that merely omits the tag cannot say which happened. Having to choose a constructor is the point: it makes "nobody has filed this" a statement rather than an empty field. A deviation may also name a capability that IS declared. The capability can hold while one of the scenarios it gates does not, and a provider with two modes may fail that scenario in one and pass it for the wrong reason in the other -- which is exactly the case worth recording, because the passing half is what hides it. Validation is narrow on purpose. A deviation is prose written for a human comparing providers and the suite cannot check prose; what it can check is that the prose exists and that the capability named is real. So an empty Summary is rejected -- an entry recording that something is broken without saying what is worth less than the bare skip it accompanies -- and so is a reserved capability, for the same reason declaring one is rejected: no scenario carries the tag, so nothing was skipped for the deviation to explain. An empty Capability stays legal, being the gap against a mandatory scenario. This goes on the base rather than on the reporting branch because it is something an adopter writes, alongside Capabilities, and therefore part of the suite an adopter adopts. Whatever reads the declaration -- a machine-readable report, a build check, a human -- is downstream of it and does not widen it. The reporting branch changes no adopter-facing API file, and this keeps it that way. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
The conformance assets gain an @reinitialization tag on the scenario "A provider that was shut down can be initialized again", which until now was untagged and therefore mandatory. Requirement 2.5.2 says a provider SHOULD revert to its uninitialized state after shutdown, and its supporting text says "some providers MAY allow reinitialization from this state". Reuse is permitted, not required, so the scenario had been asserting more than the specification does -- the third rule in this suite found to do that. Moving the pin is the whole of adopting it: the assets arrive as a Go module, so `go get` is the only way the definition of conformance changes here. The tag gates nothing until the next commit teaches the vocabulary about it, which is the order the pin and the vocabulary have to move in. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
The pull_request filter matches the BASE branch, so only the suite PR -- the one targeting main -- was ever checked. The report and adoption PRs stacked on it have never run CI, which is why their green ticks meant nothing: the checks on display belong to the base PR. One line, and temporary for the duration of review. The workflow is taken from the head branch, so it has to sit on the base and reach the children by rebase. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
The README told an adopter to drive the control API and offered them the OpenAPI document to do it from, which was accurate while the client lived in the flagd adoption and is now misleading: the client ships here. The example is the real constructor signature, options struct and error and all. Writing it from memory produced a single-string call that does not compile, which is the failure mode a README example has -- nothing checks it, and the reader finds out instead of the author. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
`defer resp.Body.Close()` discards an error errcheck requires to be handled, so `make lint` failed on the suite module and, by inheritance, on all three stacked branches. The shape was invisible until this client moved onto the base branch: at its previous home in providers/flagd/e2e it sat behind `//go:build e2e`, and golangci-lint does not analyse files excluded by a build tag. Nothing about the code changed -- only whether anything looked at it. Discarded explicitly rather than logged: the request has already returned its status, the body is drained immediately above, and a failure to close a body the caller is finished with tells an adopter nothing it can act on. `_ =` matches the `_, _ = io.Copy` on the following line. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
8e436f6 to
7ad62c7
Compare
…geting Follows the canonical assets to spec 26362f85, which makes two changes the suite cannot absorb silently. @Variants is new, and it is the one this suite was getting wrong. Every evaluation scenario asserted a variant, which reads as obviously correct until a backend with no variant concept for a plain flag is put under test: its response carries no such key, the provider never receives one, and no seeding can produce one. Requirement 2.2.4 is a SHOULD and types.md types the field "variant (string, optional)", so ten scenarios were failing a conformant provider against a MUST neither of them states -- and there was nothing to record as a known deviation, because there was no capability to hang one on. The assertions now live in one gated Scenario Outline of eight rows; the value and reason assertions stay untagged, because 2.2.3 makes the value a MUST. @targeting stops being reserved. It was reserved on the reasoning that asserting anything about context passthrough needed an echo endpoint on the control API. It does not: the canonical set now carries one flag with one rule, and a matching context resolves it to a different value, so a provider that drops the context is caught by the resolved value itself. Declaring it was rejected outright before this; now it is a capability like any other, and only @caching is left reserved. That flag also closes the untested half of requirement 2.2.1. The evaluation context is a parameter of every resolve method and no scenario supplied one, so a provider that threw on any context, or serialised it into a malformed request, passed the whole suite. There is now one mandatory scenario that supplies a context to an untargeted flag, a step definition for Appendix B's own wording -- copied rather than paraphrased, because a second way to say "a context containing a targeting key" is the divergence the appendix exists to prevent -- and evaluate() passes the scenario's context through instead of discarding it for an empty literal. Canonical scenario instances go from 40 to 52. The self-tests declare @Variants on evidence: memprovider resolves a named variant and reports its name, and all eight rows pass. They do not declare @targeting, and that is a fact about the flag set rather than the provider. CanonicalFlagSet deliberately ignores targeting-key-flag's JsonLogic rule -- translating it into a ContextEvaluator would make these suites a test of a rule engine written here -- so the flag resolves to its miss variant whatever the context. Undeclared skips the three scenarios with that reason, which is the accurate report. TestCanonicalFlagSetEvaluatesNoTargeting pins that, because otherwise the judgement is invisible: it lives as an absence in four Config literals, and a decoder that later started honouring the rule would make all four wrong with nothing failing to say so. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
Follows the canonical assets to spec 009afe06, which adds one gated Scenario Outline of four rows and the four DISABLED flags it asks for. Canonical scenario instances go from 52 to 56, and the canonical flag set from 14 flags to 18. @disabled-flags is gated for a reason no other capability here has: the answer depends on where the substitution happens, not on provider quality. A provider that evaluates locally -- flagd's in-process resolver, an in-memory flag set -- holds the flag's state and can hand back the value the caller passed in. A provider whose backend decides cannot, and OFREP is the clean case: the caller's default never leaves the process, so the server has never seen it and no response it could send would carry it. The same flag cannot behave the same way across those two architectures and neither of them is wrong. Nothing in the specification says what a provider owes a disabled flag either. Requirement 1.4.7 is about the SDK propagating whatever reason arrived, and 2.2.5 only lists DISABLED among the reason strings a provider may use. So the behaviour is stated by Appendix F, the way @numeric-coercion is, and gated rather than required. The four scenarios assert the value and the absence of an error and deliberately not the reason: each row's caller default differs from the flag's configured value, so a provider that ignores the state returns the configured value and is caught on the value alone, which rests on 2.2.3, a MUST. Pinning reason "DISABLED" would rest on 2.2.5, a SHOULD that permits "some other string". It does not compose with @Variants, and that is not an omission -- a disabled flag has resolved no variant, so there is no name for a variant assertion to be about. None of the three self-tests declares it, and this omission is a defect rather than an absence -- which makes it the odd one out, because an in-memory provider is the one architecture guaranteed to be able to satisfy the capability: there is no backend to ask, so the caller's default is right there. memprovider.InMemoryProvider does return that default, and then attaches a GENERAL resolution error to it while setting reason DISABLED. Those contradict each other, 2.2.5 lists DISABLED among the reasons a resolution that worked may carry, and all four rows fail on `the error-code should be ""` -- only on that step, measured with the capability declared in all three suites rather than inferred from the SDK's source. TestCanonicalFlagSetDisabledFlagsCarryAnError pins it and fails when the SDK stops doing it, at which point the fix is to declare the capability rather than to relax the assertion. The flag table in TestCanonicalFlagSetMatchesTheFile now asserts the state each flag was written with, because state has become load-bearing in the same way the numeric literals are: every row there names the variant a flag is configured to serve, and for the four disabled rows that configuration is exactly what must not reach an evaluation. A decoder that dropped State would leave them serving their configured value -- the one thing these scenarios exist to catch -- with every other assertion in that test still passing. TestOnlyTheDisabledFlagsAreDisabled is the same statement from the other side: nothing outside the four is disabled, because every other scenario assumes the flag it asks for serves its own value. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
… stack Three changes to the adopter-facing API, in one pass so it churns once. THE MODULE IS tools/tck, package tck at its root. erka's review of #940: provider-tck names the thing after what it tests rather than what it is, and pkg/ is no longer common practice, so the import path stuttered as tools/provider-tck/pkg/tck. A flat tck also leaves room for a tck that tests something other than a provider later, instead of a second hook-tck package duplicating the harness. The same rename is being made in Java, JavaScript and Python at the same time, because a Go-only one would break naming parity. Nothing is published anywhere, so the starting version becomes 0.1.0 here rather than being a 0.0.x nobody can move later. Run TAKES FUNCTIONAL OPTIONS RATHER THAN A Config STRUCT. Also erka's, and the reason he gave is the one that matters: options leave room to run suites for different areas without a second config type. They also let a required setting be named in one place instead of being a zero value someone has to remember means "unset" -- a missing option is now reported by name, with the same messages the struct's validation gave -- and let "declare no optional capability at all" be said, which a nil slice could not distinguish from silence. tck.Run(t, tck.WithName("my-provider"), tck.WithControl(control), tck.WithProvider(newProvider), tck.WithCapabilities(tck.Events, tck.Object), tck.WithSteps(func(ctx *godog.ScenarioContext) { ... }), tck.WithFeatures(os.DirFS("testdata/tck-extensions")), ) Config is now the unexported config, reachable only through an Option, so every field it had has exactly one option and the prose that lived on the field lives on the option. THE SUITE OWNS THE CONTAINER STACK. An adopter names a Docker Compose file, says which service and ports to expose, and supplies a factory that builds a provider from a discovered endpoint. The suite starts the stack once, discovers the dynamically mapped host ports, builds the HTTP control against the stack's control API, waits until it accepts commands, constructs a provider per scenario and tears the stack down after the last one. tck.Run(t, tck.WithName("flagd-rpc"), tck.WithComposeFile("testdata/tck/docker-compose.yaml"), tck.WithBackendService("flagd"), tck.WithBackendPorts(8013), tck.WithProviderFromEndpoint(func(_ context.Context, e tck.BackendEndpoint) (openfeature.FeatureProvider, error) { return flagd.NewProvider(flagd.WithHost(e.Host()), flagd.WithPort(uint16(e.Port(8013)))) }), ) Until now only Java owned the stack, so every flagd adoption in the other three languages hand-rolled a container wrapper -- 462 lines in Go's -- and that cost was re-paid by every future adopter. Java's ContainerizedProviderTckTest is the reference this matches: the same concepts, the same defaults (backend service "backend", control port 8080, configuration "default", 60 second startup timeout), spelled as Go options. Two behaviours are worth stating because they are not preferences. The stack starts once and is never restarted. Testcontainers cannot reliably preserve dynamically mapped host ports across a restart, so a restart would silently invalidate every provider already pointed at the old port, and the resulting failure looks like a flaky provider. Unavailability is always simulated inside the running stack through the control API. HTTPControl.AwaitReady probes GET /healthz before the first command, bounded by the startup timeout, treating 404 as ready because that path is optional and the TCP port wait is its documented fallback. There is no settle after a control call, and none is being added: the control API's promise is that a command has taken effect when it returns, and a suite that sleeps instead of holding it to that promise stops being able to detect when it breaks. Java's 50ms post-command sleep is being removed for the same reason. One thing the compose module gets wrong quietly, and that this works around: it keeps a single wait strategy per service name, so waiting on two ports of one service with two WaitForService calls waits only for the last. Every declared port of a service goes into one wait.ForAll. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
Settled for all four languages at once, because a consumer comparing
four reports was reading the same field three ways. Go's doc comment was
the closest of the four -- it is the only one that already stated both
shapes -- so this mostly makes the preference between them explicit and
adds the definition the other three are being changed to match.
An entry says one thing: this provider fails to do something it is
REQUIRED to do. The requirement has to be a numbered MUST, or a rule the
implementation bound itself to elsewhere; flagd measured against its own
accepted numeric-coercion ADR is the worked example, and is why the
"required" half cannot simply read "the specification". Where the
specification permits the choice, withholding the capability IS the
honest report and an entry would assert a defect that does not exist.
Two shapes are legitimate, and the preference between them is now
stated:
1. The capability is declared, the scenario runs, and it fails.
Prefer this -- the failure stays visible and the deviation says it
is known and why.
2. The capability is withheld and its scenarios skip. Legitimate only
when the provider cannot attempt the behaviour at all, so running
the scenario would establish nothing.
Withdrawing a capability IN ORDER TO turn a failing scenario into a skip
is the failure mode the field exists to prevent, which is what makes the
preference worth writing down rather than leaving to taste.
Also stated rather than implied: Summary is required, Issue is optional
and has a tracked and an untracked form, an entry may name no capability
when the gap is against a mandatory ungated scenario, and it may never
name a reserved capability because no scenario carries that tag.
The validation messages now name tck.WithKnownDeviations rather than a
Config field that no longer exists.
Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
Two changes to the adopter-facing surface, both settled across the four language implementations so that one word does not mean two things depending on which language you read it in. BackendControl gains ControlAPI() ControlAPI: required, with no default and nothing inferred from the control's concrete type. The value is closed to the report schema's two members by a defined type rather than a bare string, so a control cannot return "HTTP" and produce a report that fails validation somewhere its author never looks. The same scenarios passing over the HTTP control API and passing through in-process manipulation of a provider that does have a backend are not the same claim, and this is the only field that separates them -- so an omitted value is not "no claim made" but an unfalsifiable one. Both controls shipped here answer it, so the only author who writes anything is the one writing a control of their own, which is exactly the case where the value cannot be guessed. WithConfiguration becomes WithBackendConfiguration, and HTTPControlOptions's Configuration and the DefaultConfiguration constant follow it. The word was already taken: in the conformance report, "configuration" is the provider's own mode -- flagd RPC against flagd in-process -- and that is what WithName feeds. The backend's named flag configuration is a different thing, and three of the four languages had used one word for both. The control API's promise is documented where it now applies. POST /start, /change and /reset must each not return until the new state is being served; how long the provider under test takes to notice is a property of its transport and is what the event timeout covers. Confusing the two makes the provider's detection latency unmeasurable, because the clock starts before there is anything to detect. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
…ot run Two standing decisions that exist nowhere but in a review thread, plus the prose for the two renames in the commit before this one. The Compose harness stays in package tck rather than moving to a second module. The cost is real and is what a reviewer notices first: an adopter with no container to start still takes testcontainers-go, docker/compose and the ~40 transitive pins behind them into its go.sum. It is written down with the reason -- a tools/tck/compose module would need its own version and release-please entry, and a home for BackendEndpoint that both modules can see, which is this package, so it would import the first module and buy nothing but a second coordinate to publish -- so that the next reader finds the answer instead of raising the question again. The containerised conformance suites are excluded from a default build, and now say so. make e2e runs every module's e2e-tagged tests, so a suite that is expected to be red while a known deviation stands would make every pull request red with it; a report that records a deviation and a CI job that fails on it are two answers to the same question and only one of them is readable. The gate is a runtime skip rather than a second build tag, so the adoption stays compiled under -tags=e2e and a signature change here cannot rot an adoption unnoticed. Also fixes a path the rename missed: assets.go, not pkg/tck/assets.go. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
Moves the pin on github.com/open-feature/spec/specification/assets/provider-tck
from 009afe06 to 93eb1a58, which is due because control-api.yaml changed under
it:
- POST /start, /change and /reset must each not return until the new state is
actually being served. Only /start said so before, and /reset is the one a
suite calls before every scenario, so a window there is re-rolled per
scenario rather than once per suite.
- POST /restart drops to [OPTIONAL]. It had been [REQUIRED] on the strength
of a claim in its own description that a TCK used it for the
disconnect/reconnect scenarios; no language's TCK ever called it, and this
one deliberately has no binding for it.
- The @disabled-flags gate's rationale is rewritten. That is the only change
touching the Gherkin this suite runs, and it is a comment.
ControlAPISpec's doc comment follows the endpoints it describes.
Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
The anchor named a heading this README does not have. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
The appendix branch's tip, which is the revision this suite claims conformance to. Moved with `go get` and `go mod tidy` rather than by hand, so go.sum moves with it: v0.0.0-20260912135310-93eb1a58d2d2 -> v0.0.0-20260912211427-ccdb88790bb4 The conformance artifacts themselves are unchanged between the two revisions -- `git diff 93eb1a58 ccdb8879 -- specification/assets/provider-tck/` is empty -- so no scenario, flag or control-API contract moves with this and no test count changes. What changed is the appendix prose: Appendix F gained "Running the suite in CI" and an extended caching gap entry, both of which this module's README now points at instead of restating. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
…stead of restating it Three changes to the README, all of them about not saying something twice. The environment variable loses its PROVIDER_ prefix: PROVIDER_TCK_RUN becomes TCK_RUN, following the package rename to `tck`. PROVIDER_TCK_REPORT_DIR becomes TCK_REPORT_DIR on the report branch, and the two move together on purpose -- a partial rename would leave TCK_RUN beside PROVIDER_TCK_REPORT_DIR in one repository, which is worse than either consistent answer. Nothing is published and nothing is scripted against either name. "What a default build runs" keeps the mechanism -- the command a maintainer runs, and where the exclusion lives in this build -- and hands the reasoning to Appendix F's "Running the suite in CI", which is where it is now settled for all four languages. The Go-specific half is worth keeping and is kept: `make e2e` applies `-tags=e2e` to every module in the workspace, so a build tag is not an exclusion here but the opposite, which is the first of the two mistakes the appendix names and the one this repository was making. The appendix also now asks that an excluded suite keep compiling, which is what the runtime skip buys over `//go:build e2e && tck`. The caching gap entry is the other half. The flagd RPC LRU cache warning was this README's alone among the four, and it is now Appendix F's caching entry with the scenario-authoring constraint spelled out there: no scenario may evaluate the same flag twice without a configuration change in between. That is a constraint on scenario authors rather than a Go concern, so it belongs with the scenarios. What stays here is the Go adoption's own position -- the cache is not turned off, so the suite really does run against a caching provider -- and the one consequence the appendix does not record, that the configuration-change scenario already depends on cache invalidation working. The known-deviation guidance is left in place but now says outright that it is the appendix's normative wording rather than a Go restatement, and that the appendix wins if they ever disagree. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
Three rules in control-api.yaml constrain the order and the repetition of
control calls rather than any single call, so none of them is visible in the
code of one method:
- /reset is the preferred scenario-isolation primitive, because it causes no
availability blip and so cannot inject a spurious lifecycle event into the
next scenario. /start is the fallback, not the default.
- "The fallback is detected once per suite and cached." A backend answering
404 or 501 must be asked once, not once per scenario.
- "/reset MUST NOT be expected to start a backend that is currently stopped",
so the scenario following a disconnect is prepared with /start. A suite that
reset instead would run its next scenario against a backend that is still
down, and /reset would answer 200 either way.
All three were previously reachable only by starting Docker and reading a
container's logs, which means in practice they were not checked. Python and JS
already test them; Go and Java did not.
The control API is HTTP, so a recording httptest.Server is a complete stand-in
for a backend -- and it answers things a real launchpad cannot be asked for on
demand: a 501, a single 500, a /healthz that is unready for exactly three
probes. Twelve tests, no Docker, about a second.
Two of them are about what must NOT happen, which is why the assertions are on
the exact call sequence rather than on a count: a 500 from /reset is not the
documented way to say "not implemented" and must not silently enable the /start
fallback for the rest of the suite, and a reconnect must clear the outage flag
so that the preference for /reset comes straight back rather than being lost
for every later scenario.
Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
…enario JS fails a run if a feature file ever carries a reserved tag; Go, Java and Python would silently report the new scenarios as skipped for an undeclared capability. That is the unclaimable-capability failure Appendix F describes, and it is silent in both directions: a reserved capability cannot be declared -- newCapabilitySet refuses it -- so the day the specification adds the first @caching scenario, every adoption reports it as skipped for something no adopter is permitted to claim. Green suite, well-formed report, question withdrawn. Nothing else here would catch it. The under-collection guard is satisfied because the scenario was collected and gated rather than dropped, and a capability-gated skip is explicitly not a gap -- the rule that exists so a provider need not declare every capability. So expiredReservation runs before the capability gate in beforeScenario and returns an ordinary error rather than one wrapping godog.ErrSkip. It reads sc.Tags, which is the scenario godog parsed, so it cannot disagree with the run about which tags a scenario carries -- including tags inherited from the feature and tags on an Examples block. The message names the single line to delete. TestTheCanonicalScenariosCarryNoReservedTag is the same check against the pinned assets, so moving the pin trips it here rather than in an adopter's run. It is deliberately crude, and the first version of it was too crude: searching the whole feature source failed immediately, because events.feature carries a comment saying which scenario belongs behind @caching once someone writes it -- which is the opposite of the thing being guarded against. It now looks for the tag as a whole token on a line that is not a Gherkin comment, and the runtime check is what is authoritative. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
…llection The assets move to spec c342461a, which is the first pin that moves real artifacts rather than appendix prose: it adds gherkin/reason.feature and strips thirteen resolution-reason assertions out of evaluation.feature, errors.feature and lifecycle.feature. A new feature file arriving through `//go:embed gherkin/*.feature` is picked up by the pattern, and that is exactly the thing worth checking rather than assuming. Measured: the collected scenario count across the three in-memory self-test suites goes from 56 to 65 over this pin, and the nine are reason.feature's four outline rows plus its five single scenarios. So the check is now permanent. TestEveryCanonicalFeatureFileIsCollected spells the canonical feature set out rather than deriving it, and also fails a collected-but-empty file and a subdirectory the one-level-deep embed pattern would not reach. Under-collection is the silent half of the pair whose loud half is the reserved-tag expiry check: a suite that stays green while asking fewer questions than it advertises. Verified to trip by running it against the previous pin, where it names reason.feature as the missing file. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
Requirement 2.2.5 is a SHOULD, and it goes further than 2.2.4 does: it lets a provider populate reason with one of the listed values "or some other string indicating the semantic reason for the returned flag value". A provider whose backend reports vendor-specific reasons is conformant, and the suite used to assert an exact reason in thirteen places across three feature files, which narrowed that SHOULD into a MUST for every adopter. It bought very little -- every canonical flag resolves to a value distinct from the caller's default, so a provider that silently falls back was already caught by the value. The reasons now live in reason.feature, gated as a whole, and tck.StandardReasons is the capability that gates it. Declaring it is a provider saying "I use the standard vocabulary with the standard meanings"; a provider that does not declare it loses nothing, because its values, variants and error codes are asserted everywhere else on MUSTs. The wording here and in the README mirrors Appendix F's "@standard-reasons: a claim, not an exemption" rather than restating it in Go's own terms. It is declarable, not reserved, and the doc comment carries the meanings the claim commits to -- including that STATIC for a rule-less flag is a call the specification leaves open, so a provider answering DEFAULT there is not defective and should simply not declare the tag. All three in-memory self-tests declare it, on evidence rather than on reading the provider: memprovider reports STATIC for a rule-less flag and the SDK reports ERROR for the unknown-flag and type-mismatch cases, so six of the nine executed rows pass in each suite. The other three carry @targeting or @disabled-flags, which these suites withhold, and are skipped for those. The multi-provider suite is the interesting one -- a wrapper that rewrote STATIC to DEFAULT or lost ERROR on a type mismatch is caught by reason.feature and by nothing else. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
… go stale Moves the pin from c342461a to 89b1519a. Two commits, and only one of them touches the assets module: e616ff4d adds Appendix F's fifth declaring rule, which is prose outside the module, and 89b1519a fixes two $comment blocks in canonical-flags.json that still told adopters "every scenario expects reason STATIC" -- untrue since the reason assertions moved into reason.feature. Java found it while re-pinning, which is the argument for four implementations: the file is copied verbatim into four languages' artifacts and was wrong in a way no single language's tests could see. The Gherkin is byte-identical across the two versions, verified by diffing the two module cache directories rather than inferred from the commit range, so the suite still collects 65 scenarios and all three self-test suites still pass 65 of 65. The README gains the paragraph the effort has been missing: a pin here cannot silently go stale. Three of four languages hit a version of that problem, one of them running a whole adoption suite against the previous pin's assets while reporting byte-identical numbers, because a rebase moves a gitlink and not the submodule working tree a suite actually parses. Go consumes the assets as a module dependency, so the version is source -- a rebase moves go.mod and go.sum the way it moves any other line and leaves no checked-out copy behind to disagree with them. Verified rather than asserted: moving the pin back without its hashes fails with "missing go.sum entry" before a scenario is collected, tampering with the recorded hash fails with "SECURITY ERROR ... checksum mismatch", and the cache holds each version read-only in its own directory. The contrast is one directory away in this repository, which is why the paragraph names it: the four non-TCK flagd suites read their Gherkin out of the flagd-testbed submodule working tree. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
…as none Appendix F's fifth declaring rule (spec e616ff4d) says a capability the language's SDK cannot express is refused by the implementation rather than left to adopters. Two exist across the four languages: @large-integers in Java, whose integer accessor is a 32-bit Integer, and @numeric-coercion in JavaScript, which has one numeric type and so cannot ask "this float, as an integer?" at all. Go has neither, and this commit establishes that by measurement rather than by comment. Client.IntValueDetails takes and returns an int64 and FloatValueDetails a float64, pinned off the SDK's own method signatures by two new tests, so a narrowing in a future SDK fails there instead of surfacing later as an apparent provider defect. The behaviour matches: the @large-integers scenario runs and passes in all three self-test suites, asking for and getting 9007199254740991 exactly, and OFREP declares @numeric-coercion and satisfies all three of its scenarios. A throwaway provider implementing the lossless-coercion rule over memprovider passes all three locally, and fails exactly the lossy one when the check is removed -- so the assertions are live rather than vacuous. inexpressibleCapabilities is therefore empty. The mechanism is added anyway, because the alternative is that the first Go capability to hit this rule is discovered by an adopter publishing a claim no scenario could have verified -- which is the failure the reserved-capability rules already exist to prevent, reached by another route. Adding a line to that map is the whole change: the configuration error, the default set, the known-deviation check and the skip reason all read from it. The two refusals are kept apart on purpose, in the message and in the skip reason. A reserved capability is global and expires when the specification adds scenarios; an inexpressible one is this language's and lasts until the SDK changes. A reader seeing a capability absent from a report has to be able to tell "this provider declined" from "no provider in this language can be asked", and only the first says anything about the provider. Collapsing them into one predicate or one message is the tidy-up that destroys that, so a test fails if the two messages ever match. It is exercised rather than dead. The tests install an entry and drive the refusal, the default set, the deviation check and both skip reasons; seven mutations of the mechanism were applied one at a time and every one was killed. Installing a false entry for @large-integers also reds all three self-test suites, because they declare it -- so the path runs end to end through a real suite and not only through units. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
Four places in this module illustrated the choice-versus-defect distinction
with "a provider that does not declare @numeric-coercion because it narrows 0.5
to 0 with no error code has a bug". Read literally that endorses withholding a
capability for a defect, which is the combination the known-deviation rules two
paragraphs later tell an adopter to avoid -- and the flagd adoption one
directory away did exactly what it illustrated, until pass 7.
The sentence was not invented here. It mirrors Appendix F's own
@numeric-coercion note, which said a withholding provider "should say which it
is -- a deliberate choice, or a tracked defect". The specification corrected
itself in open-feature/spec 045950ca; this follows it.
The distinction being drawn is sound and is kept: an undeclared capability can
be a choice or a defect, the results cannot tell them apart, and that is why
knownDeviations exists. Only the illustration changes, to one capability
withheld twice -- a provider with no streaming transport declining
@configuration-change by choice, and the Go SDK's memprovider declining the same
tag because it cannot update its flag set at all, which Appendix A requires it
to support. Same absence, two meanings, and both are real and measured in this
repository. A provider that narrows 0.5 declares the capability, fails the lossy
scenario and records the deviation against the failure; the files now say so.
Two rules the appendix has since settled are cited rather than re-argued, which
matters more than it sounds: four independent arguments for one rule is how this
effort keeps diverging.
- The sixth declaring rule (spec 4cab0320): declare when at least one scenario
gating the tag can be put to the provider, withhold only when none can --
the unit is the scenario, not the tag. The README's capability section now
points at it and names the two adoptions that turn on it, instead of the
reader having to infer the rule from the two outcomes.
- The self-test carve-out (spec 045950ca): a TCK's own self-tests may withhold
for an identified defect, on the condition that the defect is pinned by a
test of its own. selftest_inmemory_test.go withholds @disabled-flags for a
memprovider defect and meets that condition through
TestCanonicalFlagSetDisabledFlagsCarryAnError. It now cites the carve-out and
names the condition it meets rather than arguing the case again.
No behaviour, no counts: the three self-test suites collect 65 scenarios and
pass 65 before and after.
Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
Prose-only in the assets module: 89b1519a..4cab0320 touches specification/appendix-f-provider-conformance.md and nothing else, so the Gherkin, the canonical flag set and the control API are unchanged. Confirmed by content rather than by the commit range. The two module cache directories are byte-identical -- diff -r is empty, and a sha256 over each tree taken relative to the module root (so the version string is not an input) is ebf99124fcafe8127260e07cbd271a1270fe176c71daa2dd87b69322ab28e799 for both, over the same thirteen files. Note that go.sum's h1 hashes DO differ across the two versions and say nothing about this: dirhash prefixes every file name with module@version, so a byte-identical module hashes differently under a different pseudo-version. The counts therefore do not move, measured rather than assumed: the three in-memory self-test suites collect 65 scenarios and pass 65 on both pins, with the same seven skips. Moved with go get plus go mod tidy, never by hand. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
|
It looks great. Is there any reason to use |
to be honest, I have not thought about dedicated execution, and if this should run under the e2e build tag, an own build tag, or in general. The env var is currently just a small first draft version. But you got me thinking, and most likely we even want to have an own build step for this. But this is definitely worth a discussion, i have so many findings, and currently the approach is highly opinionated. What would be your ideal approach? @erka |
Appendix F now asks for a conformance suite to be a step of its own rather than a slice of an existing end-to-end suite, because of what a failure says: a red conformance step reports that conformance failed, where the same scenarios inside an e2e suite report that a test failed, and the two mean different things. An e2e suite is expected green, so a failure is a regression; a conformance suite fails scenarios by design wherever a known deviation is declared. JavaScript has `nx tck` and Python has `poe test-tck`; Go had `make test`, `make e2e` and a hand-typed command in a README. `make tck` is that step. It runs every test whose name matches `Conformance` and nothing else; `make e2e` is the same sweep over the workspace with `-skip` in place of `-run`, so the two targets are the two halves of one filter and every e2e-tagged test runs in exactly one of them. The filter is a test-name filter and not a second build tag on purpose, and that is also the answer to why the gate was never a build tag in the first place. A tag takes the adoption out of the build, and the appendix asks for the opposite -- the suite must keep compiling in the default build even when it does not execute, so that a signature change in tools/tck cannot rot an adoption unnoticed. Both targets still compile every module under -tags=e2e, so that coverage is kept. A tag would also not have excluded anything here, since `make e2e` applies -tags=e2e to every module in the workspace. The naming contract this creates is stated where an adopter reads it -- the package doc, the module README, and CONTRIBUTING.md next to the two targets it now sits beside -- and each adoption grows a test that holds it, in the commits that remove the environment variable. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
`make tck` was `go test -run 'Conformance'` and `make e2e` was the same sweep with `-skip 'Conformance'`, so which step a suite ran in depended on what its tests were called. That made a rename silently move a suite from one step to the other, and it needed a test in each adoption parsing its own package to hold the correspondence in both directions. A provider's conformance adoption is now a module of its own at providers/<name>/tck, beside the provider's e2e suite rather than inside it, and the two targets partition the module list instead of the test names: `tck` runs the modules named tck under a component directory, `e2e` runs the others. A suite is in the conformance step because of where its files live, and a file is in a directory or it is not. tools/tck is the harness rather than an adoption, so it stays on the e2e side. `make e2e` gains a second command: the conformance modules again, under -tags=tck with an empty -run pattern. Excluding them from the sweep must not take them out of the build -- Appendix F asks for a suite that keeps compiling against tools/tck even when it does not execute, so that a signature change in the harness cannot rot an adoption unnoticed. That command is also what makes a `tck` build tag safe, and the adoptions now carry `//go:build tck` in place of the `//go:build e2e` they had while they were packages inside the e2e suites. The objection to a build tag was that it takes the suite out of compilation, which is only true while nothing in the pipeline builds with the tag; something now does. The two mechanisms do two jobs and neither substitutes for the other: the module path decides which of the two targets runs a suite, and the tag keeps the suite out of every invocation that asks for no tags -- `make test`, and a bare `go test ./...` in the module. The adoptions themselves move on their own branches; this is the repository tooling and the documentation, which all three children inherit. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
I see that using env vars is a different approach compared to previous e2e tests. I'm just curious about it and don't have any strong ideas. @aepfli |
de17804 to
abe0f9a
Compare
The base README had grown to 70 KB by accreting every finding, decision
and justification as it was discovered, which is larger than Appendix F
itself -- the normative document the whole cross-language effort rests
on. An adopter looking for a forty-line adoption example had to read
tens of thousands of words to find it.
The rule applied, sentence by sentence: would this be equally true in
another language's README? If so it belongs upstream, and this file
should link to it rather than restate it. Two upstream documents carry
it: Appendix F, and the assets module's own README beside the Gherkin.
Deleted because Appendix F already carries it, normatively:
- the capability tag table's Meaning column, which was the appendix's
prose word for word. What is kept is the tck.<Constant> to tag
mapping, which is the only part that is Go's.
- the long-form arguments for @reinitialization being permitted rather
than required, @caching being reserved, @lifecycle and @events being
separate, what @standard-reasons claims, @disabled-flags turning on
where substitution happens, and numeric coercion coming from flagd's
ADR rather than a requirement.
- the rules for declaring a capability and the two shapes of a known
deviation, now cited with the short form an adopter needs at the
moment they are writing one.
- the known-gaps list, which is the appendix's open questions.
Deleted because the assets module's README carries it: that the
directory is a Go module whose only content is an embed.FS, why a
submodule would arrive empty for anyone running , and the shape
of the nested module's release tag. The canonical flag set's traps are
documented there too, in more detail than this file ever had them, so
the spec-module section now points at it rather than paraphrasing.
Deleted because it is recorded elsewhere and is not documentation of
this library: the Findings section. Both findings are filed upstream --
go-sdk#530 for the in-memory provider having no way to update its flag
set, go-sdk#552 for a disabled flag carrying a GENERAL error -- and both
are in the pull request body. The self-tests section now names them in a
sentence each with the issue links, because that is where a reader meets
the withheld capabilities.
Kept, because they are Go's own and measured rather than assumed: that
Client.IntValueDetails is int64 and Client.FloatValueDetails float64, so
neither @large-integers nor @numeric-coercion is inexpressible here and
inexpressibleCapabilities is empty; the reserved-tag and
under-collection tripwires; that the pin cannot silently go stale
because there is no working tree to disagree with go.mod; the three
translation decisions behind "no exception should have been thrown",
the direct StateHandler calls and the suite-scoped domain; and the
module-path-versus-build-tag split that decides what runs where.
The options surface is now one reference table with every knob and its
default, rather than prose spread over five sections.
70035 bytes to 25160, and no behaviour or count changes.
Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
The capability vocabulary, the rules for declaring, the known-deviation shapes and the control API's invariants are documented in open-feature/spec. This package was restating them at length beside its own API, so a reader had two sources for one rule and no way to tell which was current -- and two of them had already drifted apart once. Each of those now carries a sentence and a link, and keeps only what is Go's: how an adopter decides a capability against the Go SDK (StateHandler for @lifecycle, the int64 accessor and the distinct numeric accessors, both measured by tests), the memprovider defects the self-tests turn on, and the in-process decoder's own properties. The package doc comment stops being an essay: the sections restating capability.go, config.go, control.go and compose.go are gone, and the one-module trade-off it recorded moves to the README, which is where a Go-specific packaging decision belongs. Comments only. Not one non-comment line changed, verified by diffing the comment-stripped files; tests, vet, gofmt and golangci-lint all green. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
Closes #938
Part of open-feature/spec#417. The
language-agnostic artifacts are in spec#423; the report envelope in
spec#425.
The PR chain
The same four-branch shape in every language, so understanding one is understanding all four:
reportandflagdare siblings, not a sequence: the follow-ups fill gaps without changing theadopter-facing API, so neither blocks the other and either can land first.
What this is
A conformance suite any Go provider can adopt to check that it implements the provider contract in
the specification — the Go implementation of Appendix F, running the same Gherkin
scenarios, against the same canonical flag set, driven through the same control API as every other
language's TCK. That shared basis is the point: "conformant" only means something if the question is
identical everywhere.
Java is the reference (java-sdk-contrib#1830 and its stack). This is not a transcription of
it — Go has no inheritance, no JUnit Platform Suite and no exceptions, and each of those forced a
decision rather than a translation.
The adoption surface
tck.Runwith functional options. Three are required — a name, a provider factory, and the seamthrough which the backend is driven:
Options rather than a
Configstruct, at @erka's request:it leaves room for a
tckthat tests something other than a provider later, instead of a secondpackage duplicating the harness.
Runvalidates up front and names what is missing.The suite owns the container stack.
WithComposeFileplusWithBackendPortsis the whole of it— starting Compose, discovering dynamically mapped host ports, building the control client, waiting
for the control API, tearing down. Conventions with working defaults:
WithBackendService(
backend),WithControlPort(8080),WithAdditionalPorts,WithBackendConfiguration(
default),WithStartupTimeout(60s). This is now required by Appendix F, after three of fourlanguages originally shipped only the control-API client and left orchestration to the adopter — at
which point every adoption hand-rolled between 130 and 460 lines of the same wrapper.
A provider with no backend passes
tck.WithControland its ownBackendControlinstead, andsupplies
tck.WithProvider— a factory taking no endpoint. Two factory options rather than onebecause Go cannot overload, and a single signature would hand backend-less adopters a zero-valued
endpoint that means nothing.
Each scenario becomes a Go subtest, so
-runselects one and a failure names a scenario. Thecanonical Gherkin and flag set arrive through the spec's own nested Go module, so adopters need no
submodule and no vendored copy — a copy would be a second place for conformance to drift.
Declaring what a provider can and cannot do
WithCapabilitiesnames which optional parts of the contract this provider supports, from fourteentags —
tck.Events,tck.Lifecycle,tck.Stale,tck.Targeting,tck.DisabledFlags,tck.NumericCoercion,tck.LargeIntegers,tck.Reinitialization,tck.ConfigurationChange,tck.Object,tck.Variants,tck.UnavailableInit,tck.StandardReasons, andtck.Caching,which is reserved and
cannot be declared. A scenario gated on an undeclared capability is reported skipped with its
reason, never passed.
WithKnownDeviationsis the separate statement that the provider fails something it is required todo. Appendix F settles which of its two shapes to prefer: declare the capability, let the scenario
fail, and record the deviation beside the failure — rather than withdrawing the capability so the
scenario skips, which hides a defect behind something that looks deliberate.
reasonis asserted in two places only, and they mean different things. Requirement 2.2.5 is aSHOULDpermitting "some other string", so the scenarios that resolve a value no longer assert areason at all — an exact-match assertion there fails providers the specification permits. What
replaced it is a capability.
tck.StandardReasonsdeclares that this provider speaks thespecification's own vocabulary, and a feature file then checks the whole of it:
STATIC,TARGETING_MATCH,DEFAULT,DISABLED. Undeclared, those scenarios skip with their reason, andnothing else in the suite cares what a provider calls a reason.
The error cases are not part of that bargain and are asserted unconditionally. Requirement 1.4.8
makes an error code mandatory in abnormal execution, so
ERRORthere is not a dialect — a reasonthat disagrees with the error code shipped beside it is a contradiction. The assertion is on the two
agreeing, not on which of them is authoritative.
Three Go-specific decisions
1. "no exception should have been thrown" asserts the evaluation did not panic. Go has no
exceptions, and the returned
erroris not one — an errored evaluation correctly returns the codedefault alongside a non-nil error, which is the normal shape of the API. What the feature files
forbid is an unhandled failure escaping a flag evaluation and taking the host application down,
which here is a panic. Provider registration is guarded the same way.
2. Providers are registered under a suite-scoped domain, not a per-scenario one. Registering in
a domain replaces and shuts down the previous provider. A fresh domain per scenario reads cleaner
but would leave every provider of the suite registered and running — for a provider holding a
network connection, one leaked connection per scenario.
3. Capability gating is
godog.ErrSkipreturned from theBeforehook. godog treats that as"skip this scenario and all its steps" rather than a failure, and
errors.Isstill matches aftergodog wraps it. Every skip is collected and printed with its reason at the end of the suite, because
a suite that quietly goes green on scenarios it did not run is worse than no suite at all.
A non-obvious constraint the design depends on: the SDK compares providers with
reflect.DeepEqualunless the dynamic type is a pointer. A value-typed provider holding an equal flag set would compare
as the same provider, and the per-scenario replacement the TCK relies on for isolation would
silently not happen.
ControllableProvideris therefore pointer-only, and says so.Self-tests: three suites, no Docker
TestInMemoryProvidermemprovider.InMemoryProviderTestControllableProvidertck.ControllableProviderTestMultiProvidermulti.Providerwrapping one childTestMultiProviderwraps exactly one child deliberately. That is the interesting configurationrather than a degenerate one: the correct answer is precisely what
TestControllableProvideralready asserts about the child alone, so any difference between the two suites is attributable to
the multi-provider and nothing else — a variant that does not survive the hop, a reason rewritten to
DEFAULT, an error code flattened toGENERAL, an event that never reaches the client.Two findings against the Go SDK
The in-memory provider cannot update its flag set. Appendix A is unambiguous:
memprovider.InMemoryProviderhas no update method, is not anEventHandler, and is not aStateHandler. JavaScript hasputConfiguration()and Java hasupdateFlag(); Go has neither. Theknock-on is not confined to this suite — Appendix A also requires SDK end-to-end tests to use the
in-memory provider, so go-sdk's own Appendix B suite cannot cover configuration-change
events either. Tracked as go-sdk#530.
Handled honestly rather than worked around:
TestInMemoryProviderleavesConfigurationChangeundeclared so the scenario is skipped with the reason, and
ControllableProvidersupplies themissing behaviour by wrapping the SDK's provider rather than reimplementing it — every
resolution decision is still
memprovider's — so it doubles as a reference for what the SDK'sprovider should grow.
A disabled flag returns a
GENERALerror alongside the default.in_memory_provider.goreturnsdefaultValuewithDisabledReasonandNewGeneralResolutionError, which contradictsrequirement 2.2.6. Already filed as go-sdk#552
and fixed by #574, but no release carries the fix
yet — so this module still pins v1.18.0 and
TestInMemoryProviderwithholds@disabled-flags. Thepin and the withheld capability should both be revisited once a release lands.
Running it
The self-test suites run in the default build with no Docker. The adoption suites are gated:
Appendix F now carries the reasoning: an adoption suite's honest output is red while real gaps
remain — filed provider defects, missing backend fixtures — so making it a required gate forces
someone to silence it, and the cheapest way to silence a conformance suite is to stop asking the
question.
Two mechanisms, doing two different jobs. Neither substitutes for the other, and saying which is
which is the whole of the answer to
@erka's question about a build tag:
providers/<name>/tck, beside the provider's e2e suite rather than inside it.make tckruns thosemodules;
make e2eruns the others. A file is in the directory or it is not.//go:build tcktag decides nothing about that. It keeps the suite out of every invocationthat asks for no tags at all —
make test, and a barego test ./...typed in the module — whichwould otherwise start a Docker stack.
Why a build tag is safe here when the objection to one was that it is not. The objection is that
a tag takes the suite out of compilation, and Appendix F asks for the opposite: the suite must keep
compiling even when it does not execute, so a signature change in the harness cannot rot an adoption
unnoticed. That holds only while nothing in the pipeline builds with the tag. Something now does:
make e2e's second command builds the conformance modules under-tags=tckwith an empty-runpattern, which keeps the build and drops the run. Verified, not asserted:
So the honest answer to "why not a build tag?" is neither "a tag is wrong" nor "an env var is
better". The question had two halves — an exclusion, and a named thing to run — and only the first
had an answer at the time.
make tckis the second half, and with it in place the tag is right.Two earlier mechanisms are gone. A
TCK_RUNenvironment variable, because an exclusion hiddeninside a test function is invisible from the build and is not the step the appendix asks for. And a
-run 'Conformance'/-skip 'Conformance'test-name split, because it made the name of a testload-bearing and needed a guard in every adoption to hold the correspondence. What each adoption
still carries is the other half of that guard: a test that fails if nothing in the module reaches
tck.Run, since a conformance module whose tests have stopped running the suite leavesmake tckgreen by running nothing. It is untagged, so it runs in the build the suite is absent from and reads
the tagged file off disk.
Worth stating plainly: this PR previously described the adoptions as excluded when
make e2ewas infact running them, red, on every PR. All four languages had a version of that, each defeated by a
different mechanism.
Verification
At
abe0f9a6:go build ./...,go vet ./...,go test ./...in the modulegolangci-lint runfrom inside the modulego.mod, so no copy exists to driftKnown gaps
targeting-key-flagresolvesdifferently for a matching context, so a provider that drops the context entirely is caught by the
resolved value. What is still unverified is that the whole context arrives: a provider
forwarding the targeting key and silently discarding every other attribute passes. Closing it
needs an echo operation on the control API, or a canonical flag whose rule keys on a custom
attribute.
@cachingis reserved, no scenarios yet. Note for whoever writes them: flagd's RPCresolver runs an LRU cache by default and reports
CACHEDon a repeat evaluation, so any scenarioevaluating the same flag twice sees a different reason the second time from a correct provider.
one designated flag; finer-grained operations would need endpoints that do not exist.
Open questions
testcontainersand the Composeclient in
tools/tck, so a backend-less adopter carries thego.sumentries and compilesnothing it does not import. The alternative is a second module with
BackendEndpointvisible toboth. Java made it
provided/optional, Python an extra, JS an optional peer — four answers,each written down, none obviously wrong.
what makes reports comparable, so this is the weakest link in the design, and it wants one answer
rather than four.
ControllableProvidership here, or should the fix land in go-sdk first and thismodule depend on it?