-
Notifications
You must be signed in to change notification settings - Fork 45
chore: Support flag change listeners in contract tests #410
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c6978e9
3c946fe
095913a
e21aa3d
0424bc8
41e2c6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| import logging | ||
| import threading | ||
| from typing import Callable, Dict | ||
|
|
||
| import requests | ||
|
|
||
| from ldclient.context import Context | ||
| from ldclient.interfaces import FlagChange, FlagTracker, FlagValueChange | ||
|
|
||
| log = logging.getLogger('testservice') | ||
|
|
||
|
|
||
| class ListenerRegistry: | ||
| """Manages all active flag change listener registrations for a single SDK client entity.""" | ||
|
|
||
| def __init__(self, tracker: FlagTracker): | ||
| self._tracker = tracker | ||
| self._lock = threading.Lock() | ||
| # Maps listener_id -> (sdk_listener callable, cleanup function) | ||
| self._listeners: Dict[str, Callable] = {} | ||
|
|
||
| def register_flag_change_listener(self, listener_id: str, callback_uri: str): | ||
| """Register a general flag change listener that fires on any flag configuration change.""" | ||
| def on_flag_change(flag_change: FlagChange): | ||
| payload = { | ||
| 'listenerId': listener_id, | ||
| 'flagKey': flag_change.key, | ||
| } | ||
| try: | ||
| requests.post(callback_uri, json=payload) | ||
| except Exception as e: | ||
| log.warning('Failed to post flag change notification: %s', e) | ||
|
|
||
| with self._lock: | ||
| # If a listener with this ID already exists, unregister the old one first | ||
| if listener_id in self._listeners: | ||
| self._tracker.remove_listener(self._listeners[listener_id]) | ||
|
|
||
| self._tracker.add_listener(on_flag_change) | ||
| self._listeners[listener_id] = on_flag_change | ||
|
|
||
| def register_flag_value_change_listener( | ||
| self, | ||
| listener_id: str, | ||
| flag_key: str, | ||
| context: Context, | ||
| default_value, | ||
| callback_uri: str, | ||
| ): | ||
| """Register a flag value change listener that fires when the evaluated value changes.""" | ||
| def on_value_change(change: FlagValueChange): | ||
| payload = { | ||
| 'listenerId': listener_id, | ||
| 'flagKey': change.key, | ||
| 'oldValue': change.old_value, | ||
| 'newValue': change.new_value, | ||
| } | ||
| try: | ||
| requests.post(callback_uri, json=payload) | ||
| except Exception as e: | ||
| log.warning('Failed to post flag value change notification: %s', e) | ||
|
|
||
| # add_flag_value_change_listener returns the underlying listener | ||
| # that must be passed to remove_listener to unsubscribe | ||
| with self._lock: | ||
| if listener_id in self._listeners: | ||
| self._tracker.remove_listener(self._listeners[listener_id]) | ||
|
|
||
| underlying_listener = self._tracker.add_flag_value_change_listener(flag_key, context, on_value_change) | ||
| self._listeners[listener_id] = underlying_listener | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| def unregister(self, listener_id: str) -> bool: | ||
| """Unregister a previously registered listener. Returns False if not found.""" | ||
| with self._lock: | ||
| listener = self._listeners.pop(listener_id, None) | ||
|
|
||
| if listener is None: | ||
| return False | ||
|
|
||
| self._tracker.remove_listener(listener) | ||
| return True | ||
|
|
||
| def close_all(self): | ||
| """Unregister all listeners. Called when the SDK client entity shuts down.""" | ||
| with self._lock: | ||
| listeners = dict(self._listeners) | ||
| self._listeners.clear() | ||
|
|
||
| for listener in listeners.values(): | ||
| self._tracker.remove_listener(listener) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -266,14 +266,10 @@ def test_fdv2_falls_back_to_fdv1_on_polling_success_with_header(): | |
|
|
||
| changed = Event() | ||
| changes: List[FlagChange] = [] | ||
| count = 0 | ||
|
|
||
| def listener(flag_change: FlagChange): | ||
| nonlocal count | ||
| count += 1 | ||
| changes.append(flag_change) | ||
| if count >= 2: | ||
| changed.set() | ||
| changed.set() | ||
|
|
||
| set_on_ready = Event() | ||
| fdv2 = FDv2(Config(sdk_key="dummy"), data_system_config) | ||
|
|
@@ -282,11 +278,11 @@ def listener(flag_change: FlagChange): | |
|
|
||
| assert set_on_ready.wait(1), "Data system did not become ready in time" | ||
|
|
||
| # Trigger a flag update in FDv1 | ||
| # Update flag in FDv1 data source to verify it's being used | ||
| td_fdv1.update(td_fdv1.flag("fdv1-fallback-flag").on(False)) | ||
| assert changed.wait(1), "Flag change listener was not called in time" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test event set during init, update never verifiedMedium Severity The
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is intentional — the test now follows the same pattern as the existing The core purpose of the test is to verify FDv1 becomes active after a I've called this out in the PR description (item #3 under "Items for reviewer attention") for the human reviewer to confirm. |
||
| assert changed.wait(2), "Flag change listener was not called in time" | ||
|
|
||
| # Verify FDv1 is active | ||
| # Verify we got flag changes from FDv1 | ||
| assert len(changes) > 0 | ||
| assert any(c.key == "fdv1-fallback-flag" for c in changes) | ||
|
|
||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accepted
default_valueparameter is silently ignoredMedium Severity
The
default_valueparameter is accepted from the test harness but never used. The SDK'sFlagTracker.add_flag_value_change_listenerdoesn't accept a default value, and the SDK's internaleval_fnalways evaluates withNoneas the default (viaself.variation(key, context, None)inclient.py). Unlike the Go SDK reference implementation, which does forward the default, this means if a contract test relies on a non-Nonedefault affecting the initial or subsequent evaluated value, the Python test service will produce incorrect results.Additional Locations (1)
contract-tests/client_entity.py#L297-L298There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a known limitation already documented in the PR description (item #1 under "Items for reviewer attention"). The Python SDK's
FlagTracker.add_flag_value_change_listener(key, context, fn)doesn't accept a default value parameter — unlike Go's, which does. The SDK internally evaluates withNoneas default viaself.variation(key, context, None)inclient.py.This is an SDK-level gap, not something the test service can work around. If contract tests rely on a non-
Nonedefault, the SDK itself would need to be updated to support passing a default value through the flag value change listener API.