From 2d308ef2c4650265aba0212a6f8b929d51d874e5 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Thu, 14 Dec 2023 16:26:52 -0600 Subject: [PATCH] test: Fix silent swallowing of unexpected subp error We throw an error when supb isn't mocked in a test. However, there are many broad exception handlers that wind up swallowing the error. This commit changes it from an AssertionError to a new error that inherits from BaseException so that it doesn't get silently swallowed. Also fix any tests that are impacted. --- conftest.py | 14 +++++++-- tests/unittests/analyze/test_boot.py | 17 ++++------ tests/unittests/sources/azure/test_errors.py | 33 ++++++++++++++++---- tests/unittests/sources/azure/test_kvp.py | 15 +++++++-- tests/unittests/test_conftest.py | 16 +++++++--- 5 files changed, 68 insertions(+), 27 deletions(-) diff --git a/conftest.py b/conftest.py index bcf5a670f1a..ca4743e2341 100644 --- a/conftest.py +++ b/conftest.py @@ -79,6 +79,14 @@ def closest_marker_first_arg_or(request, marker_name: str, default): return result[0] +class UnexpectedSubpError(BaseException): + """Error thrown when subp.subp is unexpectedly used. + + We inherit from BaseException so it doesn't get silently swallowed + by other error handlers. + """ + + @pytest.fixture(autouse=True) def disable_subp_usage(request, fixture_utils): """ @@ -142,12 +150,12 @@ def test_several_things(self): if allow_all_subp is None and allow_subp_for is None: # No marks, default behaviour; disallow all subp.subp usage def side_effect(args, *other_args, **kwargs): - raise AssertionError("Unexpectedly used subp.subp") + raise UnexpectedSubpError("Unexpectedly used subp.subp") elif allow_all_subp is not None and allow_subp_for is not None: # Both marks, ambiguous request; raise an exception on all subp usage def side_effect(args, *other_args, **kwargs): - raise AssertionError( + raise UnexpectedSubpError( "Test marked both allow_all_subp and allow_subp_for: resolve" " this either by modifying your test code, or by modifying" " disable_subp_usage to handle precedence." @@ -161,7 +169,7 @@ def side_effect(args, *other_args, **kwargs): def side_effect(args, *other_args, **kwargs): cmd = args[0] if cmd not in allow_subp_for: - raise AssertionError( + raise UnexpectedSubpError( "Unexpectedly used subp.subp to call {} (allowed:" " {})".format(cmd, ",".join(allow_subp_for)) ) diff --git a/tests/unittests/analyze/test_boot.py b/tests/unittests/analyze/test_boot.py index d54b9fac731..bd99d361e52 100644 --- a/tests/unittests/analyze/test_boot.py +++ b/tests/unittests/analyze/test_boot.py @@ -28,17 +28,12 @@ def test_subp_fails(self, m_subp): class TestSystemCtlReader: - @pytest.mark.parametrize( - "args", - [ - pytest.param(["dummyProperty"], id="invalid_property"), - pytest.param( - ["dummyProperty", "dummyParameter"], id="invalid_parameter" - ), - ], - ) - def test_systemctl_invalid(self, args): - reader = SystemctlReader(*args) + def test_systemctl_invalid(self, mocker): + mocker.patch( + "cloudinit.analyze.show.subp.subp", + return_value=("", "something_invalid"), + ) + reader = SystemctlReader("dont", "care") with pytest.raises(RuntimeError): reader.parse_epoch_as_float() diff --git a/tests/unittests/sources/azure/test_errors.py b/tests/unittests/sources/azure/test_errors.py index f310be72e32..03e53ca8b84 100644 --- a/tests/unittests/sources/azure/test_errors.py +++ b/tests/unittests/sources/azure/test_errors.py @@ -121,7 +121,10 @@ def test_reportable_errors( assert error.as_encoded_report() == "|".join(data) -def test_dhcp_lease(): +def test_dhcp_lease(mocker): + mocker.patch( + "cloudinit.sources.azure.identity.query_vm_id", return_value="foo" + ) error = errors.ReportableErrorDhcpLease(duration=5.6, interface="foo") assert error.reason == "failure to obtain DHCP lease" @@ -129,7 +132,10 @@ def test_dhcp_lease(): assert error.supporting_data["interface"] == "foo" -def test_dhcp_interface_not_found(): +def test_dhcp_interface_not_found(mocker): + mocker.patch( + "cloudinit.sources.azure.identity.query_vm_id", return_value="foo" + ) error = errors.ReportableErrorDhcpInterfaceNotFound(duration=5.6) assert error.reason == "failure to find DHCP interface" @@ -180,7 +186,10 @@ def test_dhcp_interface_not_found(): ), ], ) -def test_imds_url_error(exception, reason): +def test_imds_url_error(exception, reason, mocker): + mocker.patch( + "cloudinit.sources.azure.identity.query_vm_id", return_value="foo" + ) duration = 123.4 fake_url = "fake://url" @@ -195,7 +204,11 @@ def test_imds_url_error(exception, reason): assert error.supporting_data["url"] == fake_url -def test_imds_metadata_parsing_exception(): +def test_imds_metadata_parsing_exception(mocker): + mocker.patch( + "cloudinit.sources.azure.identity.query_vm_id", return_value="foo" + ) + exception = ValueError("foobar") error = errors.ReportableErrorImdsMetadataParsingException( @@ -206,7 +219,11 @@ def test_imds_metadata_parsing_exception(): assert error.supporting_data["exception"] == repr(exception) -def test_unhandled_exception(): +def test_unhandled_exception(mocker): + mocker.patch( + "cloudinit.sources.azure.identity.query_vm_id", return_value="foo" + ) + source_error = None try: raise ValueError("my value error") @@ -227,7 +244,11 @@ def test_unhandled_exception(): assert f"|{quoted_value}|" in error.as_encoded_report() -def test_imds_invalid_metadata(): +def test_imds_invalid_metadata(mocker): + mocker.patch( + "cloudinit.sources.azure.identity.query_vm_id", return_value="foo" + ) + key = "compute" value = "Running" error = errors.ReportableErrorImdsInvalidMetadata(key=key, value=value) diff --git a/tests/unittests/sources/azure/test_kvp.py b/tests/unittests/sources/azure/test_kvp.py index f0f4a999dba..ad6cd83d7aa 100644 --- a/tests/unittests/sources/azure/test_kvp.py +++ b/tests/unittests/sources/azure/test_kvp.py @@ -39,7 +39,10 @@ def telemetry_reporter(tmp_path): class TestReportFailureToHost: - def test_report_failure_to_host(self, caplog, telemetry_reporter): + def test_report_failure_to_host(self, caplog, telemetry_reporter, mocker): + mocker.patch( + "cloudinit.sources.azure.identity.query_vm_id", return_value="foo" + ) error = errors.ReportableError(reason="test") assert kvp.report_failure_to_host(error) is True assert ( @@ -52,7 +55,10 @@ def test_report_failure_to_host(self, caplog, telemetry_reporter): } assert report in list(telemetry_reporter._iterate_kvps(0)) - def test_report_skipped_without_telemetry(self, caplog): + def test_report_skipped_without_telemetry(self, caplog, mocker): + mocker.patch( + "cloudinit.sources.azure.identity.query_vm_id", return_value="foo" + ) error = errors.ReportableError(reason="test") assert kvp.report_failure_to_host(error) is False @@ -83,6 +89,9 @@ def test_report_success_to_host( } assert report in list(telemetry_reporter._iterate_kvps(0)) - def test_report_skipped_without_telemetry(self, caplog): + def test_report_skipped_without_telemetry(self, caplog, mocker): + mocker.patch( + "cloudinit.sources.azure.identity.query_vm_id", return_value="foo" + ) assert kvp.report_success_to_host() is False assert "KVP handler not enabled, skipping host report." in caplog.text diff --git a/tests/unittests/test_conftest.py b/tests/unittests/test_conftest.py index 68903430366..e9f7a43243d 100644 --- a/tests/unittests/test_conftest.py +++ b/tests/unittests/test_conftest.py @@ -1,6 +1,7 @@ import pytest from cloudinit import subp +from conftest import UnexpectedSubpError from tests.unittests.helpers import CiTestCase @@ -8,7 +9,7 @@ class TestDisableSubpUsage: """Test that the disable_subp_usage fixture behaves as expected.""" def test_using_subp_raises_assertion_error(self): - with pytest.raises(AssertionError): + with pytest.raises(UnexpectedSubpError): subp.subp(["some", "args"]) def test_typeerrors_on_incorrect_usage(self): @@ -17,6 +18,13 @@ def test_typeerrors_on_incorrect_usage(self): # pylint: disable=no-value-for-parameter subp.subp() + def test_subp_exception_escapes_exception_handling(self): + with pytest.raises(UnexpectedSubpError): + try: + subp.subp(["some", "args"]) + except Exception: + pytest.fail("Unexpected exception raised") + @pytest.mark.allow_all_subp def test_subp_usage_can_be_reenabled(self): subp.subp(["whoami"]) @@ -25,14 +33,14 @@ def test_subp_usage_can_be_reenabled(self): def test_subp_usage_can_be_conditionally_reenabled(self): # The two parameters test each potential invocation with a single # argument - with pytest.raises(AssertionError) as excinfo: + with pytest.raises(UnexpectedSubpError) as excinfo: subp.subp(["some", "args"]) assert "allowed: whoami" in str(excinfo.value) subp.subp(["whoami"]) @pytest.mark.allow_subp_for("whoami", "bash") def test_subp_usage_can_be_conditionally_reenabled_for_multiple_cmds(self): - with pytest.raises(AssertionError) as excinfo: + with pytest.raises(UnexpectedSubpError) as excinfo: subp.subp(["some", "args"]) assert "allowed: whoami,bash" in str(excinfo.value) subp.subp(["bash", "-c", "true"]) @@ -41,7 +49,7 @@ def test_subp_usage_can_be_conditionally_reenabled_for_multiple_cmds(self): @pytest.mark.allow_all_subp @pytest.mark.allow_subp_for("bash") def test_both_marks_raise_an_error(self): - with pytest.raises(AssertionError, match="marked both"): + with pytest.raises(UnexpectedSubpError, match="marked both"): subp.subp(["bash"])