simplify, rely on env var everywhere. Stop using findspec

Author: cmaloney

Lib/test/libregrtest/main.py

@@ -285,8 +285,7 @@ def _rerun_failed_tests(self, runtests: RunTests) -> RunTests:
             forever=False,
             fail_fast=False,
             match_tests_dict=match_tests_dict,
-            output_on_failure=False,
-            github_annotations=bool(os.environ.get("GITHUB_STEP_SUMMARY")))
+            output_on_failure=False)
         self.logger.set_tests(runtests)
 
         msg = f"Re-running {len(tests)} failed tests in verbose mode"
@@ -397,8 +396,7 @@ def run_test(
             result = run_single_test(test_name, runtests)
 
         self.results.accumulate_result(result, runtests)
-        if runtests.github_annotations:
-            result.print_github_annotation(runtests)
+        result.print_github_annotation(runtests)
 
         return result
 
@@ -540,13 +538,6 @@ def display_summary(self) -> None:
         print(f"Result: {state}")
 
     def create_run_tests(self, tests: TestTuple) -> RunTests:
-        # Annotate test failures in the GitHub Actions job log of the last
-        # run (the re-run, if any), if it reports failures: -v, -W or --pgo
-        will_rerun = self.want_rerun and not self.python_cmd
-        github_annotations = (bool(os.environ.get("GITHUB_STEP_SUMMARY"))
-                              and not will_rerun
-                              and bool(self.verbose or self.output_on_failure
-                                       or self.pgo))
         return RunTests(
             tests,
             fail_fast=self.fail_fast,
@@ -564,7 +555,6 @@ def create_run_tests(self, tests: TestTuple) -> RunTests:
             hunt_refleak=self.hunt_refleak,
             test_dir=self.test_dir,
             use_junit=(self.junit_filename is not None),
-            github_annotations=github_annotations,
             coverage=self.coverage,
             memory_limit=self.memory_limit,
             gc_threshold=self.gc_threshold,
@@ -627,10 +617,16 @@ def _run_tests(self, selected: TestTuple, tests: TestList | None) -> int:
         if use_load_tracker:
             self.logger.start_load_tracker()
         try:
-            if self.num_workers:
-                self._run_tests_mp(runtests, self.num_workers)
-            else:
-                self.run_tests_sequentially(runtests)
+            with os_helper.EnvironmentVarGuard() as env:
+                # In GitHub Actions, only annotate failures of the last run:
+                # not the first run if failed tests will be re-run (with
+                # --python, they are not: see rerun_failed_tests())
+                if self.want_rerun and not self.python_cmd:
+                    env.unset("GITHUB_STEP_SUMMARY")
+                if self.num_workers:
+                    self._run_tests_mp(runtests, self.num_workers)
+                else:
+                    self.run_tests_sequentially(runtests)
 
             coverage = self.results.get_coverage_results()
             self.display_result(runtests)

Lib/test/libregrtest/result.py

@@ -1,13 +1,15 @@
 import dataclasses
-import importlib.util
 import json
+import os
 from _colorize import get_colors  # type: ignore[import-not-found]
 from typing import Any
 
+from .findtests import findtestdir
 from .runtests import RunTests
 from .utils import (
-    StrJSON, TestName, FilterTuple, abs_module_name,
-    format_duration, normalize_test_name, print_warning, github_annotation)
+    StrJSON, TestName, FilterTuple,
+    format_duration, normalize_test_name, print_warning,
+    print_github_annotation)
 
 
 @dataclasses.dataclass(slots=True)
@@ -190,11 +192,14 @@ def print_github_annotation(self, runtests: RunTests) -> None:
         if (self.is_failed(runtests.fail_env_changed)
                 and not self.errors and not self.failures):
             message = "\n".join([str(self), *(self.env_changed_reasons or ())])
-            spec = importlib.util.find_spec(
-                abs_module_name(self.test_name, runtests.test_dir))
-            filename = spec.origin if spec is not None else None
-            print(github_annotation(self.test_name, message, filename),
-                  flush=True)
+            # Test file: "test_x" is test_x.py or test_x/__init__.py,
+            # "test.test_x.test_y" is test_x/test_y.py
+            name = self.test_name.removeprefix("test.")
+            path = os.path.join(findtestdir(runtests.test_dir), *name.split("."))
+            filename = path + ".py"
+            if not os.path.exists(filename):
+                filename = os.path.join(path, "__init__.py")
+            print_github_annotation(self.test_name, message, filename)
 
     def set_env_changed(self, *reasons):
         if self.state is None or self.state == State.PASSED:

Lib/test/libregrtest/run_workers.py

@@ -625,8 +625,7 @@ def _process_result(self, item: QueueOutput) -> TestResult:
             if stdout:
                 print(stdout, flush=True)
         # Annotate after the output: env changed warnings, crash traceback
-        if self.runtests.github_annotations:
-            result.print_github_annotation(self.runtests)
+        result.print_github_annotation(self.runtests)
 
         return result
 

Lib/test/libregrtest/runtests.py

@@ -93,8 +93,6 @@ class RunTests:
     hunt_refleak: HuntRefleak | None
     test_dir: StrPath | None
     use_junit: bool
-    # Annotate test failures in the GitHub Actions job log
-    github_annotations: bool
     coverage: bool
     memory_limit: str | None
     gc_threshold: int | None

Lib/test/libregrtest/save_env.py

@@ -139,18 +139,6 @@ def restore_os_environ(self, saved_environ):
         os.environ = saved_environ[1]
         os.environ.clear()
         os.environ.update(saved_environ[2])
-    @staticmethod
-    def describe_os_environ(original, current):
-        # Only list the keys: values can be secrets
-        before, after = original[2], current[2]
-        changes = (
-            ('added', after.keys() - before.keys()),
-            ('removed', before.keys() - after.keys()),
-            ('changed', {key for key in before.keys() & after.keys()
-                         if before[key] != after[key]}),
-        )
-        return '; '.join(f'{change} {", ".join(sorted(keys))}'
-                         for change, keys in changes if keys)
 
     def get_sys_path(self):
         return id(sys.path), sys.path, sys.path[:]
@@ -359,12 +347,7 @@ def __exit__(self, exc_type, exc_val, exc_tb):
             current = get()
             # Check for changes to the resource's value
             if current != original:
-                reason = f"{name} was modified"
-                if name == 'os.environ':
-                    delta = self.describe_os_environ(original, current)
-                    if delta:
-                        reason = f"{reason}: {delta}"
-                support.set_environment_altered(reason)
+                support.set_environment_altered(f"{name} was modified")
                 restore(original)
                 if not self.quiet and not self.pgo:
                     print_warning(

Lib/test/libregrtest/setup.py

@@ -118,7 +118,6 @@ def setup_tests(runtests: RunTests) -> None:
         RegressionTestResult.USE_XML = True
     else:
         support.junit_xml_list = None
-    RegressionTestResult.GITHUB_ANNOTATIONS = runtests.github_annotations
 
     if runtests.memory_limit is not None:
         support.set_memlimit(runtests.memory_limit)

Lib/test/libregrtest/testresult.py

@@ -10,11 +10,10 @@
 import traceback
 import unittest
 from test import support
-from test.libregrtest.utils import github_annotation, sanitize_xml
+from test.libregrtest.utils import print_github_annotation, sanitize_xml
 
 class RegressionTestResult(unittest.TextTestResult):
     USE_XML = False
-    GITHUB_ANNOTATIONS = False
 
     def __init__(self, stream, descriptions, verbosity):
         super().__init__(stream=stream, descriptions=descriptions,
@@ -135,11 +134,10 @@ def addUnexpectedSuccess(self, test):
     def printErrorList(self, flavour, errors):
         for test, err in errors:
             super().printErrorList(flavour, [(test, err)])
-            if self.GITHUB_ANNOTATIONS:
-                # Write the annotation just after the failure report, so
-                # that it links to the report in the job log
-                self.stream.writeln(github_annotation(str(test), err,
-                                                      self._test_file(test)))
+            # Annotate just after the failure report, so that the annotation
+            # links to the report in the job log
+            print_github_annotation(str(test), err, self._test_file(test),
+                                    file=self.stream)
 
     @staticmethod
     def _test_file(test):

Lib/test/libregrtest/utils.py

@@ -153,25 +153,25 @@ def repository_path(filename: str) -> str:
     return "Lib/" + filename.removeprefix(stdlib_dir).replace(os.sep, "/")
 
 
-def github_annotation(title: str, message: str,
-                      filename: str | None) -> str:
-    """Format a GitHub Actions error annotation.
+# Escape the data and the properties of GitHub Actions workflow commands
+GITHUB_ESCAPE_DATA = str.maketrans({"%": "%25", "\r": "%0D", "\n": "%0A"})
+GITHUB_ESCAPE_PROPERTY = GITHUB_ESCAPE_DATA | str.maketrans({":": "%3A",
+                                                             ",": "%2C"})
+
+
+def print_github_annotation(title: str, message: str, filename: str | None,
+                            file=None) -> None:
+    """Print a GitHub Actions error annotation, if run in GitHub Actions.
 
     message is a traceback or a failure description. Only keep the exception
     which ends the traceback: the job log has the full traceback. Locate the
     annotation in filename, the test file, at the last frame of the traceback
     in this file, if any.
     """
+    if not os.environ.get("GITHUB_STEP_SUMMARY"):
+        return
     if not filename:
         raise ValueError(f"missing test file of annotation {title!r}")
-
-    def escape(text: str) -> str:
-        return (text.replace("%", "%25").replace("\r", "%0D")
-                .replace("\n", "%0A"))
-
-    def escape_property(text: str) -> str:
-        return escape(text).replace(":", "%3A").replace(",", "%2C")
-
     message = decolor(message)
     props = {"file": repository_path(filename)}
     # Line of the last traceback frame ('  File "filename", line 123') or
@@ -184,9 +184,10 @@ def escape_property(text: str) -> str:
     # '  File' line followed by its indented source lines
     message = re.split(r'^  File .*\n(?:    .*\n)*', message,
                        flags=re.MULTILINE)[-1].strip()
-    props_text = ",".join(f"{key}={escape_property(value)}"
+    props_text = ",".join(f"{key}={value.translate(GITHUB_ESCAPE_PROPERTY)}"
                           for key, value in props.items())
-    return f"::error {props_text}::{escape(message)}"
+    print(f"::error {props_text}::{message.translate(GITHUB_ESCAPE_DATA)}",
+          file=file, flush=True)
 
 
 def regrtest_unraisable_hook(unraisable) -> None:

Lib/test/libregrtest/worker.py

@@ -28,8 +28,6 @@ def create_worker_process(runtests: WorkerRunTests, output_fd: int,
     cmd.extend(['-m', 'test.libregrtest.worker', worker_json])
 
     env = dict(os.environ)
-    # Only the main process writes the GitHub Actions job summary
-    env.pop('GITHUB_STEP_SUMMARY', None)
     if tmp_dir is not None:
         env['TMPDIR'] = tmp_dir
         env['TEMP'] = tmp_dir

Lib/test/test_profiling/test_sampling_profiler/test_live_collector_ui.py

@@ -11,7 +11,8 @@
 import time
 import unittest
 from unittest import mock
-from test.support import requires, requires_remote_subprocess_debugging
+from test.support import (
+    os_helper, requires, requires_remote_subprocess_debugging)
 from test.support.import_helper import import_module
 
 # Only run these tests if curses is available
@@ -856,6 +857,9 @@ def mock_init_curses_side_effect(self, n_times, mock_self, stdscr):
 
     def test_run_failed_module_live(self):
         """Test that running a existing module that fails exits with clean error."""
+        # Don't write to the GitHub Actions job summary of the test suite
+        env = self.enterContext(os_helper.EnvironmentVarGuard())
+        env.unset('GITHUB_STEP_SUMMARY')
 
         args = [
             "profiling.sampling.cli", "run", "--live", "-m", "test",