Simplify tests, reduce comments

Author: cmaloney

Lib/test/libregrtest/reporter.py

@@ -5,54 +5,43 @@
 from _colorize import decolor  # type: ignore[import-not-found]
 from typing import TextIO
 
+from .findtests import findtestdir
 from .result import TestResult
 lazy from .results import TestResults
 from .runtests import RunTests
 from .utils import count
 
 
 class Reporter:
-    """Report test failures, for example to a CI service.
-
-    The default reporter does nothing.
-    """
+    """Report test failures, for example to a CI service. A no-op here."""
 
     def run_started(self, runtests: RunTests) -> None:
-        """Start a run of tests.
-
-        Called in the main process at the start of each run (the first run
-        and the re-run), and in the process running the tests before each
-        test file.
-        """
+        """Called at the start of each run, and in the test process."""
 
     def test_case_failed(self, test_case: str, traceback: str,
                          stream: TextIO) -> None:
-        """Report a failed test case.
-
-        Called just after unittest printed the failure, or at the end of the
-        test file in quiet mode.
-        """
+        """Called in the test process after a test case failed."""
 
     def test_result(self, result: TestResult) -> None:
-        """Report the result of a test file, in the main process."""
+        """Called in the main process after a test file completed."""
 
     def summary(self, results: TestResults, state: str) -> None:
-        """Report the results of all runs, in the main process."""
+        """Called in the main process after all runs."""
 
     @staticmethod
     def parse_traceback(traceback: str) -> tuple[str | None, str | None, str]:
-        """Parse the traceback of a failure into (filename, line, exception).
-
-        The location is the last frame in a test file, or else the first frame
-        (the test).
-        """
+        """Return (filename, line, exception) of a failure."""
         frames = re.findall(r'^ *File "(.*)", line (\d+)', traceback,
                             re.MULTILINE)
         test_frames = [frame for frame in frames
                        if pathlib.Path(frame[0]).name.startswith('test_')]
-        filename, line = (test_frames[-1] if test_frames
-                          else frames[0] if frames
-                          else (None, None))
+        # Prefer the last frame in a test file, the assertion
+        if test_frames:
+            filename, line = test_frames[-1]
+        elif frames:
+            filename, line = frames[0]
+        else:
+            filename = line = None
         exception = re.split(r'^  File .*\n(?:    .*\n)*', traceback,
                              flags=re.MULTILINE)[-1].strip()
         return filename, line, exception
@@ -65,10 +54,8 @@ def get_reporter() -> Reporter:
 
 
 class GitHubActionsReporter(Reporter):
-    """Annotate failures in the GitHub Actions job log and list them in the
-    job summary."""
+    """Annotate failures in the job log and list them in the job summary."""
 
-    # Escape the data and the properties of workflow commands
     ESCAPE_DATA = str.maketrans({"%": "%25", "\r": "%0D", "\n": "%0A"})
     ESCAPE_PROPERTY = ESCAPE_DATA | str.maketrans({":": "%3A", ",": "%2C"})
 
@@ -81,32 +68,39 @@ def run_started(self, runtests: RunTests) -> None:
 
     def test_case_failed(self, test_case: str, traceback: str,
                          stream: TextIO) -> None:
-        # Only report the failures which decide the result
         if not self.runtests.final_run:
             return
-        # The job log has the full traceback: only keep the exception
         filename, line, exception = self.parse_traceback(decolor(traceback))
         self.annotate(test_case, exception, filename, line, stream)
 
     def test_result(self, result: TestResult) -> None:
-        # Failed test cases are annotated by test_case_failed()
-        if (not self.runtests.final_run
-                or not result.is_failed(self.runtests.fail_env_changed)
-                or result.failed_test_cases):
+        if not self.runtests.final_run:
+            return
+        if not result.is_failed(self.runtests.fail_env_changed):
+            return
+        # Already annotated by test_case_failed()
+        if result.failed_test_cases:
             return
+        # "test.test_x.test_y" is test_x/test_y.py or test_x/test_y/__init__.py
+        name = result.test_name.removeprefix("test.")
+        path = pathlib.Path(findtestdir(self.runtests.test_dir),
+                            *name.split("."))
+        filename = None
+        for candidate in (path.with_suffix(".py"), path / "__init__.py"):
+            if candidate.exists():
+                filename = str(candidate)
         message = "\n".join([str(result), *(result.env_changed_reasons or ())])
-        self.annotate(result.test_name, decolor(message),
-                      result.find_filename(self.runtests.test_dir))
+        self.annotate(result.test_name, decolor(message), filename)
 
     def annotate(self, title: str, message: str, filename: str | None,
                  line: str | None = None, stream: TextIO | None = None) -> None:
         if filename:
-            # The stdlib can be outside of the repository checkout
+            # Make installed stdlib paths relative to the checkout
             path = pathlib.Path(filename)
             stdlib_dir = pathlib.Path(os.__file__).parent
             if path.is_relative_to(stdlib_dir):
                 filename = ("Lib" / path.relative_to(stdlib_dir)).as_posix()
-            # Without a line, the annotation is not shown on the file
+            # GitHub needs a line to show the annotation on the file
             line = line or "1"
         props = {"file": filename, "line": line, "title": title}
         props_text = ",".join(f"{key}={value.translate(self.ESCAPE_PROPERTY)}"
@@ -115,7 +109,6 @@ def annotate(self, title: str, message: str, filename: str | None,
               file=stream, flush=True)
 
     def summary(self, results: TestResults, state: str) -> None:
-        # Test files which failed in the last run (the re-run, if any)
         failed = [result for result in results.rerun_results
                   if result.is_failed(self.runtests.fail_env_changed)]
         if not failed:
@@ -131,12 +124,13 @@ def summary(self, results: TestResults, state: str) -> None:
                                         for reason in result.env_changed_reasons))
             for name, traceback in result.failed_test_cases:
                 # Expand short tracebacks if there are only a few
-                is_open = ncases <= 5 and traceback.count("\n") < 30
-                blocks += [f"<details{' open' if is_open else ''}>"
-                           f"<summary>{html.escape(name)}</summary>",
+                if ncases <= 5 and traceback.count("\n") < 30:
+                    details = "<details open>"
+                else:
+                    details = "<details>"
+                blocks += [f"{details}<summary>{html.escape(name)}</summary>",
                            f"```pytb\n{decolor(traceback).rstrip()}\n```",
                            "</details>"]
-        # Markdown blocks are separated by an empty line
         with open(os.environ["GITHUB_STEP_SUMMARY"], "a",
                   encoding="utf-8") as fp:
             fp.write("".join(f"{block}\n\n" for block in blocks))

Lib/test/libregrtest/result.py

@@ -1,12 +1,10 @@
 import dataclasses
 import json
-import pathlib
 from _colorize import get_colors  # type: ignore[import-not-found]
 from typing import Any
 
-from .findtests import findtestdir
 from .utils import (
-    StrJSON, StrPath, TestName, FilterTuple,
+    StrJSON, TestName, FilterTuple,
     format_duration, normalize_test_name, print_warning)
 
 
@@ -105,9 +103,6 @@ class TestResult:
     # short descriptions of how the test altered the execution environment
     env_changed_reasons: list[str] | None = None
 
-    # test file, set once the test module is imported
-    filename: str | None = None
-
     def is_failed(self, fail_env_changed: bool) -> bool:
         if self.state == State.ENV_CHANGED:
             return fail_env_changed
@@ -187,19 +182,6 @@ def has_meaningful_duration(self):
     def failed_test_cases(self) -> list[tuple[str, str]]:
         return (self.errors or []) + (self.failures or [])
 
-    def find_filename(self, test_dir: StrPath | None) -> str | None:
-        """Get the filename of the test, or None if it is not found."""
-        if self.filename:
-            return self.filename
-        # The worker crashed or timed out before importing the test:
-        # "test.test_x.test_y" is test_x/test_y.py or test_x/test_y/__init__.py
-        name = self.test_name.removeprefix("test.")
-        path = pathlib.Path(findtestdir(test_dir), *name.split("."))
-        for filename in (path.with_suffix(".py"), path / "__init__.py"):
-            if filename.exists():
-                return str(filename)
-        return None
-
     def set_env_changed(self, *reasons):
         if self.state is None or self.state == State.PASSED:
             self.state = State.ENV_CHANGED

Lib/test/libregrtest/single.py

@@ -153,7 +153,6 @@ def _load_run_test(result: TestResult, runtests: RunTests) -> None:
     test_name = result.test_name
     module_name = abs_module_name(test_name, runtests.test_dir)
     test_mod = importlib.import_module(module_name)
-    result.filename = test_mod.__file__
 
     if hasattr(test_mod, "test_main"):
         # https://github.com/python/cpython/issues/89392