test: gate test_isort_clean.py visibly and put isort on the test extra

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

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 '

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()

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.