Add unit tests for osism/services/listener.py

[berendt]

Closes #2358.

Creates tests/unit/services/test_listener.py with unit tests for the RabbitMQ notification listener (osism/services/listener.py), which previously had no coverage.

Coverage

• Module constants: EXCHANGES_CONFIG contains exactly the six OpenStack services with the expected exchange/routing-key/queue naming; the legacy EXCHANGE_NAME/ROUTING_KEY/QUEUE_NAME constants match the ironic entry
• BaremetalEvents.get_handler(): all eight registered event types resolve to their bound methods (parametrized over the full _handler tree), unknown leaf/branch events fall back to the default handler (logs, no NetBox task), extra event-type segments are ignored, short event types leak an IndexError (documented with pytest.raises), get_object_data() raises KeyError on missing ironic_object.data
• Handler methods: each handler calls the expected netbox.set_power_state/set_provision_state/set_maintenance task via .delay (incl. the state= keyword for maintenance and the double reset in node_delete_end)
• NotificationsDump.__init__(): no API session without OSISM_API_URL, trailing-slash stripping when set, event bridge singleton wiring and the ImportError fallback via sys.modules, initial discovery state
• Exchange discovery: _get_exchange_properties() (passive declare, exception → None), _check_for_new_exchanges() (new/known/none/channel-error paths), _exchange_discovery_loop() driven synchronously with a scripted stop event (stop request, all-available break, restart signaling via _new_exchanges_found/should_stop, quiet continuation), thread start/stop with threading.Thread patched, _wait_for_exchanges() retry with EXCHANGE_RETRY_INTERVAL
• get_consumers(): consumer per available exchange with passive=True exchanges and no_ack=True queues, exchange_props defaults, per-service error isolation, empty-exchange error path
• on_message(): payload-info extraction for baremetal/nova/neutron/other services, missing event_type behavior (documented KeyError), event bridge forwarding incl. both error logs and the unknown fallback for non-ironic payloads, OSISM API delivery (204 success with exact JSON body, retry/backoff sleep(3)/sleep(9) on ConnectionError/Timeout, early give-up on HTTP errors <= 500 incl. the 500 edge case, retries on 503 and on the 200-falls-through path), handler dispatch without an API session
• main(): ConnectionRefusedError retry with sleep(60), and the consumer restart path when the discovery thread finds new exchanges (exchange carry-over into the next NotificationsDump instance)

Notes

• Threads and timeouts are never real: the discovery loop is called synchronously with a scripted _stop_discovery mock, threading.Thread and time.sleep are patched (the sleep mock carries a bounded guard so a regression cannot hang pytest), and main()'s while True loop is escaped with a sentinel exception.
• The NetBox Celery tasks are only invoked via .delay, so the three osism.services.listener.netbox.*.delay attributes are patched; no broker is needed.
• tests/unit/services/__init__.py is byte-identical to the one created by Add unit tests for osism/services websocket_manager and event_bridge #2460, so whichever PR lands second merges cleanly.

🤖 Generated with Claude Code

[sourcery-ai]

Hey - I've found 2 issues, and left some high level feedback:

• The test_listener.py file is quite large and mixes many concerns (constants, event dispatch, discovery, API delivery, main loop); consider splitting into smaller test modules or grouping related tests with helper functions/fixtures to keep the structure easier to navigate and maintain.
• Many tests assert on log messages via substring matching, which makes them sensitive to minor wording changes; where possible, prefer asserting on behavior (state changes, calls, flags) over exact log content or centralize log helpers to reduce brittleness.
• Several on_message tests repeatedly set up consumer.event_bridge, osism_api_session, and baremetal_events in similar ways; extracting a dedicated fixture or helper for common consumer configurations would reduce duplication and make it clearer which behavior each test is targeting.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `test_listener.py` file is quite large and mixes many concerns (constants, event dispatch, discovery, API delivery, main loop); consider splitting into smaller test modules or grouping related tests with helper functions/fixtures to keep the structure easier to navigate and maintain.
- Many tests assert on log messages via substring matching, which makes them sensitive to minor wording changes; where possible, prefer asserting on behavior (state changes, calls, flags) over exact log content or centralize log helpers to reduce brittleness.
- Several `on_message` tests repeatedly set up `consumer.event_bridge`, `osism_api_session`, and `baremetal_events` in similar ways; extracting a dedicated fixture or helper for common `consumer` configurations would reduce duplication and make it clearer which behavior each test is targeting.

## Individual Comments

### Comment 1
<location path="tests/unit/services/test_listener.py" line_range="836-858" />
<code_context>
+# ---------------------------------------------------------------------------
+
+
+def test_on_message_api_delivery_success(api_consumer, loguru_logs):
+    api_consumer.osism_api_session.post.return_value = MagicMock(status_code=204)
+    data = _make_data()
+
+    api_consumer.on_message(_make_body(data), MagicMock())
+
+    api_consumer.osism_api_session.post.assert_called_once_with(
+        API_URL,
+        timeout=5,
+        json={
+            "priority": data["priority"],
+            "event_type": data["event_type"],
+            "timestamp": data["timestamp"],
+            "publisher_id": data["publisher_id"],
+            "message_id": data["message_id"],
+            "payload": data["payload"],
+        },
+    )
+    assert _has_log(loguru_logs, "INFO", "Successfully delivered notification")
+    api_consumer.baremetal_events.get_handler.assert_not_called()
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for non-baremetal events when an OSISM API session is configured

Currently API delivery tests only exercise baremetal notifications. Please add a case where `osism_api_session` is configured but the event type is non-baremetal (e.g. `compute.instance.update` or `identity.user.created`), asserting that no POST is sent and the event is still handled/logged as expected. This will help catch regressions where non-baremetal events might incorrectly start using the baremetal API endpoint.

```suggestion
# ---------------------------------------------------------------------------


def test_on_message_api_delivery_success(api_consumer, loguru_logs):
    api_consumer.osism_api_session.post.return_value = MagicMock(status_code=204)
    data = _make_data()

    api_consumer.on_message(_make_body(data), MagicMock())

    api_consumer.osism_api_session.post.assert_called_once_with(
        API_URL,
        timeout=5,
        json={
            "priority": data["priority"],
            "event_type": data["event_type"],
            "timestamp": data["timestamp"],
            "publisher_id": data["publisher_id"],
            "message_id": data["message_id"],
            "payload": data["payload"],
        },
    )
    assert _has_log(loguru_logs, "INFO", "Successfully delivered notification")
    api_consumer.baremetal_events.get_handler.assert_not_called()


def test_on_message_api_delivery_non_baremetal_event(api_consumer, loguru_logs):
    # Configure an OSISM API session but use a non-baremetal event type
    api_consumer.osism_api_session.post.return_value = MagicMock(status_code=204)
    data = _make_data()
    data["event_type"] = "compute.instance.update"

    api_consumer.on_message(_make_body(data), MagicMock())

    # Non-baremetal events must not be forwarded to the OSISM baremetal API
    api_consumer.osism_api_session.post.assert_not_called()

    # The event should still be processed via the normal baremetal_events dispatcher
    api_consumer.baremetal_events.get_handler.assert_called_once_with(data["event_type"])

    # And we still expect normal handling/logging (e.g. WebSocket forwarding)
    assert _has_log(loguru_logs, "INFO", "Forwarding event to WebSocket")
```
</issue_to_address>

### Comment 2
<location path="tests/unit/services/test_listener.py" line_range="163-172" />
<code_context>
+    api_consumer.baremetal_events.get_handler.assert_not_called()
+
+
+@pytest.mark.parametrize(
+    "exception",
+    [requests.ConnectionError, requests.Timeout],
+    ids=["connection", "timeout"],
+)
+def test_on_message_api_delivery_retries_and_gives_up(
+    api_consumer, sleep_mock, loguru_logs, exception
+):
+    api_consumer.osism_api_session.post.side_effect = exception
+    data = _make_data()
+
+    api_consumer.on_message(_make_body(data), MagicMock())
+
+    assert api_consumer.osism_api_session.post.call_count == 3
</code_context>
<issue_to_address>
**suggestion (testing):** Add a positive retry-path test where API delivery succeeds after a transient error

To exercise the transient-error case, mock `osism_api_session.post.side_effect` to raise `requests.ConnectionError` on the first call and return a 204 response on the second. Assert the exact number of `post` calls, that `sleep` is invoked once, and that the "Successfully delivered notification" log is emitted. This will more thoroughly validate the retry/backoff behaviour.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}