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 4xx client errors, a strict xfail asserting that a 500 is retryable — the current <= 500 guard drops it as a client error, so the test turns red the moment the boundary is fixed to < 500 — 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 that pin behavior of currently dormant code paths (API delivery without OSISM_API_URL set, non-ironic events, malformed event types) carry comments noting that the path is dormant and the pinned behavior is not the intended end-state.
• 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.}

[ideaship]

Solid, mechanically clean test suite — good coverage of the discovery loop, the handler dispatch, and the delivery retries.

One structural note. Several tests characterize behavior on code paths that a standard deployment doesn't exercise, and a couple of those paths are themselves buggy — so the tests currently freeze the bug as the expected result. Two dormant paths are involved:

• API delivery (on_message's HTTP branch) only runs when OSISM_API_URL is set. In the default configuration it isn't, and delivery happens in-process instead.
• Non-ironic events only reach the listener if a service is configured to emit versioned notifications on a <service>_versioned_notifications topic. Only ironic does that by default, so nova/neutron/etc. events don't currently arrive at all.

Because of that, none of this blocks merging — it can't affect a running default deployment. The ask is narrower: don't pin buggy behavior as the spec.

The one case worth changing here is test_on_message_api_delivery_gives_up_early_on_http_error, which parametrizes 500 alongside 404. The guard is status_code <= 500, so a server 500 is classified as a client error and dropped without retry — inconsistent with the 503-retry test just below. A 500 is retryable. Suggest splitting the server case out and marking it xfail(strict=True) (see inline comment), so it turns red the moment the one-character fix lands rather than asserting the current behavior green.

The remaining characterization tests — short/invalid event types, missing event_type, non-baremetal events posting to the baremetal endpoint, a 200 not counting as success, and single-channel exchange discovery — are reasonable to keep. Consider rewording their "documents the current behavior" notes to say the path is currently dormant, so a future reader doesn't read them as intended end-state.

[ideaship]

Bundling 500 in with 404 here pins an off-by-one as the expected result. The early-exit guard is status_code <= 500 (osism/services/listener.py), so a server-side 500 is treated as a client error and the notification is dropped without retry — inconsistent with test_on_message_api_delivery_retries_on_server_error just below, where a 503 retries three times. A 500 is retryable.

Rather than asserting this green, suggest splitting the server case out and marking it xfail(strict=True) so it turns red the moment the guard is corrected (<= 500 -> < 500):

@pytest.mark.parametrize("status_code", [404])
def test_on_message_api_delivery_gives_up_early_on_http_error(
    api_consumer, sleep_mock, loguru_logs, status_code
):
    # A 4xx client error is not retried.
    response = MagicMock(status_code=status_code)
    response.raise_for_status.side_effect = requests.HTTPError(response=response)
    api_consumer.osism_api_session.post.return_value = response

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

    assert api_consumer.osism_api_session.post.call_count == 1
    sleep_mock.assert_not_called()
    assert _has_log(loguru_logs, "ERROR", "client side error, giving up early")


@pytest.mark.xfail(
    strict=True,
    reason="listener guards with status_code <= 500; a server 500 is retryable "
    "and the boundary should be < 500",
)
def test_on_message_api_delivery_retries_on_500(api_consumer, sleep_mock):
    # A 500 is a server error and should be retried like the 503 case.
    response = MagicMock(status_code=500)
    response.raise_for_status.side_effect = requests.HTTPError(response=response)
    api_consumer.osism_api_session.post.return_value = response

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

    assert api_consumer.osism_api_session.post.call_count == 3

The new test asserts the intended retry behavior, so it fails today and passes once the guard is fixed — at which point strict=True flags it for removal.