Keep check messages brief and move details into results

tests/checks/apic_database_size_check/test_apic_database_size_check.py

@@ -386,7 +386,7 @@ def test_missing_mitmocounters_returns_error(
     result = run_check(cversion=script.AciVersion("6.0(8f)"))
 
     assert result.result == script.ERROR
-    assert result.msg == "Unable to collect APIC database object counters"
+    assert result.msg == ""
     assert result.headers == ["APIC ID", "DME", "Collection Error"]
     assert len(result.data) == 4
     assert all(row[2] == expected_error for row in result.data)

tests/checks/apic_oob_connectivity_check/test_apic_oob_connectivity_check.py

@@ -19,7 +19,7 @@
 podProfiles = 'fabricPodP.json?rsp-subtree=full'
 defaultHttps = 'uni/fabric/comm-default/https.json'
 
-# headers returned only when the check reaches its final return (early MANUAL/NA/ERROR returns have no headers)
+# Policy errors and final results share the existing connectivity columns.
 HEADERS = ["Source APIC", "Destination APIC", "OOB IP", "Port", "Status", "Command"]
 
 DEFAULT_POLICY_OUTPUTS = {
@@ -236,7 +236,7 @@ def custom_policy_outputs(port="8443"):
             [],
             script.ERROR,
             HEADERS,
-            [],
+            [["-", "-", "-", "ERR"]],
         ),
         # IPv6 OOB local probes pass, with manual mesh commands required.
         (
@@ -312,6 +312,19 @@ def mock_subprocess_call(cmd, stderr=None):
     probe_rows = [row for row in result.data if row[4] != "Manual check required"]
     assert probe_rows == [["Executing APIC"] + row + ["-"] for row in expected_data]
     assert idx[0] == len(curl_exit_codes)
+    if expected_data == [["-", "-", "-", "ERR"]]:
+        assert result.msg == ""
+        assert "Verify the Management Access Policy and HTTPS port settings" in result.recommended_action
+        aci_result = script.AciResult(test_function, "APIC OOB Connectivity check", result)
+        assert aci_result.failureDetails["data"] == [dict(zip(HEADERS, probe_rows[0]))]
+        assert aci_result.recommended_action == result.recommended_action
+        output = []
+        monkeypatch.setattr(script, "prints", output.append)
+        script.print_result(116, 117, "APIC OOB Connectivity check", **result.as_dict())
+        status_line, table_and_action = output[0].split("\n", 1)
+        assert "ERR" in table_and_action
+        assert result.recommended_action in table_and_action
+        assert "curl --max-time" not in output[0]
 
 
 @pytest.mark.parametrize("inventory, ips, port", [
@@ -367,6 +380,11 @@ def probe(*args, **kwargs):
     assert result.headers == HEADERS
     assert result.data == expected_probes + expected_commands
     assert "Run each command on its Source APIC" in result.recommended_action
+    if expected_result == script.MANUAL:
+        assert result.recommended_action.startswith("Check OOB management connectivity")
+        assert "Restore" not in result.recommended_action
+    elif expected_result == script.FAIL_UF:
+        assert result.recommended_action.startswith("Restore OOB management connectivity")
 
     aci_result = script.AciResult(test_function, "APIC OOB Connectivity check", result)
     assert aci_result.failureDetails["header"] == HEADERS

tests/checks/apic_upgrade_mixed_version_check/test_apic_upgrade_mixed_version_check.py

@@ -61,7 +61,8 @@ def test_mismatch_fails_at_every_target(run_check, target, message):
              fabric_node(201, 'spine', 'n9000-16.1(4h)')]
     result = run_mixed_check(run_check, nodes, target=target)
     assert result.result == script.FAIL_UF
-    assert message in result.msg
+    assert result.msg == ""
+    assert message in result.recommended_action
     assert result.data == [
         ['1', '101', 'leaf101', 'leaf', 'n9000-15.2(8f)', '6.1(4h)',
          'Switch differs from APIC']]
@@ -77,7 +78,8 @@ def test_every_mismatched_node_is_reported(run_check):
     assert result.result == script.FAIL_UF
     assert [row[1] for row in result.data] == ['101', '102', '202']
     assert result.data[1][6] == 'Switch newer than APIC'
-    assert 'newer than the APIC' in result.msg
+    assert result.msg == ''
+    assert 'newer than the APIC' in result.recommended_action
 
 
 def test_apic_versions_must_agree(run_check):
@@ -86,7 +88,8 @@ def test_apic_versions_must_agree(run_check):
              fabric_node(101, 'leaf', 'n9000-16.1(4h)')]
     result = run_mixed_check(run_check, nodes)
     assert result.result == script.FAIL_UF
-    assert 'APICs' in result.msg
+    assert result.msg == ''
+    assert 'APIC cluster upgrade' in result.recommended_action
     assert result.data[0][1] == '1'
 
 

tests/checks/atomic_counter_check/test_atomic_counter_check.py

@@ -32,6 +32,7 @@ def count_response(count):
         ('6.1(1a)', None, None, 0, 1, script.MANUAL, ['Configuration rollback review']),
         ('6.1(1a)', None, None, 1601, 1, script.FAIL_UF, ['TEP-to-TEP scalability', 'Configuration rollback review']),
         (None, None, None, 0, 0, script.MANUAL, []),
+        (None, None, None, 1601, 1, script.FAIL_UF, ['TEP-to-TEP scalability', 'Configuration rollback review']),
     ],
 )
 def test_atomic_counter_order_and_results(
@@ -51,6 +52,7 @@ def test_atomic_counter_order_and_results(
     assert result.result == expected_result
     assert [row[0] for row in result.data] == expected_concerns
     if expected_concerns and expected_concerns[0].startswith('Deprecated'):
-        assert 'Cleanup is mandatory' in result.msg
+        assert result.msg == ''
+        assert result.recommended_action == 'Remove configured atomic counter policies before upgrading.'
     if tversion is None:
-        assert 'deprecation could not be assessed' in result.msg
+        assert result.msg == 'Tver not supplied, deprecation not assessed'

tests/checks/bgp_timer_policy_already_existing_check/test_bgp_timer_policy_already_existing_check.py

@@ -34,7 +34,7 @@
                     "configQual:bgpProt-policy-already-existing, configSt:failed-to-apply, temporaryError:no",
                 ],
             ],
-            "Clear the fault code F0467 for bgp timer policy",
+            "",
         ),
         # target release beyond affected range on 6.1 train with existing fault: manual clearance required
         (
@@ -56,7 +56,7 @@
                     "configQual:bgpProt-policy-already-existing, configSt:failed-to-apply, temporaryError:no",
                 ],
             ],
-            "Clear the fault code F0467 for bgp timer policy",
+            "",
         ),
         # boundary version is still affected for strict newer_than check
         (
@@ -144,7 +144,7 @@
                     "configQual:bgpProt-policy-already-existing, configSt:failed-to-apply, temporaryError:no",
                 ],
             ],
-            "Clear the fault code F0467 for bgp timer policy",
+            "",
         ),
         (
             {faultDelegates: read_data(dir, "faultDelegate_NEG.json")},
@@ -170,4 +170,4 @@ def test_logic(run_check, mock_icurl, tversion, cversion, expected_result, expec
     assert result.result == expected_result
     assert result.data == expected_data
     if expected_msg is not None:
-        assert result.msg == expected_msg
+        assert result.msg == expected_msg

tests/checks/fx3_breakout_port_check/test_fx3_breakout_port_check.py

@@ -25,7 +25,7 @@
 # ethpmFcot/l1PhysIf fixtures control which of these ports actually get flagged.
 ALL_BRKOUT_PORTS = read_data(dir, "eqptBrkoutP_all_breakout_ports.json")
 
-FAIL_MSG = "Affected breakout transceivers with FEC not disabled found. This may cause an outage during the leaf upgrade."
+FAIL_MSG = ""
 
 
 def l1physif_query(legs):
@@ -126,26 +126,26 @@ def l1physif_entries(batch):
             "5.2(8g)", "6.2(1g)",
             FX3_NODES,
             {BRKOUT_QUERY: read_data(dir, "eqptBrkoutP_none.json")},
-            script.PASS, "No breakout configuration found on ports 49-52 of YC-FX3/TC-FX3 switches.", [],
+            script.PASS, "No applicable breakout ports.", [],
         ),
         # Affected versions, no YC-FX3/TC-FX3 switches in the fabric
         (
             "5.2(8g)", "5.3(2a)", NON_FX3_NODES, {},
-            script.NA, "No YC-FX3/TC-FX3 switches found. Skipping.", [],
+            script.NA, "No applicable switches.", [],
         ),
         # Affected versions, FX3 switches present, but no breakout config on ports 49-52
         (
             "5.2(8g)", "5.3(2a)",
             FX3_NODES,
             {BRKOUT_QUERY: read_data(dir, "eqptBrkoutP_none.json")},
-            script.PASS, "No breakout configuration found on ports 49-52 of YC-FX3/TC-FX3 switches.", [],
+            script.PASS, "No applicable breakout ports.", [],
         ),
         # Breakout configured, but not on ports 49-52
         (
             "5.2(8g)", "5.3(2a)",
             FX3_NODES,
             {BRKOUT_QUERY: read_data(dir, "eqptBrkoutP_port_not_in_range.json")},
-            script.PASS, "No breakout configuration found on ports 49-52 of YC-FX3/TC-FX3 switches.", [],
+            script.PASS, "No applicable breakout ports.", [],
         ),
         # Breakout on port 49, but no matching Innolight SR4 transceiver found
         (
@@ -155,7 +155,7 @@ def l1physif_entries(batch):
                 BRKOUT_QUERY: ALL_BRKOUT_PORTS,
                 FCOT_QUERY: read_data(dir, "ethpmFcot_none.json"),
             },
-            script.PASS, "No affected breakout transceivers found on ports 49-52 of YC-FX3/TC-FX3 switches.", [],
+            script.PASS, "No affected transceivers.", [],
         ),
         # Matching transceiver on brkoutport-1, but FEC is already disabled
         (

tests/checks/n9k_c93180yc_fx3_switch_memory_check/test_n9k_c93180yc_fx3_switch_memory_check.py

@@ -40,15 +40,15 @@ def make_fx3_nodes(count):
             [],
             {},
             script.NA,
-            'No N9K-C93180YC-FX3 switches found. Skipping.',
+            'No applicable switches.',
             [],
         ),
         # Non-N9K-C93180YC-FX3 node (N9K-C9508)
         (
             read_data(dir, "fabricNode_N9K-C9508.json"),
             {},
             script.NA,
-            'No N9K-C93180YC-FX3 switches found. Skipping.',
+            'No applicable switches.',
             [],
         ),
         # N9K-C93180YC-FX3 node with >=32GB memory - API returns empty (filtered by lt)
@@ -98,11 +98,7 @@ def make_fx3_nodes(count):
                 proc_mem_query: read_data(dir, "procMemUsage_lt32gb.json"),
             },
             script.FAIL_O,
-            (
-                'N9K-C93180YC-FX3 requires a minimum of 32GB RAM for proper operation in ACI mode. '
-                'One or more switches with less than 32GB of memory may experience service instability. '
-                'Upgrade the switch memory to at least 32GB.'
-            ),
+            '',
             [["101", "leaf101", "N9K-C93180YC-FX3", "16.0"]],
         ),
     ],
@@ -116,4 +112,4 @@ def test_logic(run_check, mock_icurl, fabric_nodes, expected_result, expected_ms
     if result.data:
         assert result.data == expected_data
     else:
-        assert result.unformatted_data == expected_data
+        assert result.unformatted_data == expected_data

tests/checks/nxos_ipn_multicast_rpf_defect_check/test_nxos_ipn_multicast_rpf_defect_check.py

@@ -160,7 +160,7 @@ def test_no_ospf_adj_is_inconclusive(run_check, mock_icurl, fabric_nodes, icurl_
     result = run_check(fabric_nodes=fabric_nodes)
 
     assert result.result == script.MANUAL
-    assert "No spine overlay OSPF adjacency" in result.msg
+    assert result.msg == "No overlay OSPF adjacencies found."
 
 
 def test_single_pod_is_not_applicable(run_check, mock_icurl, icurl_outputs):

tests/checks/pg_and_shared_svc_contract_check/test_pg_and_shared_svc_contract_check.py

@@ -687,9 +687,7 @@ def test_missing_provider_context_is_an_error(run_check, mock_icurl):
     )
 
     assert result.result == script.ERROR
-    assert result.msg == (
-        "Unable to resolve context for one or more derived contract relationships"
-    )
+    assert result.msg == ""
     assert result.data == [[
         (
             "cdef-[uni/tn-common/brc-AD_C]/"
@@ -726,9 +724,7 @@ def test_missing_consumer_context_is_an_error(run_check, mock_icurl):
     )
 
     assert result.result == script.ERROR
-    assert result.msg == (
-        "Unable to resolve context for one or more derived contract relationships"
-    )
+    assert result.msg == ""
     assert result.data == [[
         (
             "cdef-[uni/tn-common/brc-AD_C]/"

tests/checks/test_result_details.py

@@ -0,0 +1,138 @@
+"""Check result data and recommended actions in console and JSON output."""
+import importlib
+import json
+import os
+
+import pytest
+
+
+script = importlib.import_module('aci-preupgrade-validation-script')
+
+
+def run_check(name, **kwargs):
+    return getattr(script, name)(finalize_check=lambda *_: None, **kwargs)
+
+
+def assert_outputs(monkeypatch, name, result, detail):
+    titles = []
+    getattr(script, name)(initialize_check=lambda _, title: titles.append(title))
+    output = []
+    monkeypatch.setattr(script, 'prints', output.append)
+    script.print_result(117, 117, titles[0], **result.as_dict())
+    status_line, table_and_action = output[0].split('\n', 1)
+    assert status_line.endswith(' ' + result.result)
+    assert detail not in status_line
+    assert detail in table_and_action
+    assert result.recommended_action in table_and_action
+
+    payload = json.loads(json.dumps(script.AciResult(name, titles[0], result).as_dict()))
+    assert payload['recommended_action'] == result.recommended_action
+    assert payload['failureDetails']['header'] == result.headers
+    assert payload['failureDetails']['data'] == [dict(zip(result.headers, row)) for row in result.data]
+
+
+@pytest.mark.parametrize('failing_class', ['svccoreCtrlr', 'svccoreNode'])
+def test_svccore_collection_error_uses_existing_table(monkeypatch, failing_class):
+    detail = 'APIC query timed out while collecting core counts'
+
+    def icurl(_, query):
+        if query.startswith(failing_class + '.'):
+            raise RuntimeError(detail)
+        return [{'moCount': {'attributes': {'count': '0'}}}]
+
+    monkeypatch.setattr(script, 'icurl', icurl)
+    name = 'svccore_excessive_data_check'
+    result = run_check(name)
+    assert result.result == script.ERROR
+    assert result.msg == ''
+    assert result.headers == ['Class Name', 'Count']
+    assert result.data == [['svccoreCtrlr/svccoreNode', 'ERR']]
+    assert detail in result.recommended_action
+    assert_outputs(monkeypatch, name, result, detail)
+
+
+@pytest.mark.parametrize('failing_class', ['vnsGraphInst', 'fvCtx', 'vnsEPgDef'])
+def test_service_graph_query_errors_keep_details_in_action(monkeypatch, failing_class):
+    fixture_dir = os.path.join(os.path.dirname(__file__), 'vzany_svcgraph_stretched_vrf_check')
+    fixtures = {
+        'vnsGraphInst': 'vnsGraphInst_with_consumer.json',
+        'fvCtx': 'fvCtx_stretched_vrf.json',
+        'vzRsAnyToCons': 'vzRsAnyToCons_consumer.json',
+    }
+    detail = 'API collection failed: ' + 'long diagnostic text ' * 12
+
+    def icurl(_, query):
+        mo_class = query.split('.')[0]
+        if mo_class == failing_class:
+            raise RuntimeError(detail)
+        if mo_class == 'vzRsAnyToProv':
+            return []
+        with open(os.path.join(fixture_dir, fixtures[mo_class])) as fixture_file:
+            return json.load(fixture_file)
+
+    monkeypatch.setattr(script, 'icurl', icurl)
+    name = 'vzany_svcgraph_stretched_vrf_check'
+    result = run_check(name, cversion=script.AciVersion('6.0(1a)'), tversion=script.AciVersion('6.1(4a)'))
+    assert result.result == script.ERROR
+    assert result.msg == ''
+    assert result.headers == ['Tenant', 'VRF', 'Contract', 'Graph', 'Issue']
+    assert result.data == [['-', '-', '-', '-', 'ERR']]
+    assert detail in result.recommended_action
+    assert_outputs(monkeypatch, name, result, detail)
+
+
+def test_openssl_failure_uses_certificate_response_column(monkeypatch, tmpdir):
+    monkeypatch.chdir(tmpdir)
+    monkeypatch.setattr(script, 'icurl', lambda *_: [
+        {'pkiFabricSelfCAEp': {'attributes': {'currCertReqPassphrase': 'test-passphrase'}}}
+    ])
+    class FailedProcess(object):
+        returncode = 7
+
+        def communicate(self):
+            return b'error', None
+
+    monkeypatch.setattr(script.subprocess, 'Popen', lambda *args, **kwargs: FailedProcess())
+    name = 'apic_ca_cert_validation'
+    result = run_check(name)
+    assert result.result == script.ERROR
+    assert result.msg == ''
+    assert result.headers == ['Certreq Response']
+    assert result.data == [['ERR']]
+    assert str(FailedProcess.returncode) in result.recommended_action
+    assert_outputs(monkeypatch, name, result, 'OpenSSL command failed (exit code 7).')
+
+
+IMAGE_32 = 'aci-n9000-dk9.16.0.2h.bin'
+IMAGE_64 = 'aci-n9000-dk9.16.0.2h-cs_64.bin'
+OLD_IMAGE = 'aci-n9000-dk9.15.2.8h.bin'
+
+
+@pytest.mark.parametrize('current, target, available, missing', [
+    ('6.0(3a)', '5.2(8h)', [], [['Target', '32-bit', OLD_IMAGE, 'Missing']]),
+    ('6.0(3a)', '6.0(2h)', [], [
+        ['Target', '32-bit', IMAGE_32, 'Missing'], ['Target', '64-bit', IMAGE_64, 'Missing']]),
+    ('6.0(3a)', '6.0(2h)', [IMAGE_32], [['Target', '64-bit', IMAGE_64, 'Missing']]),
+    ('6.0(3a)', '6.0(2h)', [IMAGE_64], [['Target', '32-bit', IMAGE_32, 'Missing']]),
+    ('5.2(8h)', '6.0(2h)', [IMAGE_32, IMAGE_64], [['Current', '32-bit', OLD_IMAGE, 'Missing']]),
+])
+def test_missing_images_are_results_with_repository_action(monkeypatch, current, target, available, missing):
+    def icurl(_, query):
+        if query.startswith('eqptcapacityFSPartition.'):
+            return [{'eqptcapacityFSPartition': {'attributes': {
+                'dn': 'topology/pod-1/node-101/sys/eqptcapacity/fspartition-bootflash', 'avail': '1'}}}]
+        if query.startswith('maintUpgJob.'):
+            return []
+        assert query.startswith('firmwareFirmware.')
+        return [{'firmwareFirmware': {'attributes': {'isoname': name, 'size': '2000000000'}}}
+                for name in available]
+
+    monkeypatch.setattr(script, 'icurl', icurl)
+    name = 'switch_bootflash_usage_check'
+    result = run_check(name, sw_cversion=script.AciVersion(current), tversion=script.AciVersion(target))
+    assert result.result == script.MANUAL
+    assert result.msg == ''
+    assert result.headers == ['Image Role', 'Architecture', 'Filename', 'Status']
+    assert result.data == missing
+    assert result.recommended_action
+    assert_outputs(monkeypatch, name, result, missing[0][2])