Skip to content

Add unit tests for hardware CLI commands#2486

Merged
ideaship merged 3 commits into
mainfrom
unit-tests-hardware-commands
Jul 23, 2026
Merged

Add unit tests for hardware CLI commands#2486
ideaship merged 3 commits into
mainfrom
unit-tests-hardware-commands

Conversation

@berendt

@berendt berendt commented Jul 18, 2026

Copy link
Copy Markdown
Member

Closes #2364

Adds unit tests for the hardware-related CLI command modules, following the mocking patterns established in #2193 (no Celery broker, no real cloud/NetBox access):

New test modules

  • tests/unit/commands/test_redfish.py: column name normalization, per-resource-type column mappings, column filtering (case-insensitive selection, unknown-column warning), JSON data filtering, table rendering, and all take_action output paths (JSON full/filtered/empty, table dispatch per resource type, unknown resource type, no result). Verifies the task lock is consulted before the Celery task is dispatched.
  • tests/unit/commands/test_stress.py: default command construction of the openstack-simple-stress wrapper, boolean flag and custom value propagation, return code passthrough, FileNotFoundError/generic error handling, setup failure guard, and cleanup in all paths.

Extended test modules

  • tests/unit/commands/test_baremetal.py: _apply_metalbox_vars, a parametrized cloud-setup failure guard over all commands of the module, BaremetalList (sorting, n/a power state, query flags, NetBox device-role column, cleanup on error), BaremetalDeploy (provision-state decision matrix, instance_info refresh, validation failure, NetBox local_context_data incl. cf_inventory_hostname fallback, netplan/FRR parameters, Supermicro boot device, RAID deploy steps, config-drive failure, --all loop resilience — playbook content is asserted by parsing the actual playbook.yml handed to configdrive.pack), BaremetalDump (Ironic and NetBox sources), BaremetalUndeploy (state matrix, SSH known_hosts cleanup), BaremetalPing (_ping_host output parsing incl. macOS/Linux variants, device discovery/filtering, summary), BaremetalBurnIn (clean/service step construction, manage flow, active-node confirmation), BaremetalClean (RAID step prepending, manage flow), the provide/maintenance/power wrappers, and BaremetalDelete (port-before-node ordering, NetBox state clearing on primary and secondary instances, failure resilience).
  • tests/unit/commands/test_server.py: ServerMigrate (defaults, --target/--force, prompt handling, wait loop, --no-wait, PAUSED), ServerList (domain/project/user happy paths, domain-name fallback, age-based BUILD/ERROR reporting), and ServerClean (build timeout handling, prompt handling, ERROR servers, cleanup in finally).

No existing test cases were duplicated; flake8 and black pass locally.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MRk7qVcmoKuk74USmgY8Mm

sourcery-ai[bot]

This comment was marked as off-topic.

@berendt
berendt force-pushed the unit-tests-hardware-commands branch from 3c17b73 to d1df374 Compare July 18, 2026 11:57
@berendt berendt moved this from New to In progress in Human Board Jul 18, 2026
@berendt berendt moved this from In progress to Ready for review in Human Board Jul 18, 2026
@berendt
berendt requested a review from ideaship July 18, 2026 12:14
Comment thread tests/unit/commands/test_baremetal.py Outdated

rc, _, _ = _run_dump(["node1"], nb=nb, metalbox_side_effect=RuntimeError("boom"))

assert rc is None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This asserts rc is None after the command's primary operation — playbook generation — fails inside a caught except (osism/commands/baremetal.py:564-566: log the error, then fall through with no return). cliff turns that None into exit code 0 (return_code = self.take_action(...) or 0), so a failed generation reports success — unsafe in set -e scripts and && chains.

This is the behavior the "Return non-zero on precondition and operational failures" change (Related-Bug: #2314) set out to eliminate; it fixed a subset and explicitly left the remaining baremetal operational/not-found failures open, and this is one of them. The same command already returns 1 for NetBox-unavailable and device-not-found (baremetal.py:574, 590), so the generation path exiting 0 is an internal inconsistency.

Please fix the production paths to return 1 on primary-operation failure — in their own commit — then assert exit 1 here. The same applies to the sibling assertions at lines 690, 1204, 1368, 1454, 1476, 1512, and 1667. For the batch commands (e.g. the --all delete at 1667), keep processing the remaining nodes but retain a failure flag and return non-zero at the end, so a partially-failed run doesn't report success — assert both the continuation and the final exit 1.

assert cmd._get_filtered_columns(mappings, ["bogus"]) == ([], [])


# --- _filter_json_data ---

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This checks only that the private helper returns ([], []); it never exercises what take_action does with that result. In the JSON branch (osism/commands/redfish.py:175) an empty data_keys flows into _filter_json_data(result, []), which treats an empty key list as "no filtering" (redfish.py:88) and returns the full records — so --format json --column bogus on a known resource type silently prints every field, while the table path warns and prints "No valid columns specified" (redfish.py:108). The two formats disagree, and the JSON behavior has no coverage.

Please add a take_action test for --format json with an all-invalid column selection that asserts the intended behavior. Aligning the JSON path with the table path is a production fix that belongs in its own commit.

@github-project-automation github-project-automation Bot moved this from Ready for review to In review in Human Board Jul 23, 2026
berendt added 3 commits July 23, 2026 10:41
The baremetal commands logged per-node operational failures - failed
validation, config drive builds, provision state changes, maintenance
and power state changes, node deletion and playbook generation - and
then fell through take_action without a return value, which cliff maps
to exit code 0. A failed operation therefore reported success, which is
unsafe in scripts that rely on the exit code.

Single-shot commands (dump, maintenance set/unset, power on/off) now
return 1 directly from their error handlers. The node-loop commands
(deploy, undeploy, burn-in, clean, provide, delete) keep processing the
remaining nodes but track the failure and return 1 at the end, so a
partially failed batch run no longer reports success. Nodes skipped
because they are not in a supported provision state are still treated
as warnings only.

Related-Bug: #2314
Assisted-by: Claude:claude-fable-5
Signed-off-by: Christian Berendt <berendt@osism.tech>
When a column selection matched no columns of a known resource type,
the JSON path passed the empty key list to _filter_json_data, which
treats an empty list as "no filtering" and printed every field of every
record, while the table path reported "No valid columns specified". The
JSON path now reports the same message instead of silently ignoring the
selection; the per-column warnings from _get_filtered_columns are
emitted in both formats.

Assisted-by: Claude:claude-fable-5
Signed-off-by: Christian Berendt <berendt@osism.tech>
Cover the hardware-related command modules with focused unit tests:

- baremetal: provision-state decision matrix and config-drive assembly
  of BaremetalDeploy, playbook generation of BaremetalDump (Ironic and
  NetBox sources), undeploy state handling including SSH known_hosts
  cleanup, ping output parsing and device discovery, burn-in and clean
  step construction, the provide/maintenance/power wrappers, NetBox
  state clearing on delete, and the shared cloud-setup failure guard
- server: live-migration flow including prompt handling and wait loop,
  the domain/project/user listing paths, age-based reporting of stuck
  servers, and ServerClean deletion behavior
- redfish: new test module for the column mapping and filtering helpers
  and the table/JSON output paths of the list command
- stress: new test module for command construction, flag propagation
  and error handling of the openstack-simple-stress wrapper

Assisted-by: Claude:claude-fable-5
Signed-off-by: Christian Berendt <berendt@osism.tech>
@berendt
berendt force-pushed the unit-tests-hardware-commands branch from d1df374 to 0f13f08 Compare July 23, 2026 08:56
@berendt
berendt requested a review from ideaship July 23, 2026 09:05
@ideaship
ideaship merged commit 610018a into main Jul 23, 2026
3 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in Human Board Jul 23, 2026
@ideaship
ideaship deleted the unit-tests-hardware-commands branch July 23, 2026 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Unit tests for osism/commands/ — hardware (baremetal, redfish, server, stress)

3 participants