Various cleanups in src/_pytest/python.py (#5599)

Various cleanups in src/_pytest/python.py

Author: nicoddemus

src/_pytest/python.py

@@ -1,11 +1,12 @@
 """ Python test discovery, setup and run of test functions. """
-import collections
 import enum
 import fnmatch
 import inspect
 import os
 import sys
 import warnings
+from collections import Counter
+from collections.abc import Sequence
 from functools import partial
 from textwrap import dedent
 
@@ -240,9 +241,6 @@ class PyobjContext:
 class PyobjMixin(PyobjContext):
     _ALLOW_MARKERS = True
 
-    def __init__(self, *k, **kw):
-        super().__init__(*k, **kw)
-
     @property
     def obj(self):
         """Underlying Python object."""
@@ -394,12 +392,8 @@ def _genfunctions(self, name, funcobj):
             methods.append(module.pytest_generate_tests)
         if hasattr(cls, "pytest_generate_tests"):
             methods.append(cls().pytest_generate_tests)
-        if methods:
-            self.ihook.pytest_generate_tests.call_extra(
-                methods, dict(metafunc=metafunc)
-            )
-        else:
-            self.ihook.pytest_generate_tests(metafunc=metafunc)
+
+        self.ihook.pytest_generate_tests.call_extra(methods, dict(metafunc=metafunc))
 
         if not metafunc._calls:
             yield Function(name, parent=self, fixtureinfo=fixtureinfo)
@@ -444,13 +438,12 @@ def _inject_setup_module_fixture(self):
         Using a fixture to invoke this methods ensures we play nicely and unsurprisingly with
         other fixtures (#517).
         """
-        setup_module = _get_non_fixture_func(self.obj, "setUpModule")
-        if setup_module is None:
-            setup_module = _get_non_fixture_func(self.obj, "setup_module")
-
-        teardown_module = _get_non_fixture_func(self.obj, "tearDownModule")
-        if teardown_module is None:
-            teardown_module = _get_non_fixture_func(self.obj, "teardown_module")
+        setup_module = _get_first_non_fixture_func(
+            self.obj, ("setUpModule", "setup_module")
+        )
+        teardown_module = _get_first_non_fixture_func(
+            self.obj, ("tearDownModule", "teardown_module")
+        )
 
         if setup_module is None and teardown_module is None:
             return
@@ -472,8 +465,10 @@ def _inject_setup_function_fixture(self):
         Using a fixture to invoke this methods ensures we play nicely and unsurprisingly with
         other fixtures (#517).
         """
-        setup_function = _get_non_fixture_func(self.obj, "setup_function")
-        teardown_function = _get_non_fixture_func(self.obj, "teardown_function")
+        setup_function = _get_first_non_fixture_func(self.obj, ("setup_function",))
+        teardown_function = _get_first_non_fixture_func(
+            self.obj, ("teardown_function",)
+        )
         if setup_function is None and teardown_function is None:
             return
 
@@ -557,15 +552,15 @@ def __init__(self, fspath, parent=None, config=None, session=None, nodeid=None):
     def setup(self):
         # not using fixtures to call setup_module here because autouse fixtures
         # from packages are not called automatically (#4085)
-        setup_module = _get_non_fixture_func(self.obj, "setUpModule")
-        if setup_module is None:
-            setup_module = _get_non_fixture_func(self.obj, "setup_module")
+        setup_module = _get_first_non_fixture_func(
+            self.obj, ("setUpModule", "setup_module")
+        )
         if setup_module is not None:
             _call_with_optional_argument(setup_module, self.obj)
 
-        teardown_module = _get_non_fixture_func(self.obj, "tearDownModule")
-        if teardown_module is None:
-            teardown_module = _get_non_fixture_func(self.obj, "teardown_module")
+        teardown_module = _get_first_non_fixture_func(
+            self.obj, ("tearDownModule", "teardown_module")
+        )
         if teardown_module is not None:
             func = partial(_call_with_optional_argument, teardown_module, self.obj)
             self.addfinalizer(func)
@@ -656,27 +651,6 @@ def collect(self):
                 pkg_prefixes.add(path)
 
 
-def _get_xunit_setup_teardown(holder, attr_name, param_obj=None):
-    """
-    Return a callable to perform xunit-style setup or teardown if
-    the function exists in the ``holder`` object.
-    The ``param_obj`` parameter is the parameter which will be passed to the function
-    when the callable is called without arguments, defaults to the ``holder`` object.
-    Return ``None`` if a suitable callable is not found.
-    """
-    # TODO: only needed because of Package!
-    param_obj = param_obj if param_obj is not None else holder
-    result = _get_non_fixture_func(holder, attr_name)
-    if result is not None:
-        arg_count = result.__code__.co_argcount
-        if inspect.ismethod(result):
-            arg_count -= 1
-        if arg_count:
-            return lambda: result(param_obj)
-        else:
-            return result
-
-
 def _call_with_optional_argument(func, arg):
     """Call the given function with the given argument if func accepts one argument, otherwise
     calls func without arguments"""
@@ -689,14 +663,15 @@ def _call_with_optional_argument(func, arg):
         func()
 
 
-def _get_non_fixture_func(obj, name):
+def _get_first_non_fixture_func(obj, names):
     """Return the attribute from the given object to be used as a setup/teardown
     xunit-style function, but only if not marked as a fixture to
     avoid calling it twice.
     """
-    meth = getattr(obj, name, None)
-    if fixtures.getfixturemarker(meth) is None:
-        return meth
+    for name in names:
+        meth = getattr(obj, name, None)
+        if meth is not None and fixtures.getfixturemarker(meth) is None:
+            return meth
 
 
 class Class(PyCollector):
@@ -736,7 +711,7 @@ def _inject_setup_class_fixture(self):
         Using a fixture to invoke this methods ensures we play nicely and unsurprisingly with
         other fixtures (#517).
         """
-        setup_class = _get_non_fixture_func(self.obj, "setup_class")
+        setup_class = _get_first_non_fixture_func(self.obj, ("setup_class",))
         teardown_class = getattr(self.obj, "teardown_class", None)
         if setup_class is None and teardown_class is None:
             return
@@ -760,7 +735,7 @@ def _inject_setup_method_fixture(self):
         Using a fixture to invoke this methods ensures we play nicely and unsurprisingly with
         other fixtures (#517).
         """
-        setup_method = _get_non_fixture_func(self.obj, "setup_method")
+        setup_method = _get_first_non_fixture_func(self.obj, ("setup_method",))
         teardown_method = getattr(self.obj, "teardown_method", None)
         if setup_method is None and teardown_method is None:
             return
@@ -1070,12 +1045,9 @@ def _resolve_arg_value_types(self, argnames, indirect):
             * "params" if the argname should be the parameter of a fixture of the same name.
             * "funcargs" if the argname should be a parameter to the parametrized test function.
         """
-        valtypes = {}
-        if indirect is True:
-            valtypes = dict.fromkeys(argnames, "params")
-        elif indirect is False:
-            valtypes = dict.fromkeys(argnames, "funcargs")
-        elif isinstance(indirect, (tuple, list)):
+        if isinstance(indirect, bool):
+            valtypes = dict.fromkeys(argnames, "params" if indirect else "funcargs")
+        elif isinstance(indirect, Sequence):
             valtypes = dict.fromkeys(argnames, "funcargs")
             for arg in indirect:
                 if arg not in argnames:
@@ -1086,6 +1058,13 @@ def _resolve_arg_value_types(self, argnames, indirect):
                         pytrace=False,
                     )
                 valtypes[arg] = "params"
+        else:
+            fail(
+                "In {func}: expected Sequence or boolean for indirect, got {type}".format(
+                    type=type(indirect).__name__, func=self.function.__name__
+                ),
+                pytrace=False,
+            )
         return valtypes
 
     def _validate_if_using_arg_names(self, argnames, indirect):
@@ -1213,7 +1192,7 @@ def idmaker(argnames, parametersets, idfn=None, ids=None, config=None, item=None
     if len(set(ids)) != len(ids):
         # The ids are not unique
         duplicates = [testid for testid in ids if ids.count(testid) > 1]
-        counters = collections.defaultdict(lambda: 0)
+        counters = Counter()
         for index, testid in enumerate(ids):
             if testid in duplicates:
                 ids[index] = testid + str(counters[testid])
@@ -1402,14 +1381,11 @@ def __init__(
         # https://github.com/pytest-dev/pytest/issues/4569
 
         self.keywords.update(
-            dict.fromkeys(
-                [
-                    mark.name
-                    for mark in self.iter_markers()
-                    if mark.name not in self.keywords
-                ],
-                True,
-            )
+            {
+                mark.name: True
+                for mark in self.iter_markers()
+                if mark.name not in self.keywords
+            }
         )
 
         if fixtureinfo is None:

testing/python/metafunc.py

@@ -599,6 +599,17 @@ def func(x, y):
         assert metafunc._calls[0].funcargs == dict(x="a", y="b")
         assert metafunc._calls[0].params == {}
 
+    def test_parametrize_indirect_wrong_type(self):
+        def func(x, y):
+            pass
+
+        metafunc = self.Metafunc(func)
+        with pytest.raises(
+            pytest.fail.Exception,
+            match="In func: expected Sequence or boolean for indirect, got dict",
+        ):
+            metafunc.parametrize("x, y", [("a", "b")], indirect={})
+
     def test_parametrize_indirect_list_functional(self, testdir):
         """
         #714