test(e2e): add Behave step definitions for context merging#593
test(e2e): add Behave step definitions for context merging#593balgaly wants to merge 1 commit intoopen-feature:mainfrom
Conversation
Add the missing E2E step definitions so the contextMerging.feature scenarios from the OpenFeature spec run against python-sdk. Fixes open-feature#500 Changes: - Bump spec submodule to 130df3eb so contextMerging.feature is copied in during the `poe e2e` task. - Add tests/features/environment.py with a before_scenario hook that resets provider/hook/API-context/transaction-context state, so scenarios cannot leak state between features. - Add tests/features/steps/context_merging_steps.py: - RetrievableContextProvider captures the merged EvaluationContext it receives, so assertions can inspect what the SDK merged. - Step definitions for all scenarios in contextMerging.feature: single-level insert, multi-level insert, and per-key overwrite precedence across API / Transaction / Client / Invocation / Before Hooks. - Client-level context is set via direct attribute assignment on OpenFeatureClient.context (no new setter), since merging already honors client.context (openfeature/client.py:422-429). Runs clean: 4 features / 50 scenarios / 233 steps.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces BDD test steps and environment setup for testing context merging logic in the OpenFeature Python SDK. It includes a custom provider to capture merged contexts and steps to populate context at various precedence levels (API, Transaction, Client, Invocation, and Hooks). Feedback focuses on correctly handling the "targeting_key" as a first-class property rather than a generic attribute during both context population and assertion, suggesting the use of the SDK's built-in merge method for consistency.
| if level == "API": | ||
| current = api.get_evaluation_context() | ||
| api.set_evaluation_context( | ||
| EvaluationContext( | ||
| targeting_key=current.targeting_key, | ||
| attributes={**current.attributes, key: value}, | ||
| ) | ||
| ) | ||
| elif level == "Transaction": | ||
| current = api.get_transaction_context() | ||
| set_transaction_context( | ||
| EvaluationContext( | ||
| targeting_key=current.targeting_key, | ||
| attributes={**current.attributes, key: value}, | ||
| ) | ||
| ) | ||
| elif level == "Client": | ||
| current = context.client.context | ||
| context.client.context = EvaluationContext( | ||
| targeting_key=current.targeting_key, | ||
| attributes={**current.attributes, key: value}, | ||
| ) | ||
| elif level == "Invocation": | ||
| current = context.invocation_context | ||
| context.invocation_context = EvaluationContext( | ||
| targeting_key=current.targeting_key, | ||
| attributes={**current.attributes, key: value}, | ||
| ) | ||
| elif level == "Before Hooks": | ||
| current = context.before_hook_context | ||
| context.before_hook_context = EvaluationContext( | ||
| targeting_key=current.targeting_key, | ||
| attributes={**current.attributes, key: value}, | ||
| ) |
There was a problem hiding this comment.
The current implementation of _add_entry_at_level treats targeting_key as a regular attribute. However, in the OpenFeature Python SDK, targeting_key is a first-class property of the EvaluationContext and has specific merging logic (as seen in EvaluationContext.merge).
If the Gherkin scenario uses targeting_key as a key, it should be mapped to the targeting_key field of the EvaluationContext to correctly exercise the SDK's merging behavior. Additionally, using the built-in merge method simplifies the logic and ensures consistency with the SDK's implementation.
new_entry = (
EvaluationContext(targeting_key=value)
if key == "targeting_key"
else EvaluationContext(attributes={key: value})
)
if level == "API":
api.set_evaluation_context(api.get_evaluation_context().merge(new_entry))
elif level == "Transaction":
set_transaction_context(api.get_transaction_context().merge(new_entry))
elif level == "Client":
context.client.context = context.client.context.merge(new_entry)
elif level == "Invocation":
context.invocation_context = context.invocation_context.merge(new_entry)
elif level == "Before Hooks":
context.before_hook_context = context.before_hook_context.merge(new_entry)| attributes = context.provider.last_context.attributes | ||
| assert key in attributes, f"key {key!r} missing from merged context: {attributes!r}" | ||
| assert attributes[key] == value, ( | ||
| f"expected {key!r}={value!r}, got {attributes[key]!r}" | ||
| ) |
There was a problem hiding this comment.
This assertion only checks the attributes dictionary. If the key is targeting_key, it should check the targeting_key property of the EvaluationContext instead, as the SDK separates this field from general attributes.
| attributes = context.provider.last_context.attributes | |
| assert key in attributes, f"key {key!r} missing from merged context: {attributes!r}" | |
| assert attributes[key] == value, ( | |
| f"expected {key!r}={value!r}, got {attributes[key]!r}" | |
| ) | |
| last_context = context.provider.last_context | |
| actual_value = ( | |
| last_context.targeting_key | |
| if key == "targeting_key" | |
| else last_context.attributes.get(key) | |
| ) | |
| assert actual_value == value, f"expected {key!r}={value!r}, got {actual_value!r}" |
1 participant
Fixes #500
Per @gruebel's note on #565 — the actual fix for #500 is the missing E2E tests, not a new setter, since Python idiomatically uses direct attribute access. The merging precedence in the SDK is already correct (openfeature/client.py:422-429); this PR proves it with Gherkin scenarios from the spec.
Summary
poe e2etask.Client-level context is set via direct attribute assignment on OpenFeatureClient.context rather than a new setter. This matches the Pythonic pattern called out in #565 review and requires no API surface change.
Test plan
Supersedes
Closes #590 in favor of this approach.