tests: use markers to configure disable_subp_usage (#473)

This is an improvement over indirect parameterisation for a few reasons:

* The test code is much easier to read, the mark names are much more
  intuitive than the indirect parameterisation invocation, and there's
  less boilerplate to boot
* The fixture no longer has to overload the single parameter that
  fixtures can take with multiple meanings

5 files changed, 76 insertions, 43 deletions

diff --git a/cloudinit/tests/test_conftest.py b/cloudinit/tests/test_conftest.py
index a6537248..6f1263a5 100644
--- a/cloudinit/tests/test_conftest.py
+++ b/cloudinit/tests/test_conftest.py
@@ -17,12 +17,11 @@ class TestDisableSubpUsage:
             #  pylint: disable=no-value-for-parameter
             subp.subp()
 
-    @pytest.mark.parametrize('disable_subp_usage', [False], indirect=True)
+    @pytest.mark.allow_all_subp
     def test_subp_usage_can_be_reenabled(self):
         subp.subp(['whoami'])
 
-    @pytest.mark.parametrize(
-        'disable_subp_usage', [['whoami'], 'whoami'], indirect=True)
+    @pytest.mark.allow_subp_for("whoami")
     def test_subp_usage_can_be_conditionally_reenabled(self):
         # The two parameters test each potential invocation with a single
         # argument
@@ -31,8 +30,7 @@ class TestDisableSubpUsage:
         assert "allowed: whoami" in str(excinfo.value)
         subp.subp(['whoami'])
 
-    @pytest.mark.parametrize(
-        'disable_subp_usage', [['whoami', 'bash']], indirect=True)
+    @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:
             subp.subp(["some", "args"])
@@ -40,6 +38,12 @@ class TestDisableSubpUsage:
         subp.subp(['bash', '-c', 'true'])
         subp.subp(['whoami'])
 
+    @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"):
+            subp.subp(["bash"])
+
 
 class TestDisableSubpUsageInTestSubclass(CiTestCase):
     """Test that disable_subp_usage doesn't impact CiTestCase's subp logic."""
diff --git a/conftest.py b/conftest.py
index 251bca59..faf13804 100644
--- a/conftest.py
+++ b/conftest.py
@@ -5,6 +5,18 @@ import pytest
 from cloudinit import subp
 
 
+def _closest_marker_args_or(request, marker_name: str, default):
+    """Get the args for the closest ``marker_name`` or return ``default``"""
+    try:
+        marker = request.node.get_closest_marker(marker_name)
+    except AttributeError:
+        # Older versions of pytest don't have the new API
+        marker = request.node.get_marker(marker_name)
+    if marker is not None:
+        return marker.args
+    return default
+
+
 @pytest.yield_fixture(autouse=True)
 def disable_subp_usage(request):
     """
@@ -15,25 +27,23 @@ def disable_subp_usage(request):
     imports happen before the patching here (or the CiTestCase monkey-patching)
     happens, so are left untouched.
 
-    To allow a particular test method or class to use subp.subp you can set the
-    parameter passed to this fixture to False using pytest.mark.parametrize::
+    To allow a particular test method or class to use subp.subp you can mark it
+    as such::
 
-        @pytest.mark.parametrize("disable_subp_usage", [False], indirect=True)
+        @pytest.mark.allow_all_subp
         def test_whoami(self):
             subp.subp(["whoami"])
 
-    To instead allow subp.subp usage for a specific command, you can set the
-    parameter passed to this fixture to that command:
+    To instead allow subp.subp usage for a specific command, you can use the
+    ``allow_subp_for`` mark::
 
-        @pytest.mark.parametrize("disable_subp_usage", ["bash"], indirect=True)
+        @pytest.mark.allow_subp_for("bash")
         def test_bash(self):
             subp.subp(["bash"])
 
-    To specify multiple commands, set the parameter to a list (note the
-    double-layered list: we specify a single parameter that is itself a list):
+    You can pass multiple commands as values; they will all be permitted::
 
-        @pytest.mark.parametrize(
-            "disable_subp_usage", ["bash", "whoami"], indirect=True)
+        @pytest.mark.allow_subp_for("bash", "whoami")
         def test_several_things(self):
             subp.subp(["bash"])
             subp.subp(["whoami"])
@@ -43,30 +53,43 @@ def disable_subp_usage(request):
     tests, CiTestCase's allowed_subp does take precedence (and we have
     TestDisableSubpUsageInTestSubclass to confirm that).
     """
-    should_disable = getattr(request, "param", True)
-    if should_disable:
-        if not isinstance(should_disable, (list, str)):
-            def side_effect(args, *other_args, **kwargs):
-                raise AssertionError("Unexpectedly used subp.subp")
-        else:
-            # Look this up before our patch is in place, so we have access to
-            # the real implementation in side_effect
-            real_subp = subp.subp
-
-            if isinstance(should_disable, str):
-                should_disable = [should_disable]
-
-            def side_effect(args, *other_args, **kwargs):
-                cmd = args[0]
-                if cmd not in should_disable:
-                    raise AssertionError(
-                        "Unexpectedly used subp.subp to call {} (allowed:"
-                        " {})".format(cmd, ",".join(should_disable))
-                    )
-                return real_subp(args, *other_args, **kwargs)
-
-        with mock.patch('cloudinit.subp.subp', autospec=True) as m_subp:
-            m_subp.side_effect = side_effect
-            yield
+    allow_subp_for = _closest_marker_args_or(request, "allow_subp_for", None)
+    # Because the mark doesn't take arguments, `allow_all_subp` will be set to
+    # [] if the marker is present, so explicit None checks are required
+    allow_all_subp = _closest_marker_args_or(request, "allow_all_subp", None)
+
+    if allow_all_subp is not None and allow_subp_for is None:
+        # Only allow_all_subp specified, don't mock subp.subp
+        yield
+        return
+
+    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")
+
+    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(
+                "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."
+            )
     else:
+        # Look this up before our patch is in place, so we have access to
+        # the real implementation in side_effect
+        real_subp = subp.subp
+
+        def side_effect(args, *other_args, **kwargs):
+            cmd = args[0]
+            if cmd not in allow_subp_for:
+                raise AssertionError(
+                    "Unexpectedly used subp.subp to call {} (allowed:"
+                    " {})".format(cmd, ",".join(allow_subp_for))
+                )
+            return real_subp(args, *other_args, **kwargs)
+
+    with mock.patch("cloudinit.subp.subp", autospec=True) as m_subp:
+        m_subp.side_effect = side_effect
         yield
diff --git a/tests/unittests/test_datasource/test_opennebula.py b/tests/unittests/test_datasource/test_opennebula.py
index 80841182..9c6070a5 100644
--- a/tests/unittests/test_datasource/test_opennebula.py
+++ b/tests/unittests/test_datasource/test_opennebula.py
@@ -928,8 +928,7 @@ class TestOpenNebulaNetwork(unittest.TestCase):
 
 
 class TestParseShellConfig:
-
-    @pytest.mark.parametrize('disable_subp_usage', ['bash'], indirect=True)
+    @pytest.mark.allow_subp_for("bash")
     def test_no_seconds(self):
         cfg = '\n'.join(["foo=bar", "SECONDS=2", "xx=foo"])
         # we could test 'sleep 2', but that would make the test run slower.
diff --git a/tests/unittests/test_render_cloudcfg.py b/tests/unittests/test_render_cloudcfg.py
index 696915a3..495e2669 100644
--- a/tests/unittests/test_render_cloudcfg.py
+++ b/tests/unittests/test_render_cloudcfg.py
@@ -13,7 +13,7 @@ DISTRO_VARIANTS = ["amazon", "arch", "centos", "debian", "fedora", "freebsd",
                    "netbsd", "openbsd", "rhel", "suse", "ubuntu", "unknown"]
 
 
-@pytest.mark.parametrize('disable_subp_usage', [sys.executable], indirect=True)
+@pytest.mark.allow_subp_for(sys.executable)
 class TestRenderCloudCfg:
 
     cmd = [sys.executable, os.path.realpath('tools/render-cloudcfg')]
diff --git a/tox.ini b/tox.ini
index 847a2fbd..5257f9e3 100644
--- a/tox.ini
+++ b/tox.ini
@@ -133,3 +133,10 @@ commands = {envpython} -m tests.cloud_tests {posargs}
 passenv = HOME TRAVIS
 deps =
     -r{toxinidir}/integration-requirements.txt
+
+[pytest]
+# TODO: s/--strict/--strict-markers/ once xenial support is dropped
+addopts = --strict
+markers =
+    allow_subp_for: allow subp usage for the given commands (disable_subp_usage)
+    allow_all_subp: allow all subp usage (disable_subp_usage)