diff --git a/.github/workflows/cron-vendor.yml b/.github/workflows/cron-vendor.yml index 5d0ca7e77..c2b114120 100644 --- a/.github/workflows/cron-vendor.yml +++ b/.github/workflows/cron-vendor.yml @@ -229,7 +229,7 @@ jobs: fi echo '```' echo - echo 'Const files from targets that exited zero are still isorted, diffed, committed and pushed below -- only the failing target(s) above were left as they were.' + echo 'Const files from targets that exited zero are still isorted, tested, diffed, committed and pushed below -- only the failing target(s) above were left as they were.' } >> "$GITHUB_STEP_SUMMARY" if [ -n "$failed_targets" ]; then @@ -251,6 +251,51 @@ jobs: # still skips `pcapkit/const/__init__.py` itself. isort -l100 -ppcapkit $(find pcapkit/const -mindepth 2 -name '*.py') + # #1853: neither gate above sees the regenerated tree -- `unit-tests` runs + # on the schedule alone, and `verdict` accepts the Unit Tests result for + # the commit from before the crawl -- so 1.5.0b13 was committed with a test + # failing on it. The owner's ruling: test the regenerated tree here, before + # anything is committed, under #872's rule above. A module the tests fail + # with is put back as HEAD has it and the rest go on to be committed, as + # for a sub-vendor that raised. util/vendor_test_gate.py, whose docstring + # gives the rule, decides which modules those are. + # + # `continue-on-error`, so the tests never fail the job. The script exits + # zero once it has decided; if this step fails or times out instead, the + # tree may hold modules no run passed with, and the next step discards + # every regenerated module. The test extra is installed here because only + # this step needs it. The script's budget bounds its runs together, and + # `timeout-minutes` sits above it as the backstop. Run serially, pytest + # peaked at 0.7 GB resident over tests/const, measured locally, so the + # memory cap leaves headroom on this runner; files are capped at 2 GiB, as + # in experimental.yml. + - name: Test the regenerated tree + id: vendor-tests + continue-on-error: true + timeout-minutes: 50 + run: | + set -x + + python -m pip install -e '.[test]' + python util/vendor_test_gate.py --budget-minutes 40 \ + --max-memory-mib 4096 --max-file-size-mib 2048 -- \ + python -m pytest -q -p no:cacheprovider tests/const tests/vendor + + # What the step above left is untested if it did not finish, so HEAD's + # copy of every module goes back, new modules are removed, and nothing + # from this crawl is committed. Not a failure either, per #872. + - name: Discard untested const changes + if: steps.vendor-tests.outcome != 'success' + run: | + git checkout HEAD -- pcapkit/const + git clean -fdq -- pcapkit/const + echo "::warning title=Vendor test gate did not finish::the regenerated tree could not be tested, so every regenerated module was discarded; see the step log" + { + echo '### :warning: Vendor test gate did not finish' + echo + echo 'The step that tests the regenerated tree failed or timed out, so nothing from this crawl is committed.' + } >> "$GITHUB_STEP_SUMMARY" + - name: Verify Changed files uses: tj-actions/verify-changed-files@v17 id: verify-changed-files diff --git a/pyproject.toml b/pyproject.toml index 7b881d1dd..2f4909560 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -304,9 +304,13 @@ test = [ # Enough of the ``vendor`` extra for a test to import a vendor crawler. # Without these, ``tests/vendor/test_ipx_socket_unit.py`` reports its tests as # skipped rather than failing, so the byte-for-byte regeneration check that is the - # only guard on the hand-maintained IPX socket registry never runs on any CI - # path: every ``unit-tests.yml`` job's install line builds on ``.[test]``, - # and ``cron-vendor.yml`` installs ``.[all,vendor]`` but never invokes pytest. + # only guard on the hand-maintained IPX socket registry runs in no + # ``unit-tests.yml`` job: every one's install line builds on ``.[test]``. + # ``cron-vendor.yml`` does run it, but needs nothing from here to: its job + # installs ``.[all,vendor]`` before ``.[test]``, and runs ``tests/vendor`` + # only when a crawl changed a constant module, by which point + # ``pcapkit-vendor`` has rewritten the IPX constant file from that same table + # (#1853). # # Both are needed, not just ``requests``. ``pcapkit.vendor.default`` imports # ``requests`` at module scope, and ``pcapkit.vendor.__init__`` then imports diff --git a/tests/_dependency_gates.py b/tests/_dependency_gates.py index 67e6c38ff..2f40574c0 100644 --- a/tests/_dependency_gates.py +++ b/tests/_dependency_gates.py @@ -117,9 +117,11 @@ #: The workflow holding every blocking job that runs :program:`pytest`. #: :file:`experimental.yml` runs the unit tier as well, on legs nothing waits on -#: (#1732), and is not read here. Other workflows install ``.[all]`` -#: somewhere -- a docs build, a conda recipe, a vendor crawl, the lint pass, -#: and the release packaging workflow -- and none of them invokes the suite; +#: (#1732), and :file:`cron-vendor.yml` runs ``tests/const`` and ``tests/vendor`` +#: over a freshly crawled tree, where a failure discards modules rather than +#: failing the job (#1853); neither is read here. Other workflows install +#: ``.[all]`` somewhere -- a docs build, a conda recipe, the lint pass, and the +#: release packaging workflow -- and none of them invokes the suite; #: :file:`python-compatibility.yml` installs a bare ``.`` and only compiles and #: imports, and :file:`codeql-analysis.yml` installs nothing explicitly at all #: (CodeQL's own autobuild step). That is exactly why this module keys on *jobs that @@ -1429,13 +1431,16 @@ def _engine_matrix_variants(name: 'str', section: 'str') -> 'Optional[tuple[Job, def pytest_jobs(workflow: 'Optional[pathlib.Path]' = None) -> 'tuple[Job, ...]': """The jobs of ``workflow`` that run :program:`pytest`. - Keyed on running the suite, not on holding an install line: five other - workflows install ``.[all]`` somewhere, three of them as ``.[all,dev]`` -- - which does carry ``pypcapfile``, through the ``dev`` extra #910 added when - it narrowed ``all`` to core addons only -- and never invoke - :program:`pytest`, so a guard that looked at install lines anywhere would - pass vacuously. Within this workflow the ``changelog`` job is excluded by - the same rule; it installs nothing and runs a generator. + Keyed on running the suite, not on holding an install line: other + workflows install ``.[all]`` somewhere, some as ``.[all,dev]`` -- which + does carry ``pypcapfile``, through the ``dev`` extra #910 added when it + narrowed ``all`` to core addons only -- and never invoke :program:`pytest`, + so a guard that looked at install lines anywhere would pass vacuously. The + exception, :file:`cron-vendor.yml`, runs only ``tests/const`` and + ``tests/vendor``, over a freshly crawled tree, and a failure there discards + modules rather than failing the job (#1853), so it is not read here either. + Within this workflow the ``changelog`` job is excluded by the same rule; it + installs nothing and runs a generator. The ``workflow`` argument exists so the guard can be pointed at a doctored copy and shown to fail; see diff --git a/tests/project/test_isort_clean.py b/tests/project/test_isort_clean.py index 1c0352463..a11189596 100644 --- a/tests/project/test_isort_clean.py +++ b/tests/project/test_isort_clean.py @@ -112,8 +112,8 @@ relying on ``bash`` >= 4.0 would have been exactly the mistake the Makefile's line first made. This module does not cover that fix, though: it only exercises ``make isort``, and :file:`cron-vendor.yml`'s ``vendor-update`` job -never runs this test suite at all -- it commits and pushes on its own, with -no pytest step in between. +never runs it -- the tests that job runs before it commits are +:file:`tests/const` and :file:`tests/vendor` alone (#1853). Until #766 this module gated on isort with a ``try: import isort`` inside :meth:`~TestIsortIsCleanOnThePackage.setUpClass`, raising diff --git a/tests/project/test_vendor_test_gate_1853.py b/tests/project/test_vendor_test_gate_1853.py new file mode 100644 index 000000000..fdd802e02 --- /dev/null +++ b/tests/project/test_vendor_test_gate_1853.py @@ -0,0 +1,443 @@ +# -*- coding: utf-8 -*- +"""#1853: Vendor Update tests the regenerated tree before it commits. + +:file:`.github/workflows/cron-vendor.yml` committed whatever ``pcapkit-vendor`` +regenerated with no test step in between, and 1.5.0b13 turned ``main`` red. The +owner chose to test the regenerated tree in the job itself, under #872's rule: a +failed sub-vendor's changes are discarded, the rest are kept, and the job as a +whole does not fail. + +* :class:`TestTheWorkflowTestsBeforeItCommits` reads ``vendor-update`` with + PyYAML: a step between Update Vendor and the commits runs the const and vendor + tests, installs the test extra, cannot fail the job, and is bounded in time. +* :class:`TestTheDiscardStep` runs the step that discards every regenerated + module when the test step did not finish. +* :class:`TestTheGate` runs :file:`util/vendor_test_gate.py` in throwaway + repositories, with a stand-in test command that fails on a marker, so which + modules it keeps is checked against which ones carry the marker. + +""" + +from __future__ import annotations + +import json +import os +import pathlib +import re +import shutil +import signal +import subprocess # nosec B404 +import sys +import tempfile +import time +import unittest +from typing import TYPE_CHECKING + +from tests._support import scale_timeout + +try: + import yaml +except ImportError: # pragma: no cover + yaml = None # type: ignore[assignment] + +if TYPE_CHECKING: + from typing import Any, Optional + +ROOT = pathlib.Path(__file__).resolve().parents[2] +SCRIPT = ROOT / 'util' / 'vendor_test_gate.py' +WORKFLOW = ROOT / '.github' / 'workflows' / 'cron-vendor.yml' +BASH = shutil.which('bash') + +#: The stand-in for pytest: it records what each run saw, then fails when a +#: module under ``pcapkit/const`` holds ``BROKEN``, when modules hold both halves +#: of the ``TOGETHER`` pair, or when ``$REQUIRE`` names a module that is gone. +#: ``$SLEEP``, ``$ALLOCATE``, ``$WRITE`` and ``$STRAY`` make it hang, hold +#: memory, write a large file, or leave a module behind. +FAKE_TESTS = r''' +import json, os, pathlib, sys, time + +texts = {str(path): path.read_text() for path in sorted(pathlib.Path('pcapkit/const').rglob('*.py'))} +with open(os.environ['GATE_LOG'], 'a') as log: + log.write(json.dumps({'argv': sys.argv[1:], 'addopts': os.environ.get('PYTEST_ADDOPTS', ''), + 'prefix': os.environ.get('PYTHONPYCACHEPREFIX'), 'pid': os.getpid(), + 'files': texts}) + '\n') +if os.environ.get('STRAY'): + pathlib.Path(os.environ['STRAY']).write_text('# left behind\n') +if os.environ.get('ALLOCATE'): + held = b'\x01' * int(os.environ['ALLOCATE']) + time.sleep(30) +if os.environ.get('WRITE'): + with open(os.environ['WRITE'], 'wb') as file: + file.write(b'\x01' * (4 << 20)) +if os.environ.get('SLEEP'): + time.sleep(float(os.environ['SLEEP'])) +broken = any('BROKEN' in text for text in texts.values()) +together = all(any(mark in text for text in texts.values()) for mark in ('TOGETHER-A', 'TOGETHER-B')) +missing = bool(os.environ.get('REQUIRE')) and os.environ['REQUIRE'] not in texts +sys.exit(1 if broken or together or missing else 0) +''' + + +def module(name: 'str', marker: 'str' = '') -> 'str': + """A generated module's text, carrying ``marker`` when given.""" + return f'# -*- coding: utf-8 -*-\n"""{name}"""\n\nVALUE = {name!r} # {marker}\n' + + +def vendor_update() -> 'dict[str, Any]': + if yaml is None: # pragma: no cover + raise unittest.SkipTest('PyYAML, from the test extra, is not installed') + return yaml.safe_load(WORKFLOW.read_text(encoding='utf-8'))['jobs']['vendor-update'] + + +class TestTheWorkflowTestsBeforeItCommits(unittest.TestCase): + """The step the ruling on #1853 asks for, as ``vendor-update`` declares it.""" + + steps = [] # type: list[dict[str, Any]] + names = [] # type: list[Optional[str]] + + @classmethod + def setUpClass(cls) -> None: + cls.steps = vendor_update()['steps'] + cls.names = [step.get('name') for step in cls.steps] + + def index(self, name: 'str') -> 'int': + if name not in self.names: + self.fail(f'`vendor-update` has no step {name!r}') + return self.names.index(name) + + def gate(self) -> 'tuple[int, dict[str, Any]]': + """The step that runs the tests on the regenerated tree, and its index.""" + found = [(index, step) for index, step in enumerate(self.steps) + if 'util/vendor_test_gate.py' in str(step.get('run', ''))] + self.assertEqual(len(found), 1, 'expected one step of `vendor-update` to run util/vendor_test_gate.py') + return found[0] + + def test_a_test_step_runs_between_the_crawl_and_the_commit(self) -> None: + """After the crawl and its isort, before anything decides what changed.""" + index, step = self.gate() + self.assertLess(self.index('Update Vendor'), index) + for later in ('Verify Changed files', 'Bump Version', 'Commit changes', + 'Commit documentation changes', 'Push changes'): + with self.subTest(step=later): + self.assertLess(index, self.index(later)) + self.assertNotIn('if', step, 'the tests run whenever the crawl did') + + def test_it_runs_the_const_and_vendor_tests(self) -> None: + _, step = self.gate() + command = str(step['run']).replace('\\\n', ' ').split(' -- ', 1) + self.assertEqual(len(command), 2, 'the test command follows `--`') + words = command[1].split() + self.assertEqual(words[:3], ['python', '-m', 'pytest']) + self.assertIn('tests/const', words) + self.assertIn('tests/vendor', words) + self.assertTrue(SCRIPT.exists(), f'{SCRIPT.relative_to(ROOT)} does not exist') + + def test_it_installs_the_test_dependencies(self) -> None: + """The job's own install is ``.[all,vendor]``, which carries no pytest.""" + _, step = self.gate() + installs = re.findall(r"pip install -e '\.\[([^]]*)\]'", str(step['run'])) + self.assertEqual(len(installs), 1, step['run']) + self.assertIn('test', [extra.strip() for extra in installs[0].split(',')]) + run = str(step['run']) + self.assertLess(run.index('pip install'), run.index('util/vendor_test_gate.py')) + + def test_it_cannot_fail_the_job(self) -> None: + """#872: a failing test discards modules, never the job.""" + index, step = self.gate() + self.assertIs(step.get('continue-on-error'), True) + self.assertIn('id', step) + + # A step that did not finish may leave modules no run passed with, so + # the next one puts HEAD's tree back before anything is committed. + discard = self.steps[index + 1] + self.assertEqual(discard.get('if'), f"steps.{step['id']}.outcome != 'success'") + run = str(discard.get('run', '')) + self.assertIn('git checkout HEAD -- pcapkit/const', run) + self.assertIn('git clean -fdq -- pcapkit/const', run) + self.assertIn('::warning ', run) + self.assertLess(index + 1, self.index('Verify Changed files')) + + def test_it_has_a_timeout_above_its_budget(self) -> None: + """The script's budget ends the runs first; ``timeout-minutes`` is the backstop.""" + _, step = self.gate() + timeout = step.get('timeout-minutes') + self.assertIsInstance(timeout, int) + budget = re.search(r'--budget-minutes (\d+)', str(step['run'])) + self.assertIsNotNone(budget, step['run']) + assert budget is not None + self.assertLess(int(budget.group(1)), timeout) + for option in ('--max-memory-mib', '--max-file-size-mib'): + with self.subTest(option=option): + self.assertRegex(str(step['run']), rf'{option} \d+') + + +class ScratchRepository(unittest.TestCase): + """A throwaway repository whose ``HEAD`` holds a few generated modules.""" + + MODULES = ('pcapkit/const/a.py', 'pcapkit/const/b.py', 'pcapkit/const/reg/c.py', + 'pcapkit/const/reg/d.py') + + def setUp(self) -> None: + tmp = tempfile.TemporaryDirectory() + self.addCleanup(tmp.cleanup) + self.root = pathlib.Path(tmp.name) + self.repo = self.root / 'repo' + + # As in test_vendor_comment_only_no_bump_1674.py: none of the caller's + # git configuration, and no automatic maintenance to outlive the test. + home = self.root / 'home' + home.mkdir() + (home / '.gitconfig').write_text( + '[init]\n\tdefaultBranch = main\n[commit]\n\tgpgsign = false\n' + '[maintenance]\n\tauto = false\n\tautoDetach = false\n' + '[gc]\n\tauto = 0\n\tautoDetach = false\n' + '[user]\n\tname = Test\n\temail = test@example.com\n', encoding='utf-8') + self.env = { + 'PATH': os.environ.get('PATH', ''), + 'HOME': str(home), + 'XDG_CONFIG_HOME': str(home / '.config'), + 'GIT_CONFIG_NOSYSTEM': '1', + 'LC_ALL': 'C', + } + self.git('init', '-q', str(self.repo), cwd=self.root) + for path in self.MODULES: + self.write(path, module(path)) + self.write('pcapkit/const/README.txt', 'Generated by pcapkit-vendor.\n') + self.write('.gitignore', '__pycache__/\n') + self.git('add', '.') + self.git('commit', '-qm', 'the generated tree') + + self.log = self.root / 'gate.log' + self.output = self.root / 'output' + self.summary = self.root / 'summary.md' + self.fake = self.root / 'fake_tests.py' + self.fake.write_text(FAKE_TESTS, encoding='utf-8') + + def git(self, *args: 'str', cwd: 'Optional[pathlib.Path]' = None) -> 'str': + result = subprocess.run(['git', *args], cwd=cwd or self.repo, env=self.env, + capture_output=True, text=True, check=False, + timeout=scale_timeout(30)) + self.assertEqual(result.returncode, 0, f'git {" ".join(args)}: {result.stderr}') + return result.stdout + + def write(self, path: 'str', text: 'str') -> None: + file = self.repo / path + file.parent.mkdir(parents=True, exist_ok=True) + file.write_text(text, encoding='utf-8') + + def read(self, path: 'str') -> 'Optional[str]': + file = self.repo / path + return file.read_text(encoding='utf-8') if file.exists() else None + + def changed(self) -> 'list[str]': + """``git status --porcelain`` under ``pcapkit/const``, one entry per line.""" + return self.git('status', '--porcelain', '--untracked-files=all', '--', 'pcapkit/const').splitlines() + + +class TestTheDiscardStep(ScratchRepository): + """``Discard untested const changes``' script, run as the runner runs it.""" + + @unittest.skipIf(not BASH, 'needs bash on PATH') + def test_it_puts_back_every_module_and_warns(self) -> None: + steps = [step for step in vendor_update()['steps'] if step.get('name') == 'Discard untested const changes'] + self.assertEqual(len(steps), 1, 'expected one `Discard untested const changes` step') + script = self.root / 'step.sh' + script.write_text(steps[0]['run'], encoding='utf-8') + + self.write('pcapkit/const/a.py', module('a', 'BROKEN')) + self.write('pcapkit/const/new/e.py', module('e')) + (self.repo / 'pcapkit/const/reg/c.py').unlink() + self.assertEqual(len(self.changed()), 3, self.changed()) + + # `bash -e {0}`, as for a `run:` with no `shell:`. + result = subprocess.run(['bash', '-e', str(script)], cwd=self.repo, + env=dict(self.env, GITHUB_STEP_SUMMARY=str(self.summary)), + capture_output=True, text=True, check=False, timeout=scale_timeout(60)) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertEqual(self.changed(), []) + self.assertIn('::warning title=Vendor test gate did not finish::', result.stdout) + self.assertIn('Vendor test gate did not finish', self.summary.read_text(encoding='utf-8')) + + +class TestTheGate(ScratchRepository): + """:file:`util/vendor_test_gate.py`, run on a regenerated tree.""" + + def run_gate(self, *, budget: 'float' = 5, memory: 'float' = 4096, file_size: 'float' = 2048, + **env: 'str') -> 'subprocess.CompletedProcess[str]': + self.assertTrue(SCRIPT.exists(), f'{SCRIPT.relative_to(ROOT)} does not exist') + environment = dict(self.env, GATE_LOG=str(self.log), GITHUB_OUTPUT=str(self.output), + GITHUB_STEP_SUMMARY=str(self.summary), **env) + result = subprocess.run( + [sys.executable, str(SCRIPT), '--budget-minutes', str(budget), '--max-memory-mib', str(memory), + '--max-file-size-mib', str(file_size), '--', sys.executable, str(self.fake), '-q'], + cwd=self.repo, env=environment, capture_output=True, text=True, check=False, + timeout=scale_timeout(120)) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + return result + + def runs(self) -> 'list[dict[str, Any]]': + if not self.log.exists(): + return [] + return [json.loads(line) for line in self.log.read_text(encoding='utf-8').splitlines()] + + def verdict(self) -> 'Optional[str]': + written = re.findall(r'^verdict=(.*)$', self.output.read_text(encoding='utf-8'), re.M) \ + if self.output.exists() else [] + self.assertLessEqual(len(written), 1, written) + return written[0] if written else None + + def test_nothing_changed_runs_nothing(self) -> None: + """A change outside the ``.py`` modules is not this script's either.""" + self.write('pcapkit/const/README.txt', 'Generated weekly.\n') + self.run_gate() + self.assertEqual(self.verdict(), 'unchanged') + self.assertEqual(self.runs(), []) + self.assertEqual(self.read('pcapkit/const/README.txt'), 'Generated weekly.\n') + + def test_a_regeneration_that_passes_is_kept_whole(self) -> None: + self.write('pcapkit/const/a.py', module('a', 'new member')) + self.write('pcapkit/const/new/e.py', module('e')) + result = self.run_gate() + self.assertEqual(self.verdict(), 'passed') + runs = self.runs() + self.assertEqual(len(runs), 1) + # The first run lists every failure, so it is not -x, and its arguments are passed as given. + self.assertEqual(runs[0]['addopts'], '') + self.assertEqual(runs[0]['argv'], ['-q']) + self.assertEqual(self.read('pcapkit/const/a.py'), module('a', 'new member')) + self.assertEqual(self.read('pcapkit/const/new/e.py'), module('e')) + self.assertNotIn('::warning', result.stdout) + self.assertIn('the regeneration passed', self.summary.read_text(encoding='utf-8')) + + def test_only_the_module_the_tests_fail_with_is_discarded(self) -> None: + """#872's rule, for a sub-vendor whose output fails the tests.""" + for path in self.MODULES: + self.write(path, module(path, 'BROKEN' if path == 'pcapkit/const/b.py' else 'regenerated')) + result = self.run_gate() + self.assertEqual(self.verdict(), 'partial') + self.assertEqual(self.read('pcapkit/const/b.py'), module('pcapkit/const/b.py')) + for path in set(self.MODULES) - {'pcapkit/const/b.py'}: + with self.subTest(path=path): + self.assertEqual(self.read(path), module(path, 'regenerated')) + self.assertIn('::warning title=Vendor test gate discarded a module::pcapkit/const/b.py', result.stdout) + self.assertEqual(result.stdout.count('::warning'), 1, result.stdout) + + runs = self.runs() + # The whole regeneration, HEAD's tree, the halves, and the kept modules together. + self.assertEqual([sorted(path for path, text in run['files'].items() if 'regenerated' in text + or 'BROKEN' in text) for run in runs], + [sorted(self.MODULES), [], ['pcapkit/const/a.py', 'pcapkit/const/b.py'], + ['pcapkit/const/a.py'], ['pcapkit/const/b.py'], + ['pcapkit/const/reg/c.py', 'pcapkit/const/reg/d.py'], + ['pcapkit/const/a.py', 'pcapkit/const/reg/c.py', 'pcapkit/const/reg/d.py']]) + self.assertEqual([run['addopts'] for run in runs], [''] + ['-x'] * (len(runs) - 1)) + prefixes = [run['prefix'] for run in runs] + self.assertEqual(len(set(prefixes)), len(prefixes), 'each run gets a cache prefix of its own') + for prefix in prefixes: + self.assertFalse(pathlib.Path(prefix).exists(), prefix) + + summary = self.summary.read_text(encoding='utf-8') + self.assertIn('| `pcapkit/const/b.py` | no |', summary) + self.assertIn('| `pcapkit/const/a.py` | yes |', summary) + + def test_a_new_module_the_tests_fail_with_is_removed(self) -> None: + self.write('pcapkit/const/a.py', module('a', 'regenerated')) + self.write('pcapkit/const/new/e.py', module('e', 'BROKEN')) + self.run_gate() + self.assertEqual(self.verdict(), 'partial') + self.assertIsNone(self.read('pcapkit/const/new/e.py')) + self.assertEqual(self.read('pcapkit/const/a.py'), module('a', 'regenerated')) + + def test_a_deleted_module_the_tests_need_is_put_back(self) -> None: + (self.repo / 'pcapkit/const/reg/c.py').unlink() + self.run_gate(REQUIRE='pcapkit/const/reg/c.py') + self.assertEqual(self.verdict(), 'discarded') + self.assertEqual(self.read('pcapkit/const/reg/c.py'), module('pcapkit/const/reg/c.py')) + self.assertEqual(self.changed(), []) + + def test_a_failure_on_heads_own_tree_discards_everything(self) -> None: + """The tests cannot tell good modules from bad ones then.""" + self.write('pcapkit/const/a.py', module('a', 'BROKEN')) + self.git('commit', '-qam', 'a broken HEAD') + self.write('pcapkit/const/b.py', module('b', 'regenerated')) + result = self.run_gate() + self.assertEqual(self.verdict(), 'discarded') + self.assertEqual(self.changed(), []) + self.assertEqual(len(self.runs()), 2) + self.assertIn("HEAD's own tree", result.stdout) + self.assertIn('::warning title=Vendor test gate discarded every regenerated module::', result.stdout) + + def test_modules_that_fail_together_are_all_discarded(self) -> None: + """Each passes on its own, the kept set does not: nothing untested is left.""" + self.write('pcapkit/const/a.py', module('a', 'TOGETHER-A')) + self.write('pcapkit/const/b.py', module('b', 'BROKEN')) + self.write('pcapkit/const/reg/c.py', module('c', 'TOGETHER-B')) + result = self.run_gate() + self.assertEqual(self.verdict(), 'discarded') + self.assertEqual(self.changed(), []) + self.assertIn('fail together', result.stdout) + + def test_a_run_over_the_budget_discards_everything(self) -> None: + self.write('pcapkit/const/a.py', module('a', 'regenerated')) + start = time.monotonic() + result = self.run_gate(budget=0.05, SLEEP='60') + self.assertLess(time.monotonic() - start, 40, 'the run was not stopped at the budget') + self.assertEqual(self.verdict(), 'discarded') + self.assertEqual(self.changed(), []) + self.assertIn('budget ran out', result.stdout) + + def test_a_run_over_the_memory_cap_fails(self) -> None: + self.write('pcapkit/const/a.py', module('a', 'regenerated')) + if subprocess.run(['ps', '-A', '-o', 'pgid=', '-o', 'rss='], capture_output=True, + check=False).returncode != 0: # pragma: no cover + self.skipTest('ps cannot list processes here, so memory cannot be sampled') + start = time.monotonic() + result = self.run_gate(memory=32, ALLOCATE=str(256 << 20)) + self.assertLess(time.monotonic() - start, 40, 'the run was not stopped at the cap') + self.assertEqual(self.verdict(), 'discarded') + self.assertEqual(self.changed(), []) + self.assertRegex(result.stdout, r'killed: \d+ MiB resident, over the 32 MiB cap') + + def test_a_run_cannot_write_past_the_file_size_cap(self) -> None: + self.write('pcapkit/const/a.py', module('a', 'regenerated')) + large = self.root / 'large.bin' + self.run_gate(file_size=1, WRITE=str(large)) + self.assertEqual(self.verdict(), 'discarded') + self.assertLessEqual(large.stat().st_size, 1 << 20) + + def test_a_module_the_tests_leave_behind_is_removed(self) -> None: + self.write('pcapkit/const/a.py', module('a', 'regenerated')) + result = self.run_gate(STRAY='pcapkit/const/stray.py') + self.assertEqual(self.verdict(), 'passed') + self.assertIsNone(self.read('pcapkit/const/stray.py')) + self.assertEqual(self.changed(), [' M pcapkit/const/a.py']) + self.assertIn('::warning title=Vendor test gate restored a module::pcapkit/const/stray.py', result.stdout) + + def test_a_stopped_gate_puts_head_back_and_kills_its_run(self) -> None: + """``SIGTERM`` mid-run: no module is left that no run passed with, nor the run itself.""" + self.assertTrue(SCRIPT.exists(), f'{SCRIPT.relative_to(ROOT)} does not exist') + self.write('pcapkit/const/a.py', module('a', 'regenerated')) + gate = subprocess.Popen( + [sys.executable, str(SCRIPT), '--budget-minutes', '5', '--max-memory-mib', '4096', + '--max-file-size-mib', '2048', '--', sys.executable, str(self.fake)], + cwd=self.repo, env=dict(self.env, GATE_LOG=str(self.log), SLEEP='120'), + stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + try: + deadline = time.monotonic() + scale_timeout(60) + while not self.runs() and time.monotonic() < deadline: + time.sleep(0.1) + runs = self.runs() + self.assertEqual(len(runs), 1, 'the test command never started') + gate.send_signal(signal.SIGTERM) + self.assertNotEqual(gate.wait(timeout=scale_timeout(60)), 0) + finally: + if gate.poll() is None: # pragma: no cover + gate.kill() + gate.wait() + self.assertEqual(self.changed(), []) + with self.assertRaises(ProcessLookupError): + os.kill(runs[0]['pid'], 0) + + +if __name__ == '__main__': + unittest.main() diff --git a/util/vendor_test_gate.py b/util/vendor_test_gate.py new file mode 100644 index 000000000..028e158b8 --- /dev/null +++ b/util/vendor_test_gate.py @@ -0,0 +1,418 @@ +# -*- coding: utf-8 -*- +"""Test a Vendor Update regeneration before it is committed (#1853). + +:file:`.github/workflows/cron-vendor.yml` runs ``pcapkit-vendor`` and isort, then +commits what changed. Neither of its gates sees that tree: the Vendor test gate +runs on the schedule alone, and a ``workflow_run`` reads the Unit Tests verdict +on the commit from before the crawl. So 1.5.0b13 was committed with a pinned +test failing on it, and ``main`` stayed red until #1820. The owner's ruling on +#1853 is that the job tests the regenerated tree before it commits, under +#872's rule: a failed sub-vendor's changes are discarded, the others are kept, +and the job as a whole does not fail. + +**Modules.** Each crawler writes the module its ``_dest_path()`` names, which +mirrors its own module from ``pcapkit/vendor/`` into :data:`GENERATED_ROOT`, and +nothing else. So a changed module there names the sub-vendor that wrote it, and +discarding that sub-vendor's changes is putting its module back as ``HEAD`` has +it, or removing it where ``HEAD`` has none. + +**Attribution.** A failing test does not name the module at fault: the AppType +tests read every per-transport registry, for one. So no test is mapped to a +module. The tests are asked instead, each time with some of the changed modules +on top of ``HEAD``'s tree: + +1. All of them. If the tests pass, every module is kept. +2. None of them. If the tests fail on ``HEAD``'s own tree as well, the failure + is not the regeneration's, and nothing here can tell its good modules from + its bad ones, so every change is discarded. +3. Halves. A half the tests pass with is kept; a half they fail with is halved + again, down to a single module, which is discarded. +4. The kept modules together, unless step 3 already ran exactly that set. If + they fail together, every change is discarded. + +So the tree left behind is one the tests passed on, or ``HEAD``'s own. A set of +modules is never run twice, so one that failed once is not kept. Every run +after the first gets ``-x`` through ``PYTEST_ADDOPTS``: it only has to say pass +or fail, and the first run's log already names every failing test. + +**Limits.** Each run is a child in a session of its own, with ``RLIMIT_FSIZE`` +at ``--max-file-size-mib``. It is killed, and counts as failing, once its +process group holds more than ``--max-memory-mib`` resident, sampled with +``ps``: ``RLIMIT_AS`` caps address space rather than resident memory, and the +job runs on ``macos-latest``, which has no cgroup to cap it with. +``--budget-minutes`` bounds every run together; when it runs out before a +decision, every change is discarded. Each run imports through a +``PYTHONPYCACHEPREFIX`` of its own, removed after it, since a module swapped +back within the same second at the same size would otherwise load from a stale +cache. The run still writes bytecode there: without it, the const and vendor +tests took 234s rather than 157s, measured locally. + +Run from the repository root, with the test command after ``--``. Writes +``verdict=`` ``unchanged``, ``passed``, ``partial`` or ``discarded`` to +``$GITHUB_OUTPUT``, the modules and runs to ``$GITHUB_STEP_SUMMARY``, and a +``::warning::`` for what it discards. Exits non-zero only on an error of its +own or when it is stopped, after putting ``HEAD``'s copy of every changed module +back. + +""" + +from __future__ import annotations + +import argparse +import contextlib +import os +import resource +import shutil +import signal +# git, ps and the test command, each with a fixed argument list and no shell. +import subprocess # nosec B404 +import sys +import tempfile +import time +from typing import TYPE_CHECKING, NamedTuple + +if TYPE_CHECKING: + from typing import FrozenSet, NoReturn, Optional, Sequence + +#: The tree ``pcapkit-vendor`` writes, relative to the repository root, as in +#: :file:`util/vendor_code_changed.py`. +GENERATED_ROOT = 'pcapkit/const/' + +#: Seconds between two samples of a run's memory. +POLL = 1.0 + +#: Seconds a run has to stop after ``SIGINT`` before it gets ``SIGKILL``. +GRACE = 10.0 + +MIB = 1 << 20 + + +class Change(NamedTuple): + """One changed module: its contents in ``HEAD`` and after the crawl, :data:`None` where absent.""" + + path: str + head: 'Optional[bytes]' + regenerated: 'Optional[bytes]' + + +class Run(NamedTuple): + """One run of the test command.""" + + number: int + modules: int + result: str + seconds: float + + +class BudgetExhausted(Exception): + """The budget ran out before the tests could decide.""" + + +def _git(*args: 'str') -> 'bytes': + result = subprocess.run(['git', *args], check=False, # nosec B603 B607 + capture_output=True) + if result.returncode != 0: + raise RuntimeError(f'git {" ".join(args)} failed: ' + + result.stderr.decode(errors='replace').strip()) + return result.stdout + + +def changed_modules() -> 'list[str]': + """Each ``.py`` path under :data:`GENERATED_ROOT` that differs from ``HEAD``, + new and deleted ones included, as ``Commit changes`` stages them. + + """ + listing = _git('status', '--porcelain=v1', '-z', '--untracked-files=all', '--no-renames', + '--', GENERATED_ROOT) + paths = {os.fsdecode(entry[3:]) for entry in listing.split(b'\0') if entry} + return sorted(path for path in paths if path.endswith('.py')) + + +def snapshot(paths: 'Sequence[str]') -> 'list[Change]': + """Both versions of each of ``paths``.""" + changes = [] + for path in paths: + head = _git('cat-file', 'blob', f'HEAD:{path}') if _git('ls-tree', '-z', 'HEAD', '--', path) else None + regenerated = None + if os.path.lexists(path): + with open(path, 'rb') as file: + regenerated = file.read() + changes.append(Change(path, head, regenerated)) + return changes + + +def apply(changes: 'Sequence[Change]', selected: 'FrozenSet[str]') -> None: + """Leave the regenerated version of each module in ``selected``, and ``HEAD``'s of the rest.""" + for change in changes: + content = change.regenerated if change.path in selected else change.head + if content is None: + with contextlib.suppress(FileNotFoundError): + os.remove(change.path) + continue + os.makedirs(os.path.dirname(change.path), exist_ok=True) + with open(change.path, 'wb') as file: + file.write(content) + + +def group_rss(pgid: 'int') -> 'Optional[int]': + """Bytes resident in the processes of group ``pgid``, or :data:`None` if ``ps`` cannot say.""" + try: + listing = subprocess.run(['ps', '-A', '-o', 'pgid=', '-o', 'rss='], # nosec B603 B607 + capture_output=True, check=False, timeout=30) + except (OSError, subprocess.TimeoutExpired): + return None + if listing.returncode != 0: + return None + total = 0 + for line in listing.stdout.splitlines(): + fields = line.split() + if len(fields) == 2 and fields[1].isdigit() and fields[0] == str(pgid).encode(): + total += int(fields[1]) * 1024 + return total + + +def escape(text: 'str') -> 'str': + """``text`` as the data of a workflow command.""" + return text.replace('%', '%25').replace('\r', '%0D').replace('\n', '%0A') + + +class Gate: + """The test command, run on some of ``changes`` at a time.""" + + def __init__(self, changes: 'Sequence[Change]', command: 'Sequence[str]', budget: 'float', + max_memory: 'int', max_file_size: 'int') -> None: + self.changes = changes + self.command = list(command) + self.deadline = time.monotonic() + budget + self.max_memory = max_memory + self.max_file_size = max_file_size + self.results = {} # type: dict[FrozenSet[str], bool] + self.runs = [] # type: list[Run] + self.unsampled = False + + def test(self, selected: 'FrozenSet[str]', fail_fast: 'bool' = True) -> 'bool': + """Whether the tests pass with ``selected`` on top of ``HEAD``'s tree. + + Raises :exc:`BudgetExhausted` when the budget ran out first. + + """ + if selected in self.results: + return self.results[selected] + number = len(self.runs) + 1 + if len(selected) == len(self.changes): + label = f'Run {number}: the regeneration' + elif selected: + label = f'Run {number}: {len(selected)} of {len(self.changes)} regenerated modules on top of HEAD' + else: + label = f"Run {number}: HEAD's tree" + + apply(self.changes, selected) + start = time.monotonic() + result = self._run(label, fail_fast) + self.runs.append(Run(number, len(selected), result, time.monotonic() - start)) + print(f'{label}: {result}', flush=True) + if result == 'budget': + raise BudgetExhausted + self.results[selected] = result == 'passed' + return self.results[selected] + + def _run(self, label: 'str', fail_fast: 'bool') -> 'str': + """``'passed'``, ``'budget'``, or why the run failed.""" + if time.monotonic() >= self.deadline: + return 'budget' + env = dict(os.environ) + if fail_fast: + env['PYTEST_ADDOPTS'] = ' '.join(filter(None, (env.get('PYTEST_ADDOPTS'), '-x'))) + limit = self.max_file_size + + def cap() -> None: + # In the child, between fork and exec. + resource.setrlimit(resource.RLIMIT_FSIZE, (limit, limit)) + + cache = tempfile.mkdtemp(prefix='vendor-test-gate-') + env['PYTHONPYCACHEPREFIX'] = cache + print(f'::group::{label}', flush=True) + try: + child = subprocess.Popen(self.command, env=env, # nosec B603 + start_new_session=True, preexec_fn=cap) + try: + stopped = self._watch(child) + finally: + # Whatever the run left running in its session goes with it, + # and the run itself is reaped, however this was left. + with contextlib.suppress(ProcessLookupError, PermissionError): + os.killpg(child.pid, signal.SIGKILL) + with contextlib.suppress(subprocess.TimeoutExpired): + child.wait(timeout=GRACE) + finally: + print('::endgroup::', flush=True) + shutil.rmtree(cache, ignore_errors=True) + if stopped: + return stopped + return 'passed' if child.returncode == 0 else f'failed (exit {child.returncode})' + + def _watch(self, child: 'subprocess.Popen[bytes]') -> 'str': + """Wait for ``child``, stopping it on the budget or the memory cap: why, or ``''``.""" + while True: + try: + child.wait(timeout=POLL) + return '' + except subprocess.TimeoutExpired: + pass + if time.monotonic() >= self.deadline: + self._stop(child, signal.SIGINT) + return 'budget' + rss = group_rss(child.pid) + if rss is None: + self.unsampled = True + elif rss > self.max_memory: + self._stop(child, signal.SIGKILL) + return f'killed: {rss // MIB} MiB resident, over the {self.max_memory // MIB} MiB cap' + + @staticmethod + def _stop(child: 'subprocess.Popen[bytes]', signum: 'int') -> None: + with contextlib.suppress(ProcessLookupError, PermissionError): + os.killpg(child.pid, signum) + try: + child.wait(timeout=GRACE) + except subprocess.TimeoutExpired: + with contextlib.suppress(ProcessLookupError, PermissionError): + os.killpg(child.pid, signal.SIGKILL) + child.wait() + + +def bisect(gate: 'Gate', paths: 'Sequence[str]') -> 'list[str]': + """The modules of ``paths``, a set the tests fail with, that they pass with by halves.""" + if len(paths) == 1: + return [] + middle = len(paths) // 2 + kept = [] # type: list[str] + for half in (paths[:middle], paths[middle:]): + kept.extend(half if gate.test(frozenset(half)) else bisect(gate, half)) + return kept + + +def decide(gate: 'Gate', paths: 'Sequence[str]') -> 'tuple[FrozenSet[str], str]': + """The modules to keep, and why the rest are discarded.""" + everything = frozenset(paths) + if gate.test(everything, fail_fast=False): + return everything, '' + if not gate.test(frozenset()): + return frozenset(), ("The tests fail on HEAD's own tree too, so they cannot tell a good " + 'module from a bad one') + if len(paths) == 1: + return frozenset(), "The tests fail with the regenerated module on top of HEAD's tree" + kept = frozenset(bisect(gate, paths)) + if not kept: + return kept, "The tests fail with each regenerated module on top of HEAD's tree" + if not gate.test(kept): + return frozenset(), 'The modules the tests passed with by halves fail together' + return kept, '' + + +def report(changes: 'Sequence[Change]', kept: 'FrozenSet[str]', reason: 'str', gate: 'Gate', + command: 'str') -> 'str': + """Write the summary and the annotations; the verdict.""" + discarded = [change.path for change in changes if change.path not in kept] + if not discarded: + verdict = 'passed' + lines = ['### Vendor test gate: the regeneration passed', '', + f'`{command}` passed with every regenerated module, so all of them go on to be committed.'] + print(f'The tests passed with every regenerated module: {", ".join(sorted(kept))}') + else: + verdict = 'partial' if kept else 'discarded' + if kept: + heading = 'discarded the modules the tests fail with' + body = (f"`{command}` fails with each module not kept below on top of HEAD's tree, so " + "HEAD's copy of it stays, as #872 does for a sub-vendor that raised. The other " + 'regenerated modules passed and go on to be committed.') + for path in discarded: + print(f'::warning title=Vendor test gate discarded a module::{escape(path)}: ' + f"{escape(command)} fails with it on top of HEAD's tree, so HEAD's copy is kept " + 'and the rest of the regeneration is committed; see the step summary') + else: + heading = 'discarded every regenerated module' + body = (f"{reason}. HEAD's copy of every module is kept, so nothing from this crawl is " + f'committed. The tests are `{command}`.') + print(f'::warning title=Vendor test gate discarded every regenerated module::' + f'{escape(reason)}; see the step summary') + lines = [f'### :warning: Vendor test gate {heading}', '', body, '', + '| Module | Kept |', '| --- | --- |'] + lines.extend(f'| `{change.path}` | {"yes" if change.path in kept else "no"} |' for change in changes) + lines.extend(['', '| Run | Modules applied | Result | Seconds |', '| --- | --- | --- | --- |']) + lines.extend(f'| {run.number} | {run.modules} of {len(changes)} | {run.result} | {run.seconds:.0f} |' + for run in gate.runs) + if gate.unsampled: + print('::warning title=Vendor test gate could not sample memory::ps did not answer, so ' + 'the memory cap was not enforced on every sample') + summary = os.environ.get('GITHUB_STEP_SUMMARY') + if summary: + with open(summary, 'a', encoding='utf-8') as file: + print('\n'.join(lines), file=file) + return verdict + + +def write_verdict(verdict: 'str') -> None: + output = os.environ.get('GITHUB_OUTPUT') + if output: + with open(output, 'a', encoding='utf-8') as file: + print(f'verdict={verdict}', file=file) + + +def _terminate(signum: 'int', frame: 'object') -> 'NoReturn': # pylint: disable=unused-argument + sys.exit(128 + signum) + + +def main(argv: 'Optional[list[str]]' = None) -> int: + """Test the regeneration, and keep what the tests pass with.""" + parser = argparse.ArgumentParser(description=__doc__.split('\n', 1)[0], + usage='%(prog)s OPTIONS -- COMMAND...') + parser.add_argument('--budget-minutes', type=float, required=True, + help='wall time for every run together') + parser.add_argument('--max-memory-mib', type=float, required=True, + help="resident memory a run's process group may hold") + parser.add_argument('--max-file-size-mib', type=float, required=True, + help='largest file a run may write') + # Split by hand: how argparse treats `--` before a REMAINDER differs + # between Python versions. + argv = sys.argv[1:] if argv is None else list(argv) + split = argv.index('--') if '--' in argv else len(argv) + args = parser.parse_args(argv[:split]) + command = argv[split + 1:] + if not command: + parser.error('no test command after --') + + paths = changed_modules() + if not paths: + print(f'No module under {GENERATED_ROOT} changed, so there is nothing to test.') + write_verdict('unchanged') + return 0 + + changes = snapshot(paths) + gate = Gate(changes, command, args.budget_minutes * 60, + int(args.max_memory_mib * MIB), int(args.max_file_size_mib * MIB)) + # SIGTERM, like SIGINT, still puts HEAD's copies back below, with the run + # in flight killed. The workflow's next step covers a SIGKILL. + signal.signal(signal.SIGTERM, _terminate) + try: + try: + kept, reason = decide(gate, paths) + except BudgetExhausted: + kept, reason = frozenset(), (f'The {args.budget_minutes:g}-minute budget ran out ' + 'before the tests could decide') + apply(changes, kept) + # Nothing the tests themselves left under the tree is committed either. + strays = [path for path in changed_modules() if path not in set(paths)] + apply(snapshot(strays), frozenset()) + for path in strays: + print(f'::warning title=Vendor test gate restored a module::{escape(path)} changed ' + 'during the test runs, so it is put back as HEAD has it') + except BaseException: + apply(changes, frozenset()) + raise + write_verdict(report(changes, kept, reason, gate, ' '.join(command))) + return 0 + + +if __name__ == '__main__': + sys.exit(main())