Refactor Session._parsearg into a separate function for testing

Author: nicoddemus

src/_pytest/main.py

@@ -549,7 +549,9 @@ def _perform_collect(  # noqa: F811
         self._initial_parts = []  # type: List[Tuple[py.path.local, List[str]]]
         self.items = items = []  # type: List[nodes.Item]
         for arg in args:
-            fspath, parts = self._parsearg(arg)
+            fspath, parts = resolve_path_argument(
+                self.config.invocation_dir, arg, as_pypath=self.config.option.pyargs
+            )
             self._initial_parts.append((fspath, parts))
             initialpaths.append(fspath)
         self._initialpaths = frozenset(initialpaths)
@@ -673,36 +675,36 @@ def _collect(
                 return
             yield from m
 
-    def _tryconvertpyarg(self, x: str) -> str:
-        """Convert a dotted module name to path."""
-        try:
-            spec = importlib.util.find_spec(x)
-        # AttributeError: looks like package module, but actually filename
-        # ImportError: module does not exist
-        # ValueError: not a module name
-        except (AttributeError, ImportError, ValueError):
-            return x
-        if spec is None or spec.origin is None or spec.origin == "namespace":
-            return x
-        elif spec.submodule_search_locations:
-            return os.path.dirname(spec.origin)
-        else:
-            return spec.origin
-
-    def _parsearg(self, arg: str) -> Tuple[py.path.local, List[str]]:
-        """Return (fspath, names) tuple after checking the file exists."""
-        strpath, *parts = str(arg).split("::")
-        if self.config.option.pyargs:
-            strpath = self._tryconvertpyarg(strpath)
-        fspath = Path(str(self.config.invocation_dir), strpath)
-        fspath = absolutepath(fspath)
-        if not fspath.exists():
-            if self.config.option.pyargs:
-                raise UsageError(
-                    "file or package not found: " + arg + " (missing __init__.py?)"
-                )
-            raise UsageError("file not found: " + arg)
-        return py.path.local(str(fspath)), parts
+    # def _tryconvertpyarg(self, x: str) -> str:
+    #     """Convert a dotted module name to path."""
+    #     try:
+    #         spec = importlib.util.find_spec(x)
+    #     # AttributeError: looks like package module, but actually filename
+    #     # ImportError: module does not exist
+    #     # ValueError: not a module name
+    #     except (AttributeError, ImportError, ValueError):
+    #         return x
+    #     if spec is None or spec.origin is None or spec.origin == "namespace":
+    #         return x
+    #     elif spec.submodule_search_locations:
+    #         return os.path.dirname(spec.origin)
+    #     else:
+    #         return spec.origin
+    #
+    # def _parsearg(self, arg: str) -> Tuple[py.path.local, List[str]]:
+    #     """Return (fspath, names) tuple after checking the file exists."""
+    #     strpath, *parts = str(arg).split("::")
+    #     if self.config.option.pyargs:
+    #         strpath = self._tryconvertpyarg(strpath)
+    #     fspath = Path(str(self.config.invocation_dir), strpath)
+    #     fspath = absolutepath(fspath)
+    #     if not fspath.exists():
+    #         if self.config.option.pyargs:
+    #             raise UsageError(
+    #                 "file or package not found: " + arg + " (missing __init__.py?)"
+    #             )
+    #         raise UsageError("file not found: " + arg)
+    #     return py.path.local(str(fspath)), parts
 
     def matchnodes(
         self, matching: Sequence[Union[nodes.Item, nodes.Collector]], names: List[str],
@@ -770,3 +772,59 @@ def genitems(
                 for subnode in rep.result:
                     yield from self.genitems(subnode)
             node.ihook.pytest_collectreport(report=rep)
+
+
+def search_pypath(module_name: str) -> str:
+    """Search sys.path for the given a dotted module name, and return its file system path."""
+    try:
+        spec = importlib.util.find_spec(module_name)
+    # AttributeError: looks like package module, but actually filename
+    # ImportError: module does not exist
+    # ValueError: not a module name
+    except (AttributeError, ImportError, ValueError):
+        return module_name
+    if spec is None or spec.origin is None or spec.origin == "namespace":
+        return module_name
+    elif spec.submodule_search_locations:
+        return os.path.dirname(spec.origin)
+    else:
+        return spec.origin
+
+
+def resolve_path_argument(
+    invocation_dir: py.path.local, arg: str, *, as_pypath=False
+) -> Tuple[py.path.local, List[str]]:
+    """Parse path arguments optionally containing selection parts and return (fspath, names).
+
+    Command-line arguments can point to files and/or directories, and optionally contain
+    parts for specific tests selection, for example:
+
+        "pkg/tests/test_foo.py::TestClass::test_foo"
+
+    This function ensures the path exists, and returns a tuple:
+
+        (py.path.path("/full/path/to/pkg/tests/test_foo.py"), ["TestClass", "test_foo"])
+
+    When as_pypath is True, expects that the command-line argument actually contains
+    module paths instead of file-system paths:
+
+        "pkg.tests.test_foo::TestClass::test_foo"
+
+    In which case we search sys.path for a matching module, and then return the *path* to the
+    found module.
+
+    If the path doesn't exist, raise UsageError.
+    """
+    strpath, *parts = str(arg).split("::")
+    if as_pypath:
+        strpath = search_pypath(strpath)
+    fspath = Path(str(invocation_dir), strpath)
+    fspath = absolutepath(fspath)
+    if not fspath.exists():
+        msg = (
+            "module or package not found: {arg} (missing __init__.py?)"
+            if as_pypath
+            else "file or directory not found: {arg}"
+        )
+        raise UsageError(msg.format(arg=arg))
+    return py.path.local(str(fspath)), parts

testing/test_collection.py

@@ -443,25 +443,6 @@ def pytest_collect_file(path, parent):
 
 
 class TestSession:
-    def test_parsearg(self, testdir) -> None:
-        p = testdir.makepyfile("def test_func(): pass")
-        subdir = testdir.mkdir("sub")
-        subdir.ensure("__init__.py")
-        target = subdir.join(p.basename)
-        p.move(target)
-        subdir.chdir()
-        config = testdir.parseconfig(p.basename)
-        rcol = Session.from_config(config)
-        assert rcol.fspath == subdir
-        fspath, parts = rcol._parsearg(p.basename)
-
-        assert fspath == target
-        assert len(parts) == 0
-        fspath, parts = rcol._parsearg(p.basename + "::test_func")
-        assert fspath == target
-        assert parts[0] == "test_func"
-        assert len(parts) == 1
-
     def test_collect_topdir(self, testdir):
         p = testdir.makepyfile("def test_func(): pass")
         id = "::".join([p.basename, "test_func"])
@@ -1426,42 +1407,3 @@ def test_modules_not_importable_as_side_effect(self, testdir):
                 "* 1 failed in *",
             ]
         )
-
-
-def test_module_full_path_without_drive(testdir):
-    """Collect and run test using full path except for the drive letter (#7628)
-
-    Passing a full path without a drive letter would trigger a bug in py.path.local
-    where it would keep the full path without the drive letter around, instead of resolving
-    to the full path, resulting in fixtures node ids not matching against test node ids correctly.
-    """
-    testdir.makepyfile(
-        **{
-            "project/conftest.py": """
-                import pytest
-                @pytest.fixture
-                def fix(): return 1
-            """,
-        }
-    )
-
-    testdir.makepyfile(
-        **{
-            "project/tests/dummy_test.py": """
-                def test(fix):
-                    assert fix == 1
-            """
-        }
-    )
-    fn = testdir.tmpdir.join("project/tests/dummy_test.py")
-    assert fn.isfile()
-
-    drive, path = os.path.splitdrive(str(fn))
-
-    result = testdir.runpytest(path, "-v")
-    result.stdout.fnmatch_lines(
-        [
-            os.path.join("project", "tests", "dummy_test.py") + "::test PASSED *",
-            "* 1 passed in *",
-        ]
-    )

testing/test_main.py

@@ -1,8 +1,14 @@
 import argparse
+import os
+import re
 from typing import Optional
 
+import py.path
+
 import pytest
 from _pytest.config import ExitCode
+from _pytest.config import UsageError
+from _pytest.main import resolve_path_argument
 from _pytest.main import validate_basetemp
 from _pytest.pytester import Testdir
 
@@ -98,3 +104,137 @@ def test_validate_basetemp_fails(tmp_path, basetemp, monkeypatch):
 def test_validate_basetemp_integration(testdir):
     result = testdir.runpytest("--basetemp=.")
     result.stderr.fnmatch_lines("*basetemp must not be*")
+
+
+class TestResolvePathArgument:
+    @pytest.fixture(autouse=True)
+    def setup(self, tmp_path, monkeypatch):
+        monkeypatch.chdir(tmp_path)
+        monkeypatch.syspath_prepend(tmp_path / "src")
+
+        pkg = tmp_path.joinpath("src/pkg")
+        pkg.mkdir(parents=True)
+        pkg.joinpath("__init__.py").touch()
+        fn = pkg.joinpath("test.py")
+        fn.touch()
+
+    def test_file(self, tmpdir):
+        """File and parts."""
+        assert resolve_path_argument(tmpdir, "src/pkg/test.py") == (
+            tmpdir / "src/pkg/test.py",
+            [],
+        )
+        assert resolve_path_argument(tmpdir, "src/pkg/test.py::") == (
+            tmpdir / "src/pkg/test.py",
+            [""],
+        )
+        assert resolve_path_argument(tmpdir, "src/pkg/test.py::foo::bar") == (
+            tmpdir / "src/pkg/test.py",
+            ["foo", "bar"],
+        )
+        assert resolve_path_argument(tmpdir, "src/pkg/test.py::foo::bar::") == (
+            tmpdir / "src/pkg/test.py",
+            ["foo", "bar", ""],
+        )
+
+    def test_dir(self, tmpdir):
+        """Directory and parts."""
+        assert resolve_path_argument(tmpdir, "src/pkg") == (tmpdir / "src/pkg", [])
+        assert resolve_path_argument(tmpdir, "src/pkg::") == (tmpdir / "src/pkg", [""])
+        assert resolve_path_argument(tmpdir, "src/pkg::foo::bar") == (
+            tmpdir / "src/pkg",
+            ["foo", "bar"],
+        )
+        assert resolve_path_argument(tmpdir, "src/pkg::foo::bar::") == (
+            tmpdir / "src/pkg",
+            ["foo", "bar", ""],
+        )
+
+    def test_pypath(self, tmpdir):
+        """Dotted name and parts."""
+        assert resolve_path_argument(tmpdir, "pkg.test", as_pypath=True) == (
+            tmpdir / "src/pkg/test.py",
+            [],
+        )
+        assert resolve_path_argument(tmpdir, "pkg.test::foo::bar", as_pypath=True) == (
+            tmpdir / "src/pkg/test.py",
+            ["foo", "bar"],
+        )
+        assert resolve_path_argument(tmpdir, "pkg", as_pypath=True) == (
+            tmpdir / "src/pkg",
+            [],
+        )
+        assert resolve_path_argument(tmpdir, "pkg::foo::bar", as_pypath=True) == (
+            tmpdir / "src/pkg",
+            ["foo", "bar"],
+        )
+
+    def test_does_not_exist(self, tmpdir):
+        """Given a file/module that does not exist raises UsageError."""
+        with pytest.raises(
+            UsageError, match=re.escape("file or directory not found: foobar")
+        ):
+            resolve_path_argument(tmpdir, "foobar")
+
+        with pytest.raises(
+            UsageError,
+            match=re.escape(
+                "module or package not found: foobar (missing __init__.py?)"
+            ),
+        ):
+            resolve_path_argument(tmpdir, "foobar", as_pypath=True)
+
+    def test_absolute_paths_are_resolved_correctly(self, tmpdir):
+        """Absolute paths resolve back to absolute paths."""
+        full_path = str(tmpdir / "src")
+        assert resolve_path_argument(tmpdir, full_path) == (
+            py.path.local(os.path.abspath("src")),
+            [],
+        )
+
+        # ensure full paths given in the command-line without the drive letter resolve
+        # to the full path correctly (#7628)
+        drive, full_path_without_drive = os.path.splitdrive(full_path)
+        assert resolve_path_argument(tmpdir, full_path_without_drive) == (
+            py.path.local(os.path.abspath("src")),
+            [],
+        )
+
+
+def test_module_full_path_without_drive(testdir):
+    """Collect and run test using full path except for the drive letter (#7628).
+
+    Passing a full path without a drive letter would trigger a bug in py.path.local
+    where it would keep the full path without the drive letter around, instead of resolving
+    to the full path, resulting in fixtures node ids not matching against test node ids correctly.
+    """
+    testdir.makepyfile(
+        **{
+            "project/conftest.py": """
+                import pytest
+                @pytest.fixture
+                def fix(): return 1
+            """,
+        }
+    )
+
+    testdir.makepyfile(
+        **{
+            "project/tests/dummy_test.py": """
+                def test(fix):
+                    assert fix == 1
+            """
+        }
+    )
+    fn = testdir.tmpdir.join("project/tests/dummy_test.py")
+    assert fn.isfile()
+
+    drive, path = os.path.splitdrive(str(fn))
+
+    result = testdir.runpytest(path, "-v")
+    result.stdout.fnmatch_lines(
+        [
+            os.path.join("project", "tests", "dummy_test.py") + "::test PASSED *",
+            "* 1 passed in *",
+        ]
+    )

testing/test_terminal.py

@@ -442,7 +442,9 @@ def test_collectonly_missing_path(self, testdir):
         have the items attribute."""
         result = testdir.runpytest("--collect-only", "uhm_missing_path")
         assert result.ret == 4
-        result.stderr.fnmatch_lines(["*ERROR: file not found*"])
+        result.stderr.fnmatch_lines(
+            ["*ERROR: file or directory not found: uhm_missing_path"]
+        )
 
     def test_collectonly_quiet(self, testdir):
         testdir.makepyfile("def test_foo(): pass")