Add integration test for OpenStack events WebSocket endpoint#2412
Open
berendt wants to merge 1 commit into
Open
Add integration test for OpenStack events WebSocket endpoint#2412berendt wants to merge 1 commit into
berendt wants to merge 1 commit into
Conversation
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.
35d94ea to
61ba91e
Compare
83ad893 to
417e7f2
Compare
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The tests reach into
websocket_manager.connectionsto assert connection counts, which couples them tightly to internal implementation details; consider asserting on observable behavior or a public API instead to keep the tests resilient to internal refactors. - The hard-coded 30-second
pytest.mark.timeout(30)applied at the module level may be unnecessarily high for some cases and slow down failures; consider using a shorter default or per-test timeouts tuned to the expected behavior of each scenario. - The event type strings (e.g.
"compute.instance.create.end","baremetal.node.power_set.end") are repeated inline; extracting them into named constants or shared fixtures would reduce duplication and clarify the intent of each test.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The tests reach into `websocket_manager.connections` to assert connection counts, which couples them tightly to internal implementation details; consider asserting on observable behavior or a public API instead to keep the tests resilient to internal refactors.
- The hard-coded 30-second `pytest.mark.timeout(30)` applied at the module level may be unnecessarily high for some cases and slow down failures; consider using a shorter default or per-test timeouts tuned to the expected behavior of each scenario.
- The event type strings (e.g. `"compute.instance.create.end"`, `"baremetal.node.power_set.end"`) are repeated inline; extracting them into named constants or shared fixtures would reduce duplication and clarify the intent of each test.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
44a5d73 to
cbb5d85
Compare
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 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-fable-5 Signed-off-by: Christian Berendt <berendt@osism.tech>
cbb5d85 to
b50ace6
Compare
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.
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