deprecate direct node construction and introduce Node.from_parent

Author: RonnyPfannschmidt

changelog/5975.deprecation.rst

@@ -0,0 +1,6 @@
+Deprecate using direct constructors for ``Nodes``.
+
+Instead they are new constructed via ``Node.from_parent``.
+
+This transitional mechanism enables us to detangle the very intensely
+entangled ``Node`` relationships by enforcing more controlled creation/configruation patterns.

src/_pytest/deprecated.py

@@ -9,6 +9,7 @@
 in case of warnings which need to format their messages.
 """
 from _pytest.warning_types import PytestDeprecationWarning
+from _pytest.warning_types import UnformattedWarning
 
 # set of plugins which have been integrated into the core; we use this list to ignore
 # them during registration to avoid conflicts
@@ -35,6 +36,11 @@
     "as a keyword argument instead."
 )
 
+NODE_USE_FROM_PARENT = UnformattedWarning(
+    PytestDeprecationWarning,
+    "direct construction of {name} has been deprecated, please use {name}.from_parent",
+)
+
 JUNIT_XML_DEFAULT_FAMILY = PytestDeprecationWarning(
     "The 'junit_family' default value will change to 'xunit2' in pytest 6.0.\n"
     "Add 'junit_family=legacy' to your pytest.ini file to silence this warning and make your suite compatible."

src/_pytest/doctest.py

@@ -108,9 +108,9 @@ def pytest_collect_file(path, parent):
     config = parent.config
     if path.ext == ".py":
         if config.option.doctestmodules and not _is_setup_py(config, path, parent):
-            return DoctestModule(path, parent)
+            return DoctestModule.from_parent(parent, fspath=path)
     elif _is_doctest(config, path, parent):
-        return DoctestTextfile(path, parent)
+        return DoctestTextfile.from_parent(parent, fspath=path)
 
 
 def _is_setup_py(config, path, parent):
@@ -215,6 +215,10 @@ def __init__(self, name, parent, runner=None, dtest=None):
         self.obj = None
         self.fixture_request = None
 
+    @classmethod
+    def from_parent(cls, parent, *, name, runner, dtest):
+        return cls._create(name=name, parent=parent, runner=runner, dtest=dtest)
+
     def setup(self):
         if self.dtest is not None:
             self.fixture_request = _setup_fixtures(self)
@@ -370,7 +374,9 @@ def collect(self):
         parser = doctest.DocTestParser()
         test = parser.get_doctest(text, globs, name, filename, 0)
         if test.examples:
-            yield DoctestItem(test.name, self, runner, test)
+            yield DoctestItem.from_parent(
+                self, name=test.name, runner=runner, dtest=test
+            )
 
 
 def _check_all_skipped(test):
@@ -467,7 +473,9 @@ def _find(self, tests, obj, name, module, source_lines, globs, seen):
 
         for test in finder.find(module, module.__name__):
             if test.examples:  # skip empty doctests
-                yield DoctestItem(test.name, self, runner, test)
+                yield DoctestItem.from_parent(
+                    self, name=test.name, runner=runner, dtest=test
+                )
 
 
 def _setup_fixtures(doctest_item):

src/_pytest/main.py

@@ -184,7 +184,7 @@ def pytest_addoption(parser):
 
 def wrap_session(config, doit):
     """Skeleton command line program"""
-    session = Session(config)
+    session = Session.from_config(config)
     session.exitstatus = ExitCode.OK
     initstate = 0
     try:
@@ -395,6 +395,10 @@ def __init__(self, config):
 
         self.config.pluginmanager.register(self, name="session")
 
+    @classmethod
+    def from_config(cls, config):
+        return cls._create(config)
+
     def __repr__(self):
         return "<%s %s exitstatus=%r testsfailed=%d testscollected=%d>" % (
             self.__class__.__name__,

src/_pytest/nodes.py

@@ -18,6 +18,7 @@
 from _pytest.compat import cached_property
 from _pytest.compat import getfslineno
 from _pytest.config import Config
+from _pytest.deprecated import NODE_USE_FROM_PARENT
 from _pytest.fixtures import FixtureDef
 from _pytest.fixtures import FixtureLookupError
 from _pytest.fixtures import FixtureLookupErrorRepr
@@ -73,7 +74,16 @@ def ischildnode(baseid, nodeid):
     return node_parts[: len(base_parts)] == base_parts
 
 
-class Node:
+class NodeMeta(type):
+    def __call__(self, *k, **kw):
+        warnings.warn(NODE_USE_FROM_PARENT.format(name=self.__name__), stacklevel=2)
+        return super().__call__(*k, **kw)
+
+    def _create(self, *k, **kw):
+        return super().__call__(*k, **kw)
+
+
+class Node(metaclass=NodeMeta):
     """ base class for Collector and Item the test collection tree.
     Collector subclasses have children, Items are terminal nodes."""
 
@@ -133,6 +143,10 @@ def __init__(
             if self.name != "()":
                 self._nodeid += "::" + self.name
 
+    @classmethod
+    def from_parent(cls, parent, *, name):
+        return cls._create(parent=parent, name=name)
+
     @property
     def ihook(self):
         """ fspath sensitive hook proxy used to call pytest hooks"""
@@ -418,6 +432,10 @@ def __init__(
 
         super().__init__(name, parent, config, session, nodeid=nodeid, fspath=fspath)
 
+    @classmethod
+    def from_parent(cls, parent, *, fspath):
+        return cls._create(parent=parent, fspath=fspath)
+
 
 class File(FSCollector):
     """ base class for collecting tests from a file. """

src/_pytest/pytester.py

@@ -744,7 +744,7 @@ def getnode(self, config, arg):
         :param arg: a :py:class:`py.path.local` instance of the file
 
         """
-        session = Session(config)
+        session = Session.from_config(config)
         assert "::" not in str(arg)
         p = py.path.local(arg)
         config.hook.pytest_sessionstart(session=session)
@@ -762,7 +762,7 @@ def getpathnode(self, path):
 
         """
         config = self.parseconfigure(path)
-        session = Session(config)
+        session = Session.from_config(config)
         x = session.fspath.bestrelpath(path)
         config.hook.pytest_sessionstart(session=session)
         res = session.perform_collect([x], genitems=False)[0]

src/_pytest/python.py

@@ -190,8 +190,8 @@ def path_matches_patterns(path, patterns):
 
 def pytest_pycollect_makemodule(path, parent):
     if path.basename == "__init__.py":
-        return Package(path, parent)
-    return Module(path, parent)
+        return Package.from_parent(parent, fspath=path)
+    return Module.from_parent(parent, fspath=path)
 
 
 @hookimpl(hookwrapper=True)
@@ -203,7 +203,7 @@ def pytest_pycollect_makeitem(collector, name, obj):
     # nothing was collected elsewhere, let's do it here
     if safe_isclass(obj):
         if collector.istestclass(obj, name):
-            outcome.force_result(Class(name, parent=collector))
+            outcome.force_result(Class.from_parent(collector, name=name, obj=obj))
     elif collector.istestfunction(obj, name):
         # mock seems to store unbound methods (issue473), normalize it
         obj = getattr(obj, "__func__", obj)
@@ -222,7 +222,7 @@ def pytest_pycollect_makeitem(collector, name, obj):
             )
         elif getattr(obj, "__test__", True):
             if is_generator(obj):
-                res = Function(name, parent=collector)
+                res = Function.from_parent(collector, name=name)
                 reason = "yield tests were removed in pytest 4.0 - {name} will be ignored".format(
                     name=name
                 )
@@ -381,7 +381,7 @@ def _genfunctions(self, name, funcobj):
         cls = clscol and clscol.obj or None
         fm = self.session._fixturemanager
 
-        definition = FunctionDefinition(name=name, parent=self, callobj=funcobj)
+        definition = FunctionDefinition.from_parent(self, name=name, callobj=funcobj)
         fixtureinfo = fm.getfixtureinfo(definition, funcobj, cls)
 
         metafunc = Metafunc(
@@ -396,7 +396,7 @@ def _genfunctions(self, name, funcobj):
         self.ihook.pytest_generate_tests.call_extra(methods, dict(metafunc=metafunc))
 
         if not metafunc._calls:
-            yield Function(name, parent=self, fixtureinfo=fixtureinfo)
+            yield Function.from_parent(self, name=name, fixtureinfo=fixtureinfo)
         else:
             # add funcargs() as fixturedefs to fixtureinfo.arg2fixturedefs
             fixtures.add_funcarg_pseudo_fixture_def(self, metafunc, fm)
@@ -408,9 +408,9 @@ def _genfunctions(self, name, funcobj):
 
             for callspec in metafunc._calls:
                 subname = "{}[{}]".format(name, callspec.id)
-                yield Function(
+                yield Function.from_parent(
+                    self,
                     name=subname,
-                    parent=self,
                     callspec=callspec,
                     callobj=funcobj,
                     fixtureinfo=fixtureinfo,
@@ -626,7 +626,7 @@ def collect(self):
         if init_module.check(file=1) and path_matches_patterns(
             init_module, self.config.getini("python_files")
         ):
-            yield Module(init_module, self)
+            yield Module.from_parent(self, fspath=init_module)
         pkg_prefixes = set()
         for path in this_path.visit(rec=self._recurse, bf=True, sort=True):
             # We will visit our own __init__.py file, in which case we skip it.
@@ -677,6 +677,10 @@ def _get_first_non_fixture_func(obj, names):
 class Class(PyCollector):
     """ Collector for test methods. """
 
+    @classmethod
+    def from_parent(cls, parent, *, name, obj=None):
+        return cls._create(name=name, parent=parent)
+
     def collect(self):
         if not safe_getattr(self.obj, "__test__", True):
             return []
@@ -702,7 +706,7 @@ def collect(self):
         self._inject_setup_class_fixture()
         self._inject_setup_method_fixture()
 
-        return [Instance(name="()", parent=self)]
+        return [Instance.from_parent(self, name="()")]
 
     def _inject_setup_class_fixture(self):
         """Injects a hidden autouse, class scoped fixture into the collected class object
@@ -1454,6 +1458,10 @@ def __init__(
         #: .. versionadded:: 3.0
         self.originalname = originalname
 
+    @classmethod
+    def from_parent(cls, parent, **kw):
+        return cls._create(parent=parent, **kw)
+
     def _initrequest(self):
         self.funcargs = {}
         self._request = fixtures.FixtureRequest(self)

src/_pytest/unittest.py

@@ -23,7 +23,7 @@ def pytest_pycollect_makeitem(collector, name, obj):
     except Exception:
         return
     # yes, so let's collect it
-    return UnitTestCase(name, parent=collector)
+    return UnitTestCase.from_parent(collector, name=name, obj=obj)
 
 
 class UnitTestCase(Class):
@@ -51,15 +51,16 @@ def collect(self):
             if not getattr(x, "__test__", True):
                 continue
             funcobj = getimfunc(x)
-            yield TestCaseFunction(name, parent=self, callobj=funcobj)
+            yield TestCaseFunction.from_parent(self, name=name, callobj=funcobj)
             foundsomething = True
 
         if not foundsomething:
             runtest = getattr(self.obj, "runTest", None)
             if runtest is not None:
                 ut = sys.modules.get("twisted.trial.unittest", None)
                 if ut is None or runtest != ut.TestCase.runTest:
-                    yield TestCaseFunction("runTest", parent=self)
+                    # TODO: callobj consistency
+                    yield TestCaseFunction.from_parent(self, name="runTest")
 
     def _inject_setup_teardown_fixtures(self, cls):
         """Injects a hidden auto-use fixture to invoke setUpClass/setup_method and corresponding
@@ -190,20 +191,21 @@ def stopTest(self, testcase):
     def _handle_skip(self):
         # implements the skipping machinery (see #2137)
         # analog to pythons Lib/unittest/case.py:run
-        testMethod = getattr(self._testcase, self._testcase._testMethodName)
+        test_method = getattr(self._testcase, self._testcase._testMethodName)
         if getattr(self._testcase.__class__, "__unittest_skip__", False) or getattr(
-            testMethod, "__unittest_skip__", False
+            test_method, "__unittest_skip__", False
         ):
             # If the class or method was skipped.
             skip_why = getattr(
                 self._testcase.__class__, "__unittest_skip_why__", ""
-            ) or getattr(testMethod, "__unittest_skip_why__", "")
+            ) or getattr(test_method, "__unittest_skip_why__", "")
             self._testcase._addSkip(self, self._testcase, skip_why)
             return True
         return False
 
     def runtest(self):
         if self.config.pluginmanager.get_plugin("pdbinvoke") is None:
+            # TODO: move testcase reporter into separate class, this shouldnt be on item
             self._testcase(result=self)
         else:
             # disables tearDown and cleanups for post mortem debugging (see #1890)

testing/deprecated_test.py

@@ -1,5 +1,8 @@
+import inspect
+
 import pytest
 from _pytest import deprecated
+from _pytest import nodes
 
 
 @pytest.mark.filterwarnings("default")
@@ -73,3 +76,17 @@ def test_foo():
         result.stdout.no_fnmatch_line(warning_msg)
     else:
         result.stdout.fnmatch_lines([warning_msg])
+
+
+def test_node_direct_ctor_warning():
+    class MockConfig:
+        pass
+
+    ms = MockConfig()
+    with pytest.warns(
+        DeprecationWarning,
+        match="direct construction of .* has been deprecated, please use .*.from_parent",
+    ) as w:
+        nodes.Node(name="test", config=ms, session=ms, nodeid="None")
+    assert w[0].lineno == inspect.currentframe().f_lineno - 1
+    assert w[0].filename == __file__

testing/python/collect.py

@@ -281,10 +281,10 @@ def make_function(testdir, **kwargs):
         from _pytest.fixtures import FixtureManager
 
         config = testdir.parseconfigure()
-        session = testdir.Session(config)
+        session = testdir.Session.from_config(config)
         session._fixturemanager = FixtureManager(session)
 
-        return pytest.Function(config=config, parent=session, **kwargs)
+        return pytest.Function.from_parent(config=config, parent=session, **kwargs)
 
     def test_function_equality(self, testdir, tmpdir):
         def func1():
@@ -1024,7 +1024,7 @@ def reportinfo(self):
                     return "ABCDE", 42, "custom"
             def pytest_pycollect_makeitem(collector, name, obj):
                 if name == "test_func":
-                    return MyFunction(name, parent=collector)
+                    return MyFunction.from_parent(name=name, parent=collector)
         """
         )
         item = testdir.getitem("def test_func(): pass")

testing/python/integration.py

@@ -10,7 +10,7 @@ def test_funcarg_non_pycollectobj(self, testdir):  # rough jstests usage
             import pytest
             def pytest_pycollect_makeitem(collector, name, obj):
                 if name == "MyClass":
-                    return MyCollector(name, parent=collector)
+                    return MyCollector.from_parent(collector, name=name)
             class MyCollector(pytest.Collector):
                 def reportinfo(self):
                     return self.fspath, 3, "xyz"
@@ -40,7 +40,7 @@ def test_autouse_fixture(self, testdir):  # rough jstests usage
             import pytest
             def pytest_pycollect_makeitem(collector, name, obj):
                 if name == "MyClass":
-                    return MyCollector(name, parent=collector)
+                    return MyCollector.from_parent(collector, name=name)
             class MyCollector(pytest.Collector):
                 def reportinfo(self):
                     return self.fspath, 3, "xyz"

testing/python/metafunc.py

@@ -30,7 +30,7 @@ class DefinitionMock(python.FunctionDefinition):
 
         names = fixtures.getfuncargnames(func)
         fixtureinfo = FixtureInfo(names)
-        definition = DefinitionMock(func)
+        definition = DefinitionMock._create(func)
         return python.Metafunc(definition, fixtureinfo, config)
 
     def test_no_funcargs(self, testdir):