Add integration test for OpenStack events WebSocket endpoint#2412
Add integration test for OpenStack events WebSocket endpoint#2412berendt wants to merge 1 commit into
Conversation
Cover the /v1/events/openstack WebSocket and the in-process websocket_manager via the FastAPI TestClient: connection accept, filter-message acknowledgment, delivery of a matching event, filtering-out of a non-matching event, and connection-count cleanup on disconnect. The TestClient fixture is module-scoped because the manager's module-level asyncio primitives bind to the first event loop that touches them; sharing one loop across the module keeps them valid. Events are pushed onto the loop-bound queue via the app's portal. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Christian Berendt <berendt@osism.tech>
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Several tests duplicate the WebSocket URL and common
set_filterspayloads; consider extracting small helper functions or constants to reduce repetition and make future changes to the endpoint or filter shape easier. - The tests directly inspect
websocket_manager.connections, which tightly couples them to the internal implementation; if possible, prefer asserting externally observable behavior (e.g., via events or metrics helpers) so that refactoring the manager internals won’t require test rewrites.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several tests duplicate the WebSocket URL and common `set_filters` payloads; consider extracting small helper functions or constants to reduce repetition and make future changes to the endpoint or filter shape easier.
- The tests directly inspect `websocket_manager.connections`, which tightly couples them to the internal implementation; if possible, prefer asserting externally observable behavior (e.g., via events or metrics helpers) so that refactoring the manager internals won’t require test rewrites.
## Individual Comments
### Comment 1
<location path="tests/integration/test_api_websocket.py" line_range="95-104" />
<code_context>
+def test_non_matching_event_is_filtered_out(client):
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen assertions in `test_non_matching_event_is_filtered_out` to validate the full payload
This test only checks the `id` and `event_type` of the sentinel message. To better guard against regressions, consider asserting equality with `sentinel.to_dict()` (as in `test_matching_event_is_delivered`) and/or explicitly asserting that the `received` body does not equal `non_matching.to_dict()` so the entire payload is validated, not just two fields.
Suggested implementation:
```python
# The full payload of the delivered event should match the sentinel
assert received == sentinel.to_dict()
# And it must not match the non-matching event that was filtered out
assert received != non_matching.to_dict()
```
If the existing assertions in `test_non_matching_event_is_filtered_out` differ from the `SEARCH` block above (for example, if the keys or attribute names are slightly different), adjust the `SEARCH` section to match the exact current assertions and keep the `REPLACE` section as-is.
Also ensure that within `test_non_matching_event_is_filtered_out`:
1. `non_matching` is defined in the same scope as the new assertions (as in your snippet it is).
2. A `sentinel` event is created, enqueued, and assigned to the `sentinel` variable before `received = ws.receive_json()` so that `sentinel.to_dict()` is meaningful, similar to how it is done in `test_matching_event_is_delivered`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def test_non_matching_event_is_filtered_out(client): | ||
| """An event that does not match the filters is not delivered.""" | ||
| with client.websocket_connect("/v1/events/openstack") as ws: | ||
| ws.send_json( | ||
| {"action": "set_filters", "event_filters": ["compute.instance.create.end"]} | ||
| ) | ||
| ws.receive_json() # filter acknowledgment | ||
|
|
||
| non_matching = EventMessage( | ||
| "baremetal.node.power_set.end", "openstack", {"node": str(uuid.uuid4())} |
There was a problem hiding this comment.
suggestion (testing): Strengthen assertions in test_non_matching_event_is_filtered_out to validate the full payload
This test only checks the id and event_type of the sentinel message. To better guard against regressions, consider asserting equality with sentinel.to_dict() (as in test_matching_event_is_delivered) and/or explicitly asserting that the received body does not equal non_matching.to_dict() so the entire payload is validated, not just two fields.
Suggested implementation:
# The full payload of the delivered event should match the sentinel
assert received == sentinel.to_dict()
# And it must not match the non-matching event that was filtered out
assert received != non_matching.to_dict()If the existing assertions in test_non_matching_event_is_filtered_out differ from the SEARCH block above (for example, if the keys or attribute names are slightly different), adjust the SEARCH section to match the exact current assertions and keep the REPLACE section as-is.
Also ensure that within test_non_matching_event_is_filtered_out:
non_matchingis defined in the same scope as the new assertions (as in your snippet it is).- A
sentinelevent is created, enqueued, and assigned to thesentinelvariable beforereceived = ws.receive_json()so thatsentinel.to_dict()is meaningful, similar to how it is done intest_matching_event_is_delivered.
Closes #2407
What this does
Adds an integration test for the OpenStack events WebSocket endpoint
GET /v1/events/openstack, driving it throughfastapi.testclient.TestClient. The endpoint upgrades to a WebSocket served by the in-processwebsocket_manager(osism/services/websocket_manager.py), which broadcasts via an in-processasyncioqueue — so the test exercises the full connect / set-filters / broadcast / disconnect path end-to-end without any service beyond theTestClient.Change set (single commit)
35d94ea— Add integration test for OpenStack events WebSocket endpointtests/integration/test_api_websocket.py(new, 133 lines) — five test cases covering:test_websocket_connect_is_accepted— the WebSocket upgrade is accepted.test_set_filters_is_acknowledged— aset_filtersmessage is processed and acknowledged verbatim (filter_update/success, echoing the event/node/service filters).test_matching_event_is_delivered— an event matching the connection's filters is delivered intact (compared againstEventMessage.to_dict()).test_non_matching_event_is_filtered_out— a non-matching event is dropped; a FIFO-following sentinel event proves the drop without relying on an absence/timeout.test_disconnect_drops_connection_count— disconnect runs thefinallycleanup and the connection count returns to its pre-connect value.Pipfile/Pipfile.lock— addpytest-timeout==2.4.0to dev dependencies.Notes for the reviewer
clientfixture. The globalwebsocket_managerowns module-levelasyncioprimitives (event_queue,_lock) that bind to the first event loop that touches them and raise "bound to a different event loop" otherwise. A single module-scopedTestClientkeeps one loop alive across the module so those primitives stay valid.osism.apiis imported lazily inside the fixture because importing it wires the event bridge to Redis at module load — safe only in the integration environment.client.portal.call(websocket_manager.add_event, ...)rather than from the test thread, because the queue is loop-bound.pytest-timeout==2.4.0as a dev dependency and appliespytest.mark.timeout(30)to the module. Without it, the blockingws.receive_json()calls (Starlette's untimedqueue.get()) would hang forever on a regression and only die on the CI wall-clock timeout; the cap turns such a hang into a precise 30-second failure at the exactreceive_json()call. The test is also taggedintegrationto share the FastAPI/httpxsetup with the sibling facts test and stay in the same Tier 2 batch.Implemented by planwerk-review a24f4d9 with Claude:claude-opus-4-8