From e51becd50ec660a6be34735c9be72bdae60713c9 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sat, 19 Sep 2026 11:46:36 -0400 Subject: [PATCH 1/2] test: sort sample_path() calls into real source order in the tier guard `sample_path_calls()` documented "in source order" but collected via `ast.walk`, which is breadth-first: a call nested inside a method near the top of a file is yielded *after* one at module level near the bottom. Measured on a four-call module, an unsorted walk reports lines 14, 11, 7, 7 -- the file read backwards. - sort by `(lineno, col_offset)`, so the docstring's promise is kept by the code rather than by luck; `col_offset` breaks ties for two calls on one line - add a regression test that builds a module whose walk order and source order differ, so the sort cannot silently regress - route the last hand-built capture path in `test_pcapng_unit.py` through `sample_path()`, and rewrite the `_tiers` module docstring that cited it as the outstanding example `audit_module()` emits one finding per call in this order, so breadth-first order made the same set of violations list differently between runs and a reader could not tell a reordering from a new finding. tests/test_tier_guard.py 25 passed / 19 subtests; tests/protocols/misc/test_pcapng_unit.py 37 passed / 139 subtests. --- tests/_tiers.py | 60 +++++++++++++++++++----- tests/protocols/misc/test_pcapng_unit.py | 17 +++++-- tests/test_tier_guard.py | 54 ++++++++++++++++++++- 3 files changed, 114 insertions(+), 17 deletions(-) diff --git a/tests/_tiers.py b/tests/_tiers.py index f55b253fb2..e32155c88c 100644 --- a/tests/_tiers.py +++ b/tests/_tiers.py @@ -55,15 +55,23 @@ 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 module in the suite 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 -- but nothing stops the next +one, 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``" raised +about fifteen false positives across the suite (``'out.pcap'``, +``'input.pcap'``, ``'capture.pcap'``, temporary paths written by tests that read +no fixture at all), 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 +422,44 @@ 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`` breaks the tie for two + calls on one line, e.g. ``(sample_path('a.pcap'), sample_path('b.pcap'))``. + + """ 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..1e24e90bdd 100644 --- a/tests/protocols/misc/test_pcapng_unit.py +++ b/tests/protocols/misc/test_pcapng_unit.py @@ -3019,12 +3019,23 @@ 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 -- it degrades to a skip + # on a fresh clone instead of failing -- and it is what + # GeneratedFixtureInUnitTierError subclassing FileNotFoundError is for. + # It also drops a dependency on pytest's working directory, which the + # relative path this replaces quietly had. 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..aa4f3b2c84 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 of them are listed in the order they 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,54 @@ 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. :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 a breadth-first collection reports them + 14, 11, 7, 7 -- exactly backwards -- and this test fails. It passes only + once the calls are really sorted by position, which is what + :func:`~tests._tiers.sample_path_calls` documents. + + The two calls sharing line 7 pin the ``col_offset`` tie-break: they come + out left to right rather than in whatever order the walk happened to + reach them. + + """ + 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'), sample_path('http6.cap') + return helper() + + def test_from_a_method(self): + return sample_path('http.cap') + + + TOP_LEVEL = sample_path('dhcp_big_endian.pcapng') + """) + + calls = _tiers.sample_path_calls(str(module)) + self.assertEqual( + [(call.lineno, call.name) for call in calls], + [(7, 'test.pcap'), (7, 'http6.cap'), (11, 'http.cap'), + (14, 'dhcp_big_endian.pcapng')], + ) + + # audit_module emits one finding per call and inherits this order, which + # is what the reader of a failed collection actually sees. + findings = _tiers.audit_module(module) + located = [re.search(r':(\d+) is a unit-tier', finding) for finding in findings] + self.assertTrue(all(match is not None for match in located), findings) + self.assertEqual([int(match.group(1)) for match in located if match is not None], + [7, 7, 11, 14]) + 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', """ From e6416a3a71fd64bbe258b80a7a6f924888fc43fb Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sat, 19 Sep 2026 16:14:03 -0400 Subject: [PATCH 2/2] test: pin both halves of the tier-guard sort key, and correct three prose claims Addresses review feedback on #510. - tests/test_tier_guard.py: the synthetic module's line-7 pair was a tuple, and ast.Tuple keeps its elements in one field list, so those calls already walk left to right -- col_offset was pinned by nothing and the suite stayed green with it dropped. Replaced with a conditional expression: ast.IfExp stores test, body, orelse while source writes body if test else orelse, so the walk reaches them reversed and only col_offset puts them back. The findings assertion now matches capture names as well as line numbers, since the two line-7 calls are indistinguishable by lineno. - tests/_tiers.py: document which AST shapes need col_offset (ast.Dict, whose keys are all walked before any values, and ast.IfExp) and which do not (ast.Tuple). Re-measure the false-positive figure with its unit: 16 distinct literal values in unit-tier modules, of which the only two that are sample_path arguments are already legal. - tests/protocols/misc/test_pcapng_unit.py: correct which exception fires when a fixture is absent, and record that the relative path this replaced never resolved outside the repository root -- a silent skip on a tree that had every fixture. Both halves of the sort key now fail independently when reverted. --- tests/_tiers.py | 56 +++++++++++++++----- tests/protocols/misc/test_pcapng_unit.py | 19 +++++-- tests/test_tier_guard.py | 66 +++++++++++++++--------- 3 files changed, 98 insertions(+), 43 deletions(-) diff --git a/tests/_tiers.py b/tests/_tiers.py index e32155c88c..982c0912de 100644 --- a/tests/_tiers.py +++ b/tests/_tiers.py @@ -56,22 +56,36 @@ What neither catches, deliberately: a unit-tier module that opens a capture without going through :func:`~tests._support.sample_path` at all, e.g. by joining -:file:`examples/captures/` and a name itself. No module in the suite does this +: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 -- but nothing stops the next -one, and the shape stays invisible here. +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``" raised -about fifteen false positives across the suite (``'out.pcap'``, -``'input.pcap'``, ``'capture.pcap'``, temporary paths written by tests that read -no fixture at all), 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. +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 @@ -443,8 +457,22 @@ def sample_path_calls(module_path: 'str') -> 'tuple[SampleCall, ...]': 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`` breaks the tie for two - calls on one line, e.g. ``(sample_path('a.pcap'), sample_path('b.pcap'))``. + :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) diff --git a/tests/protocols/misc/test_pcapng_unit.py b/tests/protocols/misc/test_pcapng_unit.py index 1e24e90bdd..849381a6fa 100644 --- a/tests/protocols/misc/test_pcapng_unit.py +++ b/tests/protocols/misc/test_pcapng_unit.py @@ -3024,11 +3024,22 @@ def test_pcapng_big_endian_sample_section_options_are_intact(self) -> None: # 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 -- it degrades to a skip - # on a fresh clone instead of failing -- and it is what - # GeneratedFixtureInUnitTierError subclassing FileNotFoundError is for. + # 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 path this replaces quietly had. + # 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 diff --git a/tests/test_tier_guard.py b/tests/test_tier_guard.py index aa4f3b2c84..3a426fb674 100644 --- a/tests/test_tier_guard.py +++ b/tests/test_tier_guard.py @@ -14,8 +14,8 @@ * 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, the legitimate reads next to it are not, - and several of them are listed in the order they appear in the file rather - than in the order a tree walk happened to reach them + 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`). @@ -294,17 +294,30 @@ def test_arithmetic(): 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. :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 a breadth-first collection reports them - 14, 11, 7, 7 -- exactly backwards -- and this test fails. It passes only - once the calls are really sorted by position, which is what - :func:`~tests._tiers.sample_path_calls` documents. - - The two calls sharing line 7 pin the ``col_offset`` tie-break: they come - out left to right rather than in whatever order the walk happened to - reach them. + 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', """ @@ -314,7 +327,7 @@ def test_calls_and_findings_come_out_in_source_order(self) -> None: class Tests: def test_from_a_nested_function(self): def helper(): - return sample_path('test.pcap'), sample_path('http6.cap') + return sample_path('test.pcap') if sample_path('http6.cap') else None return helper() def test_from_a_method(self): @@ -324,20 +337,23 @@ def test_from_a_method(self): 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], - [(7, 'test.pcap'), (7, 'http6.cap'), (11, 'http.cap'), - (14, 'dhcp_big_endian.pcapng')], - ) - - # audit_module emits one finding per call and inherits this order, which - # is what the reader of a failed collection actually sees. + 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) - located = [re.search(r':(\d+) is a unit-tier', finding) for finding in findings] + 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)) for match in located if match is not None], - [7, 7, 11, 14]) + 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."""