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, test_check.py, test_compose.py, test_sync.py, test_log.py, test_console.py
• Extended (existing tests unchanged): test_validate.py, test_wait.py, test_get.py

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.

Misbehavior pinned as strict xfail

Per review, tests that would have pinned misbehavior green now assert the corrected behavior and are marked xfail(strict=True) — they turn red (XPASS) the moment the follow-up fix lands, forcing removal of the marker:

• compose mangles multi-token subcommands into unrunnable commands: osism compose <host> <env> up -d is sent as up-d ("".join instead of " ".join, compose.py:28).
• A successful collection segment silently skips the remaining // segments and exits 0, because handle_collection returns None instead of an exit code (apply.py:218, loop at apply.py:455).
• A failed loadbalancer child playbook does not propagate its rc — handle_loadbalancer_task returns only the parent rc and reads children solely for logging; the child rcs live in the task-output stream (handle_task/fetch_task_output), not in the Celery result (apply.py:106-133). Note: since the -ng role drop this handler is no longer reachable in production (see follow-ups).

Remaining follow-up fix candidates (pinned as-is)

• Since the -ng roles were dropped (ebde575), no _prepare_task path returns a Celery group anymore, so the GroupResult dispatch in handle_role (apply.py:388) and handle_loadbalancer_task are unreachable in production — candidates for removal together with their tests (including the strict xfail on child-rc propagation).
• handle_collection under --dry-run logs "No tasks are scheduled" (and the --dry-run help says "do not initiate tasks"), yet still dispatches the noop group via apply_async() (apply.py:235, :261).
• 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

https://claude.ai/code/session_01WgHtkSVmEuwwPxiEsSnXL7

[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.}

[ideaship]

This pins misbehavior that breaks any multi-token compose command: osism compose <host> <env> up -d is sent to the host as docker compose … up-d — an unrunnable command — and the test asserts that mangled up-d string green. Cause: compose.py:28 uses "".join(parsed_args.arguments) instead of " ".join(...). I checked the broken join isn't load-bearing: compose.Run has no internal callers, arguments feeds only the SSH string, and single-token subcommands (ps, pull, down) are identical under either join — so " ".join is a safe, strictly-better change. Since this is a test-only PR: assert up -d (red until the compose fix) or xfail(strict=True); fix "".join → " ".join in a follow-up.

[ideaship]

This pins misbehavior that silently drops deploy steps. osism apply lets you chain applies with // — it runs each //-separated segment in turn — so osism apply nutshell//openstackclient should apply the nutshell collection, then apply openstackclient. Instead the second segment is skipped entirely and the command still exits 0 — a partial deploy that scripts/CI read as success. The test asserts both rc is None and handle_role.assert_not_called() green. Cause: handle_collection (apply.py:218) has no return, so take_action (:462) gets rc = None, treats it as failure at :477, breaks the // loop before reaching openstackclient, and returns None (exit 0). I checked the None isn't relied on: handle_collection is called only at :462, and for a single, unchained collection the None→exit-0 is already today's result, so an int return only changes the chained-segment path — the bug itself. Since this is a test-only PR: assert the corrected behavior (collection returns 0, the next segment runs; red until fixed) or xfail(strict=True); give handle_collection an int return in a follow-up.

[ideaship]

This pins misbehavior that reports success when a load-balancer child playbook failed: a failed child leaves the command exiting 0, so scripted callers never see the failure (an operator watching the log still does — which is why it's stayed hidden). The test asserts rc == 0 and child.get.assert_not_called() green. Cause: handle_loadbalancer_task (apply.py:106-133) returns only the parent rc and reads children solely to log; and run_ansible_in_environment (tasks/__init__.py:411-419) returns the output string while recording the rc separately via finish_task_output(rc=...) — it never raises on rc≠0, so GroupResult.get() succeeds regardless. The mock is also unrepresentative: production never calls child.get(), so child.get.return_value = 99 models nothing — and child.get() can't supply the rc in any case, because the rc lives in the task-output stream, not in the Celery result. I checked nothing depends on discarding child rcs: the return flows only to take_action at :411, so propagating the worst child rc only changes the failure case. Since this is a test-only PR: assert that a failing child propagates to a non-zero return (red until fixed) or xfail(strict=True). Follow-up fix: read each child's recorded rc the same way the parent's is read — handle_task(child, ...) (i.e. fetch_task_output(child.id, ...)), not child.get() — and propagate the worst.

[ideaship]

Cleanup-only, not blocking: this test honestly documents a real contradiction — in dry_run, handle_collection logs "No tasks are scheduled" (apply.py:235) yet still calls t.apply_async() (apply.py:261) on the no-op group, dispatching a group that needs live workers and occupies the queue. It's not only the log: the --dry-run help (apply.py:90) also says "do not initiate tasks", so a wording-only fix would leave the help inaccurate too. Impact is mild (noop tasks do no deploy work). Follow-up: either skip apply_async() under dry_run, or make both the log and the --dry-run help say plainly that no-op tasks are scheduled.