From 53bcc80626090bc287f2de943e779837257b2f42 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 23 Oct 2026 01:00:28 -0400 Subject: [PATCH] ci: copy the watcher's superseded summary line to the uploaded job summary (#2078) A stopped experimental leg's only job-summary line came from util/superseded.py, which wrote $GITHUB_STEP_SUMMARY alone, so job-summary.md was never created and `Upload the job summary` found no file (run 39898485708). `watch` now takes `--summary-copy PATH`, which `Run unit tests` passes, and appends both of its summary lines there too. It is an option rather than JOB_SUMMARY_COPY so the tests the watcher runs never inherit the copy's path. --- .github/workflows/experimental.yml | 11 +- docs/source/contributing/workflows.rst | 14 +- tests/project/test_superseded_runs_1938.py | 4 +- .../test_superseded_summary_copy_2078.py | 193 ++++++++++++++++++ util/_actions.py | 3 +- util/superseded.py | 18 +- 6 files changed, 228 insertions(+), 15 deletions(-) create mode 100644 tests/project/test_superseded_summary_copy_2078.py diff --git a/.github/workflows/experimental.yml b/.github/workflows/experimental.yml index a8c6de4eea..40c8125d80 100644 --- a/.github/workflows/experimental.yml +++ b/.github/workflows/experimental.yml @@ -438,7 +438,10 @@ jobs: set -- bash -c '"$@"' bash "$@" watched=pcapkit-tests.scope fi - set -- "$WATCHER_PYTHON" util/superseded.py watch --interval 60 --grace 30 --scope "$watched" -- "$@" + # #2078: its summary line goes to the job's copy as well, through an + # option, so the tests, which inherit its environment, never see it. + set -- "$WATCHER_PYTHON" util/superseded.py watch --interval 60 --grace 30 --scope "$watched" \ + --summary-copy "$RUNNER_TEMP/job-summary.md" -- "$@" else echo "No watcher: its interpreter or requests did not install (${WATCHER:-skipped}), so the tests run unwatched." fi @@ -572,9 +575,9 @@ jobs: # #2030: the job's summary, for the report ci-reports.yml posts, as in # unit-tests.yml's `test` job. `Containment summary` and `Experimental - # summary` copy theirs to job-summary.md; `Run unit tests` does not, as - # its tests write summaries of their own, so util/superseded.py's line - # for a stopped run stays in the job's summary alone. + # summary` copy theirs to job-summary.md, and `Run unit tests` passes it + # to util/superseded.py's `--summary-copy` (#2078) rather than setting + # JOB_SUMMARY_COPY, as its tests write summaries of their own. - name: Name the job summary if: ${{ always() }} continue-on-error: true diff --git a/docs/source/contributing/workflows.rst b/docs/source/contributing/workflows.rst index b16a65c0cb..bbb735e9f8 100644 --- a/docs/source/contributing/workflows.rst +++ b/docs/source/contributing/workflows.rst @@ -1448,12 +1448,14 @@ the job summary``, which writes the job's name beside it, and ``Upload the job summary``, which uploads both as the ``job-summary--`` artifact (7 days) for the report to download; both run on ``always()`` and are ``continue-on-error``. The summaries share the room a comment leaves, smallest first, -and one cut to fit links the run. ``Run unit tests`` in ``experimental.yml`` does -not copy its summary, so the line :file:`util/superseded.py` writes there for a -stopped run stays in the job's summary alone. The post step runs only where the -report step's ``post`` output is ``true``, and sets ``GITHUB_WORKFLOW`` to the -relayed run's workflow, which :file:`util/discussion_report.py` labels the run -with. +and one cut to fit links the run. ``Run unit tests`` in ``experimental.yml`` sets +no ``JOB_SUMMARY_COPY`` either: the line :file:`util/superseded.py` writes there +for a stopped run reaches the copy through the watcher's ``--summary-copy`` +option, which the tests it runs never see (`#2078 +`__). The post step runs only +where the report step's ``post`` output is ``true``, and sets ``GITHUB_WORKFLOW`` +to the relayed run's workflow, which :file:`util/discussion_report.py` labels the +run with. ``codeql-analysis.yml`` posts to ``codeql-analysis: CodeQL report``, as ``scheduled`` on its ``schedule`` and ``push`` on a push to ``main``, and never on diff --git a/tests/project/test_superseded_runs_1938.py b/tests/project/test_superseded_runs_1938.py index a7fdf68147..966b2bd65a 100644 --- a/tests/project/test_superseded_runs_1938.py +++ b/tests/project/test_superseded_runs_1938.py @@ -879,8 +879,8 @@ def test_the_watcher_runs_the_scope_and_names_it(self) -> None: capped = 'set -- prlimit --fsize=2147483648 "$gnu_timeout" --signal=INT --kill-after=3m 110m "$@"\n' scoped = 'set -- scoped pcapkit-tests.scope "$@"\n' named = 'watched=pcapkit-tests.scope\n' - watched = ('set -- "$WATCHER_PYTHON" util/superseded.py watch --interval 60 --grace 30 --scope "$watched" ' - '-- "$@"\n') + watched = ('set -- "$WATCHER_PYTHON" util/superseded.py watch --interval 60 --grace 30 --scope "$watched" \\\n' + ' --summary-copy "$RUNNER_TEMP/job-summary.md" -- "$@"\n') self.assertLess(script.index(capped), script.index(scoped)) self.assertLess(script.index(scoped), script.index(named)) self.assertLess(script.index(named), script.index(watched)) diff --git a/tests/project/test_superseded_summary_copy_2078.py b/tests/project/test_superseded_summary_copy_2078.py new file mode 100644 index 0000000000..bef8fadec9 --- /dev/null +++ b/tests/project/test_superseded_summary_copy_2078.py @@ -0,0 +1,193 @@ +# -*- coding: utf-8 -*- +"""#2078: a stopped experimental leg's summary line reaches the uploaded job summary. + +Experimental Tests run 39898485708 uploaded no ``job-summary-*`` artifact: its +legs were stopped as superseded, and the one line their job summaries held was +util/superseded.py's, which went to ``$GITHUB_STEP_SUMMARY`` alone, so +``job-summary.md`` was never written and ``Upload the job summary`` found nothing. +The watcher now takes the copy's path as ``--summary-copy``, which ``Run unit +tests`` passes it, and the tests it runs never see. + +* :class:`TestTheSteps` runs ``Run unit tests`` and ``Name the job summary`` as + experimental.yml has them, against a local server standing in for the API, + and reads the files where ``Upload the job summary`` looks for them. +* :class:`TestTheWatchersCopy` drives util/superseded.py's ``watch`` itself. + +""" + +from __future__ import annotations + +import io +import os +import pathlib +import shutil +import signal +import sys +import tempfile +import threading +import unittest +from typing import TYPE_CHECKING + +from tests.project.test_python_versions_1660 import ROOT +from tests.project.test_resource_monitor_1740 import BASH, DEFAULT, TESTS, sh, step +from tests.project.test_superseded_runs_1938 import (NEWER, OURS, REPO, SLEEPER, FakeApi, Files, Handler, Server, + main_at) +from util import superseded + +if TYPE_CHECKING: + from typing import Dict, List, Tuple + +SAVE, UPLOAD = 'Name the job summary', 'Upload the job summary' +JOB_NAME = 'Python 3.14t (experimental)' +#: The line a stopped run's watcher writes, up to its elapsed seconds. +STOPPED = f'- Superseded: main is at {NEWER[:9]}, no longer at {OURS[:9]}. The tests were stopped after ' + + +@unittest.skipUnless(sys.platform.startswith('linux') and all(map(shutil.which, ('bash', 'timeout', 'prlimit'))), + 'the tests step runs coreutils timeout and util-linux prlimit, as on the Ubuntu runners') +class TestTheSteps(unittest.TestCase): + """The tests step, then the step that names the copy, then the upload's own paths.""" + + def setUp(self) -> None: + tmp = tempfile.TemporaryDirectory(prefix='pcapkit-2078-') # pylint: disable=consider-using-with + self.addCleanup(tmp.cleanup) + self.tmp = pathlib.Path(tmp.name) + self.temp = self.tmp / 'temp' + self.temp.mkdir() + self.seen = self.tmp / 'seen' + bindir = self.tmp / 'bin' + bindir.mkdir() + (self.temp / 'resource-monitor.log').write_text('', encoding='utf-8') + # No real sudo, so the tests run uncapped; `python`, the tests, records whether it sees the copy. + for name, body in (('sudo', 'echo "sudo: a password is required" >&2\nexit 1\n'), + ('python', f'echo "${{JOB_SUMMARY_COPY-unset}}" > {self.seen}\nexec sleep 60\n')): + (bindir / name).write_text('#!/bin/sh\n' + body, encoding='utf-8') + (bindir / name).chmod(0o755) + self.server = Server(('127.0.0.1', 0), Handler) + thread = threading.Thread(target=self.server.serve_forever, daemon=True) + thread.start() + self.addCleanup(self.server.server_close) + self.addCleanup(self.server.shutdown) + self.summary = self.tmp / 'summary.md' + self.env = {key: value for key, value in os.environ.items() + if not key.startswith('GITHUB_') and key != 'JOB_SUMMARY_COPY'} + self.env.update(RUNNER_TEMP=str(self.temp), GITHUB_WORKSPACE=str(ROOT), WORKERS='3', GRAAL_PYTHON_VM_ARGS='', + GITHUB_STEP_SUMMARY=str(self.summary), GH_TOKEN='token', + GITHUB_API_URL=f'http://127.0.0.1:{self.server.server_address[1]}', + GITHUB_EVENT_NAME='push', GITHUB_REF='refs/heads/main', GITHUB_SHA=OURS, + GITHUB_REPOSITORY=REPO, PATH=str(bindir) + os.pathsep + os.environ.get('PATH', ''), + WATCHER='success', WATCHER_PYTHON=sys.executable) + + def tests(self, **env: 'str') -> 'None': + script = step(TESTS)['run'] + quick = script.replace('watch --interval 60 --grace 30 ', 'watch --interval 0.5 --grace 2 --kill-wait 2 ') + self.assertNotEqual(quick, script) + result = sh(quick, dict(self.env, **env), DEFAULT) + self.assertEqual(result.returncode, 0 if self.server.head == NEWER else 1, result.stderr) + + def save(self) -> 'None': + save = step(SAVE) + self.assertEqual(save['env']['JOB_NAME'], 'Python ${{ matrix.python-version }} (experimental)') + result = sh(save['run'], dict(self.env, JOB_NAME=JOB_NAME), BASH) + self.assertEqual(result.returncode, 0, result.stderr) + + def uploaded(self) -> 'Dict[str, str]': + """What ``Upload the job summary`` finds, by file name: each of its paths, with ``runner.temp`` resolved.""" + found = {} + for line in step(UPLOAD)['with']['path'].splitlines(): + self.assertTrue(line.startswith('${{ runner.temp }}/'), line) + path = self.temp / line[len('${{ runner.temp }}/'):] + if path.exists(): + found[path.name] = path.read_text(encoding='utf-8') + return found + + def test_a_stopped_run_uploads_its_summary_line(self) -> None: + self.tests() + self.save() + page = self.summary.read_text(encoding='utf-8') + self.assertTrue(page.startswith(STOPPED), page) + # The copy holds exactly what the job's summary does, and the name is written beside it. + self.assertEqual(self.uploaded(), {'job-summary.md': page, 'job-summary.name': JOB_NAME + '\n'}) + + def test_the_tests_never_see_the_copy(self) -> None: + self.assertNotIn('JOB_SUMMARY_COPY', step(TESTS).get('env') or {}) + self.tests() + self.assertEqual(self.seen.read_text(encoding='utf-8'), 'unset\n') + + def test_a_run_left_to_finish_has_nothing_to_upload(self) -> None: + """A current commit's run writes no summary line here, so there is no copy, and no artifact.""" + self.server.head = OURS + bindir = pathlib.Path(self.env['PATH'].split(os.pathsep)[0]) + (bindir / 'python').write_text(f'#!/bin/sh\necho "${{JOB_SUMMARY_COPY-unset}}" > {self.seen}\nexit 1\n', + encoding='utf-8') + for watcher in ('success', 'failure'): + with self.subTest(watcher=watcher): + self.tests(WATCHER=watcher) + self.save() + self.assertEqual(self.uploaded(), {}) + self.assertEqual(self.seen.read_text(encoding='utf-8'), 'unset\n') + + +@unittest.skipUnless(os.name == 'posix', 'the watcher signals a process group, as on the Ubuntu runners') +class TestTheWatchersCopy(unittest.TestCase): + """``watch --summary-copy``: each summary line goes to the copy as well as to the job's summary.""" + + def watch(self, command: 'List[str]', copy: 'pathlib.Path', *options: 'str', + **env: 'str') -> 'Tuple[int, str, Files]': + files, out = Files(self), io.StringIO() + code = superseded.main(['watch', '--interval', '0.2', '--grace', '2', '--summary-copy', str(copy), *options, + '--', *command], env=files.env(**env), transport=FakeApi(main_at(NEWER)), out=out) + return code, out.getvalue(), files + + def test_a_stopped_runs_line_goes_to_both(self) -> None: + copy = Files(self).root / 'job-summary.md' + code, _, files = self.watch(SLEEPER, copy) + self.assertEqual(code, 0) + self.assertTrue(files.read(files.summary).startswith(STOPPED)) + self.assertEqual(files.read(copy), files.read(files.summary)) + + def test_a_run_not_stopped_has_its_line_copied_too(self) -> None: + tmp = Files(self).root + copy, pidfile = tmp / 'job-summary.md', tmp / 'pid' + command = [sys.executable, '-c', f'import os, time; open({str(pidfile)!r}, "w").write(str(os.getpid())); ' + 'time.sleep(60)'] + + def cleanup() -> 'None': + try: + os.kill(int(pidfile.read_text(encoding='utf-8')), signal.SIGKILL) + except (OSError, ValueError): + pass + self.addCleanup(cleanup) + files, out = Files(self), io.StringIO() + code = superseded.main(['watch', '--interval', '0.2', '--grace', '0.5', '--kill-wait', '0.5', + '--scope', 'pcapkit-tests.scope', '--summary-copy', str(copy), '--', *command], + env=files.env(), transport=FakeApi(main_at(NEWER)), out=out, + run=lambda argv: 1, kill=lambda pid, signum: None) + self.assertEqual(code, superseded.STOP_FAILED, out.getvalue()) + self.assertTrue(files.read(files.summary).startswith('- Superseded run not stopped: main is at')) + self.assertEqual(files.read(copy), files.read(files.summary)) + + def test_without_a_job_summary_nothing_is_copied(self) -> None: + copy = Files(self).root / 'job-summary.md' + code, _, _ = self.watch(SLEEPER, copy, GITHUB_STEP_SUMMARY='') + self.assertEqual(code, 0) + self.assertFalse(copy.exists()) + + def test_a_copy_that_cannot_be_written_is_left_out(self) -> None: + copy = Files(self).root / 'no' / 'job-summary.md' + code, out, files = self.watch(SLEEPER, copy) + self.assertEqual(code, 0) + self.assertTrue(files.read(files.summary).startswith(STOPPED)) + self.assertNotIn('Could not write', out) + + def test_a_summary_that_cannot_be_written_is_logged_and_not_copied(self) -> None: + tmp = Files(self).root + copy = tmp / 'job-summary.md' + code, out, _ = self.watch(SLEEPER, copy, GITHUB_STEP_SUMMARY=str(tmp / 'no' / 'summary.md')) + self.assertEqual(code, 0) + self.assertIn('Could not write the job summary: ', out) + self.assertFalse(copy.exists()) + + +if __name__ == '__main__': + unittest.main() diff --git a/util/_actions.py b/util/_actions.py index 7063ba53c5..fd72ac7e1e 100644 --- a/util/_actions.py +++ b/util/_actions.py @@ -26,7 +26,8 @@ whose runs :file:`util/check_suite_report.py` reports upload the copy as an artifact for the report to fold in. Only a step that writes the summary sets it, never one that runs the test suite, whose tests write summaries of their own -through this module. +through this module; :file:`util/superseded.py`, which runs the suite, takes the +copy's path as its ``--summary-copy`` option instead (#2078). """ diff --git a/util/superseded.py b/util/superseded.py index 97d884ce8d..457baabe19 100644 --- a/util/superseded.py +++ b/util/superseded.py @@ -52,6 +52,13 @@ script still runs, and every request fails, so the commit counts as current: the gate writes ``current=true``, and the watcher runs the tests to the end. +``watch --summary-copy PATH`` appends each summary line it writes to ``PATH`` +too, the job's copy of its summary that experimental.yml uploads for +util/check_suite_report.py to fold into its report (#2078). It is an option, +not util/_actions.py's ``JOB_SUMMARY_COPY`` in the step's environment, because +``COMMAND`` inherits the watcher's environment, and the tests it runs must not +write to the copy. + """ from __future__ import annotations @@ -352,12 +359,17 @@ def stop(child: 'subprocess.Popen[bytes]', grace: 'float', *, scope: 'str' = '', def watch(command: 'Sequence[str]', context: 'Context', api: 'Api', env: 'Mapping[str, str]', out: 'IO[str]', *, interval: 'float' = INTERVAL, grace: 'float' = GRACE, scope: 'str' = '', kill_wait: 'float' = KILL_WAIT, run: 'Callable[[Sequence[str]], int]' = run_command, kill: 'Callable[[int, int], None]' = signal_session, - clock: 'Callable[[], float]' = time.monotonic) -> 'int': + clock: 'Callable[[], float]' = time.monotonic, summary_copy: 'str' = '') -> 'int': """Run ``command``, stopping it once ``context``'s commit is superseded. :data:`STOP_FAILED` where it cannot be stopped: see the module's docstring. + Each summary line goes to ``summary_copy`` too, where given (#2078): it + joins ``env``, which the watcher's own writes read, and never reaches the + command, whose environment is the watcher's process environment. """ + if summary_copy: + env = dict(env, **{_actions.SUMMARY_COPY: summary_copy}) started = clock() # A session of its own, so a signal to the group reaches every process the command started. child = subprocess.Popen(list(command), start_new_session=True) # nosec B603 pylint: disable=consider-using-with @@ -414,6 +426,8 @@ def main(argv: 'Optional[List[str]]' = None, *, env: 'Optional[Mapping[str, str] watcher.add_argument('--grace', type=float, default=GRACE, help='seconds from SIGTERM to SIGKILL') watcher.add_argument('--kill-wait', type=float, default=KILL_WAIT, help='seconds to wait after SIGKILL') watcher.add_argument('--scope', default='', help='the systemd scope the command runs its tests in, if any') + watcher.add_argument('--summary-copy', default='', help="the job's copy of its summary, which gets each summary " + 'line too (#2078)') watcher.add_argument('command', nargs=argparse.REMAINDER, help='the command, after --') args = parser.parse_args(argv) @@ -428,7 +442,7 @@ def main(argv: 'Optional[List[str]]' = None, *, env: 'Optional[Mapping[str, str] if not command: parser.error('watch needs a command after --') return watch(command, context, api, env, out, interval=args.interval, grace=args.grace, scope=args.scope, - kill_wait=args.kill_wait, run=run, kill=kill) + kill_wait=args.kill_wait, run=run, kill=kill, summary_copy=args.summary_copy) parser.error('name an action: check or watch') return 2 # unreachable: parser.error exits