Make .test parser friendlier (python#15385)

Author: ikonst

mypy/test/data.py

@@ -12,7 +12,7 @@
 from abc import abstractmethod
 from dataclasses import dataclass
 from pathlib import Path
-from typing import Any, Iterator, NamedTuple, Pattern, Union
+from typing import Any, Iterator, NamedTuple, NoReturn, Pattern, Union
 from typing_extensions import Final, TypeAlias as _TypeAlias
 
 import pytest
@@ -77,11 +77,19 @@ def parse_test_case(case: DataDrivenTestCase) -> None:
     targets: dict[int, list[str]] = {}  # Fine-grained targets (per fine-grained update)
     test_modules: list[str] = []  # Modules which are deemed "test" (vs "fixture")
 
+    def _case_fail(msg: str) -> NoReturn:
+        pytest.fail(f"{case.file}:{case.line}: {msg}", pytrace=False)
+
     # Process the parsed items. Each item has a header of form [id args],
     # optionally followed by lines of text.
     item = first_item = test_items[0]
     test_modules.append("__main__")
     for item in test_items[1:]:
+
+        def _item_fail(msg: str) -> NoReturn:
+            item_abs_line = case.line + item.line - 2
+            pytest.fail(f"{case.file}:{item_abs_line}: {msg}", pytrace=False)
+
         if item.id in {"file", "fixture", "outfile", "outfile-re"}:
             # Record an extra file needed for the test case.
             assert item.arg is not None
@@ -132,9 +140,11 @@ def parse_test_case(case: DataDrivenTestCase) -> None:
             # File/directory to delete during a multi-step test case
             assert item.arg is not None
             m = re.match(r"(.*)\.([0-9]+)$", item.arg)
-            assert m, f"Invalid delete section: {item.arg}"
+            if m is None:
+                _item_fail(f"Invalid delete section {item.arg!r}")
             num = int(m.group(2))
-            assert num >= 2, f"Can't delete during step {num}"
+            if num < 2:
+                _item_fail(f"Can't delete during step {num}")
             full = join(base_path, m.group(1))
             deleted_paths.setdefault(num, set()).add(full)
         elif re.match(r"out[0-9]*$", item.id):
@@ -150,29 +160,18 @@ def parse_test_case(case: DataDrivenTestCase) -> None:
                 if arg.startswith("version"):
                     compare_op = arg[7:9]
                     if compare_op not in {">=", "=="}:
-                        raise ValueError(
-                            "{}, line {}: Only >= and == version checks are currently supported".format(
-                                case.file, item.line
-                            )
-                        )
+                        _item_fail("Only >= and == version checks are currently supported")
                     version_str = arg[9:]
                     try:
                         version = tuple(int(x) for x in version_str.split("."))
                     except ValueError:
-                        raise ValueError(
-                            '{}, line {}: "{}" is not a valid python version'.format(
-                                case.file, item.line, version_str
-                            )
-                        )
+                        _item_fail(f"{version_str!r} is not a valid python version")
                     if compare_op == ">=":
                         version_check = sys.version_info >= version
                     elif compare_op == "==":
                         if not 1 < len(version) < 4:
-                            raise ValueError(
-                                "{}, line {}: Only minor or patch version checks "
-                                'are currently supported with "==": "{}"'.format(
-                                    case.file, item.line, version_str
-                                )
+                            _item_fail(
+                                f'Only minor or patch version checks are currently supported with "==": {version_str!r}'
                             )
                         version_check = sys.version_info[: len(version)] == version
             if version_check:
@@ -189,10 +188,11 @@ def parse_test_case(case: DataDrivenTestCase) -> None:
         elif item.id == "triggered" and item.arg is None:
             triggered = item.data
         else:
-            raise ValueError(f"Invalid section header {item.id} in {case.file}:{item.line}")
+            section_str = item.id + (f" {item.arg}" if item.arg else "")
+            _item_fail(f"Invalid section header [{section_str}] in case {case.name!r}")
 
     if out_section_missing:
-        raise ValueError(f"{case.file}, line {first_item.line}: Required output section not found")
+        _case_fail(f"Required output section not found in case {case.name!r}")
 
     for passnum in stale_modules.keys():
         if passnum not in rechecked_modules:
@@ -204,11 +204,7 @@ def parse_test_case(case: DataDrivenTestCase) -> None:
             and passnum in rechecked_modules
             and not stale_modules[passnum].issubset(rechecked_modules[passnum])
         ):
-            raise ValueError(
-                (
-                    "Stale modules after pass {} must be a subset of rechecked modules ({}:{})"
-                ).format(passnum, case.file, first_item.line)
-            )
+            _case_fail(f"Stale modules after pass {passnum} must be a subset of rechecked modules")
 
     output_inline_start = len(output)
     input = first_item.data
@@ -219,10 +215,7 @@ def parse_test_case(case: DataDrivenTestCase) -> None:
     seen_files = set()
     for file, _ in files:
         if file in seen_files:
-            raise ValueError(
-                f"{case.file}, line {first_item.line}: Duplicated filename {file}. Did you include"
-                " it multiple times?"
-            )
+            _case_fail(f"Duplicated filename {file}. Did you include it multiple times?")
 
         seen_files.add(file)
 
@@ -367,12 +360,13 @@ def setup(self) -> None:
         self.steps = [steps.get(num, []) for num in range(2, max_step + 1)]
 
     def teardown(self) -> None:
-        assert self.old_cwd is not None and self.tmpdir is not None, "test was not properly set up"
-        os.chdir(self.old_cwd)
-        try:
-            self.tmpdir.cleanup()
-        except OSError:
-            pass
+        if self.old_cwd is not None:
+            os.chdir(self.old_cwd)
+        if self.tmpdir is not None:
+            try:
+                self.tmpdir.cleanup()
+            except OSError:
+                pass
         self.old_cwd = None
         self.tmpdir = None
 
@@ -634,6 +628,16 @@ def pytest_pycollect_makeitem(collector: Any, name: str, obj: object) -> Any | N
     return None
 
 
+_case_name_pattern = re.compile(
+    r"(?P<name>[a-zA-Z_0-9]+)"
+    r"(?P<writescache>-writescache)?"
+    r"(?P<only_when>-only_when_cache|-only_when_nocache)?"
+    r"(-(?P<platform>posix|windows))?"
+    r"(?P<skip>-skip)?"
+    r"(?P<xfail>-xfail)?"
+)
+
+
 def split_test_cases(
     parent: DataFileCollector, suite: DataSuite, file: str
 ) -> Iterator[DataDrivenTestCase]:
@@ -644,40 +648,33 @@ def split_test_cases(
     """
     with open(file, encoding="utf-8") as f:
         data = f.read()
-    # number of groups in the below regex
-    NUM_GROUPS = 7
-    cases = re.split(
-        r"^\[case ([a-zA-Z_0-9]+)"
-        r"(-writescache)?"
-        r"(-only_when_cache|-only_when_nocache)?"
-        r"(-posix|-windows)?"
-        r"(-skip)?"
-        r"(-xfail)?"
-        r"\][ \t]*$\n",
-        data,
-        flags=re.DOTALL | re.MULTILINE,
-    )
-    line_no = cases[0].count("\n") + 1
+    cases = re.split(r"^\[case ([^]+)]+)\][ \t]*$\n", data, flags=re.DOTALL | re.MULTILINE)
+    cases_iter = iter(cases)
+    line_no = next(cases_iter).count("\n") + 1
     test_names = set()
-    for i in range(1, len(cases), NUM_GROUPS):
-        name, writescache, only_when, platform_flag, skip, xfail, data = cases[i : i + NUM_GROUPS]
+    for case_id in cases_iter:
+        data = next(cases_iter)
+
+        m = _case_name_pattern.fullmatch(case_id)
+        if not m:
+            raise RuntimeError(f"Invalid testcase id {case_id!r}")
+        name = m.group("name")
         if name in test_names:
             raise RuntimeError(
                 'Found a duplicate test name "{}" in {} on line {}'.format(
                     name, parent.name, line_no
                 )
             )
-        platform = platform_flag[1:] if platform_flag else None
         yield DataDrivenTestCase.from_parent(
             parent=parent,
             suite=suite,
             file=file,
             name=add_test_name_suffix(name, suite.test_name_suffix),
-            writescache=bool(writescache),
-            only_when=only_when,
-            platform=platform,
-            skip=bool(skip),
-            xfail=bool(xfail),
+            writescache=bool(m.group("writescache")),
+            only_when=m.group("only_when"),
+            platform=m.group("platform"),
+            skip=bool(m.group("skip")),
+            xfail=bool(m.group("xfail")),
             data=data,
             line=line_no,
         )

mypy/test/meta/__init__.py

@@ -0,0 +1,66 @@
+"""
+A "meta test" which tests the parsing of .test files. This is not meant to become exhaustive
+but to ensure we maintain a basic level of ergonomics for mypy contributors.
+"""
+import subprocess
+import sys
+import textwrap
+from pathlib import Path
+
+from mypy.test.config import test_data_prefix
+from mypy.test.helpers import Suite
+
+
+class ParseTestDataSuite(Suite):
+    def _dedent(self, s: str) -> str:
+        return textwrap.dedent(s).lstrip()
+
+    def _run_pytest(self, data_suite: str) -> str:
+        p = Path(test_data_prefix) / "check-__fixture__.test"
+        assert not p.exists()
+        try:
+            p.write_text(data_suite)
+            test_nodeid = f"mypy/test/testcheck.py::TypeCheckSuite::{p.name}"
+            args = [sys.executable, "-m", "pytest", "-n", "0", "-s", test_nodeid]
+            proc = subprocess.run(args, capture_output=True, check=False)
+            return proc.stdout.decode()
+        finally:
+            p.unlink()
+
+    def test_parse_invalid_case(self) -> None:
+        # Arrange
+        data = self._dedent(
+            """
+            [case abc]
+            s: str
+            [case foo-XFAIL]
+            s: str
+            """
+        )
+
+        # Act
+        actual = self._run_pytest(data)
+
+        # Assert
+        assert "Invalid testcase id 'foo-XFAIL'" in actual
+
+    def test_parse_invalid_section(self) -> None:
+        # Arrange
+        data = self._dedent(
+            """
+            [case abc]
+            s: str
+            [unknownsection]
+            abc
+            """
+        )
+
+        # Act
+        actual = self._run_pytest(data)
+
+        # Assert
+        expected_lineno = data.splitlines().index("[unknownsection]") + 1
+        expected = (
+            f".test:{expected_lineno}: Invalid section header [unknownsection] in case 'abc'"
+        )
+        assert expected in actual

mypy/test/meta/test_parse_data.py

@@ -1,3 +1,8 @@
+"""
+A "meta test" which tests the `--update-data` feature for updating .test files.
+Updating the expected output, especially when it's in the form of inline (comment) assertions,
+can be brittle, which is why we're "meta-testing" here.
+"""
 import shlex
 import subprocess
 import sys
@@ -16,7 +21,7 @@ def _run_pytest_update_data(self, data_suite: str, *, max_attempts: int) -> str:
         """
         p_test_data = Path(test_data_prefix)
         p_root = p_test_data.parent.parent
-        p = p_test_data / "check-update-data.test"
+        p = p_test_data / "check-__fixture__.test"
         assert not p.exists()
         try:
             p.write_text(textwrap.dedent(data_suite).lstrip())