test(ofrep): adopt the OpenFeature Provider TCK - #1840
Draft
aepfli wants to merge 13 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 |
aepfli
force-pushed
the
feat/provider-tck-ofrep
branch
from
September 11, 2026 10:36
f51b3f1 to
a53854b
Compare
aepfli
changed the base branch from
feat/provider-tck
to
feat/provider-tck-flagd
September 11, 2026 10:36
github-actions
Bot
requested review from
Kavindu-Dodan,
beeme1mr,
thisthat and
toddbaert
September 11, 2026 10:36
aepfli
force-pushed
the
feat/provider-tck-flagd
branch
from
September 11, 2026 10:49
3180328 to
20cf419
Compare
aepfli
force-pushed
the
feat/provider-tck-ofrep
branch
from
September 11, 2026 10:50
a53854b to
36be24c
Compare
aepfli
force-pushed
the
feat/provider-tck-flagd
branch
from
September 11, 2026 13:44
20cf419 to
177ebe0
Compare
aepfli
force-pushed
the
feat/provider-tck-ofrep
branch
from
September 11, 2026 13:44
36be24c to
5be8740
Compare
aepfli
force-pushed
the
feat/provider-tck-flagd
branch
from
September 11, 2026 16:18
177ebe0 to
5849212
Compare
aepfli
force-pushed
the
feat/provider-tck-ofrep
branch
from
September 11, 2026 16:18
5be8740 to
83d0cc4
Compare
aepfli
force-pushed
the
feat/provider-tck-flagd
branch
from
September 11, 2026 17:13
5849212 to
76f441a
Compare
aepfli
force-pushed
the
feat/provider-tck-ofrep
branch
from
September 11, 2026 17:13
83d0cc4 to
acadba1
Compare
aepfli
force-pushed
the
feat/provider-tck-flagd
branch
from
September 11, 2026 17:17
76f441a to
e120ae2
Compare
aepfli
force-pushed
the
feat/provider-tck-ofrep
branch
2 times, most recently
from
September 12, 2026 06:42
660cc32 to
1036a65
Compare
aepfli
force-pushed
the
feat/provider-tck-flagd
branch
2 times, most recently
from
September 12, 2026 10:47
5325453 to
74f2c67
Compare
aepfli
force-pushed
the
feat/provider-tck-ofrep
branch
from
September 12, 2026 10:47
1036a65 to
d7a8c3a
Compare
aepfli
force-pushed
the
feat/provider-tck-flagd
branch
from
September 12, 2026 13:47
74f2c67 to
4ba9735
Compare
aepfli
force-pushed
the
feat/provider-tck-ofrep
branch
from
September 12, 2026 13:47
d7a8c3a to
27ff382
Compare
aepfli
force-pushed
the
feat/provider-tck-flagd
branch
from
September 12, 2026 15:56
4ba9735 to
45cf4f6
Compare
aepfli
force-pushed
the
feat/provider-tck-ofrep
branch
from
September 12, 2026 15:56
27ff382 to
cd633bb
Compare
aepfli
force-pushed
the
feat/provider-tck-flagd
branch
from
September 12, 2026 21:58
45cf4f6 to
df06bac
Compare
aepfli
force-pushed
the
feat/provider-tck-ofrep
branch
from
September 12, 2026 21:58
cd633bb to
64d5aa6
Compare
aepfli
force-pushed
the
feat/provider-tck-flagd
branch
from
September 12, 2026 23:58
df06bac to
fdbe6a8
Compare
aepfli
force-pushed
the
feat/provider-tck-ofrep
branch
from
September 12, 2026 23:58
64d5aa6 to
c826be5
Compare
aepfli
force-pushed
the
feat/provider-tck-flagd
branch
from
September 13, 2026 08:48
fdbe6a8 to
8ac8f62
Compare
aepfli
force-pushed
the
feat/provider-tck-ofrep
branch
from
September 13, 2026 08:48
c826be5 to
08fa51a
Compare
OFREP is a protocol, not a vendor, so the suite needs no new infrastructure: flagd already serves the OFREP HTTP API on 8016 inside the flagd-testbed image that the flagd TCK suites use, alongside the launchpad control API on 8080. The Compose stack is therefore the same image with a different port exposed, and the whole adoption is one test class plus one dependency. Four capabilities are withheld, all traceable to the same fact: OfrepProvider implements FeatureProvider rather than extending EventProvider and overrides no lifecycle method, so it has no state, no stream, no poll loop and no initialize(). It cannot emit events (EVENTS), cannot observe the backend going away (STALE) or changing (CONFIGURATION_CHANGE), and cannot fail initialisation against a dead port (UNAVAILABLE_INIT). Each omission is justified against specific lines of the provider in the capabilities() javadoc. events.feature and lifecycle.feature are both tagged @events at feature level, so 5 scenarios are reported as skipped and 24 run. OBJECT and STRICT_NUMERIC_TYPING are both declared. Unlike the flagd provider, OFREP does not silently narrow a float to an integer: values are deserialised by a plain Jackson ObjectMapper into an untyped Object, so a JSON fraction arrives as Double and a JSON integer as Integer, and handleResolved admits a value only on an exact type.isInstance check. float-flag requested as an integer is reported as TYPE_MISMATCH with the code default rather than truncated to 0. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
The base renamed the containerised base class, retired @strict-numeric-typing in favour of @numeric-coercion, and now refuses a declaration that names a reserved or not-applicable capability. EnumSet.complementOf swept up @large-integers, @targeting and @caching, so the suite would have stopped at startup; Capability.declarableExcept leaves those out on its own. LIFECYCLE is withheld as well, which the complement had quietly claimed since the capability appeared: OfrepProvider has no initialize(), so the readiness scenario passed exactly as it does for NoOpProvider, and the new shutdown scenarios gated by the same tag are skipped rather than passed vacuously. NUMERIC_COERCION is withheld because the tag now requires the lossless direction too, and Resolver.handleResolved admits a value only on an exact type.isInstance check: integer-flag requested as a float arrives from Jackson as an Integer and is refused. Strict typing in both directions is a choice under the suite's model, not a defect, so no KnownDeviation goes with it. Read from the source, not from a run. The class Javadoc records that flagd-testbed v3.8.0 serves none of the six new canonical flags, so the four untagged scenarios that read them fail FLAG_NOT_FOUND until the testbed is updated; that is the stack's gap, not the provider's, so it is documented rather than declared. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
…tbed lacks The same note the flagd adoption carries, next to the image tag it is about: flagd-testbed v3.8.0 serves none of the six flags the bumped assets added, so the untagged scenarios that read them fail FLAG_NOT_FOUND until the testbed is updated, and the tag here is what to bump when it is. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
…vidence The falsy-flag rename in the base removes three failures here without touching the provider: flagd-testbed already served boolean-zero-flag, integer-zero-flag and string-zero-flag with zero/non-zero variants, and spec ba002ce8 moved the canonical names onto the testbed's rather than the other way round. The suite goes from four untagged failures to one. The notes now say what is actually missing -- large-integer-flag, and only that, because huge-integer-flag sits behind @large-integers and integral-float-flag behind @numeric-coercion, neither of which this provider declares. @numeric-coercion stays withheld, now on stated evidence rather than a shorter argument. Go's OFREP provider declares it and this one does not, which looked like an unexamined declaration on one side; it is not. handleResolved admits a value only on an exact type.isInstance check (Resolver.java:183-191) with no integral check and no round trip anywhere in the path, so of the tag's three scenarios this provider passes one: the lossy case is right for the wrong reason, and both lossless cases fail, integer-flag requested as a float and integral-float-flag requested as an integer alike. Declaring it would turn two scenarios red -- three, counting that the testbed cannot serve integral-float-flag at all. Go coerces and this does not; the two declarations describe two implementations, not one protocol, which is possible precisely because OFREP is JSON and integer-ness is the provider's decision. No KnownDeviation accompanies it, and that is a decision rather than silence. Appendix F is explicit that this is the one capability the specification does not define, that its rule is borrowed from flagd's ADR, and that "a provider that behaves differently is not violating the specification" -- having retracted an earlier draft that called non-declaration an admission of a known bug. A deviation entry would assert a defect the spec says is not one: the opposite mistake from a vacuous declaration, in the same currency. What the entry does record is that the reasoning is source-derived, that no unit test pins the numeric pair, and what a run would have to show for the declaration to change. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
The base gates "A provider that was shut down can be initialized again" on @reinitialization, because Requirement 2.5.2 permits reuse after shutdown rather than requiring it. declarableExcept(...) hands the new tag out by default, so a provider that cannot be restarted has to say so or it publishes a claim nothing examined -- the one failure mode this declaration exists to prevent. OfrepProvider cannot be restarted. shutdown() terminates the executor the HTTP client runs on (OfrepProvider.java:90-108) and the class overrides no initialize(), so nothing recreates it. That is the choice 2.5.2 offers rather than a defect, and no KnownDeviation accompanies the omission. It changes no result on its own: the scenario carries @lifecycle too, which this provider already withholds because it has no initialisation to observe, so the skip was happening either way. The declaration is what stops being a half-truth. The @lifecycle bullet no longer counts re-initialisation among the scenarios that tag gates, since it does not any more. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
The TCK no longer sets @large-integers apart as "not applicable in Java", so declarableExcept(...) no longer leaves it out on its own and this suite names it alongside the six capabilities it already withholds. The run is unchanged: the scenario was skipped before and is skipped now, because the SDK's integer accessor is a 32-bit Integer and 2^53 - 1 has no room in it. That is a fact about the SDK rather than about OFREP, which is why it is recorded once in Appendix F and needs no knownDeviations entry here -- unlike the numeric-coercion gap, which is the provider's own. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
@Variants and @targeting arrived with the base's submodule bump and declarableExcept picks both up, so the declaration grew by two capabilities while the javadoc that argues every withheld tag at length said nothing about either. One run against flagd-testbed v3.8.0 through the OFREP HTTP API: 38 pass, 12 skipped, 2 failed. @targeting is worth more here than its name suggests. The provider sends the evaluation context in the request body and the backend evaluates the rule, so targeting-key-flag's three scenarios are the only ones in the canonical set that would notice a context dropped on the way out -- every other flag resolves the same way with or without one. All three pass. @Variants passes seven of its eight rows. The eighth asks for large-integer-flag's max-int32 and is answered with no variant, because v3.8.0 does not serve that flag at all -- the gap the Compose header already records for the untagged precision scenario, now reached twice rather than once, so "one untagged scenario" is a scenario short. Withholding the tag would hide both failures behind a claim about the provider that the run does not support. The complementOf paragraph called @targeting reserved alongside @caching. It is declarable now; the argument survives with one tag, because what makes the form of the call right is not how large the reserved set happens to be. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
Measured, not assumed, and the measurement overturned the expectation it was made to confirm. Declared, all four rows of the new outline fail: 56 scenarios, 38 passing, 12 skipped, 6 failed, the four extra failures beyond the two testbed ones all reading "expected: null but was: FLAG_NOT_FOUND". The value assertion passes, because the provider returns the code default on an error -- right answer, wrong reason. The expectation was that this is architectural: the caller's default never leaves the process, so a backend cannot return it. That is not what the backend does. flagd's OFREP endpoint answers a disabled flag with 200, reason DISABLED and no value member, probed directly against the pinned v3.8.0 image. OFREP's evaluationSuccess requires only key and reason; value is not required, because one shape a success may take is codeDefaultFlag -- "This schema has no value property. The provider must use the code default value when processing this response." DISABLED is in the reason enum. The response is well-formed and says exactly what the scenario asserts. So this is a provider gap against a MUST in the protocol the provider implements, and it is wider than the rows that found it: every codeDefaultFlag response reaches the application as FLAG_NOT_FOUND, so an application checking the error code sees a failure on an evaluation that succeeded. handleResolved treats a null value as an absent flag and discards the reason it parsed a field earlier. The tag stays withheld, because it fails and a conformance run must not pass it. But it carries a KnownDeviation, which is the opposite call from @numeric-coercion next to it, and the difference is where the rule lives: numeric coercion is Appendix F borrowing flagd's ADR with no specification behind it, whereas codeDefaultFlag is normative OFREP. A bare omission would read as the same kind of choice, and this is not a choice. Untracked for now; delete both once handleResolved honours a value-less success. Nothing is owed upstream. The four disabled-* flags are served by the image already pinned, which the Compose header now records so the failures are not mistaken for another testbed gap. 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>
…tainers
Two fixes to this module's POM, both of which were latent.
testExclusions was missing. The parent POM feeds <exclude>${testExclusions}</exclude>
to Surefire but defines no default, so a module that wants the Docker gate has
to declare the property and a module that forgets one runs a Docker-dependent
suite in every job. providers/flagd declares it; this module did not, so
OfrepTckTest started a Compose stack during plain `mvn verify` and ended
"Tests run: 56, Failures: 2" on the recorded testbed gaps. So the ofrep PR was
red in CI while the flagd PR was green for the opposite reason - one ran a suite
it should gate, the other gated a suite nobody ran. Adding the exclusion makes
the policy uniform: `mvn -Pcodequality -pl providers/ofrep -am verify` is now
BUILD SUCCESS with 18 tests and no Compose stack.
Testcontainers is now declared here. The TCK moved it to provided/optional so
that the majority of adopters, which have no backend and never load
ContainerizedProviderTckTest, stop resolving it; a containerised adopter brings
its own instead. A previous pass asserted that both adoptions in this repository
already did - that was true of providers/flagd and false of this module, which
was relying on the transitive edge. Stated plainly because the assertion was
wrong, not because the fix is interesting.
Verified with the suite actually run: 56 scenarios, 38 passed, 16 skipped for
withheld capabilities, 2 failed on the flags flagd-testbed v3.8.0 does not
serve - unchanged, so the dependency now resolves from here rather than through
the TCK.
Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
The other half of adding testExclusions. An exclusion nobody writes down is indistinguishable from an oversight, and that is not a hypothetical here: this module's exclusion was missing precisely because nothing said the convention existed. So the README states the policy and its reasoning - Docker-gated, excluded from every job that exists, run by hand by a maintainer before merging a change to resolution or error behaviour, with the result quoted in the pull request. A scheduled or path-filtered workflow was considered and declined. It also gives the command and says which failures are expected, so a reader can tell a regression from the recorded testbed gaps, and notes that several capabilities are withheld because OFREP puts the decision on the server rather than in the provider - a fact about the protocol, not a defect. 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 and the local record -- that this module had no
testExclusions at all until it was added, which is the appendix's
"exclusion nobody wrote down" -- and links to the appendix for the argument
rather than paraphrasing it.
Also states that no profile in this module touches the property, resolved rather
than read:
mvn -Pe2e -pl providers/ofrep help:evaluate -Dexpression=testExclusions
-> **/e2e/*.java
mvn -pl providers/ofrep help:evaluate -Dexpression=testExclusions
-> **/e2e/*.java
Unlike providers/flagd, which needs an e2e profile for its legacy suites and
therefore narrows the pattern instead of clearing it.
Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
This suite declares everything declarable except nine capabilities, so it picked @standard-reasons up by default the moment the TCK gained it. Measured before it was written down. Eight of reason.feature's nine scenarios run and pass: STATIC for the four rule-less flags, ERROR beside FLAG_NOT_FOUND and TYPE_MISMATCH, and -- because @targeting is declared here -- TARGETING_MATCH and DEFAULT either side of targeting-key-flag's rule. The ninth carries @disabled-flags as well and is skipped for that omission, which is the right outcome rather than a second report of the same gap: what this provider does wrong with a value-less success is already stated once, in the withheld capability and its KnownDeviation, and a reason it never reaches is not more evidence of it. So the tag means "the standard vocabulary, over the responses this provider actually completes", and a reader sees the withheld @disabled-flags beside it and can tell which scenario went unasked. A clean run is 65 scenarios, 46 passing, 17 skipped and 2 failing, up from 56, 38 and 16. The two failures are the same testbed gaps as before. Also records something this pass measured rather than introduced: the suite is intermittently flaky. About half of the runs carry one or two extra failures where an evaluation comes back as the code default, or as FLAG_NOT_FOUND where TYPE_MISMATCH was expected, or with reason ERROR where a resolution was expected. The victim moves between errors.feature, evaluation.feature and reason.feature, so it is not a property of any assertion. Eight runs were measured, five at this revision and three at ccdb8879, and the old pin produced a seven-failure run and a two-failure run from the same tree -- so this predates the reason scenarios and is not caused by them. That is the flagd-testbed readiness window of open-feature/flagd-testbed#394 reaching a provider that holds nothing between calls, so every evaluation races the stack afresh. Recorded in the class javadoc and the README with an explicit instruction not to cover it with a settle after control calls, because 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>
aepfli
force-pushed
the
feat/provider-tck-flagd
branch
from
September 13, 2026 09:09
8ac8f62 to
a0bd1eb
Compare
aepfli
force-pushed
the
feat/provider-tck-ofrep
branch
from
September 13, 2026 09:09
08fa51a to
94d2c35
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
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.
What
The OFREP provider under the conformance suite. Three files, 159 insertions:
providers/ofrep/src/test/java/.../e2e/OfrepTckTest.java— the whole adoption, one class, no test infrastructureproviders/ofrep/src/test/resources/tck/docker-compose.yaml— the sameflagd-testbedimage the flagd suites use, exposing 8016 (OFREP) and 8080 (launchpad), no pinned host portsproviders/ofrep/pom.xml— theprovider-tcktest dependency, copied verbatim from the flagd moduleNo new infrastructure, and no surefire executions were needed:
*TckTestmatches surefire's default includes, andprovider-tckbrings Testcontainers, Cucumber and the JUnit platform launcher transitively.Capabilities:
OBJECTandSTRICT_NUMERIC_TYPING— 24 of 29 scenarios runEvery omission traces to one fact:
OfrepProviderhas no lifecycle of its own.EVENTS—OfrepProvider.java:19isimplements FeatureProvider, notextends EventProvider. NoProviderEventreference, noemit*call anywhere in the file.STALE— nothing survives an evaluation.Resolver.resolvebuilds its result purely from the current response and records nothing on failure (Resolver.java:93-96,OfrepApi.java:114-115).CONFIGURATION_CHANGE— the only outbound call is the per-evaluationPOST /ofrep/v1/evaluate/flags/{key}(OfrepApi.java:27,93-109). No bulk endpoint, no ETag, no watch.UNAVAILABLE_INIT—initializeis not overridden, so the interface default runs and cannot fail.constructProvidervalidates arguments and never touches the network (OfrepProvider.java:38-68). A provider aimed at a dead port reachesREADY.One judgement call worth flagging. I verified against
sdk-1.22.0.jarthatFeatureProviderStateManageremitsPROVIDER_READY/PROVIDER_ERRORaroundinitializefor any provider, independent ofEventProvider. Solifecycle.feature's readiness scenario would pass ifEVENTSwere declared, and the other@eventsscenarios carry finer tags that would still gate them — declaring it is mechanically possible and would give 25 running instead of 24. I withheld it anyway: that READY is synthesised by the SDK and would appear identically forNoOpProvider, so declaringEVENTSwould assert aPROVIDER_ERRORcapability the provider can never demonstrate. This is a decision, not a forced outcome, and I would like it challenged.STRICT_NUMERIC_TYPINGis declared — the OFREP provider does not share flagd's defect. Different code path, and it holds up:OfrepResponse.java:16is an untypedObject valuefilled by a plain JacksonObjectMapper, so a JSON fraction arrives asDoubleand a JSON integer asInteger.handleResolvedadmits the value only on an exacttype.isInstance(...)check and otherwise returnsTYPE_MISMATCHwith the code default (Resolver.java:183-190). Nothing widens or narrows, sofloat-flag(0.5) requested as an integer is rejected rather than truncated to0.@eventsis a feature-level tag on bothevents.featureandlifecycle.feature, so all 5 skip. What runs is the full evaluation and error-code matrix: 7 fromevaluation.feature, 17 fromerrors.feature.Verification — this one was actually built
Neither
mvnnorjavawas on PATH, but IntelliJ's bundled JBR 21 and the repo's cachedmvnwwere, so:mvnw --projects tools/provider-tck,providers/ofrep --also-make test-compilemvnw --projects tools/provider-tck,providers/ofrep testOfrepTckTestis discovered by surefire, the Cucumber engine loads it as a@Suite, harness discovery resolves, and it fails at exactly one point —IllegalStateException: Could not find a valid Docker environmentspotless:checkspotless:apply, re-verified clean-PcodequalityThe wiring is therefore proven end to end up to the container boundary. The 24/5 split is arithmetic from the feature files and the capability gate, not an observed run.
No CI will run on this PR while it is stacked:
.github/workflows/ci.ymltriggers onpull_request: branches: [main]and this targetsfeat/provider-tck.Worth filing against the Java OFREP provider
ParseErrorescapes the provider's own error mapping.Resolver.resolve:93catches onlyGeneralError, butOfrepApithrowsParseErroronJsonProcessingException(OfrepApi.java:111-112), and the two are siblings underOpenFeatureError. A malformed or empty response body propagates out unmapped. The SDK's blanket catch means an application still gets details rather than a throw, so the TCK will not catch this — but the provider reportsPARSE_ERRORwhere its own design intendsGENERAL.initialize()means a misconfigured provider reportsREADY. Point it at a bogusbaseUrland every evaluation silently returns the code default withGENERAL, while the client status says everything is fine. This is the single biggest conformance gap and the reasonUNAVAILABLE_INITis withheld; one round trip ininitialize()would fix it and unlock both@unavailablescenarios.EventProvider. OFREP's bulk-evaluation endpoint with ETag support is designed for exactly this; polling it would unlockEVENTS,STALEandCONFIGURATION_CHANGEand take the suite from 24 to 29.nextAllowedRequestTime(OfrepApi.java:127-131), after which every flag's evaluation throwsGeneralErroruntil the deadline — including flags that were never rate-limited.OfrepResponse.getMetadata()callsImmutableMap.copyOf(metadata)on a field that staysnullwhen the JSON omitsmetadata. Every live path launders the object throughResolution, whose constructor reads the field directly, so it is safe today — but a direct use of a deserialisedOfrepResponsewould NPE.