diff --git a/tests/_tiers.py b/tests/_tiers.py index f55b253fb2..982c0912de 100644 --- a/tests/_tiers.py +++ b/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 ) diff --git a/tests/protocols/misc/test_pcapng_unit.py b/tests/protocols/misc/test_pcapng_unit.py index d43a3f476a..849381a6fa 100644 --- a/tests/protocols/misc/test_pcapng_unit.py +++ b/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 diff --git a/tests/test_tier_guard.py b/tests/test_tier_guard.py index 013b64ebb8..3a426fb674 100644 --- a/tests/test_tier_guard.py +++ b/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', """