Add unit tests for osism/services websocket_manager and event_bridge#2460
Open
berendt wants to merge 1 commit into
Open
Add unit tests for osism/services websocket_manager and event_bridge#2460berendt wants to merge 1 commit into
berendt wants to merge 1 commit into
Conversation
Create the tests/unit/services package with test modules for osism/services/websocket_manager.py and osism/services/event_bridge.py, which previously had no unit test coverage. test_websocket_manager.py covers the EventMessage value object, the per-connection filter logic in WebSocketConnection.matches_filters, the WebSocketManager connection registry (connect/disconnect/update_filters), the event queue helpers (add_event/send_heartbeat), the per-service identifier extraction in broadcast_event_from_notification and the _broadcast_events broadcaster loop including disconnect cleanup and cancellation handling. test_event_bridge.py covers Redis initialization including environment variable overrides and connection failures, the publish path in add_event with reconnect and local-queue fallback, thread startup guards in set_websocket_manager, the _redis_subscriber_loop including resubscribe and retry exhaustion, _process_single_event, the _process_events loop and shutdown. The thread loops are called synchronously and terminated via _shutdown_event-driven side effects, so no real threads, sockets or sleeps are involved. Most WebSocketManager methods are coroutines, so pytest-asyncio is added to the Pipfile dev packages (with asyncio_default_fixture_loop_scope set in setup.cfg); async tests are marked with pytest.mark.asyncio explicitly to keep --strict-markers happy. Loop-driving tests carry pytest-timeout markers so a regression cannot hang CI. Assisted-by: Claude:claude-fable-5 Signed-off-by: Christian Berendt <berendt@osism.tech>
bdab8be to
322dfc7
Compare
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Several tests reach deeply into EventBridge/WebSocketManager internals (e.g. _redis_client, _redis_subscriber, _shutdown_event, private thread fields), which may make future refactoring harder; consider adding small public helpers or using dependency injection to exercise behavior without coupling to private attributes.
- The WebSocketManager broadcaster tests rely on fixed asyncio.sleep durations to assert behavior; using synchronization primitives (e.g. an event set after send_text is awaited or draining the queue) would make these tests more deterministic and less timing‑sensitive.
- The event extraction parametrization in test_broadcast_event_from_notification is quite dense; extracting the payload-building logic into named helpers or smaller focused tests per service type could improve readability and ease future changes to the extraction rules.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several tests reach deeply into EventBridge/WebSocketManager internals (e.g. _redis_client, _redis_subscriber, _shutdown_event, private thread fields), which may make future refactoring harder; consider adding small public helpers or using dependency injection to exercise behavior without coupling to private attributes.
- The WebSocketManager broadcaster tests rely on fixed asyncio.sleep durations to assert behavior; using synchronization primitives (e.g. an event set after send_text is awaited or draining the queue) would make these tests more deterministic and less timing‑sensitive.
- The event extraction parametrization in test_broadcast_event_from_notification is quite dense; extracting the payload-building logic into named helpers or smaller focused tests per service type could improve readability and ease future changes to the extraction rules.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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 #2357.
Creates the
tests/unit/services/package with unit tests forosism/services/websocket_manager.pyandosism/services/event_bridge.py, which previously had no coverage.test_websocket_manager.py
EventMessage: constructor attributes, UUID4 id uniqueness, ISO 8601Z-suffixed timestamp, exactto_dict()keys,to_json()round tripWebSocketConnection.matches_filters(): no-filter pass-through, event/node/service filters incl.node_name=Nonerejection,""→unknownservice mapping and AND-combination of filtersWebSocketManager.connect()/disconnect(): accept + registration, broadcaster started only once while running (done()check), restart after completion, no-op disconnect for unknown websocketsupdate_filters(): full and partial updates, clearing via[], unknown websocket no-opadd_event()/send_heartbeat(): queueing, early return without connections, heartbeat event shapebroadcast_event_from_notification(): identifier extraction for baremetal/compute/nova/network/neutron/volume/image/identity payloads incl. fallback keys, missing payload keys, unknown service types, empty event type, payload copy semantics and the swallowed-exception path_broadcast_events(): filtered delivery,WebSocketDisconnectand generic-error connection cleanup, consumption without connections and clean cancellationtest_event_bridge.py
_init_redis(): default connection parameters,REDIS_HOST/REDIS_PORT/REDIS_DBoverrides, ping failure handling and theREDIS_AVAILABLE = Falselocal-queue-only pathset_websocket_manager(): thread startup guards for subscriber and processor threadsadd_event(): publish payload, zero-subscriber warning, reconnect-then-publish, fallback to the local queue when reconnection fails,queue.Fulland generic error swallowing_redis_subscriber_loop(): missing subscriber, subscribe/get_message flow, message dispatch with and without a WebSocket manager, invalid JSON, processing errors, resubscribe afterget_messagefailures, back-off with_init_redis()re-invocation whensubscribe()raises, retry exhaustion and swallowedclose()errors_process_single_event(): missing-manager warning,AsyncMockbroadcast dispatch and swallowed coroutine errors_process_events(): queued event processing withtask_done(), empty-queue continuation, pre-set shutdown exit and error continuationshutdown(): shutdown event, subscriber close incl. error path and conditional thread joinsNotes
WebSocketManager/EventBridgeinstances instead of the module-level singletons;EventBridgeis always constructed withredis.Redispatched._shutdown_event-driven side effects; the asyncio broadcaster tests bound the task with cancellation infinallyand carrypytest-timeoutmarkers, so no test can hang CI.pytest-asyncio1.4.0 is added to the Pipfile[dev-packages](compatible with pytest 9,pytest<10,>=8.4); async tests are marked explicitly with@pytest.mark.asyncio, so--strict-markersstays happy.Pipfile.lockwas regenerated on linux/amd64 with Python 3.12 to match CI.get_message()only breaks the inner loop and triggers a resubscribe; the retry counter and_shutdown_event.wait()back-off only engage whensubscribe()itself raises. The tests pin the actual behavior for both paths.🤖 Generated with Claude Code