From 89e5996c095146749be5a3b414a16bafcd90a6c8 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Mon, 28 Sep 2026 09:14:24 -0400 Subject: [PATCH] fix(vendor): make main() exit non-zero when any crawler target raises (#872) run() swallowed every crawler exception as a filterable VendorRuntimeWarning, and main() always returned 0 regardless -- so a crawler failure looked identical to a successful regeneration at the only boundary CI or a script checks. Vendor.__init__ fetches and renders before it ever opens the const file for writing, so a failed crawler leaves the previous file untouched: a no-op indistinguishable from success (hit for real in #870's docstring-propagation bug). - run() now returns whether its target succeeded; main() attempts every target regardless of earlier failures (a dead crawler should not block the rest) and returns 1 if any of them raised. - A raising target's qualified name and exception repr now also go to stderr unconditionally via print(), since VendorRuntimeWarning is filterable and PCAPKIT_VERBOSE defaults false. - run() now wraps every vendor() call in pcapkit.vendor.__main__._snapshot_and_restore, a @contextlib.contextmanager that copies a target's existing const file aside before the crawler runs and restores it if the run raises, dropping the copy on success (via contextlib.suppress, not a bare call, so a cleanup failure after a successful run cannot itself turn that success into run() -> False). Per the owner's ruling ("I prefer we use contextlib over manually manage the temp file deletion based on pure best intent (try-finally) and for atomic writing, an easier path is simply keep a copy before running the sub-vendor and revert if anything failed"), this supersedes rounds 4-8's atomic write inside Vendor.__init__ entirely -- _write_atomic is gone, and __init__'s last line is once again a plain open(const_file, 'w'). Snapshot-and-restore is a *wider* guarantee: it covers any way a crawler's own code could touch its const file, not just one open()/print() pair, and it puts the discarding exactly at the per-target boundary the owner's earlier (b) ruling asked for. The except/else split is structural, not a comment: an except clause that always re-raises is followed by an else that only runs on success, rather than an unindented statement after the whole try/except relying on "raise never falls through" -- a mutation test (deleting the raise) proved the fallthrough form lets the exception escape the generator entirely undetected once an else exists to gate the cleanup, whereas the comment-only version let a *different*, wrong exception (FileNotFoundError from the orphaned backup) reach run() instead of the crawler's real one, with every existing test still green either way. - Two capability regressions relative to `main`, both measured and disclosed rather than assumed: a 0o444 (read-only) destination now fails to regenerate (a plain open() needs file write permission, unlike os.replace()'s directory-only requirement), and a writable file inside a non-writable directory also now fails (mkstemp needs directory write permission to create the backup, which open() on an existing file never needed). Both leave the previous file untouched -- fail-safe -- and neither is new relative to rounds 4-8's os.replace()-based design, only relative to `main`. The two failure modes are asymmetric: an unresolvable _dest_path skips the snapshot and lets the target proceed (it fails loudly anyway, since Vendor.__init__ calls _dest_path itself), while a mkstemp failure fails the target outright before vendor() is ever attempted. - Vendor._dest_path is now a classmethod rather than an instance method -- it only ever consulted type(self), so cls is exactly that, with no instance required. This lets _snapshot_and_restore resolve a crawler's destination via vendor._dest_path() before instantiating it at all. No crawler overrides _dest_path, so this is a behaviour-preserving refactor for all of them; confirmed against tests/vendor/test_vendor_dest_path_unit.py (not part of this PR's file set, run read-only for verification), which still passes unchanged. - .github/workflows/cron-vendor.yml's "Update Vendor" step is unchanged this round; re-verified against the snapshot/restore rewrite. - tests/vendor/test_vendor_exit_code_unit.py unchanged this round (still 5 methods) -- its stub crawlers don't override _dest_path, so they hit the "skip the snapshot" fallback and behave exactly as before. - tests/vendor/test_vendor_atomic_write_unit.py renamed to tests/vendor/test_vendor_snapshot_restore_unit.py (git mv), since the mechanism it tests is _snapshot_and_restore, not an atomic write. Its regression pin now asserts on the error run() actually *reported* (assertWarnsRegex against the crawler's own RuntimeError text), not merely that run() returned falsy -- the mutation above is exactly what that distinction catches, and a bare assertFalse(run(...)) passed on it. It also asserts the destination's mode (0o640 in, 0o640 out) alongside content, which is the measurement that actually justifies collapsing rounds 6-8's three mode-permutation tests into one. Verified this round: tests/vendor/test_vendor_snapshot_restore_unit.py (3 methods) and tests/vendor/test_vendor_exit_code_unit.py (5 methods), 8/8 together, under plain unittest. All three mutations are now caught: deleting the `raise` fails 2 of 3 methods, deleting os.replace() fails 1 of 3, and removing the `with` from run() fails 1 of 3. Against both of __main__.py and default.py taken from origin/main together, 2 of 3 methods fail (the byte-for-byte pin with '' != 'GOOD CONTENT\n', and the happy path with None is not true, since pre-fix run() returns nothing); the 0o444 method passes there too, since it is a measurement of a case main() already left untouched, not a regression pin. --- .github/workflows/cron-vendor.yml | 61 +++- pcapkit/vendor/__main__.py | 153 ++++++++- pcapkit/vendor/default.py | 18 +- tests/vendor/test_vendor_exit_code_unit.py | 217 ++++++++++++ .../test_vendor_snapshot_restore_unit.py | 315 ++++++++++++++++++ 5 files changed, 755 insertions(+), 9 deletions(-) create mode 100644 tests/vendor/test_vendor_exit_code_unit.py create mode 100644 tests/vendor/test_vendor_snapshot_restore_unit.py diff --git a/.github/workflows/cron-vendor.yml b/.github/workflows/cron-vendor.yml index a4733b47e1..600fd5b1ff 100644 --- a/.github/workflows/cron-vendor.yml +++ b/.github/workflows/cron-vendor.yml @@ -120,7 +120,66 @@ jobs: export PCAPKIT_VENDOR_RETRY=10 export PCAPKIT_CI_MODE=1 - pcapkit-vendor + # `pcapkit-vendor` (#872) exits non-zero if any target raised, and + # the owner's ruling on that is: discard a failed sub-vendor's own + # changes, keep every zero-exited one's, and never fail the whole + # job over it -- the targets that DID succeed still need isort, + # diff, commit and push below. `set +e`/`set -e` around the one + # command (not a bare `pcapkit-vendor || true`) so $vendor_exit + # actually carries the exit code instead of it being thrown away. + set +e + pcapkit-vendor 2> "$RUNNER_TEMP/vendor-stderr.log" + vendor_exit=$? + set -e + + # Always replayed into the step log, success or failure: this is + # each failing target's qualified name and exception repr (from + # __main__.py's run()) plus its VendorRuntimeWarning line, i.e. + # exactly what a live, uncaptured `pcapkit-vendor` would have + # written to stderr directly. `|| true` deliberately: this line + # runs after `set -e` is restored above, and if $RUNNER_TEMP were + # ever unset or unwritable the redirect on the previous command + # would already have failed to create the log file at all -- + # `cat` on a missing file returns 1 under `-e` and would abort the + # whole step right here, exactly the "one dead crawler kills every + # good regeneration" failure mode this PR exists to remove. + cat "$RUNNER_TEMP/vendor-stderr.log" >&2 || true + + if [ "$vendor_exit" -ne 0 ]; then + # Anchored at the start of the line: the unanchored `failed: ` + # also matches inside the VendorRuntimeWarning line whenever the + # exception's own text happens to contain "failed: " (e.g. + # ConnectionError('request failed: 503')), since that line embeds + # the same {error!r} __main__.py's run() already put in the + # print() line above it -- double-counting one failure as two + # ::warning:: annotations. Only the print() line starts with the + # bare qualified name; the warning line starts with a file path. + failed_targets=$(grep -E '^[A-Za-z_][A-Za-z0-9_.]* failed: ' \ + "$RUNNER_TEMP/vendor-stderr.log" || true) + + { + echo "### :warning: \`pcapkit-vendor\` reported failing target(s) (exit $vendor_exit)" + echo + echo '```' + if [ -n "$failed_targets" ]; then + echo "$failed_targets" + else + echo '(no target name could be extracted from stderr; see the step log above)' + 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.' + } >> "$GITHUB_STEP_SUMMARY" + + if [ -n "$failed_targets" ]; then + while IFS= read -r line; do + echo "::warning title=pcapkit-vendor target failed::${line}" + done <<< "$failed_targets" + else + echo "::warning title=pcapkit-vendor exited $vendor_exit::see the step log above for which target(s) failed" + fi + fi + # `find -mindepth 2`, not a `pcapkit/const/*/*.py` shell glob (#765): # this job runs on `macos-latest`, whose default `bash` on PATH is # the system's, unlike the Makefile's own `SHELL := $(shell command diff --git a/pcapkit/vendor/__main__.py b/pcapkit/vendor/__main__.py index b7c5645d5d..197292330a 100644 --- a/pcapkit/vendor/__main__.py +++ b/pcapkit/vendor/__main__.py @@ -10,8 +10,12 @@ """ import argparse +import contextlib import importlib +import os +import shutil import sys +import tempfile import traceback import warnings from typing import TYPE_CHECKING @@ -23,7 +27,7 @@ if TYPE_CHECKING: from argparse import ArgumentParser - from typing import Type + from typing import Iterator, Type from pcapkit.vendor.default import Vendor @@ -43,28 +47,165 @@ def get_parser() -> 'ArgumentParser': return parser -def run(vendor: 'Type[Vendor]') -> 'None': +@contextlib.contextmanager +def _snapshot_and_restore(vendor: 'Type[Vendor]') -> 'Iterator[None]': + """Copy a target's const file aside before it runs; restore it if it raises. + + The owner's ruling on GitHub issue #872, verbatim: *"an easier path is + simply keep a copy before running the sub-vendor and revert if anything + failed."* This is that -- at the per-target boundary :func:`run` already + owns, which is also exactly where the ruling's ``(b)``, "only discard + changes made by a non-zero sub-vendor", wants the discarding to happen. + + It is a *wider* guarantee than protecting the single + ``open``/``print`` pair :meth:`~pcapkit.vendor.default.Vendor.__init__` + happens to write through would give: it covers any way a crawler's own + code could end up touching its const file, not only that one write. + + The destination is resolved via + :meth:`~pcapkit.vendor.default.Vendor._dest_path` -- a classmethod for + exactly this reason -- called on ``vendor`` itself, *before* + :meth:`Vendor.__init__` ever runs its fetch, its render and its write. + If that resolution fails -- a crawler outside the real + :mod:`pcapkit.vendor` tree, such as a test double, or one whose + ``_dest_path`` genuinely cannot be resolved without instance state -- + the snapshot is skipped and ``vendor()`` runs unprotected, exactly as it + always did; not being able to *name* the file is not itself a reason to + fail the target, and this crawler goes on to fail loudly anyway once + :meth:`Vendor.__init__` calls the same ``_dest_path`` itself. + + That is one of two ways this function can decline to protect a target, + and the two are not symmetric. The other is :func:`tempfile.mkstemp` + failing to create the backup file -- most commonly a destination + directory with no write permission, which needs a *directory*-level + operation this function performs (creating a new entry) that + :meth:`Vendor.__init__`'s own ``open(const_file, 'w')`` never needed (a + *file*-level operation on a file that already exists). Unlike a + ``_dest_path`` failure, this one is **not** swallowed: it happens before + ``vendor()`` is ever called, so it propagates straight out of this + function and fails the target outright, with the previous file + untouched because nothing was ever attempted. So: a path that cannot be + named lets the target proceed unprotected; a path that can be named but + not backed up stops the target before it starts. Both leave the + previous file exactly as it was, for different reasons. + + A symlinked destination has a related divergence from the pre-#872 + behaviour -- documented against the write path by rounds 4-8 (deleted + along with :meth:`~pcapkit.vendor.default.Vendor._write_atomic`, though + the underlying behaviour persists here instead): ``open(const_file, + 'w')`` writes *through* a symlink, into whatever file it points at, + leaving the link itself untouched. ``os.replace(backup, const_file)`` + on restore instead replaces the *link itself* with a regular file. + Measured, for ``link.py`` symlinked to ``real.py``: after a failure and + restore, ``real.py`` is left however the crawler's failed write left + it (truncated, in this case -- nothing here restores the file a + symlink used to point at), ``link.py`` is now a regular file holding + the backup's content, and ``os.path.islink(link.py)`` is + :data:`False`. ``find pcapkit/const -type l`` is still empty, so this + remains latent. + + Nothing is copied at all when the destination does not exist yet: there + is no previous file for a failure to discard, so there is nothing this + context manager needs to do beyond letting the body run. + + Args: + vendor: Subclass of :class:`~pcapkit.vendor.default.Vendor` about to + be run. + + Yields: + Nothing; the caller's ``vendor()`` call belongs inside the ``with``. + + """ + try: + const_file = vendor._dest_path() # pylint: disable=protected-access + except Exception: + yield + return + + if not os.path.isfile(const_file): + yield + return + + fd, backup = tempfile.mkstemp(dir=os.path.dirname(const_file), + prefix=f'.{os.path.basename(const_file)}.', + suffix='.bak') + os.close(fd) + shutil.copy2(const_file, backup) + try: # pylint: disable=no-else-raise + yield + except BaseException: + # os.replace() disposes of the backup by moving it back onto + # const_file -- but only once it succeeds. If it does not, the + # original exception is masked (it survives only as __context__ on + # whatever os.replace raises) and the backup is left orphaned; ``raise`` + # is unconditional here on purpose, since a failed restore is still a + # failure the target must report, not a success to fall through to. + # + # The `else:` below is deliberate, not a style default pylint would + # pick: collapsing it into an unindented statement after this whole + # `try` (as an earlier round of this same PR did) makes "cleanup + # only runs on success" a fact that has to be re-derived from + # "`raise` never falls through", rather than one the structure + # states directly -- and it silently stopped being true the moment + # someone deleted that `raise`. A mutation test proved it: with the + # `raise` gone, the unindented `os.remove(backup)` still ran, after + # `os.replace` had already renamed the backup away, so it raised + # FileNotFoundError instead of the crawler's real error -- and every + # existing test still passed, because run() catches that too. + os.replace(backup, const_file) + raise + else: + # contextlib, per the owner's stated preference over a hand-rolled + # try/finally: a failure to remove the backup after a *successful* + # run must not itself turn that success into run() -> False. + with contextlib.suppress(OSError): + os.remove(backup) + + +def run(vendor: 'Type[Vendor]') -> 'bool': """Script runner. Args: vendor: Subclass of :class:`~pcapkit.vendor.default.Vendor` from :mod:`pcapkit.vendor`. + Returns: + :data:`True` if ``vendor`` ran to completion, :data:`False` if it raised -- + this is what lets :func:`main` turn a crawler failure into a non-zero exit + code instead of a silently-ignored no-op. + Warns: VendorRuntimeWarning: If failed to initiate the ``vendor`` class. """ logger.info(f'{vendor.__module__}.{vendor.__name__}: {vendor.__doc__}') try: - vendor() + with _snapshot_and_restore(vendor): + vendor() except Exception as error: if VERBOSE: traceback.print_exc() + # Unconditional, unlike the warning below: VendorRuntimeWarning is + # filterable and VERBOSE defaults false, so without a plain print a CI + # log that ignores warnings would show nothing at all about which + # target broke or why -- exactly the failure mode this fix exists for. + print(f'{vendor.__module__}.{vendor.__name__} failed: {error!r}', file=sys.stderr) warn(f'{vendor.__module__}.{vendor.__name__} <{error!r}>', VendorRuntimeWarning, stacklevel=2) + return False + return True def main() -> 'int': """Entrypoint. + Every target is attempted regardless of earlier failures -- a crawler that + is down should not block regeneration of everything else -- but the exit + code reflects whether *all* of them succeeded, so a no-op regeneration + (crawler raised, previous const file left untouched) is distinguishable + from a real one at the process boundary, which is what CI actually checks. + + Returns: + ``0`` if every target ran to completion, ``1`` if any target raised. + Warns: InvalidVendorWarning: If vendor target not found in :mod:`pcapkit.vendor` module. @@ -88,9 +229,11 @@ def main() -> 'int': # with multiprocessing.Pool() as pool: # pool.map(run, target_list) + success = True for vendor in target_list: - run(vendor) - return 0 + if not run(vendor): + success = False + return 0 if success else 1 if __name__ == '__main__': diff --git a/pcapkit/vendor/default.py b/pcapkit/vendor/default.py index c908d058da..e38495000e 100644 --- a/pcapkit/vendor/default.py +++ b/pcapkit/vendor/default.py @@ -430,7 +430,8 @@ def __init__(self) -> 'None': with open(const_file, 'w') as file: # pylint: disable=unspecified-encoding print(context, file=file) - def _dest_path(self) -> 'str': + @classmethod + def _dest_path(cls) -> 'str': """Resolve the ``const/`` file this crawler's module mirrors. :attr:`~Vendor.__module__` sits somewhere under the :mod:`pcapkit.vendor` @@ -470,12 +471,23 @@ def _dest_path(self) -> 'str': ``sys.exit(SomeCrawler())``, so running one from a second checkout, or from a module that never sits under ``vendor/`` at all, resolves :mod:`pcapkit.vendor` from wherever it is installed while - ``inspect.getfile(type(self))`` names a file elsewhere entirely -- and + ``inspect.getfile(cls)`` names a file elsewhere entirely -- and ``Vendor.__init__`` then opens that path with ``'w'``, silently truncating whatever :attr:`~Vendor.__module__` actually names instead of raising. So the escape is rejected outright, rather than trusted to produce a harmless-looking wrong path. + This is a classmethod, not an instance method, deliberately: nothing + in the body ever needed instance state -- it only ever consulted + ``type(self)``, so ``cls`` is exactly that, with no instance required + to obtain it. That lets a caller resolve a crawler's destination + *without* instantiating it, and so without triggering + :meth:`Vendor.__init__`'s network fetch, render and write -- + :func:`pcapkit.vendor.__main__.run`'s pre-run snapshot (GitHub issue + #872) is exactly such a caller. Every crawler defined in this + codebase inherits this implementation unchanged; none overrides + ``_dest_path``, so the classmethod is authoritative for all of them. + Returns: Absolute path of the constant module this crawler should write. @@ -488,7 +500,7 @@ def _dest_path(self) -> 'str': import pcapkit.vendor # pylint: disable=import-outside-toplevel vendor_root = os.path.dirname(os.path.abspath(pcapkit.vendor.__file__)) - module_file = os.path.abspath(inspect.getfile(type(self))) + module_file = os.path.abspath(inspect.getfile(cls)) rel_path = os.path.relpath(module_file, vendor_root) if rel_path.split(os.sep, 1)[0] == os.pardir: diff --git a/tests/vendor/test_vendor_exit_code_unit.py b/tests/vendor/test_vendor_exit_code_unit.py new file mode 100644 index 0000000000..92868dd094 --- /dev/null +++ b/tests/vendor/test_vendor_exit_code_unit.py @@ -0,0 +1,217 @@ +# -*- coding: utf-8 -*- +"""``python -m pcapkit.vendor`` must fail its exit code when a crawler raises. + +GitHub issue #872. Before this change, :func:`pcapkit.vendor.__main__.run` +swallowed every exception a crawler raised -- filed as a merely-filterable +:class:`~pcapkit.utilities.warnings.VendorRuntimeWarning` -- and +:func:`pcapkit.vendor.__main__.main` always ``return 0`` (line 93 pre-fix), with +no accounting of whether any target had failed. Because +:meth:`pcapkit.vendor.default.Vendor.__init__` runs its network fetch and +render *before* the ``open(const_file, 'w')`` on its last line, a failed +crawler writes nothing at all: the previous constant file is left untouched, +one warning is emitted (silently, since +:data:`~pcapkit.utilities.logging.VERBOSE` defaults to :data:`False`), and the +process reports success regardless. A no-op regeneration was therefore +indistinguishable from a real one at the only boundary a CI job or a script +actually checks -- the exit code. This was hit for real during GitHub issue +#870's work, where a docstring edit silently failed to propagate. + +The fix makes :func:`~pcapkit.vendor.__main__.run` return whether its target +succeeded, and :func:`~pcapkit.vendor.__main__.main` return ``1`` if *any* +target failed while still attempting every target regardless of an earlier +failure -- a crawler that is down should not stop the rest from regenerating. + +This suite exercises the real :func:`~pcapkit.vendor.__main__.main` entrypoint +end to end, with the default (no ``--target``) argument path, rather than +calling :func:`~pcapkit.vendor.__main__.run` directly -- the point is to pin +both ``main()`` itself (what an operator or a workflow actually invokes) and +its exit code, not just the helper function underneath it. It makes **no +network call**: +a stub :class:`~pcapkit.vendor.default.Vendor` subclass overrides +:meth:`~pcapkit.vendor.default.Vendor._request` (the low-level fetch +:meth:`Vendor.__init__` calls first) to raise directly, so the failure is +synthetic and instantaneous rather than a real fetch timing out. The default +target list itself is stubbed too, by patching the +``pcapkit.vendor.__main__.vendor_module`` name :func:`main` reads its +``__all__`` from -- so this suite never imports, runs, or writes anything +under the real 117 crawlers, and touches no file under :file:`pcapkit/const/`. + +Run against the pre-fix code (``run()`` ending on a bare ``return None`` and +``main()`` ending on a bare ``return 0``), all 5 methods below fail: +:meth:`~ExitCodeTests.test_run_returns_false_for_a_raising_crawler` (``run()`` +returns :data:`None`, not the exact value :data:`False` -- +:meth:`~unittest.TestCase.assertIs` is what makes this one discriminate: a +plain :meth:`~unittest.TestCase.assertFalse` would pass on both trees, since +:data:`None` is already falsy), +:meth:`~ExitCodeTests.test_run_returns_true_for_a_clean_crawler` (``run()`` +returns :data:`None`, not :data:`True`), +:meth:`~ExitCodeTests.test_main_returns_non_zero_when_a_target_raises` and +:meth:`~ExitCodeTests.test_main_still_attempts_every_target_after_an_earlier_one_fails` +(``main()`` returns ``0``, not ``1``), and +:meth:`~ExitCodeTests.test_failing_target_and_error_reach_stderr_unconditionally` +(with warnings filtered out, pre-fix ``run()`` writes nothing to stderr at +all). + +""" +from __future__ import annotations + +import contextlib +import importlib.util +import io +import types +import unittest +import warnings +from unittest import mock + +#: Same two-dependency gate :file:`tests/vendor/test_crawler_reachability_unit.py` +#: and :file:`tests/vendor/test_vendor_dest_path_unit.py` use. Importing +#: :mod:`pcapkit.vendor.__main__` imports :mod:`pcapkit.vendor` (for the +#: ``vendor_module`` default-target-list fallback this suite patches), which +#: pulls in every crawler subpackage unconditionally -- +#: :mod:`pcapkit.vendor.default` imports :mod:`requests` at module scope and +#: seven crawlers import :mod:`bs4` at module scope. +VENDOR_DEPS = ('requests', 'bs4') + +#: Whether the crawler machinery is importable at all. Both ship in the +#: ``test`` extra (:file:`pyproject.toml`), so this should not skip in CI; kept +#: as belt-and-braces for an environment that lacks them. +HAS_VENDOR_DEPS = all(importlib.util.find_spec(name) is not None for name in VENDOR_DEPS) + + +@unittest.skipUnless(HAS_VENDOR_DEPS, f'vendor extra not installed ({", ".join(VENDOR_DEPS)})') +class ExitCodeTests(unittest.TestCase): + """``run()``/``main()`` exit-code accounting for a raising crawler.""" + + def setUp(self) -> None: + import pcapkit.vendor.__main__ as vendor_main + from pcapkit.vendor.default import Vendor + + self.vendor_main = vendor_main + self.Vendor = Vendor + + def _make_failing_crawler(self) -> 'type': + """A ``Vendor`` subclass that raises before ever touching the network. + + Overrides :meth:`~pcapkit.vendor.default.Vendor._request` -- the first + thing :meth:`Vendor.__init__` calls -- instead of letting a real fetch + fail, so this is deterministic and makes no network call. + + """ + Vendor = self.Vendor + + class FailingCrawler(Vendor): + """Stub crawler for GitHub issue #872 -- always raises, no network.""" + + def _request(self) -> 'list[str]': + raise RuntimeError('stub crawler failure for GitHub issue #872') + + return FailingCrawler + + def _make_succeeding_crawler(self) -> 'type': + """A ``Vendor`` subclass that records it ran and touches nothing else. + + Overrides ``__init__`` entirely, bypassing + :meth:`~pcapkit.vendor.default.Vendor.__init__`'s network fetch and + file write -- this class exists only to prove a *later* target still + runs after an *earlier* one raised, not to exercise real regeneration. + + """ + Vendor = self.Vendor + + class SucceedingCrawler(Vendor): + """Stub crawler for GitHub issue #872 -- always succeeds, no I/O.""" + + ran = False + + def __init__(self) -> None: # pylint: disable=super-init-not-called + type(self).ran = True + + return SucceedingCrawler + + def test_run_returns_false_for_a_raising_crawler(self) -> None: + failing = self._make_failing_crawler() + # assertIs, not assertFalse: pre-fix run() returns None, which is + # merely falsy and would pass an assertFalse on both trees, pinning + # nothing. The exact value False is what main() actually branches + # on (`if not run(vendor): success = False`), so this has to check + # for that literal, not for anything that happens to be falsy. + self.assertIs(self.vendor_main.run(failing), False, + 'run() must return the exact value False, not merely something ' + 'falsy, when the crawler raises') + + def test_run_returns_true_for_a_clean_crawler(self) -> None: + succeeding = self._make_succeeding_crawler() + self.assertTrue(self.vendor_main.run(succeeding), + 'run() must report success when the crawler does not raise') + + def test_main_returns_non_zero_when_a_target_raises(self) -> None: + # The regression this issue is about: pre-fix, main() ends on a bare + # `return 0` with no accounting at all, so this assertion fails + # (0 != 1) against origin/main; see the module docstring for the + # full pre-fix failure list. + failing = self._make_failing_crawler() + fake_vendor_module = types.SimpleNamespace(__all__=['FailingCrawler'], + FailingCrawler=failing) + + with mock.patch.object(self.vendor_main, 'vendor_module', fake_vendor_module), \ + mock.patch('sys.argv', ['pcapkit-vendor']): + exit_code = self.vendor_main.main() + + self.assertEqual(exit_code, 1, + 'main() must return 1 (its own documented contract) when its only ' + 'target raised, so a no-op regeneration is distinguishable from a ' + 'real one; see GitHub issue #872') + + def test_main_still_attempts_every_target_after_an_earlier_one_fails(self) -> None: + # "Regenerate everything you can, then exit non-zero": a crawler that + # is down must not stop a later, unrelated target from running. + failing = self._make_failing_crawler() + succeeding = self._make_succeeding_crawler() + fake_vendor_module = types.SimpleNamespace( + __all__=['FailingCrawler', 'SucceedingCrawler'], + FailingCrawler=failing, + SucceedingCrawler=succeeding, + ) + + with mock.patch.object(self.vendor_main, 'vendor_module', fake_vendor_module), \ + mock.patch('sys.argv', ['pcapkit-vendor']): + exit_code = self.vendor_main.main() + + self.assertTrue(succeeding.ran, + 'a target after a raising one must still be attempted') + self.assertEqual(exit_code, 1, + 'a partial failure must still be reported as exit code 1, ' + "main()'s own documented contract") + + def test_failing_target_and_error_reach_stderr_unconditionally(self) -> None: + # Point 2 of the issue: VendorRuntimeWarning is filterable and VERBOSE + # defaults False, so the plain print() run() now does is the only + # thing guaranteed to reach a CI log regardless of warning filters. + # + # Warnings are suppressed here *on purpose*, and that is load-bearing + # rather than incidental: under the default filter, the pre-existing + # warn() call already writes both the qualified name and the + # exception repr to stderr on its own (Python's default "once per + # location" display of an unfiltered warning), so an unsuppressed run + # would pass this assertion whether or not the print() line exists -- + # pinning nothing. Only with warnings silenced does the print() + # become the one thing keeping this assertion true, which is the + # scenario the comment above (and the issue) is actually about: a CI + # log that has warnings turned off or filtered. + failing = self._make_failing_crawler() + buffer = io.StringIO() + with contextlib.redirect_stderr(buffer), warnings.catch_warnings(): + warnings.simplefilter('ignore') + self.vendor_main.run(failing) + + stderr = buffer.getvalue() + self.assertIn(f'{failing.__module__}.{failing.__name__}', stderr, + 'the failing target\'s qualified name must reach stderr unconditionally, ' + 'even with warnings filtered out') + self.assertIn('stub crawler failure for GitHub issue #872', stderr, + 'the exception repr must reach stderr unconditionally, even with ' + 'warnings filtered out') + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/vendor/test_vendor_snapshot_restore_unit.py b/tests/vendor/test_vendor_snapshot_restore_unit.py new file mode 100644 index 0000000000..68609309dd --- /dev/null +++ b/tests/vendor/test_vendor_snapshot_restore_unit.py @@ -0,0 +1,315 @@ +# -*- coding: utf-8 -*- +"""``run()`` must restore a target's const file when the target's run fails. + +GitHub issue #872. Rounds 4-8 made +:meth:`~pcapkit.vendor.default.Vendor.__init__`'s own write atomic -- a +temp-file-then-:func:`os.replace` at the point of the write, with permission +matching for the replacement. The owner's ruling on that, verbatim: *"I +prefer we use contextlib over manually manage the temp file deletion based +on pure best intent (try-finally) and for atomic writing, an easier path is +simply keep a copy before running the sub-vendor and revert if anything +failed."* + +That supersedes the atomic write entirely rather than adjusting it. +:meth:`~pcapkit.vendor.default.Vendor._write_atomic` no longer exists; +:meth:`Vendor.__init__`'s last line is once again a plain:: + + with open(const_file, 'w') as file: + print(context, file=file) + +The protection moved up a level, to +:func:`pcapkit.vendor.__main__._snapshot_and_restore`, a +:func:`contextlib.contextmanager` that :func:`~pcapkit.vendor.__main__.run` +wraps every ``vendor()`` call in. It copies the target's existing const file +aside *before* the crawler is instantiated at all -- resolving the +destination via :meth:`~pcapkit.vendor.default.Vendor._dest_path`, a +classmethod for exactly this reason, called on the crawler *class* rather +than an instance -- and restores that copy if the crawler raises for any +reason. This is a **wider** guarantee than an atomic write at one call site: +it covers any way a crawler's own code could end up touching its const +file, not only the single ``open``/``print`` pair the base class happens to +use, and it puts the discarding exactly at the per-target boundary the +owner's earlier ruling (b) asked for. + +Because the protection now lives in ``run()`` rather than inside +:meth:`Vendor.__init__`, this suite calls +:func:`pcapkit.vendor.__main__.run` -- not the crawler class directly, the +way rounds 4-8's version of this file did -- exactly as +:file:`tests/vendor/test_vendor_exit_code_unit.py` already does. Calling the +crawler class directly would exercise :meth:`Vendor.__init__` in isolation, +which is no longer where the guarantee lives. + +This suite makes no network call: :class:`~pcapkit.vendor.default.Vendor`'s +``_request``, ``count``, ``context`` and ``_dest_path`` are all overridden by +a stub subclass built per test, with the destination pointed at a temporary +directory rather than the real :file:`pcapkit/const/`. ``_dest_path`` is +overridden as a ``@classmethod``, matching the base class, so +``_snapshot_and_restore`` can call it on the stub class itself before +instantiating it -- overriding it as a plain instance method would raise +``TypeError`` when called that way, which ``_snapshot_and_restore`` would +(correctly, for a crawler it genuinely cannot resolve a path for) treat as +"skip the snapshot", silently exercising the unprotected fallback instead of +what these tests are actually about. + +The three permission-matching methods rounds 6-8 added +(``test_existing_permissions_survive_a_successful_write``, +``test_a_read_only_destination_is_regenerated_and_stays_read_only``, +``test_a_new_destination_gets_umask_masked_permissions``) are gone with the +code they pinned: :meth:`_write_atomic`'s mode-matching had three genuinely +different branches (existing mode preserved via :func:`os.stat`, the +``0o444``-succeeds-anyway case, and a fresh-file umask fallback), each +worth its own test. Snapshot-and-restore has no equivalent branching to +pin -- a restored file is simply the exact bytes and mode +:func:`shutil.copy2` copied out, once, for every case alike -- so keeping +three separate permutations would pin nothing that one does not already +cover, and I chose to delete them rather than repoint them at a property +that no longer varies by case. + +:meth:`SnapshotRestoreTests.test_successful_run_replaces_the_previous_file_content` +is the happy-path round trip. Run against ``origin/main`` (pre-#872, no +exit-code accounting at all), it fails too -- +``self.assertTrue(self.vendor_main.run(...))`` sees :data:`None`, since +pre-fix ``run()`` never returns anything -- so this is a second method the +fix changes, not only the one below. + +What survives, in the new shape, is the invariant the original pin actually +encoded: a failure during or around a target's regeneration leaves the +previous const file byte-for-byte intact. That is +:meth:`SnapshotRestoreTests.test_a_failure_leaves_the_previous_file_byte_for_byte_intact` +below, still simulated by making :func:`print` raise once the destination is +open for writing -- :meth:`Vendor.__init__` still opens ``const_file`` +directly (there is no temp file to redirect the write onto any more), so +this again truncates the previous content before the injected failure +fires; :func:`~pcapkit.vendor.__main__._snapshot_and_restore` is what +restores it afterwards. It asserts on the *reported* error +(:meth:`~unittest.TestCase.assertWarnsRegex` against the crawler's own +``RuntimeError`` text) rather than merely on ``run()`` returning falsy, +deliberately: a mutant that deletes the ``raise`` from +``_snapshot_and_restore``'s ``except BaseException:`` clause still returns a +falsy ``run()`` -- the ``os.remove(backup)`` that then runs unguarded raises +:exc:`FileNotFoundError` (the backup was just renamed away by +``os.replace``), which ``run()`` dutifully reports *instead of* the +crawler's own error -- so the return value alone cannot tell "failed for the +right reason" from "failed for the wrong one", and a bare +``assertFalse(run(...))`` passed on that mutant. It also asserts the +destination's mode (``0o640`` in, ``0o640`` out) alongside its content, +which is the measurement that actually justifies collapsing rounds 6-8's +three separate mode-permutation tests into one: restore preserves mode as a +side effect of :func:`shutil.copy2` plus a rename, for every case alike, +rather than needing a case-by-case check. Run against both of +:mod:`pcapkit.vendor.__main__` and :mod:`pcapkit.vendor.default` taken from +``origin/main`` (swapping only one of the two leaves the other's fix in +place and proves nothing -- see GitHub issue #872's own round 8), this +fails on the content assertion with ``'' != 'GOOD CONTENT\\n'``: the +previous content is destroyed before ``run()`` (pre-#872) ever gets a +chance to report anything. + +:meth:`SnapshotRestoreTests.test_a_read_only_destination_is_left_exactly_as_it_was` +re-measures the ``0o444`` case under the new design rather than assuming it +still behaves as rounds 6-8 documented. It does not: ``os.replace()`` +(round 4-8's last step) needed write permission on the *directory*, not the +target file, so a ``0o444`` destination was still successfully regenerated. +A plain ``open(const_file, 'w')`` (this round's design) needs write +permission on the file itself, so a ``0o444`` destination now fails to +regenerate exactly as it did on `main`, before #872 -- a capability the +atomic-write approach had and this one has deliberately traded away for +simplicity, per the owner's ruling. What the new design still guarantees is +that the failure leaves the file exactly as it was, mode included, which is +what this method actually pins. + +Run against ``origin/main``, this method passes -- it is a measurement, not +a regression pin. `main`'s plain ``open(const_file, 'w')`` already raised +:exc:`PermissionError` on a ``0o444`` destination *before* truncating +anything (the permission check happens before the write, not after), so the +file was already left untouched on the pre-#872 tree for this one specific +scenario; there is nothing here for the fix to have changed. + +The ``0o444`` file is one traded capability; there is a second, in the +opposite direction, not covered by any method here. A writable file inside +a *non-writable directory* also regenerated on `main` -- opening an +*existing* file for writing needs write permission on the file, not on its +parent directory -- but fails under this design, because +:func:`~pcapkit.vendor.__main__._snapshot_and_restore`'s +:func:`tempfile.mkstemp` call needs write permission on the directory to +create the backup, and :meth:`Vendor.__init__`'s own ``open(const_file, +'w')`` never needed that. Measured: ``run()`` returns :data:`False` with a +:exc:`PermissionError`, and the previous content survives untouched because +``vendor()`` is never even called. This is not new relative to rounds 4-8 +(``os.replace()`` needed directory write too), only relative to `main`, so +the PR now discloses two traded capabilities rather than one. See +:func:`~pcapkit.vendor.__main__._snapshot_and_restore`'s own docstring for +the asymmetry between this and the ``0o444`` case: a ``_dest_path`` +resolution failure *skips* the snapshot and lets the target proceed +unprotected, while a ``mkstemp`` failure *fails* the target before +``vendor()`` is ever attempted. + +""" +from __future__ import annotations + +import collections +import importlib.util +import os +import stat +import tempfile +import unittest +from unittest import mock + +from pcapkit.utilities.warnings import VendorRuntimeWarning + +#: Same two-dependency gate the rest of :file:`tests/vendor/` uses. Importing +#: :mod:`pcapkit.vendor.__main__` imports :mod:`pcapkit.vendor` (for its +#: ``vendor_module`` default-target-list fallback), which pulls in every +#: crawler subpackage unconditionally -- :mod:`pcapkit.vendor.default` +#: itself imports :mod:`requests` at module scope and seven crawlers import +#: :mod:`bs4` at module scope. +VENDOR_DEPS = ('requests', 'bs4') + +#: Whether the crawler machinery is importable at all. Both ship in the +#: ``test`` extra (:file:`pyproject.toml`), so this should not skip in CI; +#: kept as belt-and-braces for an environment that lacks them. +HAS_VENDOR_DEPS = all(importlib.util.find_spec(name) is not None for name in VENDOR_DEPS) + + +@unittest.skipUnless(HAS_VENDOR_DEPS, f'vendor extra not installed ({", ".join(VENDOR_DEPS)})') +class SnapshotRestoreTests(unittest.TestCase): + """``run()``'s snapshot-and-restore, isolated from fetch/render/discovery.""" + + def setUp(self) -> None: + import pcapkit.vendor.__main__ as vendor_main + from pcapkit.vendor.default import Vendor + + self.vendor_main = vendor_main + self.Vendor = Vendor + self._tempdir = tempfile.TemporaryDirectory(prefix='pcapkit-vendor-snapshot-restore-test-') + self.addCleanup(self._tempdir.cleanup) + + def _make_crawler(self, const_file: 'str', rendered: 'str') -> 'type': + """A ``Vendor`` subclass with fetch, render and destination all stubbed. + + No network call and no real CSV parsing: ``_request``/``count``/ + ``context`` return canned values and ``_dest_path`` is pinned to + ``const_file`` -- overridden as a ``@classmethod``, matching the base + class, so :func:`~pcapkit.vendor.__main__._snapshot_and_restore` can + call it on the class itself before this crawler is ever instantiated. + + Args: + const_file: Where ``_dest_path`` should report the constant file + lives. + rendered: The fixed content ``context`` should return. + + """ + Vendor = self.Vendor + + class StubCrawler(Vendor): + """Stub crawler for GitHub issue #872 -- no network, fixed destination.""" + + def _request(self) -> 'list[str]': + return [] + + def count(self, data: 'list[str]') -> 'collections.Counter[str]': + return collections.Counter() + + def context(self, data: 'list[str]') -> 'str': + return rendered + + @classmethod + def _dest_path(cls) -> 'str': + return const_file + + return StubCrawler + + def test_successful_run_replaces_the_previous_file_content(self) -> None: + const_file = os.path.join(self._tempdir.name, 'const.py') + with open(const_file, 'w', encoding='utf-8') as file: + file.write('OLD CONTENT\n') + + stub_crawler = self._make_crawler(const_file, 'NEW CONTENT') + self.assertTrue(self.vendor_main.run(stub_crawler)) + + with open(const_file, encoding='utf-8') as file: + self.assertEqual(file.read(), 'NEW CONTENT\n') + self.assertEqual(os.listdir(self._tempdir.name), ['const.py'], + 'no backup file should be left behind after a successful run') + + def test_a_failure_leaves_the_previous_file_byte_for_byte_intact(self) -> None: + const_file = os.path.join(self._tempdir.name, 'const.py') + with open(const_file, 'w', encoding='utf-8') as file: + file.write('GOOD CONTENT\n') + os.chmod(const_file, 0o640) + + stub_crawler = self._make_crawler(const_file, 'NEW CONTENT') + + # side_effect is a callable, not a bare exception, deliberately: + # run() itself calls print() -- unconditionally, to report the + # failure on stderr -- and a bare `side_effect=RuntimeError(...)` + # would raise there too, breaking run()'s own error reporting + # instead of simulating a failure inside the crawler's write. Only + # the crawler's own print(context, file=file) call -- recognisable + # by its first argument being the rendered content -- is made to + # raise; every other print() call (run()'s own included) passes + # through to the real builtin. + real_print = print + + def _raise_on_render(*args: object, **kwargs: object) -> None: + if args and args[0] == 'NEW CONTENT': + raise RuntimeError('simulated failure (GitHub issue #872)') + real_print(*args, **kwargs) + + # assertWarnsRegex against the crawler's OWN error text, not merely + # assertFalse(run(...)): a mutant that deletes the `raise` from + # _snapshot_and_restore's `except BaseException:` still returns a + # falsy run() -- the unguarded os.remove(backup) that then runs + # raises FileNotFoundError instead (the backup was just renamed away + # by os.replace), and run() reports THAT, not this test's injected + # RuntimeError. A bare falsy check cannot tell "failed for the right + # reason" from "failed for the wrong one"; this can. + with mock.patch('builtins.print', side_effect=_raise_on_render): + with self.assertWarnsRegex(VendorRuntimeWarning, 'simulated failure'): + result = self.vendor_main.run(stub_crawler) + self.assertFalse(result) + + with open(const_file, encoding='utf-8') as file: + self.assertEqual(file.read(), 'GOOD CONTENT\n', + 'a failure during a target\'s run must leave the previous const ' + 'file byte-for-byte untouched, not truncated or partially ' + 'overwritten') + # The mode check, not just content: this is what actually justifies + # collapsing rounds 6-8's three separate mode-permutation tests into + # one -- restore preserves mode as a side effect of shutil.copy2 + # plus a rename, uniformly, rather than needing a case-by-case check. + mode = stat.S_IMODE(os.stat(const_file).st_mode) + self.assertEqual(mode, 0o640, + f'a destination that existed at 0o640 must still be 0o640 after a ' + f'failed run, not {oct(mode)}') + self.assertEqual(os.listdir(self._tempdir.name), ['const.py'], + 'a failed run must not leave a backup file behind') + + def test_a_read_only_destination_is_left_exactly_as_it_was(self) -> None: + const_file = os.path.join(self._tempdir.name, 'const.py') + with open(const_file, 'w', encoding='utf-8') as file: + file.write('GOOD CONTENT\n') + os.chmod(const_file, 0o444) + + stub_crawler = self._make_crawler(const_file, 'NEW CONTENT') + + # Measured, not assumed (see the module docstring): unlike round + # 4-8's os.replace()-based design, a plain open(const_file, 'w') + # needs write permission on the file itself, so this still fails -- + # run() must still report that failure. + self.assertFalse(self.vendor_main.run(stub_crawler)) + + # What the new design does guarantee: the failure leaves the file + # exactly as it was, content and mode both, even though nothing was + # ever actually rewritten (open() raised before truncating anything). + with open(const_file, encoding='utf-8') as file: + self.assertEqual(file.read(), 'GOOD CONTENT\n', + 'a 0o444 destination must survive a failed run with its ' + 'previous content untouched') + mode = stat.S_IMODE(os.stat(const_file).st_mode) + self.assertEqual(mode, 0o444, + f'a 0o444 destination must still be 0o444 after a failed run, ' + f'not {oct(mode)}') + + +if __name__ == '__main__': + unittest.main()