diff --git a/pyproject.toml b/pyproject.toml index a5bd2c3f15..1ee7975aeb 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -263,6 +263,31 @@ test = [ # pytest captures and shows only on failure; they change no result. See #507. "requests", "beautifulsoup4", + # Same reason as the two above, for a different test: without it + # ``tests/project/test_isort_clean.py`` skipped on every CI leg, so the only + # guard on ``make isort``'s four recipe lines -- and on the target-resolving + # globs behind them, wrong three times over (#757, #765, #767) -- ran + # nowhere. #766. + # + # A lint tool in the ``test`` extra rather than in a ``lint`` extra of its + # own, deliberately. ``isort`` is what *this suite* needs in order not to + # skip, which is exactly what this extra is for; a ``lint`` extra would also + # have to carry ``mypy``, ``pylint``, ``bandit`` and ``vermin`` to mean + # anything, and carrying mypy would silently close the ``HAS_MYPY`` gap that + # #779 decided to leave open -- reversing a ruling from an unrelated change. + # + # Unlike the other four linters, isort has no CI check of its own to defer + # to: ``.github/workflows/lint.yml`` installs "vermin pylint mypy bandit" + # and says in its own header that isort appears only in + # ``cron-vendor.yml``, as a formatter rather than as a check. Cheap, too -- + # a pure-Python wheel, and all four lines together measure about 1.5s. + # + # Unpinned, matching ``requests`` and ``beautifulsoup4`` above rather than + # inventing an unmeasured floor (verified against 9.0.1). A future isort + # whose verdict moves therefore turns that test red, which is the intended + # failure: it names the file and prints the diff, where the skip it replaced + # said nothing at all. + "isort", ] # setuptools specific diff --git a/tests/_dependency_gates.py b/tests/_dependency_gates.py index 6c2f0e9989..45fe792438 100644 --- a/tests/_dependency_gates.py +++ b/tests/_dependency_gates.py @@ -173,6 +173,27 @@ # anyway, so the day some extra does carry mypy the gap closes itself # rather than needing this table rewritten. 'mypy': ('mypy',), + # The other lint-tier tool with a gate, and the counter-example to the entry + # above rather than a copy of it. ``isort`` is a ``test`` extra requirement + # (#766), so :func:`extras_providing` resolves it to ``{'test'}``, every + # pytest job installs it, and ``HAS_ISORT`` is a satisfied gate with no + # :data:`DEPENDENCY_GATE_EXCLUSIONS` entry at all -- which is the whole point + # of listing it here: the guard now *counts* a gate it could not see before, + # and finds nothing to report. + # + # Why the opposite ruling from ``mypy``, whose gap is declined above: that + # entry's reason turns on lint.yml already running mypy over the whole + # package, so a pytest leg would pay for a second copy of an existing check. + # No such check exists for isort. lint.yml installs "vermin pylint mypy + # bandit" and its own header says isort is deliberately not among them -- + # it appears only in cron-vendor.yml, as a formatter that rewrites the + # generated constants on a schedule rather than as a check -- so excluding + # this flag would have left `make isort`'s verdict unverified everywhere, + # which is the #766 complaint. Cost does not argue the other way either: + # isort is a pure-Python wheel and all four Makefile lines together measure + # about 1.5s, against the ~238 MB and licence questions that keep ``NGAP`` + # out of ``all``. + 'isort': ('isort',), # ``beautifulsoup4`` ships ``bs4``; ``html5lib`` is an *extra of* that # distribution, so ``beautifulsoup4`` alone does not provide it. That # distinction is the whole reason ``HAS_CRAWLER_DEPS`` is satisfied in CI @@ -492,8 +513,19 @@ class Exclusion(NamedTuple): 'not. Visibly is the whole point of #779: the gate was an inline ' 'try/except ImportError skipTest until then, which works but is invisible to ' 'this module by construction, since _gates_of() walks a decorator list and ' - 'never a function body. That is the dark-test hazard #745 exists to stop, and ' - '#766 carries the same shape for the sibling isort check.\n\n' + 'never a function body. That is the dark-test hazard #745 exists to stop.\n\n' + 'The sibling isort check (#766) had the identical invisible shape and was ' + 'fixed the other way -- a HAS_ISORT gate plus isort in the test extra, so it ' + 'is a satisfied gate with no entry here. The two rulings disagree for a ' + 'reason that is specific to this paragraph rather than to lint tools in ' + 'general: the "second copy of a check lint.yml already runs" argument above ' + 'is what declines mypy, and lint.yml installs no isort at all (its header ' + 'records that deliberately -- isort appears only in cron-vendor.yml, as a ' + 'formatter rather than as a check), so for isort there was no first copy to ' + 'defer to and excluding it would have left `make isort` verified nowhere. ' + 'This entry is therefore narrow: it is not a precedent that a gate on a lint ' + 'tool gets excluded, only that a gate duplicating an existing lint.yml check ' + 'does.\n\n' 'test, engine-tests and gate are exactly the three jobs whose selection ' 'reaches tests/vendor/test_vendor_reg_apptype_generator_unit.py -- the two ' 'ignore-shape legs and the whole-suite one. integration and pypcap-parity ' diff --git a/tests/project/test_isort_clean.py b/tests/project/test_isort_clean.py index 9383d614c8..24bd228af3 100644 --- a/tests/project/test_isort_clean.py +++ b/tests/project/test_isort_clean.py @@ -115,17 +115,46 @@ never runs this test suite at all -- it commits and pushes on its own, with no pytest step in between. -``isort`` is deliberately absent from both ``Pipfile`` and ``pyproject.toml``: -:file:`.github/workflows/lint.yml` notes it stays local-only, unlike the other -three linters. So this test skips outright when isort is not installed, the -same way :class:`tests.project.test_release_gates.TestYAMLAgreesWithTheScanner` -skips its PyYAML-dependent half. #766 covers why that skip is itself invisible -to :file:`tests/_dependency_gates.py`'s guard -- deferred there, not fixed here. +Until #766 this module gated on isort with a ``try: import isort`` inside +:meth:`~TestIsortIsCleanOnThePackage.setUpClass`, raising +:exc:`~unittest.SkipTest` from the body -- and isort was in no +:file:`pyproject.toml` extra, so every CI leg skipped it. Both halves of that +are fixed here, and they had to move together. + +The gate is now a module-level :data:`HAS_ISORT` read by an +``@unittest.skipUnless`` decorator, because +:func:`tests._dependency_gates._gates_of` walks a definition's +``decorator_list`` and never a function body: an inline ``skipTest`` is +invisible to that scan *by construction*, which is the dark-test hazard #745 +exists to stop and which #779 fixed the same way for the sibling ``mypy`` +check. + +The install line is the other half. ``isort`` is now a ``test`` extra +requirement, so the three jobs whose selection collects this module -- +``test``, ``engine-tests`` and ``gate`` -- run it for real rather than +reporting a skip. That is the opposite resolution from ``mypy``'s, whose +:data:`~tests._dependency_gates.DEPENDENCY_GATE_EXCLUSIONS` entry declines the +install line, and the difference is not taste: :file:`lint.yml` already runs +``make mypy`` over the whole package, so a pytest copy would be a *second* +copy, while that same workflow's header records that ``isort`` is deliberately +not one of the four linters it installs -- it appears only in +:file:`cron-vendor.yml`, as a formatter that rewrites the generated constants +on a schedule rather than as a check. So declining the install line here would +have left ``make isort``'s verdict checked nowhere at all, which is what #766 +objected to. It is cheap enough to be uncontroversial, too: all four lines +below take about 1.5s together, the "costs nothing per leg" category the test +job's own install comment puts ``DPKT`` in rather than the ``Scapy`` one. + +``isort`` remains absent from ``Pipfile``, which is what ``make isort``'s own +``pipenv run`` prefix resolves against -- so the target still depends on a +contributor having isort on ``PATH``. Out of scope here: this module is reached +by pytest, not by ``make``. """ from __future__ import annotations +import importlib.util import pathlib import subprocess import sys @@ -135,6 +164,15 @@ PACKAGE = ROOT / 'pcapkit' MAKEFILE = ROOT / 'Makefile' +#: Whether :mod:`isort` is importable, for the one test that runs it. A visible +#: ``skipUnless`` flag rather than the inline ``skipTest`` this module used +#: until #766, so :func:`~tests._dependency_gates.gated_scopes` counts the gate +#: and :mod:`tests.test_tier_guard` can audit which CI jobs it darkens -- none +#: of them, now that the ``test`` extra carries isort; see this module's own +#: docstring for why that install line was the right answer here and the wrong +#: one for ``mypy``. +HAS_ISORT = importlib.util.find_spec('isort') is not None + def _pcapkit_tree_targets() -> 'list[str]': """``pcapkit $(wildcard temp/sort.py)`` -- the scratch file is untracked @@ -276,24 +314,13 @@ def _isort_recipe_line_numbers() -> 'list[int]': ] +@unittest.skipUnless(HAS_ISORT, 'isort is not installed') class TestIsortIsCleanOnThePackage(unittest.TestCase): """``make isort`` must not be red on a clean checkout, on any of its four lines (#757). """ - @classmethod - def setUpClass(cls) -> None: - try: - import isort # noqa: F401 pylint: disable=unused-import,import-outside-toplevel - except ImportError: - raise unittest.SkipTest( - "isort is not installed -- it is deliberately absent from both " - "Pipfile and pyproject.toml (lint.yml: 'still local-only'), so " - "this test only runs when a contributor has it installed, the " - "same precondition `make isort` itself has" - ) - def test_check_only_is_clean_on_every_makefile_line(self) -> None: """Each line of the Makefile's ``isort:`` target, run the way it runs.""" line_numbers = _isort_recipe_line_numbers() diff --git a/tests/test_tier_guard.py b/tests/test_tier_guard.py index 5896bb8d86..aaa9f8e068 100644 --- a/tests/test_tier_guard.py +++ b/tests/test_tier_guard.py @@ -89,6 +89,35 @@ def doctored_workflow(case: 'unittest.TestCase', old: 'str', new: 'str') -> 'pat return path +def doctored_pyproject(case: 'unittest.TestCase', old: 'str', new: 'str') -> 'pathlib.Path': + """A throwaway copy of :file:`pyproject.toml`, with ``old`` replaced by ``new``. + + :func:`doctored_workflow` for the *other* input, and it needs one thing that + one does not. :func:`~tests._dependency_gates.dependency_gate_gaps` takes a + workflow path as an argument, so doctoring a workflow needs no patching at + all; :file:`pyproject.toml` is reached through the + :data:`~tests._dependency_gates.PYPROJECT` module constant by two + :func:`functools.lru_cache`\\ d readers, so a caller has to patch the + constant *and* clear both caches -- on the way in, or the real reading is + still cached, and on the way out, or the doctored one leaks into every test + that runs afterwards. Registering those cleanups here is the point of having + a helper rather than open-coding it. + + """ + text = _dependency_gates.PYPROJECT.read_text(encoding='utf-8') + case.assertIn(old, text, f'{old!r} is no longer in pyproject.toml') + tmpdir = tempfile.TemporaryDirectory(prefix='pcapkit-doctored-pyproject-') + case.addCleanup(tmpdir.cleanup) + path = pathlib.Path(tmpdir.name) / 'pyproject.toml' + path.write_text(text.replace(old, new, 1), encoding='utf-8') + + case.addCleanup(_dependency_gates.extras_providing.cache_clear) + case.addCleanup(_dependency_gates.declared_requirements.cache_clear) + _dependency_gates.declared_requirements.cache_clear() + _dependency_gates.extras_providing.cache_clear() + return path + + def job_section(text: 'str', name: 'str') -> 'str': """The YAML text of job ``name``, from its header to the next top-level job. @@ -1565,6 +1594,68 @@ def test_the_mypy_gate_is_visible_and_dark_on_every_job_that_reaches_it(self) -> exclusion = _dependency_gates.DEPENDENCY_GATE_EXCLUSIONS['HAS_MYPY'] self.assertEqual(set(exclusion.dark), {'test', 'engine-tests', 'gate'}) + def test_the_isort_gate_is_visible_and_closed_by_the_test_extra(self) -> None: + """#766: the same invisible shape as ``mypy``, resolved the opposite way. + + :file:`tests/project/test_isort_clean.py` gated on isort with a + ``try: import isort`` inside ``setUpClass``, which + :func:`~tests._dependency_gates._gates_of` cannot see -- it walks a + definition's ``decorator_list`` and never a function body -- and isort + was in no :file:`pyproject.toml` extra, so the module reported + ``OK (skipped=1)`` on every leg and the only guard on ``make isort``'s + four recipe lines ran nowhere. + + Where #779 gave ``mypy`` a visible gate and then declined the install + line, this closes the gap instead, so there is deliberately no + :data:`~tests._dependency_gates.DEPENDENCY_GATE_EXCLUSIONS` entry to + assert against. The reason the two rulings differ is specific rather + than stylistic: that exclusion's own argument is that lint.yml already + runs mypy over the whole package, so a pytest leg would be a second + copy -- and lint.yml installs no isort at all, by its own header's + account, so for isort there was no first copy to defer to. + + The reachability assertion below is load-bearing and is what this test + has that its ``mypy`` sibling does not need. That one asserts gaps are + *present*, which cannot pass vacuously; this one asserts they are + *absent*, which would pass just as well if no job collected the module + at all -- so which jobs reach the gate is checked directly rather than + inferred from the absence of a gap. + + """ + gates = [gate for gate in _dependency_gates.gated_scopes() + if gate.flag == 'HAS_ISORT'] + self.assertEqual( + [(gate.module, gate.class_name, gate.func_name) for gate in gates], + [('tests/project/test_isort_clean.py', 'TestIsortIsCleanOnThePackage', None)], + 'the isort gate is invisible to the scan again -- a try/except ImportError ' + 'in setUpClass is exactly the shape _gates_of() cannot see (#766). A ' + 'func_name of None is the class-level decorator, which is what gates the ' + 'whole module rather than one method of it') + + # Mapped *and* carried, which is the difference from mypy: no extra + # declares mypy, so extras_providing() answers with the empty set there. + self.assertEqual(_dependency_gates.extras_providing('isort'), + frozenset({'test'})) + + reaching = {job.name for job in _dependency_gates.pytest_jobs() + if _dependency_gates.job_reaches(job, gates[0])} + self.assertEqual( + reaching, {'test', 'engine-tests', 'gate'}, + 'the two ignore-shape legs and the whole-suite one collect ' + 'tests/project/; integration and pypcap-parity select by fixture tier and ' + 'never reach it, the same reason they are absent from HAS_MYPY above') + + gaps = {(gap.flag, gap.job) for gap in _dependency_gates.dependency_gate_gaps()} + for job in sorted(reaching): + with self.subTest(reaches=job): + self.assertNotIn(('HAS_ISORT', job), gaps) + + self.assertNotIn( + 'HAS_ISORT', _dependency_gates.DEPENDENCY_GATE_EXCLUSIONS, + 'the isort gap is closed by an install line, so an exclusion here would be ' + 'the vestigial kind test_each_exclusion_still_describes_a_gap_that_is_' + 'really_there refuses') + def test_each_exclusion_still_describes_a_gap_that_is_really_there(self) -> None: """The anti-rot half, and the reason this is an allowlist and not a skip list. @@ -1800,6 +1891,52 @@ def test_removing_crypto_from_the_test_job_is_caught(self) -> None: 'the exclusion list would swallow the gap this test relies on' ) + def test_dropping_isort_from_the_test_extra_is_caught(self) -> None: + """#766's own prediction, measured: the gap the install line closes is real. + + Every assertion in + :meth:`DependencyGateCoverageTests.test_the_isort_gate_is_visible_and_closed_by_the_test_extra` + says a gap is *absent*, and an analysis that had quietly stopped + resolving ``HAS_ISORT`` at all would satisfy every one of them. So this + takes the requirement back out on a copy and requires the three gaps to + reappear -- which is also the state #766 predicted a visible gate would + produce before the extra carried isort ("would correctly turn the + dependency-gate guard red until isort is on a pytest install line"), and + the reason the gate and the install line had to land in one change. + + This mutates :file:`pyproject.toml` rather than the workflow because + there is no isort to remove from an install line: every pytest job + already installs ``test``, so the extra's own contents are where the + dependency enters. That is why :file:`unit-tests.yml` needed no edit for + #766 at all. + + """ + doctored = doctored_pyproject(self, '\n "isort",\n', '\n') + with unittest.mock.patch.object(_dependency_gates, 'PYPROJECT', doctored): + self.assertEqual( + _dependency_gates.extras_providing('isort'), frozenset(), + 'the mutation did not take -- some other extra still carries isort') + gaps = {(gap.flag, gap.job): gap + for gap in _dependency_gates.dependency_gate_gaps()} + + self.assertEqual( + sorted(job for (flag, job) in gaps if flag == 'HAS_ISORT'), + ['engine-tests', 'gate', 'test']) + + gap = gaps[('HAS_ISORT', 'test')] + self.assertEqual(gap.missing, ('isort',)) + self.assertEqual({gate.module for gate in gap.gates}, + {'tests/project/test_isort_clean.py'}) + + message = _dependency_gates.describe_gap(gap) + self.assertIn("the 'test' job", message) + self.assertIn('isort', message) + self.assertIn('test_isort_clean.py', message) + + self.assertNotIn( + 'HAS_ISORT', _dependency_gates.DEPENDENCY_GATE_EXCLUSIONS, + 'the exclusion list would swallow the gap this test relies on') + def test_removing_dpkt_is_caught_on_every_job_that_installs_it(self) -> None: """#729's defect, restaged. Five jobs install ``DPKT``; all five go dark.