diff --git a/providers/openfeature-provider-ofrep/README.md b/providers/openfeature-provider-ofrep/README.md index cad6a584c..2a6ff703f 100644 --- a/providers/openfeature-provider-ofrep/README.md +++ b/providers/openfeature-provider-ofrep/README.md @@ -23,6 +23,57 @@ api.set_provider(OFREPProvider()) +## Provider conformance suite + +This provider runs the [OpenFeature Provider Conformance Suite][tck] against a flagd-testbed stack +serving OFREP, in `tests/tck`. The suite owns the container stack: `tests/tck/conftest.py` declares +a Compose file and the port the provider connects to, and nothing else. + +`tests/tck/docker-compose.yaml` is one definition of the backend for the whole repository, and the +flagd adoption carries a byte-identical copy — each provider package publishes its own distribution +and must not read the other's files, so the two are kept in step by `diff` rather than by sharing a +path. Change one, copy it to the other. + +**It is excluded from the default build, and a maintainer runs it by hand before merging a change to +it.** + +``` +poe test-tck # needs Docker +poe test # everything else, which is what CI runs +``` + +The exclusion lives in `pyproject.toml`: `--ignore=tests/tck` on the two tasks `build.yml` reaches, +with the reason in a comment above them. Why a conformance suite is not a required gate is +[Appendix F, "Running the suite in CI"][appendix-f], and is not restated here. + +Two things that are this provider's rather than the policy's: + +- **Docker is not what decides it.** The flagd package's `tests/e2e` needs Docker too and does run in + the default build. What decides it is the run: **2 failed, 45 passed, 17 skipped, 1 xfailed** — + both failures are canonical flags that no released flagd-testbed seeds yet, and the `xfail` is + the one genuine provider gap, recorded as a `KnownDeviation` rather than hidden. + `tests/tck/conftest.py` and `tests/tck/test_ofrep.py` account for each one, so a + reviewer running the suite can tell a new failure from a known one. +- **The default build still collects the suite** — `poe test` and `poe test-cov` end in + `pytest tests/tck --collect-only`, which imports every module and starts no container. An excluded + suite that has quietly stopped importing against the harness is worse than one that runs and + fails, and `mypy` here is configured over `src` alone, so nothing else would notice. + +**This suite races a known backend defect, and deliberately does not compensate for it.** +flagd-testbed's `POST /start` returns before it serves the reseeded flag set, which the control API +forbids, and a stateless provider has no initialisation to hide that window behind — so a run can +report `FLAG_NOT_FOUND` for flags the configuration plainly defines. The defect is +[flagd-testbed#394](https://github.com/open-feature/flagd-testbed/pull/394). + +This suite used to wrap the control in a `SettledControl` that polled until the flags were served. +That is removed. Compensating here made this suite's results incomparable with every other adoption +run against the same backend: it read a clean floor while the others bounced, and the difference was +the wait rather than the provider. **Read a red run against the documented floor and repeat it before +blaming the provider** — the race moves between scenarios, a real defect does not. + +[tck]: ../../tools/openfeature-tck/README.md +[appendix-f]: https://github.com/open-feature/spec/blob/main/specification/appendix-f-provider-conformance.md + ## License Apache 2.0 - See [LICENSE](./LICENSE) for more information. diff --git a/providers/openfeature-provider-ofrep/pyproject.toml b/providers/openfeature-provider-ofrep/pyproject.toml index b974d77dc..bc822f304 100644 --- a/providers/openfeature-provider-ofrep/pyproject.toml +++ b/providers/openfeature-provider-ofrep/pyproject.toml @@ -29,12 +29,22 @@ Homepage = "https://github.com/open-feature/python-sdk-contrib" dev = [ "coverage[toml]>=7.10.0,<8.0.0", "mypy>=1.18.0,<2.0.0", + # The OpenFeature conformance suite. Ships the feature files, the flag set, + # the control-API client and the Compose harness that owns the container + # stack, and registers its step definitions through a pytest11 entry point -- + # so tests/tck declares a Compose file and nothing else. The `compose` extra + # is what pulls testcontainers in for the harness. + "openfeature-tck[compose]", "poethepoet>=0.37.0", "pytest>=9.0.0,<10.0.0", + "pytest-bdd>=8.1.0,<9.0.0", "requests-mock>=1.12.0,<2.0.0", "types-requests>=2.32.0,<3.0.0", ] +[tool.uv.sources] +openfeature-tck = { workspace = true } + [tool.uv.build-backend] module-name = "openfeature" module-root = "src" @@ -69,8 +79,51 @@ disallow_any_generics = false strict = true [tool.poe.tasks] -test = "pytest tests" -test-cov = "coverage run -m pytest tests" +# `tests/tck` is excluded from the default build on purpose, and a maintainer +# runs `poe test-tck` by hand before merging a change to it. Why a conformance +# suite is not a required gate is Appendix F, "Running the suite in CI" -- +# linked from tools/openfeature-tck/README.md -- and is deliberately not +# restated here. +# +# What is local to this package: a full run is 2 failed, 45 passed, 17 skipped, +# 1 xfailed. Both failures are canonical flags flagd-testbed does not seed yet, +# and the xfail is the one genuine provider gap; tests/tck/conftest.py accounts +# for them. Docker is not what decides the exclusion -- the flagd package's +# `tests/e2e` needs Docker too and does run. +# +# The suite is still *collected* on every default build, so it cannot quietly +# stop importing against the harness while nobody runs it. `--ignore` would +# otherwise leave nothing checking that, since mypy here is configured over +# `src` alone. +# +# `ignore_fail = "return_non_zero"` because poe aborts a sequence at its first +# failing subtask, which would put the collect step behind the default suite's +# result. It is green in this package today, and a check that only runs while +# everything else passes is not a check. Every subtask runs and a non-zero exit +# still propagates. The flagd package carries the same pair, where the default +# suite is red and the collect step was in fact never reached. +test = { sequence = ["test-default", "test-tck-collect"], ignore_fail = "return_non_zero" } +test-cov = { sequence = ["test-cov-default", "test-tck-collect"], ignore_fail = "return_non_zero" } +test-default = "pytest tests --ignore=tests/tck" +test-cov-default = "coverage run -m pytest tests --ignore=tests/tck" +# The sync is part of the run rather than something a maintainer remembers, +# because Appendix F requires the revision check to be in force **where the +# scenarios execute** and not only in the TCK implementation's own tests. This +# is a uv workspace, so `openfeature-tck` is installed editable and the +# canonical assets this suite collects are the gitignored copies under +# `tools/openfeature-tck/src`: a rebase that moves the submodule pin leaves them +# from the previous revision, and a suite run from here would neither notice nor +# say so. That is exactly how an adoption in another language published a full +# pass against stale scenarios. `hatch_build_sync.py` checks the submodule out +# at the pin, refuses to copy if it cannot, and needs nothing but the standard +# library. +# +# Not on `test-tck-collect`, deliberately: that runs on every default build, it +# executes no scenario, and it publishes no result, so the rule does not reach +# it and the default build stays free of a submodule operation. +sync-spec-assets = "python ../../tools/openfeature-tck/hatch_build_sync.py" +test-tck = ["sync-spec-assets", {cmd = "pytest tests/tck"}] +test-tck-collect = "pytest tests/tck --collect-only -q" cov-report = "coverage xml" cov = [ "test-cov", diff --git a/providers/openfeature-provider-ofrep/tests/tck/__init__.py b/providers/openfeature-provider-ofrep/tests/tck/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/providers/openfeature-provider-ofrep/tests/tck/conftest.py b/providers/openfeature-provider-ofrep/tests/tck/conftest.py new file mode 100644 index 000000000..35edadf05 --- /dev/null +++ b/providers/openfeature-provider-ofrep/tests/tck/conftest.py @@ -0,0 +1,99 @@ +"""Session fixtures for the OFREP conformance suite, and one recorded deviation. + +A full run is ``2 failed, 45 passed, 17 skipped, 1 xfailed``. Both failures ask +for ``large-integer-flag``, which no released flagd-testbed seeds -- +open-feature/flagd-testbed#392 adds it, along with the two other canonical flags +the image is missing. Neither carries a ``KnownDeviation``: the gap is the +backend's flag set, not the provider's. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from openfeature.contrib.tools.tck import BackendControl, ComposeBackend, RunningBackend + +OFREP_PORT = 8016 +"""flagd's OFREP HTTP port, and the one port this provider connects to. + +The launchpad's control port is exposed by the harness and must not be listed +here. +""" + + +@pytest.fixture(scope="session") +def compose_backend() -> ComposeBackend: + """The stack under test, for the TCK's ``tck_backend`` fixture. + + The Compose file is the same one the flagd adoption uses -- one definition + of the backend, copied per package -- so it also publishes flagd's two + resolver ports, which nothing here declares. + + The path is absolute so that pytest run from the repository root works too; + a relative one resolves against the working directory. + """ + return ComposeBackend( + compose_file=Path(__file__).parent / "docker-compose.yaml", + backend_ports=[OFREP_PORT], + ) + + +@pytest.fixture(scope="session") +def ofrep_base_url(tck_backend: RunningBackend) -> str: + """The origin flagd serves OFREP on, resolved once the stack is up. + + A fixture rather than a constant the suite module reads, because the mapped + host port does not exist until the stack has started. The provider appends + ``ofrep/v1/evaluate/flags/{key}`` itself (``ofrep/__init__.py:115-119``), so + this is the bare origin. + """ + endpoint = tck_backend.endpoint + return f"http://{endpoint.host}:{endpoint.port(OFREP_PORT)}" + + +@pytest.fixture(scope="session") +def ofrep_control(tck_backend: RunningBackend) -> BackendControl: + """The control API client, used exactly as the harness provides it. + + Deliberately unwrapped, and do not add a wait here. This backend's + ``/start`` can return before the reseeded flags are actually served, so it + is tempting to poll the OFREP endpoint until they are -- but that made this + suite's results incomparable with every other adoption run against the same + backend: this one read a clean floor while the others bounced, and the + difference was the wait rather than the provider. The defect is + open-feature/flagd-testbed#394 and belongs there. + + So repeat a red run before blaming the provider: the race moves between + scenarios, a real defect does not. + """ + return tck_backend.control + + +# The single scenario this provider cannot satisfy, marked xfail(strict=True) so +# that it stays in the report with its reason attached and the suite fails the +# moment it starts passing -- the marker goes when the bug is fixed rather than +# lingering as a lie. + +_BOOL_AS_INT = ( + "test_requesting_the_wrong_type_returns_the_code_default[boolean-flag-Integer-1]" +) + +_REASON = ( + "bool satisfies an Integer request. OFREP carries no requested type on the " + "wire, so the whole type check is the provider's, at " + "ofrep/__init__.py:244-256: FlagType.INTEGER maps to `int` and the check is " + "isinstance(value, int), which bool is a subclass of in Python. boolean-flag " + "requested as an Integer therefore returns True with reason STATIC and no " + "error code, where the specification requires the code default and " + "TYPE_MISMATCH. The Python SDK client type-checks the same way, so fixing " + "only one of the two is not enough. " + "See https://github.com/open-feature/python-sdk/issues/619" +) + + +def pytest_collection_modifyitems(items: list[pytest.Item]) -> None: + for item in items: + if item.name == _BOOL_AS_INT: + item.add_marker(pytest.mark.xfail(reason=_REASON, strict=True)) diff --git a/providers/openfeature-provider-ofrep/tests/tck/docker-compose.yaml b/providers/openfeature-provider-ofrep/tests/tck/docker-compose.yaml new file mode 100644 index 000000000..a230ea66d --- /dev/null +++ b/providers/openfeature-provider-ofrep/tests/tck/docker-compose.yaml @@ -0,0 +1,20 @@ +# The backend every conformance suite in this repository runs against: the unmodified +# flagd testbed image, serving flagd and its launchpad control API. +# +# Not flagd-testbed's own compose file, which adds an envoy sidecar for its +# forbidden-endpoint scenarios, names its service `flagd`, and bind-mounts a flags +# directory the launchpad writes into. None of that is needed to drive the launchpad. +# +# This file exists twice, once per provider package, and the two are byte-identical: +# providers/openfeature-provider-flagd/tests/tck/docker-compose.yaml +# providers/openfeature-provider-ofrep/tests/tck/docker-compose.yaml +# Each package publishes its own distribution and must not read the other's files, so +# `diff` the two paths after changing either -- that diff is what catches drift. +services: + backend: + image: ghcr.io/open-feature/flagd-testbed:v3.10.3 + ports: + - 8013 # flagd RPC evaluation (gRPC) + - 8015 # flagd in-process sync (gRPC) + - 8016 # flagd's OFREP HTTP API + - 8080 # launchpad control API diff --git a/providers/openfeature-provider-ofrep/tests/tck/test_ofrep.py b/providers/openfeature-provider-ofrep/tests/tck/test_ofrep.py new file mode 100644 index 000000000..80298aade --- /dev/null +++ b/providers/openfeature-provider-ofrep/tests/tck/test_ofrep.py @@ -0,0 +1,176 @@ +"""The OpenFeature provider conformance suite, run against the OFREP provider. + +OFREP is the vendor-neutral remote evaluation protocol, so what is under test +here is a pure mapping: one HTTP request per evaluation, and the translation of +its JSON response -- or its error status -- into typed resolution details. There +is no cache, no stream and no local ruleset. + +The backend is flagd, which serves OFREP on port 8016 alongside its own +protocols, driven through the same launchpad control API and seeded with the +same canonical flag set as the flagd conformance suites -- so a difference in +the results is a difference an application would see when it switches provider. +""" + +from __future__ import annotations + +import pytest +from pytest_bdd import scenarios + +from openfeature.contrib.provider.ofrep import OFREPProvider +from openfeature.contrib.tools.tck import ( + BackendControl, + Capability, + TckConfig, + feature_paths, +) +from openfeature.provider import FeatureProvider + +TIMEOUT_SECONDS = 10.0 +"""Bounds a single OFREP request. + +Generous, because every scenario is preceded by a control-API ``/start`` that +restarts the flagd process. It is the only timing knob this provider has +(``ofrep/__init__.py:52``). +""" + +# Every capability below was declared, the suite run, and its scenarios seen to +# pass -- bar the one row named under VARIANTS. The code references say where the +# behaviour lives, so a reader can check the claim. +# +# OBJECT ofrep/__init__.py:105-113, and the type check at +# ofrep/__init__.py:248 admits `(dict, list)` -- so a JSON object +# comes back as one rather than rejected or flattened. +# VARIANTS ofrep/__init__.py:160 carries the response's `variant` into the +# details. Seven of the outline's eight rows pass; the eighth asks +# for `large-integer-flag`, which no released flagd-testbed seeds +# (see conftest.py). Withholding over it would say this provider +# does not name variants, which the other seven rows show is false. +# TARGETING ofrep/__init__.py:229-230 puts the targeting key into the request +# body's `context`, so flagd evaluates the rule against it. All +# three scenarios pass, and for a protocol with no types on the +# wire that is the whole of what passthrough can verify: the key +# reached flagd. +# STANDARD_REASONS +# Eight of the file's nine scenarios run here and all eight pass. +# The ninth composes with @disabled-flags, withheld below, so it +# skips with that reason. +# +# STRING_TYPING and FULLY_TYPED_VALUES +# Both declared, on a run: all four scenarios pass. **"Untyped protocol" is +# not the same claim as "untyped backend"**, and this is the tag where the +# two come apart. OFREP carries no *requested* type on the wire -- which is +# exactly why boolean-flag satisfies an Integer request here and is xfailed +# in conftest.py -- but the response body is JSON, and JSON distinguishes +# `true` from `"true"` and `10` from `"10"`. ofrep/__init__.py:249-256 maps +# FlagType.STRING to the bare `str`, so a bool, a number or a structure +# fails that isinstance and becomes TYPE_MISMATCH; `str` is not in bool's +# ancestry, so the subclass hazard behind the xfail has no counterpart here. +# Both tags rest on a run rather than on the protocol, because what they ask +# about is the backend's storage: flagd holds typed variants, and the same +# provider over a string-storing backend would withhold one or both. +# +# Not declared, and why. +# +# NUMERIC_COERCION +# Withheld, because this provider does not coerce at all -- and a declarer +# must satisfy all three scenarios. json.loads yields `int` for 10 and +# `float` for 0.5, and ofrep/__init__.py:249-256 admits a value only on an +# exact isinstance against one of them, so float-flag as an Integer is a +# TYPE_MISMATCH rather than a silent 0, and integer-flag as a Float fails. +# No KnownDeviation: nothing requires numeric coercion. +# +# DISABLED_FLAGS +# Withheld, and **this is the one declaration here that should change +# shape**: the provider attempts the behaviour and fails on one +# unconditional index, which is the declare-and-deviate case. Declaring the +# tag fails all four rows on the error code rather than the value. flagd +# answers a disabled flag with a reason and no `value` and no `variant`; the +# absent value is not the obstacle, because ofrep/__init__.py:153 already +# reads `data.get("value", default_value)`. What fails is +# ofrep/__init__.py:160, indexing `data["variant"]` unconditionally where +# types.md types the field optional -- so **the whole difference between +# passing and failing these four rows is one `.get`**, and the same index +# breaks on any variant-less OFREP response. open-feature/python-sdk-contrib +# #418; the next change here declares the tag and records the deviation. +# +# Everything below follows from the provider being stateless: it holds a +# requests.Session and a rate-limit timestamp, and nothing else survives +# between evaluations. +# +# LIFECYCLE +# `initialize` is inherited from AbstractProvider and is `pass`, so nothing +# contacts the backend before the first evaluation and initialisation has no +# outcome to observe. +# +# EVENTS +# `_on_emit` is never called anywhere in ofrep/__init__.py: no stream, no +# poll, no background thread. Declaring it would make the readiness scenario +# pass without demonstrating anything, because the SDK's registry dispatches +# PROVIDER_READY around `initialize` for any provider (python-sdk +# openfeature/provider/_registry.py:73-77). +# +# STALE, CONFIGURATION_CHANGE +# Both follow from EVENTS: every evaluation is an independent HTTP request, +# so no state between them can go stale and nothing watches the backend. A +# *change* is nonetheless visible to an application -- the next evaluation +# returns the new value -- but the signal is not. +# +# UNAVAILABLE_INIT +# A provider pointed at a closed port reaches READY, because `initialize` +# does nothing and the registry dispatches PROVIDER_READY unconditionally. +# The failure surfaces on the first evaluation as GeneralError from +# ofrep/__init__.py:167 rather than as PROVIDER_ERROR, so the scenario's +# premise does not hold -- and no `new_unavailable_provider` is supplied. +# +# REINITIALIZATION +# Reuse would in fact work -- nothing to release and nothing to rebuild -- +# but the scenario inherits @lifecycle from its feature, withheld above, so +# declaring this would leave the claim unexamined. +# +# LARGE_INTEGERS +# The one withholding here that is about the backend rather than the +# provider: the tag's one scenario asks for `huge-integer-flag` and no +# released flagd-testbed seeds it. Nothing in this path would narrow the +# value -- a JSON number decodes into an unbounded Python `int` and the type +# check admits it unchanged -- and the suite cannot show that, which is the +# point. Declare the tag once open-feature/flagd-testbed#392 is in the +# image, or the withholding starts reading as a claim about the provider. +# +# CACHING +# No scenario carries the tag. +CAPABILITIES = frozenset( + { + Capability.OBJECT, + Capability.VARIANTS, + Capability.STRING_TYPING, + Capability.FULLY_TYPED_VALUES, + Capability.TARGETING, + Capability.STANDARD_REASONS, + } +) + + +@pytest.fixture(scope="session") +def tck_config( + ofrep_base_url: str, + ofrep_control: BackendControl, +) -> TckConfig: + """Wire the provider up to the running testbed. + + Both fixtures come from ``tests/tck/conftest.py`` and both resolve after the + TCK has started the stack, which is when the mapped host port exists. + """ + base_url = ofrep_base_url + + def new_provider() -> FeatureProvider: + return OFREPProvider(base_url, timeout=TIMEOUT_SECONDS) + + return TckConfig( + name="ofrep", + control=ofrep_control, + new_provider=new_provider, + capabilities=CAPABILITIES, + ) + + +scenarios(*feature_paths()) diff --git a/uv.lock b/uv.lock index c53f6392a..ba26eb45d 100644 --- a/uv.lock +++ b/uv.lock @@ -2016,8 +2016,10 @@ dependencies = [ dev = [ { name = "coverage", extra = ["toml"] }, { name = "mypy" }, + { name = "openfeature-tck", extra = ["compose"] }, { name = "poethepoet" }, { name = "pytest" }, + { name = "pytest-bdd" }, { name = "requests-mock" }, { name = "types-requests" }, ] @@ -2032,8 +2034,10 @@ requires-dist = [ dev = [ { name = "coverage", extras = ["toml"], specifier = ">=7.10.0,<8.0.0" }, { name = "mypy", specifier = ">=1.18.0,<2.0.0" }, + { name = "openfeature-tck", extras = ["compose"], editable = "tools/openfeature-tck" }, { name = "poethepoet", specifier = ">=0.37.0" }, { name = "pytest", specifier = ">=9.0.0,<10.0.0" }, + { name = "pytest-bdd", specifier = ">=8.1.0,<9.0.0" }, { name = "requests-mock", specifier = ">=1.12.0,<2.0.0" }, { name = "types-requests", specifier = ">=2.32.0,<3.0.0" }, ]