diff --git a/.github/workflows/create-release.yml b/.github/workflows/create-release.yml index 491bc1354d..169312c7ca 100644 --- a/.github/workflows/create-release.yml +++ b/.github/workflows/create-release.yml @@ -275,6 +275,11 @@ jobs: released="$(mktemp -d)" git show "${from}:${tiers}" > "${released}/python-versions.toml" git show "${from}:util/python_versions.py" > "${released}/python_versions.py" + # #2047: the reader imports util/_actions.py from beside it, from the + # commit that has it. + if git cat-file -e "${from}:util/_actions.py" 2> /dev/null; then + git show "${from}:util/_actions.py" > "${released}/_actions.py" + fi wheels="$(python3 "${released}/python_versions.py" --file "${released}/python-versions.toml" pypi-wheels)" builds="$(python3 "${released}/python_versions.py" --file "${released}/python-versions.toml" conda-builds)" fi diff --git a/.github/workflows/deploy-pages.yml b/.github/workflows/deploy-pages.yml index 9ada8b8bd7..43afdb4616 100644 --- a/.github/workflows/deploy-pages.yml +++ b/.github/workflows/deploy-pages.yml @@ -199,7 +199,8 @@ jobs: # `deploy-pages` ran or `head`'s gate failed, so never on a pull request's # run or a verdict for another commit, # which skip `deploy-pages` deliberately. It reads no artifact, and checks - # out the script alone. The job and its step are `continue-on-error`; + # out the script and util/_actions.py, which it imports (#2047), alone. The + # job and its step are `continue-on-error`; # util/discussion_report.py writes a warning rather than failing. discussion: name: Post the Pages report @@ -218,6 +219,7 @@ jobs: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: sparse-checkout: | + util/_actions.py util/discussion_report.py sparse-checkout-cone-mode: false persist-credentials: false diff --git a/.github/workflows/experimental.yml b/.github/workflows/experimental.yml index 0b8c44a352..48eb0367e1 100644 --- a/.github/workflows/experimental.yml +++ b/.github/workflows/experimental.yml @@ -54,6 +54,7 @@ jobs: with: sparse-checkout: | .github/python-versions.toml + util/_actions.py util/python_versions.py util/superseded.py sparse-checkout-cone-mode: false @@ -100,6 +101,7 @@ jobs: with: sparse-checkout: | .github/python-versions.toml + util/_actions.py util/python_versions.py sparse-checkout-cone-mode: false persist-credentials: false diff --git a/.github/workflows/project-status.yml b/.github/workflows/project-status.yml index 3b91d05487..56252e8330 100644 --- a/.github/workflows/project-status.yml +++ b/.github/workflows/project-status.yml @@ -12,9 +12,10 @@ name: Project Status # the classic "pwn request". The job below: # # * never checks out the repository, at any ref, and has no `uses:` step at all; -# * fetches util/project_status.py through the API at `github.sha`, which under +# * fetches util/project_status.py, and util/_actions.py, which it imports +# (#2047), through the API at `github.sha`, which under # `pull_request_target` is the tip of the *base* branch, never the PR head -- -# so editing the script in a pull request changes nothing about its own run; +# so editing either in a pull request changes nothing about its own run; # * takes nothing from the event payload but the item number, passed through # `env:` rather than interpolated into the script, and re-reads labels and # state from the API. @@ -84,10 +85,13 @@ jobs: exit 0 fi - # The base branch's copy of the script -- see the security note above. - gh api -H 'Accept: application/vnd.github.raw' \ - "repos/${GITHUB_REPOSITORY}/contents/util/project_status.py?ref=${SCRIPT_REF}" \ - > "${RUNNER_TEMP}/project_status.py" + # The base branch's copy of the script, and of util/_actions.py, which + # it imports from beside it (#2047) -- see the security note above. + for script in project_status.py _actions.py; do + gh api -H 'Accept: application/vnd.github.raw' \ + "repos/${GITHUB_REPOSITORY}/contents/util/${script}?ref=${SCRIPT_REF}" \ + > "${RUNNER_TEMP}/${script}" + done if [ -n "$ITEM" ]; then python3 "${RUNNER_TEMP}/project_status.py" --item "$ITEM" diff --git a/.github/workflows/python-compatibility.yml b/.github/workflows/python-compatibility.yml index 19619eadb8..68d0cb6abc 100644 --- a/.github/workflows/python-compatibility.yml +++ b/.github/workflows/python-compatibility.yml @@ -33,6 +33,7 @@ jobs: with: sparse-checkout: | .github/python-versions.toml + util/_actions.py util/python_versions.py sparse-checkout-cone-mode: false persist-credentials: false diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index 7a161319ba..3535123a10 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -67,6 +67,7 @@ jobs: with: sparse-checkout: | .github/python-versions.toml + util/_actions.py util/python_versions.py sparse-checkout-cone-mode: false persist-credentials: false diff --git a/docs/source/contributing/workflows.rst b/docs/source/contributing/workflows.rst index 8459421e37..54479eae54 100644 --- a/docs/source/contributing/workflows.rst +++ b/docs/source/contributing/workflows.rst @@ -503,8 +503,10 @@ release waits for the next Vendor Update on ``main``. that runs with a secret on an event a fork can cause: it writes the project board's *Status* field with ``PROJECT_TOKEN``, which a fork's ``pull_request`` run would not receive. It therefore never checks out the repository and has no ``uses:`` step. It -fetches :file:`util/project_status.py` through the API at ``github.sha`` -- the base -branch's tip under that event -- and takes only the item number from the payload. +fetches :file:`util/project_status.py`, and :file:`util/_actions.py`, which it imports +(`#2047 `__), through the API at +``github.sha`` -- the base branch's tip under that event -- and takes only the item +number from the payload. ``tests/project/test_project_status.py`` asserts this over the file. Without the secret it skips with a ``::notice::``. The Status mapping it applies is documented in :doc:`conventions/process`. @@ -1365,7 +1367,8 @@ does while its findings stand, and ``failure`` where actionlint failed the job. ``cron-vendor.yml``'s rules: ``push`` on its ``workflow_run``, ``scheduled`` on its ``schedule``, and never on a pull request or a verdict for another commit. Its report is the results of ``head``, ``deploy-pages`` and ``publish-pages``, so it -uploads no artifact, and its ``discussion`` job checks out the script alone. The +uploads no artifact, and its ``discussion`` job checks out the script and +:file:`util/_actions.py`, which it imports, alone. The ``--status`` is the publish's result after a green build, the build's where the build did not pass, and the gate's where the gate skipped the build. diff --git a/tests/project/test_actions_helper_2047.py b/tests/project/test_actions_helper_2047.py index 60ad17b08e..475bc68ded 100644 --- a/tests/project/test_actions_helper_2047.py +++ b/tests/project/test_actions_helper_2047.py @@ -8,6 +8,8 @@ function's exact output, GitHub's escaping included. * :class:`TestScriptsImportIt` loads each script that imports the helper in the ways the steps and the tests do, and runs it as the steps do. +* :class:`TestTheStepsCheckItOut` holds every sparse checkout of such a script + to listing the helper too. """ @@ -29,10 +31,11 @@ from typing import TYPE_CHECKING from unittest import mock +from tests._workflows import WORKFLOWS, load from util import _actions if TYPE_CHECKING: - from typing import Iterator, Optional + from typing import Any, Iterator, Optional #: Test tier (#1958): reads and runs the tree's own scripts, a question about the tree. __tier__ = 'LATEST' @@ -41,9 +44,10 @@ HELPER = ROOT / 'util' / '_actions.py' #: The scripts that import the helper. SCRIPTS = ('apt_packages', 'check_suite_report', 'ci_timing', 'classify_changes', 'codeql_report', - 'coverage_report', 'leg_summary', 'pip_install', 'regenerate_samples', 'release_status', 'release_tags', - 'release_upload', 'release_vendor_update', 'update_python_versions', 'vendor_code_changed', - 'vendor_test_gate') + 'coverage_report', 'discussion_report', 'leg_summary', 'pip_install', 'project_status', + 'python_versions', 'regenerate_samples', 'release_status', 'release_tags', 'release_upload', + 'release_vendor_update', 'superseded', 'update_python_versions', 'vendor_code_changed', + 'vendor_test_gate', 'verdict_covers_checkout') #: The top-level modules the helper may import, each in Python 3.7's standard #: library: the running interpreter's :data:`sys.stdlib_module_names` would #: pass ``tomllib``, which 3.7 lacks. @@ -56,16 +60,21 @@ 'classify_changes': ([], {'OWN_PR': 'false'}), 'codeql_report': (['--help'], {}), 'coverage_report': (['--help'], {}), + 'discussion_report': (['--help'], {}), 'leg_summary': (['ktlo'], {'LEG': 'Python 3.8 (KTLO, non-blocking)', 'STEPS': '{}'}), 'pip_install': (['--help'], {}), + 'project_status': (['--help'], {}), + 'python_versions': (['latest'], {}), 'regenerate_samples': (['--help'], {}), 'release_status': (['--help'], {}), 'release_tags': (['--help'], {}), 'release_upload': (['--help'], {}), 'release_vendor_update': (['--help'], {}), + 'superseded': (['--help'], {}), 'update_python_versions': (['--help'], {}), 'vendor_code_changed': (['--help'], {}), 'vendor_test_gate': (['--help'], {}), + 'verdict_covers_checkout': (['--help'], {}), } #: ``-X importtime``'s line for a module, by its name. IMPORT_TIME = re.compile(r'^import time:.*\|\s*(\S+)\s*$', re.M) @@ -330,5 +339,42 @@ def test_run_as_the_steps_run_it(self) -> None: self.assertEqual(output.read_text(encoding='utf-8'), 'code=true\n') +def sparse_checkouts_missing_the_helper(spec: 'Any') -> 'list[tuple[str, list[str]]]': + """Each job of the workflow ``spec`` whose sparse checkout lists a script of :data:`SCRIPTS` but not the helper.""" + found = [] + for job_id, job in (spec.get('jobs') or {}).items(): + for step in job.get('steps') or []: + listed = str((step.get('with') or {}).get('sparse-checkout') or '').split() + scripts = [path for path in listed if path.startswith('util/') and path.endswith('.py') + and path[len('util/'):-len('.py')] in SCRIPTS] + if scripts and 'util/_actions.py' not in listed: + found.append((job_id, scripts)) + return found + + +class TestTheStepsCheckItOut(unittest.TestCase): + """A sparse checkout of a script that imports the helper holds the helper too, or the import fails.""" + + def test_every_sparse_checkout_of_a_script_lists_the_helper(self) -> None: + checked = 0 + for path in sorted(WORKFLOWS.glob('*.yml')): + with self.subTest(workflow=path.name): + spec = load(path.name) + self.assertEqual(sparse_checkouts_missing_the_helper(spec), []) + checked += sum('sparse-checkout' in (step.get('with') or {}) for job in spec['jobs'].values() + for step in job.get('steps') or []) + self.assertGreater(checked, 0, 'no sparse checkout was found to check') + + def test_the_check_sees_a_missing_helper(self) -> None: + def spec(*listed: 'str') -> 'Any': + return {'jobs': {'j': {'steps': [{'with': {'sparse-checkout': ''.join(f'{p}\n' for p in listed)}}]}}} + + self.assertEqual(sparse_checkouts_missing_the_helper(spec('.github/python-versions.toml', + 'util/python_versions.py')), + [('j', ['util/python_versions.py'])]) + self.assertEqual(sparse_checkouts_missing_the_helper(spec('util/_actions.py', 'util/python_versions.py')), []) + self.assertEqual(sparse_checkouts_missing_the_helper(spec('util/no_helper_here.py')), []) + + if __name__ == '__main__': unittest.main() diff --git a/tests/project/test_discussion_report_2030.py b/tests/project/test_discussion_report_2030.py index dac603dc16..957eebf894 100644 --- a/tests/project/test_discussion_report_2030.py +++ b/tests/project/test_discussion_report_2030.py @@ -438,6 +438,66 @@ def test_a_bad_command_line_exits_2(self) -> None: self.assertEqual(self.calls(), []) +class TestTheWorkflowCommands(_FakeGitHub): + """#2047: the whole of what the script prints and appends to the summary, byte for byte. + + The tests above read the warning with ``assertIn``; these pin each run's whole + standard output and summary file, with text each escape changes, so moving the + writes onto util/_actions.py can be shown to change nothing. + + """ + + def test_a_post_prints_two_lines_and_appends_one(self) -> None: + title = 'ci-timing: 50% [odd], title' + self.summary.write_bytes(b'before\n') + result = self.script(title=title) + url = f'https://github.com/{REPO}/discussions/100' + self.assertEqual((result.returncode, result.stdout, result.stderr), + (0, f"Opened the thread {title!r}: {url}\nPosted the report: {url}#discussioncomment-1\n", '')) + self.assertEqual(self.summary.read_bytes(), + f'before\nThe report was posted to [{title}]({url}#discussioncomment-1).\n'.encode()) + # A second post finds the thread, and appends a second line. + result = self.script(title=title) + self.assertEqual(result.stdout, f'Posted the report: {url}#discussioncomment-2\n') + self.assertEqual(self.summary.read_bytes().count(b'\n'), 3) + + def test_no_summary_file_or_one_that_will_not_open(self) -> None: + """Unset or empty, nothing is written; a directory, the post still succeeds, silently.""" + url = f'https://github.com/{REPO}/discussions/100' + for name, value in (('unset', None), ('empty', ''), ('a directory', str(self.tmp))): + with self.subTest(summary=name): + self.update(discussions=[]) + env = self.env() + if value is None: + del env['GITHUB_STEP_SUMMARY'] + else: + env['GITHUB_STEP_SUMMARY'] = value + result = self.script(env=env) + self.assertEqual((result.returncode, result.stdout, result.stderr), + (0, f"Opened the thread {TITLE!r}: {url}\nPosted the report: {url}" + '#discussioncomment-1\n', '')) + self.assertFalse(self.summary.exists()) + + def test_each_warning_is_one_escaped_line(self) -> None: + """``%`` and LF in the message escape; the title, which holds neither ``:`` nor ``,``, is as written. + + The stand-in's standard error reaches the script through universal + newlines, so a CR arrives as LF; the helper's own tests pin CR. + + """ + self.update(fail={'op': 'discussionCategories', 'message': 'a 50%\nb: c, d'}) + self.summary.write_bytes(b'before\n') + result = self.script() + self.assertEqual((result.returncode, result.stdout), + (0, '::warning title=CI report not posted::gh api graphql exited 1: gh: a 50%25%0Ab: ' + 'c, d. The report was not posted.\n')) + self.assertEqual(self.summary.read_bytes(), b'before\n') + missing = self.tmp / '50%.md' + result = self.script(report=missing) + self.assertEqual(result.stdout, f"::warning title=CI report not posted::the report '{self.tmp}/50%25.md' " + 'could not be read (No such file or directory), so nothing was posted.\n') + + #: ``(workflow, reporting job, the thread's title, the artifact, the file in it)``. POSTERS = ( ('ci-timing.yml', 'report', 'ci-timing: weekly CI timing report', 'ci-timing-report', 'report.md'), diff --git a/tests/project/test_lint_pages_reports_2030.py b/tests/project/test_lint_pages_reports_2030.py index d40ace2837..b093c587eb 100644 --- a/tests/project/test_lint_pages_reports_2030.py +++ b/tests/project/test_lint_pages_reports_2030.py @@ -159,12 +159,13 @@ def test_lint_hands_its_report_on(self) -> None: self.assertEqual(download['with'], {'name': 'lint-report', 'path': 'report'}) self.assertIs(download['continue-on-error'], True) - def test_pages_reads_no_artifact_and_checks_out_the_script_alone(self) -> None: + def test_pages_reads_no_artifact_and_checks_out_the_script_and_its_helper(self) -> None: job = load(PAGES)['jobs']['discussion'] uses = [entry['uses'] for entry in job['steps'] if 'uses' in entry] self.assertEqual(len(uses), 1) self.assertTrue(pinned(uses[0], 'actions/checkout')) - self.assertEqual(job['steps'][0]['with'], {'sparse-checkout': 'util/discussion_report.py\n', + # The script imports util/_actions.py (#2047). + self.assertEqual(job['steps'][0]['with'], {'sparse-checkout': 'util/_actions.py\nutil/discussion_report.py\n', 'sparse-checkout-cone-mode': False, 'persist-credentials': False}) diff --git a/tests/project/test_project_status.py b/tests/project/test_project_status.py index 58be0365aa..fea9c9ccb3 100644 --- a/tests/project/test_project_status.py +++ b/tests/project/test_project_status.py @@ -39,6 +39,10 @@ import os import pathlib import re +import shutil +import subprocess # nosec B404 +import sys +import tempfile import unittest from unittest import mock @@ -803,5 +807,109 @@ def test_option_ids_come_from_the_api(self) -> None: self.assertEqual(fake.mutations[-1], ('set', 'PVTI_new', 'live-Blocked')) +class TestTheWorkflowCommands(unittest.TestCase): + """#2047: the whole of what the script prints, byte for byte, its annotations through util/_actions.py.""" + + def run_main(self, argv, fake, token='x'): + env = {} if token is None else {'GH_TOKEN': token} + out = io.StringIO() + with mock.patch.dict(os.environ, env, clear=True), contextlib.redirect_stdout(out): + code = project_status.main(argv, graphql=fake, today=datetime.date(2026, 10, 5)) + return code, out.getvalue() + + def test_a_missing_token(self) -> None: + fake = FakeGraphQL([item(1, 'wip')]) + for token in (None, ''): + with self.subTest(token=token): + self.assertEqual(self.run_main(['--item', '1'], fake, token=token), + (0, '::notice title=Project Status::GH_TOKEN is not set (the PROJECT_TOKEN secret ' + 'is missing), so the project board was not updated.\n')) + + def test_a_truncated_reconcile(self) -> None: + """One warning per search, then the sync's own lines; ``:`` and ``,`` in a message are as written.""" + fake = FakeGraphQL([item(1, 'wip', on_board='Pending')], issue_count=1001) + self.assertEqual(self.run_main(['--reconcile', '--repo', 'JarryShaw/PyPCAPKit'], fake), (0, ( + "::warning title=Project Status::search 'repo:JarryShaw/PyPCAPKit is:open' matched 1001 item(s) but " + 'returned 1; this reconcile is partial (search caps at 1000).\n' + "::warning title=Project Status::search 'repo:JarryShaw/PyPCAPKit is:closed closed:>=2026-09-28' " + 'matched 1001 item(s) but returned 0; this reconcile is partial (search caps at 1000).\n' + "#1: set Status to 'WIP'\n" + '1 of 1 item(s) changed\n'))) + + def test_a_percent_in_the_repository_is_escaped(self) -> None: + """Before util/_actions.py the query went out as written; ``%`` is not in a repository name GitHub allows.""" + fake = FakeGraphQL([], issue_count=1) + code, printed = self.run_main(['--reconcile', '--repo', 'o/50%'], fake) + self.assertEqual(code, 0) + self.assertEqual(printed.splitlines()[0], "::warning title=Project Status::search 'repo:o/50%25 is:open' " + 'matched 1 item(s) but returned 0; this reconcile is partial ' + '(search caps at 1000).') + + +#: The stand-in ``gh``: the contents API answered from the tree at :data:`ROOT` +#: for the ref ``$FAKE_REF``, logged to ``$FAKE_LOG``; GraphQL refused. +FAKE_GH = '''\ +import os, pathlib, sys + +with open(os.environ['FAKE_LOG'], 'a', encoding='utf-8') as log: + log.write(' '.join(sys.argv[1:]) + '\\n') +if sys.argv[1:3] == ['api', 'graphql']: + sys.exit('gh: no network here') +path, _, ref = sys.argv[-1].partition('?ref=') +prefix = 'repos/' + os.environ['GITHUB_REPOSITORY'] + '/contents/' +if sys.argv[1:4] != ['api', '-H', 'Accept: application/vnd.github.raw'] or not path.startswith(prefix) \\ + or ref != os.environ['FAKE_REF']: + sys.exit('unexpected: %r' % (sys.argv[1:],)) +sys.stdout.write((pathlib.Path(os.environ['FAKE_ROOT']) / path[len(prefix):]).read_text(encoding='utf-8')) +''' + + +class TestTheStepRunsTheFetchedScript(unittest.TestCase): + """#2047: the step fetches util/_actions.py beside the script, at the same ref, and the script imports it.""" + + def setUp(self) -> None: + if os.name == 'nt' or shutil.which('bash') is None: # pragma: no cover + self.skipTest('the step is a bash script') + tmp = tempfile.TemporaryDirectory(prefix='pcapkit-2047-') # pylint: disable=consider-using-with + self.addCleanup(tmp.cleanup) + self.tmp = pathlib.Path(tmp.name) + self.bin = self.tmp / 'bin' + self.bin.mkdir() + (self.bin / 'gh.py').write_text(FAKE_GH, encoding='utf-8') + for name, text in (('gh', f'#!/bin/sh\nexec "{sys.executable}" "{self.bin / "gh.py"}" "$@"\n'), + ('python3', f'#!/bin/sh\nexec "{sys.executable}" "$@"\n')): + (self.bin / name).write_text(text, encoding='utf-8') + (self.bin / name).chmod(0o755) + self.temp = self.tmp / 'runner-temp' + self.temp.mkdir() + self.log = self.tmp / 'gh.log' + [self.script] = [value for path, value in scan(WORKFLOW.read_text(encoding='utf-8')) if path[-1] == 'run'] + env = {key: value for key, value in os.environ.items() if not key.startswith(('GITHUB_', 'GH_', 'PYTHON'))} + self.env = dict(env, PATH=f'{self.bin}{os.pathsep}{env.get("PATH", "")}', GH_TOKEN='x', ITEM='7', + SCRIPT_REF='0123abc', FAKE_REF='0123abc', FAKE_ROOT=str(ROOT), FAKE_LOG=str(self.log), + GITHUB_REPOSITORY='JarryShaw/PyPCAPKit', RUNNER_TEMP=str(self.temp)) + + def test_both_files_are_fetched_and_the_import_finds_the_helper(self) -> None: + result = subprocess.run(['bash', '-e', '-c', self.script], cwd=self.tmp, env=self.env, # nosec B603 B607 + capture_output=True, text=True, check=False, timeout=60) + # The board cannot be read here, so the run ends there: after the import. + self.assertNotIn('ModuleNotFoundError', result.stderr) + self.assertIn('RuntimeError: gh api graphql failed (1): gh: no network here', result.stderr) + self.assertNotEqual(result.returncode, 0) + contents = 'api -H Accept: application/vnd.github.raw repos/JarryShaw/PyPCAPKit/contents/util/{}?ref=0123abc' + self.assertEqual(self.log.read_text(encoding='utf-8').splitlines()[:2], + [contents.format('project_status.py'), contents.format('_actions.py')]) + self.assertEqual(sorted(path.name for path in self.temp.glob('*.py')), ['_actions.py', 'project_status.py']) + for name in ('_actions.py', 'project_status.py'): + self.assertEqual((self.temp / name).read_bytes(), (ROOT / 'util' / name).read_bytes()) + # And the copy imports the copy beside it, not the tree's. + imported = subprocess.run([sys.executable, '-X', 'importtime', str(self.temp / 'project_status.py'), '--help'], + cwd=self.tmp, env=self.env, capture_output=True, text=True, check=False, timeout=60) + self.assertEqual(imported.returncode, 0, imported.stderr[-2000:]) + modules = re.findall(r'^import time:.*\|\s*(\S+)\s*$', imported.stderr, re.M) + self.assertIn('_actions', modules) + self.assertNotIn('util', modules) + + if __name__ == '__main__': unittest.main() diff --git a/tests/project/test_python_versions_1660.py b/tests/project/test_python_versions_1660.py index d0fd956c94..08e9cd1dca 100644 --- a/tests/project/test_python_versions_1660.py +++ b/tests/project/test_python_versions_1660.py @@ -478,6 +478,50 @@ def test_the_command_line_writes_each_output_as_the_runner_reads_it(self) -> Non self.assertEqual(result.returncode, 0, result.stdout + result.stderr) self.assertEqual(self.parse_github_output(output.read_text(encoding='utf-8')), live_outputs()) + def test_the_workflow_commands_byte_for_byte(self) -> None: + """#2047: the whole of each run's output, so moving it onto util/_actions.py can be shown to change nothing. + + Each multi-line output's delimiter is fresh, so it is read as ``EOF`` once + each is shown to open and close one value, and to differ from the others. + + """ + values = live_outputs() + output = self.tmp / 'github-output' + output.write_bytes(b'before\n') + self.assertEqual(self.main('--github-output', github_output=str(output)), + (0, ''.join(f'{name}: {value}'.replace('\n', ' ') + '\n' for name, value in values.items()))) + written = output.read_bytes().decode('utf-8') + delimiters = re.findall(r'^[\w-]+<<(EOF_[0-9a-f]{32})$', written, re.M) + self.assertEqual(len(delimiters), sum('\n' in value for value in values.values())) + self.assertEqual(len(set(delimiters)), len(delimiters)) + for delimiter in delimiters: + self.assertEqual(written.count(delimiter), 2) + self.assertEqual(re.sub(r'EOF_[0-9a-f]{32}', 'EOF', written), 'before\n' + ''.join( + f'{name}< None: + """#2047: ``--file``'s ``%`` and line breaks are escaped, as the error text's always were. + + Before util/_actions.py the path went out as written, so a ``%`` in it read + as the start of an escape, and a line break split the command in two. + + """ + missing = self.tmp / '50%\nmissing.toml' + escaped = str(missing).replace('%', '%25').replace('\n', '%0A') + repr_escaped = repr(str(missing)).replace('%', '%25') + self.assertEqual(self.main('--file', str(missing), 'latest'), + (1, f'::error title=Cannot read the Python version tiers::{escaped}: [Errno 2] No such file ' + f'or directory: {repr_escaped}\n')) + def test_the_script_runs_on_every_runners_own_python3(self) -> None: """Standard library only, tomllib's 3.11 the floor, and each image that runs it newer. @@ -491,8 +535,10 @@ def test_the_script_runs_on_every_runners_own_python3(self) -> None: for alias in node.names} imported |= {node.module.split('.')[0] for node in ast.walk(tree) if isinstance(node, ast.ImportFrom) and node.module and not node.level} - self.assertLessEqual(imported, {'__future__', 'argparse', 'json', 'os', 'pathlib', 're', 'sys', - 'tomllib', 'typing', 'uuid'}) + # util/_actions.py (#2047), as `_actions` or `util._actions`, is held to the + # standard library by tests/project/test_actions_helper_2047.py. + self.assertLessEqual(imported, {'__future__', '_actions', 'argparse', 'json', 'os', 'pathlib', 're', 'sys', + 'tomllib', 'typing', 'util'}) self.assertIn('tomllib', imported) found = 0 for name, spec in workflows(): @@ -812,7 +858,7 @@ def test_the_coverage_leg_and_the_sdist_leg_are_latest(self) -> None: self.assertIsNotNone(condition, step['if']) self.assertEqual(resolve('create-release.yml', 'pypi', '${{ %s }}' % condition.group(1)), latest) # type: ignore[union-attr] - def test_the_versions_jobs_read_two_files_and_write_nothing(self) -> None: + def test_the_versions_jobs_read_the_tiers_and_write_nothing(self) -> None: """``unit-tests.yml`` is a reusable workflow whose callers grant only ``contents: read``.""" for name in ('unit-tests.yml', 'python-compatibility.yml'): spec = workflow(name) @@ -822,7 +868,7 @@ def test_the_versions_jobs_read_two_files_and_write_nothing(self) -> None: checkout = job['steps'][0] self.assertTrue(checkout['uses'].startswith('actions/checkout@')) self.assertEqual(checkout['with']['sparse-checkout'].split(), - ['.github/python-versions.toml', 'util/python_versions.py']) + ['.github/python-versions.toml', 'util/_actions.py', 'util/python_versions.py']) self.assertIs(checkout['with']['sparse-checkout-cone-mode'], False) self.assertEqual(job['steps'][1]['run'].strip(), SCRIPT_STEP) self.assertNotIn('if', job) diff --git a/tests/project/test_release_gates.py b/tests/project/test_release_gates.py index b9f16f78be..a79b32e28a 100644 --- a/tests/project/test_release_gates.py +++ b/tests/project/test_release_gates.py @@ -4197,8 +4197,9 @@ def setUp(self) -> None: (seed / 'conda' / 'requirements.txt').write_text( '\n'.join(self.REQUIREMENTS) + '\n', encoding='utf-8') (seed / 'util').mkdir() - shutil.copyfile(ROOT / 'util' / 'verdict_covers_checkout.py', - seed / 'util' / 'verdict_covers_checkout.py') + # It imports util/_actions.py from beside it (#2047). + for script in ('verdict_covers_checkout.py', '_actions.py'): + shutil.copyfile(ROOT / 'util' / script, seed / 'util' / script) self._git(seed, 'add', '.') self._git(seed, 'commit', '-m', 'base') self._git(seed, 'push', self.REMOTE, 'HEAD:refs/heads/main') diff --git a/tests/project/test_release_tags_the_run_sha_1634.py b/tests/project/test_release_tags_the_run_sha_1634.py index 5dc775c3bf..34d4b80623 100644 --- a/tests/project/test_release_tags_the_run_sha_1634.py +++ b/tests/project/test_release_tags_the_run_sha_1634.py @@ -1160,6 +1160,8 @@ def test_both_outputs_reach_the_jobs(self) -> None: #: The tier file as #1677 added it, before the first weekly proposal, and its reader. FIRST_TIERS = ROOT / 'tests' / 'project' / 'python_versions_fixtures' / 'python-versions.toml' READER = ROOT / 'util' / 'python_versions.py' +#: What :data:`READER` imports from beside it (#2047). +HELPER = ROOT / 'util' / '_actions.py' #: A ``curl`` answering 200 with ``$REPLY`` as the body it writes to ``-o``. FAKE_CURL = '#!/bin/sh\nwhile [ $# -gt 0 ]; do [ "$1" = -o ] && cp "$REPLY" "$2"; shift; done\nprintf 200\n' @@ -1195,6 +1197,8 @@ def setUp(self) -> None: (self.seed / 'util').mkdir() shutil.copyfile(FIRST_TIERS, tiers) shutil.copyfile(READER, self.seed / 'util' / 'python_versions.py') + # #2047: the reader imports the helper from beside it. + shutil.copyfile(HELPER, self.seed / 'util' / '_actions.py') self.added = self.commit(self.VERSION, '0', 'the tier file') self.values = {self.added: self.read(tiers)} line = '"pypy3.11" = { tier = "supported" }\n' @@ -1288,6 +1292,26 @@ def test_a_tag_older_than_the_tier_file_counts_the_tiers_it_was_added_with(self) self.assertIn(f'::notice title=Released before the tier file::v{self.VERSION} predates ' f'.github/python-versions.toml; counting the tiers {self.added} added it with.', result.stdout) + def test_the_released_reader_runs_with_its_own_helper(self) -> None: + """#2047: the reader is copied out of the tree with the helper it imports, where its commit has one.""" + self.push(self.added, f'refs/tags/v{self.VERSION}') + self.checkout(self.grown) + result, written = self.released(exists=True) + self.assertEqual(written, self.values[self.added]) + self.assertNotIn('Error', result.stdout + result.stderr) + + def test_a_reader_from_before_the_helper_runs_alone(self) -> None: + """A commit without util/_actions.py, whose reader imports nothing from it, is read as before.""" + (self.seed / 'util' / '_actions.py').unlink() + (self.seed / 'util' / 'python_versions.py').write_text( + "import sys\nprint({'pypi-wheels': 'cp37 cp38', 'conda-builds': 'py37 py38'}[sys.argv[-1]])\n", + encoding='utf-8') + older = self.commit(self.VERSION, '0', 'a reader from before the helper') + self.push(older, f'refs/tags/v{self.VERSION}') + self.checkout(self.grown) + _, written = self.released(exists=True) + self.assertEqual(written, {'pypi-wheels': 'cp37 cp38', 'conda-builds': 'py37 py38'}) + def test_a_finished_release_reads_complete_after_the_tiers_grow(self) -> None: """The two checks, fed the step's answer and, as before it, this run's tiers.""" self.push(self.added, f'refs/tags/v{self.VERSION}') diff --git a/tests/project/test_superseded_runs_1938.py b/tests/project/test_superseded_runs_1938.py index 7e24a976d6..8d215d016b 100644 --- a/tests/project/test_superseded_runs_1938.py +++ b/tests/project/test_superseded_runs_1938.py @@ -23,6 +23,7 @@ import json import os import pathlib +import re import shutil import signal import subprocess # nosec B404 @@ -626,6 +627,122 @@ def test_an_unwatched_event_never_asks(self) -> None: self.assertEqual((code, api.requests), (0, [])) +#: A pull request state each escape of a message changes: ``%``, CR and LF. +ODD_STATE = 'odd 50%\r\nstate: a, b' +#: :data:`ODD_STATE` as a message holds it. +ODD_DATA = 'odd 50%25%0D%0Astate: a, b' + + +class TestTheWorkflowCommands(unittest.TestCase): + """#2047: the whole of what ``check`` and ``watch`` print, append to the summary and output, byte for byte. + + The tests above read the commands one by one; these pin each run's whole + standard output and files, with text each escape changes, so moving the + writes onto util/_actions.py can be shown to change nothing. + + """ + + def setUp(self) -> None: + tmp = tempfile.TemporaryDirectory(prefix='pcapkit-2047-') # pylint: disable=consider-using-with + self.addCleanup(tmp.cleanup) + self.event = pathlib.Path(tmp.name, 'event.json') + self.event.write_text(json.dumps({'pull_request': {'number': 1938, 'head': {'sha': OURS}}}), + encoding='utf-8') + + def run_main(self, argv: 'List[str]', files: 'Files', **env: 'str') -> 'Tuple[int, str]': + out = io.StringIO() + code = superseded.main(argv, env=files.env(GITHUB_EVENT_NAME='pull_request', + GITHUB_EVENT_PATH=str(self.event), **env), + transport=FakeApi(pull_is(ODD_STATE)), out=out, + run=lambda argv: 1, kill=lambda pid, signum: None) + return code, out.getvalue() + + def test_the_gate(self) -> None: + for superseded_state, current in ((ODD_STATE, 'false'), ('open', 'true')): + with self.subTest(state=superseded_state): + files = Files(self) + files.output.write_bytes(b'before\n') + files.summary.write_bytes(b'before\n') + out = io.StringIO() + code = superseded.main(['check'], env=files.env(GITHUB_EVENT_NAME='pull_request', + GITHUB_EVENT_PATH=str(self.event)), + transport=FakeApi(pull_is(superseded_state)), out=out) + self.assertEqual(code, 0) + self.assertEqual(files.output.read_bytes(), f'before\ncurrent={current}\n'.encode()) + skipped = 'The rest of this run is skipped, so none of its jobs fails or is cancelled.' + if current == 'true': + self.assertEqual(out.getvalue(), f'current: {OURS[:9]} is the head of pull request #1938\n') + self.assertEqual(files.summary.read_bytes(), b'before\n') + continue + self.assertEqual(out.getvalue(), f'superseded: pull request #1938 is {ODD_STATE}\n' + f'::notice title=Superseded::pull request #1938 is {ODD_DATA}. ' + f'{skipped}\n') + self.assertEqual(files.summary.read_bytes(), + f'before\n- Superseded: pull request #1938 is {ODD_STATE}. {skipped}\n'.encode()) + + def test_no_output_or_summary_file(self) -> None: + """Unset or empty, neither is written; a summary that will not open is said in the log.""" + for name, value in (('unset', None), ('empty', '')): + with self.subTest(files=name): + files = Files(self) + env = files.env() + for key in ('GITHUB_OUTPUT', 'GITHUB_STEP_SUMMARY'): + if value is None: + del env[key] + else: + env[key] = value + code = superseded.main(['check'], env=env, transport=FakeApi(main_at(NEWER)), out=io.StringIO()) + self.assertEqual(code, 0) + self.assertEqual(sorted(path.name for path in files.root.iterdir()), []) + files = Files(self) + out = io.StringIO() + code = superseded.main(['check'], env=files.env(GITHUB_STEP_SUMMARY=str(files.root)), + transport=FakeApi(main_at(NEWER)), out=out) + self.assertEqual(code, 0) + reason = f'main is at {NEWER[:9]}, no longer at {OURS[:9]}' + self.assertEqual(out.getvalue(), f'superseded: {reason}\n::notice title=Superseded::{reason}. The rest of ' + 'this run is skipped, so none of its jobs fails or is cancelled.\n' + f"Could not write the job summary: [Errno 21] Is a directory: " + f"'{files.root}'\n") + self.assertEqual(files.output.read_bytes(), b'current=false\n') + + @unittest.skipUnless(POSIX, 'the watcher signals a process group, as on the Ubuntu runners') + def test_a_watch_that_cannot_stop_the_tests(self) -> None: + """The warning escapes the scope's ``%`` and line break too; the summary keeps them.""" + files = Files(self) + files.summary.write_bytes(b'before\n') + scope = 'odd%\nscope' + code, printed = self.run_main(['watch', '--interval', '0.1', '--grace', '0.2', '--kill-wait', '0.2', + '--scope', scope, '--', *SLEEPER], files) + self.assertEqual(code, superseded.STOP_FAILED) + match = re.search(r'the session of pid (\d+) ', printed) + self.assertIsNotNone(match, printed) + pid = match.group(1) if match else '' + os.kill(int(pid), signal.SIGKILL) + reason = (f'pull request #1938 is {ODD_STATE}, but SIGTERM and SIGKILL to the scope {scope} and the session ' + f'of pid {pid} did not end it within 0 s. The tests may run on until their timeout.') + escaped = reason.replace('%', '%25').replace('\r', '%0D').replace('\n', '%0A') + self.assertEqual(printed, f'::warning title=Superseded run not stopped::{escaped}\n') + self.assertIn('odd%25%0Ascope', escaped) + self.assertEqual(files.summary.read_bytes(), f'before\n- Superseded run not stopped: {reason}\n'.encode()) + self.assertEqual(files.read(files.output), '') + + @unittest.skipUnless(POSIX, 'the watcher signals a process group, as on the Ubuntu runners') + def test_a_watch_that_stops_the_tests(self) -> None: + files = Files(self) + out = io.StringIO() + env = files.env(GITHUB_EVENT_NAME='pull_request', GITHUB_EVENT_PATH=str(self.event)) + code = superseded.watch(SLEEPER, superseded.Context.from_env(env), + superseded.Api('token', FakeApi(pull_is(ODD_STATE))), env, out, interval=0.1, + grace=5, clock=iter([0.0, 7.9]).__next__) + self.assertEqual(code, 0) + outcome = 'The tests were stopped after 7 s, and this step succeeds.' + self.assertEqual(out.getvalue(), f'::notice title=Superseded::pull request #1938 is {ODD_DATA}. {outcome}\n') + self.assertEqual(files.summary.read_bytes(), + f'- Superseded: pull request #1938 is {ODD_STATE}. {outcome}\n'.encode()) + self.assertEqual(files.read(files.output), '') + + class Server(http.server.ThreadingHTTPServer): """The API, answering ``/repos//commits/main`` with ``head``.""" diff --git a/tests/project/test_workflow_run_verdict.py b/tests/project/test_workflow_run_verdict.py index 82fe2d3222..271d0f076c 100644 --- a/tests/project/test_workflow_run_verdict.py +++ b/tests/project/test_workflow_run_verdict.py @@ -320,6 +320,73 @@ def test_a_missing_verdict_fails_loudly_and_writes_nothing(self) -> None: self.assertIn('::error', result.stdout) self.assertIsNone(verified) + def _commands(self, verdict: 'str', head: 'str', *argv: 'str', + output: 'Optional[bytes]' = b'before\n') -> 'tuple[int, str, Optional[bytes]]': + """#2047: the script's status, whole stdout and ``$GITHUB_OUTPUT``, which holds ``output`` first. + + ``output=None`` leaves ``$GITHUB_OUTPUT`` unset, and the file is then + :data:`None` where nothing created it. + + """ + self._git('checkout', '-q', '--detach', head) + path = self.root / 'output' + if path.exists(): + path.unlink() + env = dict(self.env, VERDICT_SHA=verdict) + if output is not None: + path.write_bytes(output) + env['GITHUB_OUTPUT'] = str(path) + result = subprocess.run([sys.executable, str(SCRIPT), *argv], cwd=self.repo, env=env, capture_output=True, + timeout=scale_timeout(60)) + self.assertEqual(result.stderr, b'') + return result.returncode, result.stdout.decode('utf-8'), path.read_bytes() if path.exists() else None + + def test_the_workflow_commands_byte_for_byte(self) -> None: + """#2047: each answer's whole output, so moving it onto util/_actions.py can be shown to change nothing.""" + verdict, head = self.sha['9083163f5'], self.sha['2e941bbb7'] + refused = (f"::notice title=Skipped as the verdict is for another commit::{self.sha['bbcabf1dd']} after the " + f'verified {verdict} was authored by {HUMAN[1]} and committed by {GITHUB[1]}, not by the bot. ' + f'{head} gets its own Unit Tests run, which triggers this workflow again.\n') + self.assertEqual(self._commands(verdict, head), (0, refused, b'before\nverified=false\n')) + self.assertEqual(self._commands(verdict, head, output=None), (0, refused, None)) + covered = (f"Unit Tests verified {self.sha['2e941bbb7']}; the 1 commit(s) after it up to " + f"{self.sha['f4f94bcad']} are bot commits to conda/build, conda/requirements.txt only.\n") + self.assertEqual(self._commands(self.sha['2e941bbb7'], self.sha['f4f94bcad']), + (0, covered, b'before\nverified=true\n')) + self.assertEqual(self._commands('main', head), + (1, "::error title=No verdict to compare::VERDICT_SHA is 'main', not a commit sha\n", + b'before\n')) + self.assertEqual(self._commands(verdict, head, '--head', 'no-such-ref'), + (1, "::error title=No commit to compare::'no-such-ref' is not a commit in this checkout\n", + b'before\n')) + self.assertEqual(self._commands(self.sha['2e941bbb7'], head, '--head', self.sha['f4f94bcad'], '--exit-code'), + (0, covered, b'before\n')) + + def test_the_workflow_commands_escape_what_they_interpolate(self) -> None: + """#2047: ``%`` and LF in a path, and ``%`` in a sha or a ref, are escaped: one command, one line. + + Before util/_actions.py these went out as written: a bot commit to a path + holding a line break split its notice in two, the second line read as a + log line, or as a workflow command of its own where it began ``::``. + + """ + base = self.sha['f4f94bcad'] + odd = 'conda/50%\n::error::x.txt' + bump = self._commit('Bumped build to 4', ['conda/build', odd], BOT, BOT) + status, printed, output = self._commands(base, bump) + self.assertEqual((status, output), (0, b'before\nverified=false\n')) + self.assertEqual(printed, f'::notice title=Skipped as the verdict is for another commit::bot commit {bump} ' + f'after the verified {base} touches conda/50%25%0A::error::x.txt, which is not ' + f'bot-managed. {bump} gets its own Unit Tests run, which triggers this workflow ' + 'again.\n') + self.assertEqual(printed.count('\n'), 1) + self.assertEqual(self._commands('50%', bump), + (1, "::error title=No verdict to compare::VERDICT_SHA is '50%25', not a commit sha\n", + b'before\n')) + self.assertEqual(self._commands(base, bump, '--head', 'v50%'), + (1, "::error title=No commit to compare::'v50%25' is not a commit in this checkout\n", + b'before\n')) + def test_the_harness_git_runs_no_maintenance(self) -> None: """No git process outlives the command that started it. diff --git a/util/discussion_report.py b/util/discussion_report.py index 008b5201ea..aa401cfb11 100644 --- a/util/discussion_report.py +++ b/util/discussion_report.py @@ -59,6 +59,11 @@ import sys from typing import TYPE_CHECKING +try: # #2047: run as `python util/