test: sort sample_path() calls into real source order in the tier guard

tests/_tiers.py

@@ -55,15 +55,37 @@
 parametrised loop, but only once the call is reached.
 
 What neither catches, deliberately: a unit-tier module that opens a capture
-without going through :func:`~tests._support.sample_path` at all.
-:file:`tests/protocols/misc/test_pcapng_unit.py` does this today at the line
-holding ``os.path.join('examples', 'captures', 'dhcp_big_endian.pcapng')``, and
-it is tier-safe -- it checks :func:`os.path.isfile` and skips -- but it is
-invisible here. Flagging that shape would mean recognising a second,
-much woollier "the absence is handled" idiom on top of the ``try``/``except``
-one, and getting it wrong would fail correct code for everybody. So the rule
-this module enforces is stated as it is: capture reads go through
-:func:`~tests._support.sample_path`, and that is the door with the lock on it.
+without going through :func:`~tests._support.sample_path` at all, e.g. by joining
+:file:`examples/captures/` and a name itself. No *unit-tier* module does this
+today -- :file:`tests/protocols/misc/test_pcapng_unit.py` was the last one and
+now reads ``sample_path('dhcp_big_endian.pcapng')`` inside the ``try``/``except
+FileNotFoundError`` idiom :func:`explain` recommends. Only the *unit* tier is
+claimed there, deliberately: fixture-tier modules join the directory freely and
+are entitled to, e.g. :file:`tests/protocols/test_option_coverage_runtime.py`,
+which imports :data:`SAMPLE_ROOT` from here and joins names onto it -- legal,
+because that tier runs only once the fixtures exist. The one unit-tier module that
+touches :data:`SAMPLE_ROOT` at all, :file:`tests/test_tier_guard.py`, only stats
+:file:`in.pcap`, which is committed. But nothing stops the next unit-tier module
+from opening a *generated* capture that way, and the shape stays invisible here.
+
+Flagging it in general was considered and rejected, with a measurement behind the
+decision. A rule as broad as "any string literal ending in ``.pcap``" matches 18
+distinct literal values in the unit-tier modules under :file:`tests/`, across 85
+occurrences of them -- 199 occurrences once the fixture tier is counted too. Only
+two of the 18 are a :func:`~tests._support.sample_path` argument at all, and both
+are already legal: ``'in.pcap'`` is committed, and ``'test.pcap'`` is read inside
+the sanctioned ``try``/``except``. The other 16 name no capture at all --
+``'out.pcap'``, ``'input.pcap'``, ``'capture.pcap'``, scratch paths written by
+tests that read no fixture, and ``'pcapkit.foundation.engines.pcap'``, which is a
+dotted module name rather than a path. So the figure that matters is 16 false
+positives, counted as distinct literal values in the unit tier: the broad rule
+would invent every one of them and catch nothing this rule misses, and a false
+positive here aborts collection for everybody rather than failing one test.
+Narrowing it would mean recognising a second, much woollier "the absence is
+handled" idiom -- an :func:`os.path.isfile` check and a skip -- on top of the
+``try``/``except`` one. So the rule this module enforces is stated as it is:
+capture reads go through :func:`~tests._support.sample_path`, and that is the door
+with the lock on it.
 
 Nothing here imports :mod:`tests._support` -- the dependency runs the other way,
 and adding the reverse edge would make it a cycle. Nothing here imports
@@ -414,16 +436,58 @@ def _is_sample_path(func: 'ast.expr') -> 'bool':
 
 @functools.lru_cache(maxsize=None)
 def sample_path_calls(module_path: 'str') -> 'tuple[SampleCall, ...]':
-    """Every ``sample_path(...)`` call in ``module_path``, in source order."""
+    """Every ``sample_path(...)`` call in ``module_path``, in source order.
+
+    Source order -- ascending ``(lineno, col_offset)`` -- is a promise that has
+    to be kept by sorting, because :func:`ast.walk` does not give it. It walks
+    breadth-first, so it yields a call written near the top of the file but nested
+    inside a method *after* one written at the bottom at module level. Measured on
+    the four-call module that
+    ``test_calls_and_findings_come_out_in_source_order`` in
+    :file:`tests/test_tier_guard.py` builds for the purpose: an unsorted walk
+    reported its calls as lines 14, 11, 7, 7, i.e. the file read backwards.
+
+    That matters because :func:`audit_module` emits one finding per call in this
+    order, and this guard's entire value is a diagnostic the reader can act on.
+    Breadth-first order is not the order anything appears in the file, and it
+    shifts when unrelated code moves between nesting levels -- so the same set of
+    violations gets listed differently from one edit to the next, and a reader
+    comparing two runs cannot tell a reordering from a new finding.
+
+    Sorted rather than collected by a lexical-order visitor, because the sort is
+    the whole fix in one expression and needs nothing kept in step with it: a
+    second traversal written by hand is another thing that can disagree with
+    :func:`ast.walk` about where calls live.
+
+    ``col_offset`` is the other half of the key, and which AST shapes actually need
+    it is the reusable fact. A tuple does not: :class:`ast.Tuple` holds its
+    elements in one field list, so ``(sample_path('a.pcap'),
+    sample_path('b.pcap'))`` already comes out of the walk left to right and
+    ``lineno`` alone would do. Two shapes do need it, because they spread one
+    line's expressions across separate fields that the walk visits in an order
+    source does not use: :class:`ast.Dict`, whose ``keys`` are all visited before
+    any of its ``values``, and :class:`ast.IfExp`, which stores
+    ``test, body, orelse`` but is written ``body if test else orelse``. So
+    ``sample_path('a.pcap') if sample_path('b.pcap') else None`` is walked b then
+    a, and sorting on ``lineno`` alone keeps it that way -- the sort is stable, so
+    a tie is left in walk order. That is the shape
+    ``test_calls_and_findings_come_out_in_source_order`` puts on one line, which is
+    what makes dropping ``col_offset`` turn that test red.
+
+    """
     tree = _parse(module_path)
     if tree is None:
         return ()
 
     handled = handled_lines(module_path)
+    calls = sorted(
+        (node for node in ast.walk(tree)
+         if isinstance(node, ast.Call) and _is_sample_path(node.func)),
+        key=lambda node: (node.lineno, node.col_offset),
+    )
     return tuple(
         SampleCall(node.lineno, _literal_name(node), node.lineno in handled)
-        for node in ast.walk(tree)
-        if isinstance(node, ast.Call) and _is_sample_path(node.func)
+        for node in calls
     )
 
 

tests/protocols/misc/test_pcapng_unit.py

@@ -3019,12 +3019,34 @@ def test_pcapng_section_header_options_ignore_the_previous_section(self) -> None
     def test_pcapng_big_endian_sample_section_options_are_intact(self) -> None:
         # The same defect, on the sample capture #368 reports it against. Skipped
         # unless the samples have been generated, since they are not tracked.
+        #
+        # Through `sample_path` rather than joining the path here: that is the one
+        # door onto examples/captures/, so it is the only read the tier guard in
+        # tests/_tiers.py can see, and a hand-built path would sit outside both
+        # halves of it. The try/except is the guard's sanctioned idiom for a
+        # unit-tier test that wants a generated capture, and it is the handled-line
+        # opt-out that does the work: check_unit_tier_read() returns None for any
+        # line inside a try whose handler catches a missing file, so this read is
+        # excused and degrades to a skip on a fresh clone instead of failing. What
+        # fires when the capture really is absent is therefore the plain
+        # FileNotFoundError sample_path raises, not GeneratedFixtureInUnitTierError
+        # -- that subclass is for an *unhandled* call reached from inside a try
+        # further up the stack, where subclassing FileNotFoundError is what keeps
+        # the outer handler working.
+        #
+        # It also drops a dependency on pytest's working directory, which the
+        # relative os.path.join('examples', ...) this replaces quietly had. Run
+        # from anywhere but the repository root that path never resolved, so this
+        # test skipped with "run make_samples.py first" on a tree that in fact had
+        # every fixture -- a silent hole rather than a failure. Measured from /tmp:
+        # this version passes where the previous one skipped.
         from pcapkit.const.pcapng.option_type import OptionType
         from pcapkit.interface import extract
 
-        path = os.path.join('examples', 'captures', 'dhcp_big_endian.pcapng')
-        if not os.path.isfile(path):
-            self.skipTest('run examples/generators/make_samples.py first')
+        try:
+            path = sample_path('dhcp_big_endian.pcapng')
+        except FileNotFoundError as exc:
+            self.skipTest(str(exc))
 
         extractor = extract(fin=path, store=True, nofile=True)
         section = extractor.engine._ctx_list[0].section

tests/test_tier_guard.py

@@ -13,8 +13,10 @@
   (:class:`TierClassificationTests`, :class:`WorkflowAgreementTests`);
 * committedness comes from git rather than from a list of names that goes stale
   the moment a seventh capture is committed (:class:`CommittedCaptureTests`);
-* a violation is caught and explained, and the legitimate reads next to it are
-  not (:class:`AuditTests`, :class:`RuntimeCheckTests`);
+* a violation is caught and explained, the legitimate reads next to it are not,
+  and several violations in one module are listed in the order those violations
+  appear in the file rather than in the order a tree walk happened to reach them
+  (:class:`AuditTests`, :class:`RuntimeCheckTests`);
 * the suite as it stands is clean (:class:`SuiteIsCleanTests`), and a checkout
   without git degrades instead of failing (:class:`DegradationTests`).
 
@@ -289,6 +291,70 @@ def test_arithmetic():
         self.assertEqual(_tiers.audit_module(module), [])
         self.assertEqual(_tiers.sample_path_calls(str(module)), ())
 
+    def test_calls_and_findings_come_out_in_source_order(self) -> None:
+        """Four violations at three nesting depths, reported top to bottom.
+
+        The arrangement is the whole test, and the module below is wrong for the
+        walk in two independent ways, because each half of the ``(lineno,
+        col_offset)`` sort key needs a case that fails without it.
+
+        Across lines, ``lineno``: :func:`ast.walk` is breadth-first, so it reaches
+        the *shallowest* call first however late in the file it is written. Here
+        the earliest call is the most deeply nested one and the latest is at module
+        level, so an unsorted collection reports lines 14, 11, 7, 7 -- the file
+        read backwards.
+
+        Within line 7, ``col_offset``: the two calls there are the ``body`` and the
+        ``test`` of a conditional expression, and :class:`ast.IfExp` stores its
+        fields ``test, body, orelse`` while source writes them ``body, test,
+        orelse``. The walk therefore reaches ``'http6.cap'`` before
+        ``'test.pcap'``, which is written to its left, and sorting on ``lineno``
+        alone preserves that -- a stable sort leaves a tie in walk order. Only
+        ``col_offset`` puts the pair back.
+
+        A tuple of two calls, which is what this module used to hold here, pins
+        neither half: :class:`ast.Tuple` keeps its elements in one field list, so
+        the walk yields them left to right already and the test passed with
+        ``col_offset`` dropped. :class:`ast.Dict` is the other shape that gets a
+        single line wrong, since all of its ``keys`` are walked before any of its
+        ``values``.
+
+        """
+        module = write_module(self.tmp_path, 'test_order_unit.py', """
+            from tests._support import sample_path
+
+
+            class Tests:
+                def test_from_a_nested_function(self):
+                    def helper():
+                        return sample_path('test.pcap') if sample_path('http6.cap') else None
+                    return helper()
+
+                def test_from_a_method(self):
+                    return sample_path('http.cap')
+
+
+            TOP_LEVEL = sample_path('dhcp_big_endian.pcapng')
+            """)
+
+        expected = [(7, 'test.pcap'), (7, 'http6.cap'), (11, 'http.cap'),
+                    (14, 'dhcp_big_endian.pcapng')]
+
+        calls = _tiers.sample_path_calls(str(module))
+        self.assertEqual([(call.lineno, call.name) for call in calls], expected)
+
+        # audit_module emits one finding per call and inherits this order, which is
+        # what the reader of a failed collection actually sees. Matched on the
+        # capture name as well as the line, because the two calls on line 7 share a
+        # line and the name is the only thing that distinguishes them -- so this is
+        # the assertion that catches a lost col_offset in the diagnostic itself.
+        findings = _tiers.audit_module(module)
+        pattern = re.compile(r":(\d+) is a unit-tier test module and reads '([^']+)'")
+        located = [pattern.search(finding) for finding in findings]
+        self.assertTrue(all(match is not None for match in located), findings)
+        self.assertEqual([(int(match.group(1)), match.group(2))
+                          for match in located if match is not None], expected)
+
     def test_an_unparseable_module_is_not_this_guards_problem(self) -> None:
         """A syntax error is reported by pytest, far better than from here."""
         module = write_module(self.tmp_path, 'test_broken_unit.py', """