Skip to content

Commit cb573a1

Browse files
committed
test: widen the tier guard to catch a SAMPLE_ROOT join via a module-level tuple
The AST audit in tests/_tiers.py only recognised a generated-capture read as a literal argument to tests._support.sample_path(...). It missed the indirection PR #2421's first revision hit: a module-level CAPTURES tuple read in a for loop whose body joined SAMPLE_ROOT / name directly, bypassing sample_path(). The guard's own static pass never flagged it, which is the defect this fixes -- CI's unit-tests leg did fail on that revision, on every affected Python interpreter, because the capture genuinely was not on disk; it only passed locally where make_samples.py had already been run. - tests/_tiers.py: add sample_root_join_calls(), which follows a SAMPLE_ROOT join back to a module-level name bound to a single string, or to a for loop's target over a module-level (or inline) tuple/list of strings. audit_module() now merges these findings with the existing sample_path_calls() ones, in source order. Deliberately narrow, per the module docstring's existing rationale for rejecting a blanket ".pcap literal" rule: no new false positive across the real suite (verified). - tests/test_tier_guard.py: new AuditTests cases pin the loop and direct-join shapes being flagged, and that a committed capture, a try/except-handled read, and a for loop over sorted(CAPTURES) (a module-level tuple wrapped in a call, rather than read bare) stay unflagged. Verified: reverting the tests/_tiers.py change makes the three new "is flagged" tests fail (0 findings where 1-2 are expected, plus an AttributeError for the new function); restoring it makes them pass. Full tests/test_tier_guard.py + test_tier_guard_xdist.py + test_capture_tracking.py suite green (156 passed, 3 skipped), and a whole-suite audit_module() sweep over every unit-tier module on disk still reports zero findings. mypy and pylint report no new findings against either file. Closes #2426
1 parent 867616b commit cb573a1

2 files changed

Lines changed: 329 additions & 21 deletions

File tree

‎tests/_tiers.py‎

Lines changed: 208 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -81,25 +81,21 @@
8181
check sees the name however it was computed, e.g. ``sample_path(sample)`` in a
8282
parametrised loop, but only once the call is reached.
8383
84-
What neither catches, deliberately: a unit-tier module that opens a capture without
85-
going through :func:`~tests._support.sample_path` at all, e.g. by joining
86-
:file:`examples/captures/` and a name itself. No *unit-tier* module does this:
87-
:file:`tests/protocols/misc/test_pcapng_unit.py`, for one, reads
88-
``sample_path('dhcp_big_endian.pcapng')`` inside the
89-
``try``/``except FileNotFoundError`` idiom :func:`explain` recommends. Only the
90-
*unit* tier is claimed there, deliberately: fixture-tier modules join the directory
91-
freely and are entitled to, e.g.
84+
What :func:`audit_module` does not catch *in general*: a unit-tier module that opens
85+
a capture without going through :func:`~tests._support.sample_path` at all, e.g. by
86+
joining :file:`examples/captures/` and a name itself. Fixture-tier modules join the
87+
directory freely and are entitled to, e.g.
9288
:file:`tests/protocols/test_option_coverage_runtime.py`, which imports
9389
:data:`SAMPLE_ROOT` from here and joins names onto it -- legal, because that tier
94-
runs only once the fixtures exist. The one unit-tier module that touches
95-
:data:`SAMPLE_ROOT` at all, :file:`tests/test_tier_guard.py`, stats :file:`in.pcap`
96-
directly -- which is committed -- and separately loops over every name
97-
:func:`committed_captures` reports, each committed by construction, since that is
98-
git's own tracked listing rather than a name the test made up. But nothing stops the
99-
next unit-tier module from opening a *generated* capture that way, and the shape
100-
stays invisible here.
101-
102-
Flagging it in general was considered and rejected, with a measurement behind the
90+
runs only once the fixtures exist. Two unit-tier modules touch :data:`SAMPLE_ROOT`
91+
directly, outside the one specific indirection below, and both are safe by
92+
construction rather than by escaping notice: :file:`tests/test_tier_guard.py`
93+
stats :file:`in.pcap` directly -- which is committed -- and separately loops over
94+
every name :func:`committed_captures` reports; :file:`tests/project/
95+
test_capture_tracking.py` loops over git's own tracked listing the same way. Both
96+
read only names git already tracks, never a generated one.
97+
98+
Flagging *every* such join was considered and rejected, with a measurement behind the
10399
decision. A rule as broad as "any string literal ending in ``.pcap``" matches 18
104100
distinct literal values in the unit-tier modules under :file:`tests/`, across 85
105101
occurrences of them -- 199 occurrences once the fixture tier is counted too. Only
@@ -118,6 +114,23 @@
118114
capture reads go through :func:`~tests._support.sample_path`, and that is the door
119115
with the lock on it.
120116
117+
One indirection through that door *is* caught, narrowly (#2426):
118+
:func:`sample_root_join_calls` follows a module-level name -- a bare string, or a
119+
tuple/list of them -- that is later joined straight onto :data:`SAMPLE_ROOT`,
120+
bypassing :func:`~tests._support.sample_path` altogether. This is exactly the shape
121+
:file:`tests/foundation/reassembly/test_datagram_deferred_pickle_2417_unit.py` used,
122+
with a module-level ``CAPTURES`` tuple read in a ``for`` loop whose body joined
123+
``SAMPLE_ROOT / name`` -- invisible to :func:`sample_path_calls`, since no
124+
``sample_path`` call is made at all. CI's unit-tests leg genuinely failed on that
125+
module, on every affected interpreter, for exactly the reason this guard exists to
126+
catch early: the capture was not on disk. What missed it was this *guard*, which
127+
ran clean over a tree that in fact carried the violation -- a static-analysis gap
128+
at review time, not a passing CI run. It stays narrow on purpose, for the reason
129+
the paragraph above gives for rejecting the broad literal rule: it fires only on
130+
``SAMPLE_ROOT / NAME`` for a ``NAME`` this module can resolve to a module-level
131+
string, tuple, or list of strings, never on the general "ends in .pcap" literal
132+
that rejected rule would have invented false positives from.
133+
121134
The curated ``ALL`` tier
122135
------------------------
123136
@@ -192,7 +205,7 @@
192205
'PROJECT_PINS_DIR', 'PROJECT_PINS_SELECTOR', 'PROJECT_PIN_PACKAGE', 'LOADABLE_DIRS',
193206
'varies_by_interpreter', 'is_project_pin', 'project_pin_paths',
194207
'is_unit_tier', 'committed_captures', 'committed_capture_names',
195-
'guard_unavailable_reason', 'handled_lines', 'sample_path_calls',
208+
'guard_unavailable_reason', 'handled_lines', 'sample_path_calls', 'sample_root_join_calls',
196209
'audit_module', 'check_unit_tier_read', 'explain',
197210
'skip_idiom_modules', 'skip_idiom_test_ids', 'fixture_tier_paths',
198211
]
@@ -1279,8 +1292,11 @@ def _parse(module_path: 'str') -> 'Optional[ast.Module]':
12791292
except (OSError, UnicodeDecodeError):
12801293
return None
12811294
# Cheap gate before the parse: nearly every module in the suite never
1282-
# mentions sample_path, and parsing them all would be work for nothing.
1283-
if 'sample_path' not in source:
1295+
# mentions sample_path or SAMPLE_ROOT, and parsing them all would be work
1296+
# for nothing. The second substring is what lets this same parse feed
1297+
# sample_root_join_calls(), which looks for SAMPLE_ROOT joins rather than
1298+
# sample_path() calls.
1299+
if 'sample_path' not in source and 'SAMPLE_ROOT' not in source:
12841300
return None
12851301
try:
12861302
return ast.parse(source, filename=module_path)
@@ -1402,6 +1418,171 @@ def sample_path_calls(module_path: 'str') -> 'tuple[SampleCall, ...]':
14021418
)
14031419

14041420

1421+
def _is_sample_root(node: 'ast.expr') -> 'bool':
1422+
"""Whether ``node`` names :data:`SAMPLE_ROOT`, bare or qualified.
1423+
1424+
Matched on the bare name alone, as :func:`_is_sample_path` matches
1425+
``sample_path`` -- a false positive here only means a path join is checked
1426+
against git that did not need to be.
1427+
1428+
"""
1429+
if isinstance(node, ast.Name):
1430+
return node.id == 'SAMPLE_ROOT'
1431+
if isinstance(node, ast.Attribute):
1432+
return node.attr == 'SAMPLE_ROOT'
1433+
return False
1434+
1435+
1436+
def _string_constants(elements: 'list[ast.expr]') -> 'Optional[tuple[str, ...]]':
1437+
"""The literal strings ``elements`` holds, or :data:`None` if any of them is not one.
1438+
1439+
A plain loop rather than a generator expression guarded by :func:`all`: mypy
1440+
narrows ``element`` to :class:`ast.Constant` inside this function's own body,
1441+
but not across the boundary of a second generator expression built from the
1442+
same ``all(...)`` check, which is what the two callers used to do.
1443+
1444+
"""
1445+
values = [] # type: list[str]
1446+
for element in elements:
1447+
if not (isinstance(element, ast.Constant) and isinstance(element.value, str)):
1448+
return None
1449+
values.append(element.value)
1450+
return tuple(values)
1451+
1452+
1453+
@functools.lru_cache(maxsize=None)
1454+
def _module_level_string_tuples(tree: 'ast.Module') -> 'dict[str, tuple[str, ...]]':
1455+
"""Module-level ``NAME = '...'`` / ``NAME = ('...', ...)`` / ``[...]`` bindings, by name.
1456+
1457+
Scoped to a single assignment target naming a bare module-level variable, whose
1458+
value is a string constant, or a tuple or list every element of which is one --
1459+
the shapes ``CAPTURE = 'a.pcap'`` and ``CAPTURES = ('a.pcap', 'b.pcap')`` take.
1460+
A later assignment to the same name overwrites the earlier one, matching normal
1461+
Python shadowing, and a value this cannot describe (a call, a comprehension, an
1462+
f-string) drops the name instead of guessing at it.
1463+
1464+
This is the one indirection :func:`sample_root_join_calls` follows -- see its
1465+
docstring and the module docstring's note on #2426 for why it stops here rather
1466+
than resolving an arbitrary expression.
1467+
1468+
"""
1469+
bindings = {} # type: dict[str, tuple[str, ...]]
1470+
for node in tree.body:
1471+
if not isinstance(node, ast.Assign) or len(node.targets) != 1:
1472+
continue
1473+
target = node.targets[0]
1474+
if not isinstance(target, ast.Name):
1475+
continue
1476+
value = node.value
1477+
if isinstance(value, ast.Constant) and isinstance(value.value, str):
1478+
bindings[target.id] = (value.value,)
1479+
continue
1480+
strings = _string_constants(value.elts) if isinstance(value, (ast.Tuple, ast.List)) else None
1481+
if strings:
1482+
bindings[target.id] = strings
1483+
else:
1484+
bindings.pop(target.id, None)
1485+
return bindings
1486+
1487+
1488+
def _iterable_literal_strings(node: 'ast.expr',
1489+
bindings: 'dict[str, tuple[str, ...]]') -> 'Optional[tuple[str, ...]]':
1490+
"""The literal capture names a ``for ... in node:`` loop walks, if known.
1491+
1492+
``node`` is either a bare module-level name resolved through ``bindings``
1493+
(:func:`_module_level_string_tuples`), or a tuple/list literal written on the
1494+
``for`` line itself. Anything else -- a call such as ``sorted(CAPTURES)``, a
1495+
subscript, a comprehension -- is left as :data:`None`: this is a static,
1496+
best-effort pass, not a general evaluator.
1497+
1498+
"""
1499+
if isinstance(node, ast.Name):
1500+
return bindings.get(node.id)
1501+
if isinstance(node, (ast.Tuple, ast.List)):
1502+
return _string_constants(node.elts) or None
1503+
return None
1504+
1505+
1506+
def _sample_root_join_literals(tree: 'ast.Module') -> 'list[tuple[int, str]]':
1507+
"""``(lineno, name)`` for every capture reached by joining :data:`SAMPLE_ROOT`
1508+
onto a module-level name, in source order.
1509+
1510+
Two shapes, both requiring the join to be ``SAMPLE_ROOT / NAME`` (or
1511+
``_tiers.SAMPLE_ROOT / NAME``) literally -- a nested join such as
1512+
``SAMPLE_ROOT / subdir / name`` is not resolved, since its left operand is
1513+
itself a :class:`ast.BinOp`, not :data:`SAMPLE_ROOT`:
1514+
1515+
* ``NAME`` is bound once, at module level, to a single string constant; or
1516+
* ``NAME`` is a ``for`` loop's target, and the loop's iterable is a
1517+
module-level name bound to a tuple/list of string constants, or such a
1518+
tuple/list written inline on the ``for`` line.
1519+
1520+
"""
1521+
bindings = _module_level_string_tuples(tree)
1522+
found = [] # type: list[tuple[int, int, str]]
1523+
1524+
for node in ast.walk(tree):
1525+
if not (isinstance(node, ast.BinOp) and isinstance(node.op, ast.Div)
1526+
and _is_sample_root(node.left)):
1527+
continue
1528+
right = node.right
1529+
if isinstance(right, ast.Name) and len(bindings.get(right.id, ())) == 1:
1530+
found.append((node.lineno, node.col_offset, bindings[right.id][0]))
1531+
1532+
for for_node in ast.walk(tree):
1533+
if not isinstance(for_node, ast.For):
1534+
continue
1535+
target = for_node.target
1536+
if not isinstance(target, ast.Name):
1537+
continue
1538+
names = _iterable_literal_strings(for_node.iter, bindings)
1539+
if not names:
1540+
continue
1541+
for node in ast.walk(for_node):
1542+
if (isinstance(node, ast.BinOp) and isinstance(node.op, ast.Div)
1543+
and _is_sample_root(node.left)
1544+
and isinstance(node.right, ast.Name) and node.right.id == target.id):
1545+
for name in names:
1546+
found.append((node.lineno, node.col_offset, name))
1547+
1548+
found.sort(key=lambda item: (item[0], item[1]))
1549+
return [(lineno, name) for lineno, _, name in found]
1550+
1551+
1552+
@functools.lru_cache(maxsize=None)
1553+
def sample_root_join_calls(module_path: 'str') -> 'tuple[SampleCall, ...]':
1554+
"""Every capture name reached by joining :data:`SAMPLE_ROOT` onto a name bound
1555+
at module level, in source order (#2426).
1556+
1557+
This is the indirection the module docstring's note on #2426 describes:
1558+
:func:`sample_path_calls` only sees a literal argument to
1559+
:func:`~tests._support.sample_path` itself, so a module that skips that
1560+
function and joins :data:`SAMPLE_ROOT` directly onto a module-level name --
1561+
straight, or through a ``for`` loop over a module-level tuple/list -- was
1562+
invisible to :func:`audit_module`. :file:`tests/foundation/reassembly/
1563+
test_datagram_deferred_pickle_2417_unit.py` did exactly that with a
1564+
module-level ``CAPTURES`` tuple read in a ``for`` loop, and this guard's own
1565+
static pass missed it -- CI's unit-tests leg still failed on every affected
1566+
interpreter, since the capture genuinely was not on disk there; the guard
1567+
simply never flagged the violation to catch it before review.
1568+
1569+
Reuses :data:`SampleCall` rather than a new type: the two are positioned the
1570+
same way relative to a ``try``/``except FileNotFoundError`` -- see
1571+
:func:`handled_lines` -- and :func:`audit_module` reports both through the
1572+
same :func:`explain`.
1573+
1574+
"""
1575+
tree = _parse(module_path)
1576+
if tree is None:
1577+
return ()
1578+
1579+
handled = handled_lines(module_path)
1580+
return tuple(
1581+
SampleCall(lineno, name, lineno in handled)
1582+
for lineno, name in _sample_root_join_literals(tree)
1583+
)
1584+
1585+
14051586
def _display_path(module_path: 'pathlib.Path | str') -> 'str':
14061587
"""``module_path`` relative to the repository root, when it is inside it."""
14071588
path = pathlib.Path(module_path)
@@ -1480,6 +1661,8 @@ def audit_module(module_path: 'pathlib.Path | str') -> 'list[str]':
14801661
Only calls whose capture name is a string literal can be checked here;
14811662
``sample_path(sample)`` in a parametrised loop is invisible to a static
14821663
pass, and :func:`check_unit_tier_read` catches those at call time instead.
1664+
:func:`sample_root_join_calls` adds the one indirection that bypasses
1665+
``sample_path`` altogether (#2426); the two are merged here, in source order.
14831666
14841667
The caller decides tier membership -- this function audits whatever it is
14851668
handed, so a module can be audited by name in a test without having to be
@@ -1497,9 +1680,13 @@ def audit_module(module_path: 'pathlib.Path | str') -> 'list[str]':
14971680
if tracked is None:
14981681
return []
14991682

1683+
calls = sorted(
1684+
(*sample_path_calls(str(module_path)), *sample_root_join_calls(str(module_path))),
1685+
key=lambda call: call.lineno,
1686+
)
15001687
return [
15011688
explain(call.name, module_path, call.lineno)
1502-
for call in sample_path_calls(str(module_path))
1689+
for call in calls
15031690
if call.name is not None and not call.handled and not _is_committed(call.name, tracked)
15041691
]
15051692

0 commit comments

Comments
 (0)