test(ofrep): run the provider conformance suite against OFREP - #1840
Draft
aepfli wants to merge 1 commit into
Draft
test(ofrep): run the provider conformance suite against OFREP#1840aepfli wants to merge 1 commit into
aepfli wants to merge 1 commit into
Conversation
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>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.