test(flagd): adopt the OpenFeature Provider TCK - #1847
Draft
aepfli wants to merge 15 commits into
Draft
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 |
github-actions
Bot
requested review from
Kavindu-Dodan,
beeme1mr,
thisthat and
toddbaert
September 11, 2026 10:34
aepfli
marked this pull request as draft
September 11, 2026 10:40
aepfli
force-pushed
the
feat/provider-tck-flagd
branch
2 times, most recently
from
September 11, 2026 13:44
20cf419 to
177ebe0
Compare
aepfli
force-pushed
the
feat/provider-tck-flagd
branch
11 times, most recently
from
September 13, 2026 09:09
8ac8f62 to
a0bd1eb
Compare
Runs the conformance suite against the flagd provider in both resolver modes. The whole adoption is a shared abstract base and two subclasses that differ only in resolver and port: the TCK brings its own Gherkin, its own step definitions and its own Compose lifecycle, and works out which suite is running from the JUnit test plan, so a mode needs no registration and no build configuration. The Compose stack wraps the unmodified flagd-testbed image, which already serves both flagd and the launchpad control API that this TCK's control API contract was derived from. No host port bindings: the TCK discovers dynamically mapped ports after startup, so the suite runs in parallel and does not collide with a developer's local flagd. capabilities() is declarableExcept(NUMERIC_COERCION). Evaluating float-flag (0.5) through the integer API returns 0 with no error code rather than TYPE_MISMATCH with the code default -- the value is silently truncated. Coercion as such is permitted; it is the lossy case being accepted that is the defect, tracked as open-feature/flagd#1996. Both resolvers behave identically, which places it in the shared provider layer rather than in either transport, so it is declared once here. Delete the override when the defect is fixed. Split out of #1830 so that the suite and its first adopter are reviewed as separate questions: whether the TCK is the right contract, and whether flagd satisfies it. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
capabilities() withholding @numeric-coercion reads, from the outside, exactly like a provider with no streaming transport declining @configuration-change: the scenarios are skipped either way and nothing in the run says which of the two happened. One is a limitation, the other is a bug, and a consumer comparing providers needs to be able to tell. knownDeviations() is the only place that can say so, because only the provider author knows. Tracked against open-feature/flagd#1996. The summary names the half of the coercion rule that is broken -- the lossy one -- because "flagd coerces numbers" on its own reads as intended behaviour rather than as a defect. Delete this and the capabilities() override together, once evaluating float-flag (0.5) through the integer API reports TYPE_MISMATCH. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
…s not a deviation Three of the flags the suite's assets added are absent from flagd-testbed v3.8.0: large-integer-flag, huge-integer-flag and integral-float-flag. Only the first is reached by a scenario that runs here -- huge-integer-flag is asked for solely under @large-integers, which is not applicable in Java, and integral-float-flag solely under @numeric-coercion, which this provider withholds -- so exactly one untagged scenario, the 32-bit precision one, fails FLAG_NOT_FOUND in both modes. open-feature/flagd-testbed#392 is open for it; the Compose tag gets bumped when it lands, which is why the note lives next to the tag as well as in the class. A missing flag is a gap in the stack, not in the provider, so it is documented rather than declared as a KnownDeviation. A deviation says the provider is wrong, and the provider was never given the flag to get wrong. The three falsy flags used to fail the same way and no longer do, which is worth writing down because the failure looked identical. The testbed's zero-flags.json already served boolean-zero-flag, integer-zero-flag and string-zero-flag with zero/non-zero variants, while the canonical set called them false-flag, zero-flag and empty-string-flag; the base moved the canonical names onto the testbed's rather than the other way round, so those three scenarios now resolve against flags that were always there. Also says why capabilities() calls declarableExcept rather than EnumSet.complementOf, which now matters more than it did: the complement would claim @large-integers as well as the two reserved tags, and the suite refuses that declaration at startup. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
Two declarations examined rather than asserted, one of which retracts a mistake. @reinitialization is withheld, and no KnownDeviation accompanies it. shutdown() sets the sync resources' isShutDown flag and never clears isInitialized (FlagdProvider.java:136-155, FlagdProviderSyncResources.java:27-28, 112-115), so a later initialize() returns at its first check without rebuilding the resolver, the gRPC channel it shutdownNow()'d, the retry scheduler it terminated or the final errorExecutor it tore down (FlagdProvider.java:121-125). A shut-down flagd provider is terminally shut down. That is permitted. 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 an option, not an obligation, and declining it is one of the choices the requirement offers. An earlier version of this file recorded it as an untracked KnownDeviation against @lifecycle, which was wrong twice over: the scenario was mandatory only because the spec's assets had not yet gated it, and the entry asserted a defect against a provider behaving inside the requirement. Withholding the tag is the whole of what is owed; the one scenario it gates is now reported as skipped with that reason instead of failing in RPC mode. The lesson is more useful than the correction. Nothing had checked whether 2.5.2 requires reuse before the failure was written up as a defect -- the scenario failed, so a deviation was recorded. Find the numbered requirement first. This is the third rule in the suite found asserted more strongly than the spec states it. @lifecycle stays declared. flagd reaches its backend during initialisation in both modes, so the remaining lifecycle scenarios assert something real, and Java declaring it is what made the cross-language divergence visible in the first place -- Go and JavaScript withhold it and are being changed to match. @Stale is declared for both resolvers and that is correct. PROVIDER_STALE is emitted from FlagdProvider.onError (FlagdProvider.java:258-264), which the shared onProviderEvent switch reaches on PROVIDER_ERROR from either resolver (FlagdProvider.java:197, 236), so the emit is in the provider layer rather than a transport -- and the scenario passes in RPC mode as well as in-process. Go's flagd provider withholds the tag for RPC; on this evidence that is a difference between the implementations, not a property of the transport. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
The TCK no longer sets @large-integers apart as "not applicable in Java". It is an ordinary declarable capability, so declarableExcept(...) no longer leaves it out on its own and this suite has to say so. Nothing about the run changes. The scenario was skipped before and is skipped now, for the same reason in substance: the SDK's integer accessor is a 32-bit Integer and 2^53 - 1 has no room in it, so no Java provider can hold the tag. What changed is where that is written down -- Appendix F, once, rather than a field in every report -- and that the suite now expects the harness to withhold it rather than refusing to let it be declared. It stays out of knownDeviations, for the same reason the missing testbed flags do: flagd is not at fault for a value the accessor cannot carry. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
…ill owes @Variants and @targeting arrived with the base's submodule bump, and declarableExcept picks both up without a line of this file changing. That is the right outcome and the reason nothing here said so, which is the problem: the declaration grew by two capabilities and the file that argues every other one either way was silent about them. Two runs against flagd-testbed v3.8.0, one per resolver. RPC: 45 pass, 5 skipped, 2 failed. In-process: the same, after a first attempt whose opening scenario timed out on a cold sync stream and passed on rerun -- a warm-up flake, not a result. @targeting's three scenarios resolve targeting-key-flag through flagd's own rule evaluation and pass in both modes on the image already pinned, so the tag cost no bump; the Compose header now says so, because the note beside that tag is where someone would otherwise go looking for a reason to bump it. Both failures are the one testbed gap, and the @Variants outline reaches it a second time: once for the untagged precision scenario, once for the row asking for large-integer-flag's max-int32 variant. So the header's "one untagged scenario" is a scenario short. Withholding @Variants would hide both -- and would be a claim about flagd made to accommodate a missing flag, which is the one thing a declaration must not be. The capabilities() javadoc also still called @targeting reserved, in the paragraph explaining why complementOf is the wrong call. It is declarable now, and the paragraph is stronger for losing it: the reserved set shrinks as the vocabulary fills up, so what protects the declaration is the form of the call rather than the size of the set. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
The tag arrives declared, because capabilities() is spelt "everything declarable except", and it is right to arrive that way here — but that is not the same as having been checked, so it was run. Both modes now execute 56 scenarios and pass 49 of them, with the same five skips and the same two testbed failures as before: all four rows of the new outline pass in RPC and in-process alike. The provider substitutes the caller's default for a flag whose state is DISABLED and reports no error code. Nothing was needed upstream. The four disabled-* flags are flagd-testbed's own, from flags/disabled-flags.json, present in the v3.8.0 image already pinned and combined into the served set by the launchpad; the canonical definition took the testbed's names and values rather than inventing its own, as it did for the falsy flags. Recorded next to the image tag, where the flags the testbed does *not* serve are recorded. Worth stating rather than assuming, because @disabled-flags is gated on architecture rather than on quality, and flagd is on the side of that line that can hold it: the in-process resolver evaluates locally, and the RPC resolver still decides locally what to do with a response carrying no value. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
Import-path and coordinate churn only: the artifact is dev.openfeature.contrib.tools:tck, the version range starts at 0.1.0, and the four imports come from dev.openfeature.contrib.tools.tck. No behavioural change, and no change to what is declared or withheld. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
The first two in-process scenarios failed reproducibly on a slower host with `Initialization timeout exceeded; did not complete within the 10000 ms deadline` out of FlagdProviderSyncResources.waitForInitialization. The in-process resolver syncs the whole ruleset before it reports ready, and the first scenario pays for a cold container on top of that; flagd doubles the configured deadline, so 5000 gave it 10s and that was not enough. 15000 gives it 30s and both modes are clean apart from the two flagd-testbed gaps already documented here -- 56 scenarios, 49 passing, 5 skipped, 2 failing, in RPC and in-process alike. Diagnosed rather than guessed, because this looked at first like fallout from the base dropping its 50ms post-command settle. It is not: the settle was restored locally at 50ms and at 3000ms and fixed nothing, and the failure is present on flagd-testbed v3.10.1 as well as on the pinned v3.8.0. What it covers is the provider's own initialisation, which belongs in a bound the scenario can see rather than in a sleep after an unrelated control call. UNAVAILABLE_DEADLINE_MS is untouched, so the initialisation-failure scenarios still assert promptness. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
Switches the @numeric-coercion deviation from withheld-and-skipped to declared-and-failing, which is the shape the TCK's settled guidance prefers, and records why paying its cost is the honest report. The guidance says withdrawing a capability in order to turn a failing scenario into a skip is the failure mode the field exists to prevent, and that is exactly what the old shape did here. Measured on the pinned testbed, in both modes: of the tag's three scenarios, "An integer requested as a float is widened without loss" passes. flagd therefore does coerce, and gets the narrowing direction wrong - a skip cannot distinguish that from "flagd declines to coerce", and only the second reading was available before. The argument for the old shape was real and is recorded rather than dropped: declaring the tag also fails "An integral float requested as an integer is coerced without loss", because integral-float-flag is absent from flagd-testbed v3.8.0. That cost is accepted because it is not a new kind of cost - this adoption already carries two failures from the same missing flags and records them plainly - and because the alternative hides a real defect behind a stack gap. Measured result, both modes: 56 scenarios, 2 skipped (@reinitialization, @large-integers), 4 failing - one provider defect and three testbed gaps. Also records something the previous pass reported as fixed and which does not hold on a loaded host: the first scenario of errors.feature still errors in in-process mode with an initialisation timeout against the doubled 30000 ms deadline. Reproduced three times, and reproduced identically with @numeric-coercion withheld, so it is not a consequence of this change. Thirty seconds is not a plausible sync time for this ruleset and only the mode that must establish a sync stream after the first POST /start is affected, so it reads as stack-side readiness - the class of defect open-feature/flagd-testbed#394 closes. The deadline stays at 15000 rather than being raised again: a suite that sleeps instead of holding the control API to its promise stops being able to detect when the promise breaks. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
The two TCK suites have been excluded from the default build since they were added, via <testExclusions>**/e2e/*.java</testExclusions>, and nothing said so. An exclusion nobody writes down is indistinguishable from an oversight - which is how providers/ofrep came to run a Docker-dependent suite in its default build unnoticed, the same mistake in the other direction. So the README now states the policy and its reasoning: the suites are Docker-gated, excluded from every job that exists, and run by hand by a maintainer before merging a change to resolution, event or lifecycle behaviour, with the result quoted in the pull request. A scheduled or path-filtered workflow was considered and declined. It also gives the two commands and says which failures are expected, so that a reader can tell a regression from the recorded testbed gaps and the recorded coercion defect. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
… not doing
Narrows the e2e profile's testExclusions from empty to **/e2e/*TckTest.java, so
the exclusion the default build applies is no longer undone in the one job that
matters.
A previous pass reported that the TCK suites "are excluded from every job that
exists". That was wrong, in two steps that have to be read together:
providers/flagd's e2e profile sets <testExclusions/>, clearing the exclusion, and
ci.yml's `main` job activates that profile on every push. Verified rather than
re-read:
$ mvn -Pe2e -pl providers/flagd help:evaluate -Dexpression=testExclusions
(empty)
$ mvn -pl providers/flagd help:evaluate -Dexpression=testExclusions
**/e2e/*.java
So FlagdRpcTckTest and FlagdInProcessTckTest were running in CI, on a runner
that does have a Docker daemon, and they are expected to fail - three testbed
gaps and one recorded coercion defect. Every unrelated pull request touching this
module would have gone red for a reason that has nothing to do with it.
Narrowing rather than clearing is what keeps both halves true: the legacy
Run*Test suites over the test-harness submodule still run under -Pe2e exactly as
they do on main, and only the two TCK suites stay out. The pattern matches
AbstractFlagdTckTest.java as well, which is harmless - it is abstract and
surefire would not select it - and testExclusions only filters what runs, never
what compiles.
Not runnable locally as a cross-check: providers/flagd/test-harness is an
uninitialised submodule on this machine, so the legacy suites cannot be executed
here to prove they still get selected. The pattern is checked against the
directory's file names instead, which is deterministic:
Run{File,InProcess,Rpc}Test do not end in TckTest.
The README says all of this too, in the section added with the exclusion policy,
because the narrowing is the half a reader would not guess from the POM alone.
Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
Appendix F now carries "Running the suite in CI", promoted there because the
same reasoning restated in four adoption READMEs is where it drifted. So this
section keeps the mechanism -- the testExclusions property, what the e2e profile
narrows it to and why the narrowing rather than the clearing, that the exclusion
is Surefire's and not the compiler's, and the help:evaluate command that resolves
it -- and links to the appendix instead of paraphrasing the argument.
The resolved values are checked, not read:
mvn -Pe2e -pl providers/flagd help:evaluate -Dexpression=testExclusions
-> **/e2e/*TckTest.java
mvn -pl providers/flagd help:evaluate -Dexpression=testExclusions
-> **/e2e/*.java
The locally decided part stays stated here rather than deferred: a scheduled or
path-filtered workflow was considered and declined, and a maintainer quotes a
hand-run result in the pull request.
Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
The TCK gained @standard-reasons, and this suite declares every declarable capability except @reinitialization and @large-integers -- so it picked the new tag up by default and started running reason.feature without anyone deciding that it should. That is the right answer here, but it was measured before it was written down rather than after. All nine scenarios pass in both modes, including the two that compose with @targeting and @disabled-flags: STATIC for the rule-less flags, TARGETING_MATCH and DEFAULT either side of targeting-key-flag's rule, DISABLED for a disabled flag, and ERROR beside FLAG_NOT_FOUND and TYPE_MISMATCH. So the claim the tag makes -- the standard vocabulary with the standard meanings -- holds for the RPC resolver and the in-process one alike. Each mode is now 65 scenarios, 59 passing, 2 skipped and 4 failing, up from 56 and 50. The four failures are the same four as before and none of them is new: the lossy numeric coercion that flagd#1996 tracks, and the three flags flagd-testbed v3.8.0 does not serve. The cold-start initialisation error recorded on CONNECTED_DEADLINE_MS did not reproduce in this run; it is intermittent and host-dependent, so the note stays and the count says which run it comes from. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
capabilities() is now declarableExcept(REINITIALIZATION). @large-integers goes, and its going is the whole point: it was never a decision this suite took. flagd does not decline to resolve 2^53 - 1 -- Client.getIntegerDetails is a 32-bit Integer, so no Java provider can be asked, and the TCK refuses the capability centrally rather than having each adoption remember. The paragraph that used to explain it here was one of four saying the same thing about the same language. @reinitialization stays withheld, and stays this suite's call. That one is a fact about flagd: FlagdProviderSyncResources keeps isInitialized and isShutDown as separate flags and refuses initialize() when either is set, which Requirement 2.5.2 permits, so withholding is the honest report and no KnownDeviation is owed. Nothing about the SDK's accessor has any bearing on it, which is why one moved and the other did not. Measured, both resolvers, on the pinned testbed image: 65 scenarios, 59 passed, 2 skipped, 4 failed -- identical to the previous pass, because this changes why a scenario is skipped rather than whether it is. The two skips now read differently in the results, which is the observable part: Skipped: provider does not declare capability REINITIALIZATION (tag @reinitialization). Declared capabilities: [...] Skipped: the Java SDK cannot express capability LARGE_INTEGERS (tag @large-integers) - Client.getIntegerDetails takes and returns a 32-bit Integer ... not the provider under test declining A reader of the report can now tell which of the two absences describes flagd. The four failures are unchanged: flagd#1996's lossy coercion plus the three flags testbed v3.8.0 does not serve. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
aepfli
force-pushed
the
feat/provider-tck-flagd
branch
from
September 13, 2026 14:48
a0bd1eb to
c7c4569
Compare
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
5 participants
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.
Adopts the conformance suite from #1830 in the flagd provider, in both resolver modes.
Split out of #1830 so the suite and its first adopter are reviewed as separate questions: whether the TCK is the right contract, and whether flagd satisfies it. Depends on #1830; review that first.
What this is
Five files and two POM additions. The whole adoption is a shared abstract base and two subclasses that differ only in resolver and port — the TCK brings its own Gherkin, step definitions and Compose lifecycle, and works out which suite is running from the JUnit test plan, so a mode needs no registration and no build configuration.
AbstractFlagdTckTest— shared configuration, capability declarationFlagdRpcTckTest,FlagdInProcessTckTest— one per resolversrc/test/resources/tck/docker-compose.yaml— wraps the unmodifiedflagd-testbedimage, which already serves both flagd and the launchpad control API this TCK's contract was derived from. No host port bindings: the TCK discovers dynamically mapped ports after startup.pom.xml— the TCK test dependency and a version range so a local reactor build matches whatever is checked outThe ~18 other files in
providers/flagd/.../e2e/are untouched; they predate this work.The one declared gap
capabilities()isdeclarableExcept(NUMERIC_COERCION), andknownDeviations()says why that is a defect rather than a choice: evaluatingfloat-flag(0.5) through the integer API returns0with no error code instead ofTYPE_MISMATCHwith the code default, so the fractional part is discarded silently. Lossless coercion is permitted and is not the defect. Tracked as open-feature/flagd#1996.Both resolvers behave identically, which places it in the shared provider layer rather than in either transport, so it is declared once. Delete the override and the deviation together once it is fixed.
Independence
This branch imports only base types —
BackendEndpoint,Capability,ContainerizedProviderTckTest,KnownDeviation. It needs nothing from the reporting branch (#1841), which is its sibling rather than its parent, and the two do not collide.