Add unit tests for netbox and sonic command modules - #2487
Conversation
| cmd._reload_configuration.return_value = False | ||
| parsed_args = cmd.get_parser("test").parse_args(["sw1"]) | ||
|
|
||
| assert cmd.take_action(parsed_args) == 0 |
There was a problem hiding this comment.
This asserts take_action() == 0 when _reload_configuration() returned False. That locks in a real bug in Reload.take_action (osism/commands/sonic.py:654-677): on reload failure it logs "Skipping config save due to reload failure", skips the save, then logs "SONiC configuration reload completed successfully" and returns 0 — so automation gets a success exit code after the device-side sudo config reload -y actually failed. Note the asymmetry: the _load_configuration failure just above returns 1, so load-failure exits 1 but reload-failure exits 0, on the very operation the command is named for.
Please make production return 1 on reload failure and change this assertion to == 1 (keep _save_configuration.assert_not_called() and the log-message check). The production change should be a separate commit from the test change — mirroring how the stage-1 SSL reorder shipped on its own — but it must land in this PR, since this test must not merge asserting == 0.
| ("401 Client Error", "Error: Auth failed"), | ||
| ("Unauthorized", "Error: Auth failed"), | ||
| ("connection refused", "Error: Connection refused"), | ||
| ("certificate verify failed", "Error: SSL error"), |
There was a problem hiding this comment.
These SSL cases use clean strings ("certificate verify failed", "SSL handshake failed") that contain no "connection" substring, so they never exercise the branch ordering in _check_netbox_instance (osism/commands/netbox.py:201-204), which tests "connection" before "ssl"/"certificate". A realistic requests SSL failure surfaced through nb.status() stringifies as HTTPSConnectionPool(host='...', port=443): Max retries exceeded ... (Caused by SSLError(...)) — that contains "connection", so it would be misclassified as "Error: Connection refused". This is the same SSL-before-connection priority the stage-1 reachability check just fixed, still latent here in the string-based classifier used for secondary instances.
Please add a case like ("HTTPSConnectionPool(host='nb', port=443): Max retries exceeded (Caused by SSLError(...))", "Error: SSL error"). It will fail against the current ordering. The production ordering fix — moving the "ssl"/"certificate" check above the "connection" check — is being tracked as a separate PR, so either land both together (production fix in its own commit) or keep this case pending on that fix.
In Sync._check_netbox_connectivity the SSLError handler was unreachable: requests.exceptions.SSLError is a subclass of ConnectionError, so SSL failures in the reachability stage were reported as "Error: Connection refused". Catch SSLError first so certificate problems are reported as "Error: SSL error", matching the classification of the authentication stage. Assisted-by: Claude:claude-fable-5 Signed-off-by: Christian Berendt <berendt@osism.tech>
The string-based error classifiers in Sync._check_netbox_instance and in the authentication stage of Sync._check_netbox_connectivity tested for "connection" before "ssl"/"certificate". An SSL failure raised by requests through nb.status() stringifies as "HTTPSConnectionPool(host='...', port=443): Max retries exceeded ... (Caused by SSLError(...))", which contains "connection", so certificate problems were reported as "Error: Connection refused". Check for SSL markers first so they are reported as "Error: SSL error", matching the classification of the reachability stage. Assisted-by: Claude:claude-fable-5 Signed-off-by: Christian Berendt <berendt@osism.tech>
Reload.take_action returned 0 when "sudo config reload -y" failed on the device: it skipped the config save, logged a success summary, and exited cleanly, so callers saw a success exit code although the operation the command is named for had failed. A load failure in the same flow already exits 1. Return 1 on reload failure instead, keeping the warning that the config save is skipped. Assisted-by: Claude:claude-fable-5 Signed-off-by: Christian Berendt <berendt@osism.tech>
Cover the network-facing CLI command modules with unit tests: - tests/unit/commands/test_netbox.py: extend the existing exit-code tests with the Sync error-classification helpers (_check_netbox_instance, _check_netbox_connectivity), table building (_build_netbox_table), the --list/--check/--no-wait/--filter paths of Sync.take_action, argument assembly in Ironic and Manage, Versions, the nbcli bootstrap and argument quoting in Console, and the device lookup fallback chain and report formatting in Dump. - tests/unit/commands/test_sonic.py: SonicCommandBase helpers (device lookup, config context filtering, SSH connection details, backup filename probing, the exec-command helpers) and the take_action control flow of Load, Backup, Ztp, Reload, Reboot, Reset, Show, Console, and Dump. - tests/unit/commands/test_sonic_validate.py: source collection and config acquisition of Validate (_collect_sources, _configs_from_export_dir, _config_from_netbox, _config_from_generate), its exit-code contract and report formatting, and the provision-state derivation and sorting of List. Assisted-by: Claude:claude-fable-5 Signed-off-by: Christian Berendt <berendt@osism.tech>
4d745dd to
20ee355
Compare
Summary
Implements #2366: unit tests for the two network-facing CLI command modules
osism/commands/netbox.pyandosism/commands/sonic.py.tests/unit/commands/test_netbox.py(extended):Sync._check_netbox_instance/_check_netbox_connectivityerror classification (timeout, auth, connection refused, SSL, truncation, missinghttp_session),_build_netbox_table(primary/secondary rows,getattrfallbacks, connectivity column), the--list/--check/--no-wait/--filterpaths ofSync.take_action, argument assembly inIronicandManage,Versions, the nbcli bootstrap and argument quoting inConsole, and the device lookup fallback chain, YAML custom-field formatting and field filter ofDump.tests/unit/commands/test_sonic.py(new):SonicCommandBasehelpers (_get_device_from_netbox,_get_config_context,_save_config_context,_get_ssh_connection_details,_generate_backup_filename, the six exec-command helpers,_cleanup_temp_file,_get_ztp_status) and thetake_actioncontrol flow ofLoad,Backup,Ztp,Reload,Reboot,Reset,Show,Console, andDump(step ordering, per-step failure exit codes, paramiko exception handling,ssh.close()infinally).tests/unit/commands/test_sonic_validate.py(new):Validate._collect_sources(all four sources, hostname requirements),_configs_from_export_dir(prefix/suffix matching, hostname filter, invalid JSON),_config_from_netbox,_config_from_generate(HWSKU validation,config_version), thetake_actionexit-code contract (0/1/2, JSON payload),_print_text_reportformatting, andList(provision-state derivation, sorting, N/A fallbacks, task failure).No duplication of the existing coverage in
test_sonic_ssh.py(--refresh-host-keywiring,_create_ssh_connection) or of the pre-existing exit-code tests.Bug fix
Writing the tests surfaced that the
except requests.exceptions.SSLErrorbranch inSync._check_netbox_connectivitywas unreachable:SSLErroris a subclass ofConnectionErrorin requests, so SSL failures in the reachability stage were reported asError: Connection refused. The first commit reorders the except clauses so certificate problems are reported asError: SSL error, as the issue's expected behavior describes.Closes #2366