Add unit tests for workflow commands

[berendt]

Implements #2363: unit tests for the workflow-oriented CLI command modules under osism/commands/ (apply, check, compose, sync, get, log, console, validate, wait).

What's covered

• New test modules: test_apply.py (33 tests), test_check.py (44), test_compose.py (3), test_sync.py (27), test_log.py (16), test_console.py (27)
• Extended (existing tests unchanged): test_validate.py (+13), test_wait.py (+9), test_get.py (+14)

All test targets from the issue are implemented, including the pure helpers in check.py and the module-level host-resolution helpers in console.py that the issue flags for ≥95 % coverage. All Celery task calls, subprocesses, Docker/Netbox/Redis clients and interactive prompts are mocked; log assertions use a loguru sink (loguru_logs fixture) since caplog does not capture loguru records.

Current behavior pinned — follow-up fix candidates

The tests characterize the code as it is today; a few of the pinned behaviors look like latent bugs worth separate issues:

• apply.Run.handle_role logs task.task_id before the GroupResult isinstance check, but GroupResult has no task_id attribute — the real loadbalancer-ng path would raise AttributeError (apply.py:403).
• The collection branch of apply.Run.take_action breaks the retry loop with rc=None and returns None instead of an exit code (anticipated by the issue).
• wait --refresh is a no-op when task ids are passed on the CLI: do_refresh is only enabled when the id list comes from the inspector.
• log.File error logs use printf-style %s placeholders, which loguru does not interpolate — the placeholders appear literally in the log output.
• validate.Run._handle_task mixes t.id (fetch) and t.task_id (log/print), and its timeout message says "(sync inventory)".

flake8 and black --check pass locally; the test suite itself runs in CI per project workflow.

Closes #2363

🤖 Generated with Claude Code

[sourcery-ai]

Hey - I've found 1 issue

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="tests/unit/commands/test_apply.py" line_range="475-314" />
<code_context>
+    handle_task.assert_not_called()
+
+
+def test_handle_loadbalancer_task_wait_uses_parent_rc(mocker, loguru_logs):
+    handle_task = mocker.patch("osism.tasks.handle_task", return_value=2)
+    cmd = make_command(apply.Run)
+    t = MagicMock()
+    t.children = [MagicMock(task_id="child-1"), MagicMock(task_id="child-2")]
+
+    rc = cmd.handle_loadbalancer_task(t, True, "log", 300)
+
+    assert rc == 2
+    handle_task.assert_called_once_with(t.parent, True, "log", 300)
+    t.parent.get.assert_not_called()
+    t.get.assert_called_once_with()
+    messages = [r["message"] for r in loguru_logs]
+    assert any(
+        "Task child-1 (loadbalancer) is running in background" in m for m in messages
+    )
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for loadbalancer GroupResult failure scenarios and rc propagation

Current tests only cover successful `handle_task` returns and don’t assert behavior when children in the `GroupResult` fail or raise. Please add cases where a child errors and check the resulting rc and logging, so it’s clear whether failures in chained loadbalancer plays affect the user-visible result or are ignored.

Suggested implementation:

```python
def test_handle_loadbalancer_task_wait_uses_parent_rc(mocker, loguru_logs):
    handle_task = mocker.patch("osism.tasks.handle_task", return_value=2)
    cmd = make_command(apply.Run)
    t = MagicMock()
    t.children = [MagicMock(task_id="child-1"), MagicMock(task_id="child-2")]

    rc = cmd.handle_loadbalancer_task(t, True, "log", 300)

    assert rc == 2
    handle_task.assert_called_once_with(t.parent, True, "log", 300)
    t.parent.get.assert_not_called()
    t.get.assert_called_once_with()
    messages = [r["message"] for r in loguru_logs]
    assert any(
        "Task child-1 (loadbalancer) is running in background" in m for m in messages
    )


def test_handle_loadbalancer_task_child_error_does_not_change_rc(mocker, loguru_logs):
    handle_task = mocker.patch("osism.tasks.handle_task", return_value=2)
    cmd = make_command(apply.Run)

    t = MagicMock()
    child_1 = MagicMock(task_id="child-1")
    child_2 = MagicMock(task_id="child-2")
    t.children = [child_1, child_2]

    # Simulate failure when retrieving the child result
    child_1.get.side_effect = Exception("boom")
    child_2.get.return_value = None

    rc = cmd.handle_loadbalancer_task(t, True, "log", 300)

    # The parent rc is still the user-visible result
    assert rc == 2
    handle_task.assert_called_once_with(t.parent, True, "log", 300)

    # Parent task is not re-fetched, only children are consulted
    t.parent.get.assert_not_called()
    t.get.assert_called_once_with()
    child_1.get.assert_called_once_with()
    child_2.get.assert_called_once_with()

    messages = [r["message"] for r in loguru_logs]
    # Child failure is logged so it is visible to the user
    assert any(
        "Task child-1 (loadbalancer) failed" in m for m in messages
    )


def test_handle_loadbalancer_task_child_nonzero_rc_is_logged_but_ignored(
    mocker, loguru_logs
):
    handle_task = mocker.patch("osism.tasks.handle_task", return_value=2)
    cmd = make_command(apply.Run)

    t = MagicMock()
    child_1 = MagicMock(task_id="child-1")
    child_2 = MagicMock(task_id="child-2")
    t.children = [child_1, child_2]

    # Simulate a child play finishing with a non-zero rc
    child_1.get.return_value = 99
    child_2.get.return_value = 0

    rc = cmd.handle_loadbalancer_task(t, True, "log", 300)

    # The parent rc remains the user-visible result even if a child fails
    assert rc == 2
    handle_task.assert_called_once_with(t.parent, True, "log", 300)

    t.parent.get.assert_not_called()
    t.get.assert_called_once_with()
    child_1.get.assert_called_once_with()
    child_2.get.assert_called_once_with()

    messages = [r["message"] for r in loguru_logs]
    # Make sure the failure is clearly logged with the child rc
    assert any(
        "Task child-1 (loadbalancer) finished with rc=99" in m for m in messages
    )

```

To make these tests pass, `handle_loadbalancer_task` should:
1. Iterate over `t.children` and call `child.get()` for each child in the `GroupResult`.
2. Catch exceptions raised by `child.get()` and log a message containing `"Task {child.task_id} (loadbalancer) failed"`.
3. When `child.get()` returns a non-zero rc, log a message containing `"Task {child.task_id} (loadbalancer) finished with rc={rc}"`.
4. Always return the rc from `handle_task(t.parent, ...)` (the parent result) as the effective rc so the tests asserting rc==2 succeed.
If the current implementation behaves differently, it should be adjusted to match these semantics or the expected log messages can be adapted to the actual wording used in the logger.
</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.}