annotate env changed better. pgo runs

Author: cmaloney

Lib/test/libregrtest/main.py

@@ -541,11 +541,12 @@ def display_summary(self) -> None:
 
     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
+        # 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))
+                              and bool(self.verbose or self.output_on_failure
+                                       or self.pgo))
         return RunTests(
             tests,
             fail_fast=self.fail_fast,

Lib/test/libregrtest/run_workers.py

@@ -613,8 +613,6 @@ def _process_result(self, item: QueueOutput) -> TestResult:
         result = mp_result.result
         self.results.accumulate_result(result, self.runtests)
         self.display_result(mp_result)
-        if self.runtests.github_annotations:
-            result.print_github_annotation(self.runtests)
 
         # Display worker stdout
         if not self.runtests.output_on_failure:
@@ -626,6 +624,9 @@ def _process_result(self, item: QueueOutput) -> TestResult:
             stdout = mp_result.worker_stdout
             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)
 
         return result
 

Lib/test/libregrtest/save_env.py

@@ -139,6 +139,18 @@ 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[:]
@@ -347,7 +359,12 @@ def __exit__(self, exc_type, exc_val, exc_tb):
             current = get()
             # Check for changes to the resource's value
             if current != original:
-                support.set_environment_altered(f"{name} was modified")
+                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)
                 restore(original)
                 if not self.quiet and not self.pgo:
                     print_warning(

Lib/test/test_regrtest.py

@@ -38,6 +38,7 @@
 from test.libregrtest import utils
 from test.libregrtest.filter import get_match_tests, match_test
 from test.libregrtest.result import TestStats
+from test.libregrtest.save_env import saved_test_environment
 from test.libregrtest.utils import normalize_test_name
 
 if not support.has_subprocess_support:
@@ -1782,7 +1783,7 @@ def path(name):
             'parallel': ['-j2', '-W'],
             # iOS and WASI run tests in the main process: a crash kills it
             'single-process': ['--fast-ci', '--single-process'],
-            # Profile task of PGO and BOLT builds: no annotations
+            # Profile task of PGO and BOLT builds: failures stop the build
             'pgo': ['--pgo'],
         }
         for name, args in command_lines.items():
@@ -1793,14 +1794,11 @@ def path(name):
                 output, summary = self.run_tests_github(
                     *args, *tests, exitcode=EXITCODE_BAD_TEST)
 
-                annotations = []
-                if '--pgo' not in args:
-                    annotations += test_cases
-                    if '--fast-ci' in args:
-                        annotations.append((env_changed, path(env_changed),
-                                            None))
-                    if crash in tests:
-                        annotations.append((crash, path(crash), None))
+                annotations = list(test_cases)
+                if '--fast-ci' in args:
+                    annotations.append((env_changed, path(env_changed), None))
+                if crash in tests:
+                    annotations.append((crash, path(crash), None))
                 self.assertCountEqual(self.parse_github_annotations(output),
                                       annotations, output)
                 # Each test case is annotated right after its failure report
@@ -1810,6 +1808,13 @@ def path(name):
                         output,
                         rf'(?m)^(?:ERROR|FAIL): {title}\n'
                         rf'(?:(?!={{70}}$).*\n)*?::error .*title={title}::')
+                # The env changed annotation comes right after the warnings
+                if '--fast-ci' in args:
+                    self.assertRegex(
+                        output,
+                        rf'(?m)^Warning -- os\.environ was modified by '
+                        rf'{env_changed}\n(?:Warning -- .*\n)*'
+                        rf'::error .*title={env_changed}::')
 
                 # The job summary lists the failed tests in completion order
                 summary_lines = summary.splitlines()
@@ -1820,7 +1825,9 @@ def path(name):
                     [line for line in summary_lines
                      if line.startswith('### ')],
                     [test_files[name] for name in tests])
-                self.assertIn('- os.environ was modified', summary_lines)
+                self.assertIn('- os.environ was modified: '
+                              'added REGRTEST_GITHUB_ENV_CHANGED',
+                              summary_lines)
                 self.assertCountEqual(
                     re.findall(r'<summary>(.*)</summary>', summary),
                     [title for title, _, _ in test_cases])
@@ -2950,6 +2957,14 @@ def worker():
 
 
 class TestUtils(unittest.TestCase):
+    def test_describe_os_environ(self):
+        describe = saved_test_environment.describe_os_environ
+        before = {'A': '1', 'B': '2', 'C': '3'}
+        after = {'A': '1', 'B': 'secret', 'D': '4'}
+        self.assertEqual(describe((0, None, before), (0, None, after)),
+                         'added D; removed C; changed B')
+        self.assertEqual(describe((0, None, before), (1, None, before)), '')
+
     def test_format_duration(self):
         self.assertEqual(utils.format_duration(0),
                          '0 ms')