From 39979288389b55c7d046eff2797a52173a4b26f6 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 16 Sep 2026 09:30:04 -0400 Subject: [PATCH 1/2] chore: mark vermin's guarded-import false positives, close the engine handle Two small fixes and one measurement. **`close_extractor` leaked the engine handle.** It closed `_ifile` only, while both `Extractor._cleanup` and `Extractor.__exit__` close `_ifile` *then* `_exeng`. That matters because `Engine.close()` is a no-op on the base class but not on the subclasses: `pcap_ct` and `pypcap` close a live `pcap.pcap` handle and `pyshark` closes a temp file, so an extractor abandoned mid-file in a test teardown leaked a real OS handle. Now closes both, in the library's order, each guarded separately and in a `try/finally` so a failing stream close cannot strand the engine. `getattr` probing keeps it tolerant of an extractor that died before `_exeng` was assigned, which is the state teardown most often sees. No test depended on the leak: all ~30 call sites use it purely as teardown and never touch the extractor afterwards. New `tests/test_support_helpers.py` pins the contract, following the `tests/test_tier_guard.py` precedent for covering test-support code. **`# novermin` on three guarded imports in `compat.py`.** Vermin reported a 3.11 minimum from `enum.StrEnum` and `enum.show_flag_values`, and 3.10 from `typing.TypeAlias`, all three in the `else:` branch of a `sys.version_info` guard -- so they never execute on an older interpreter and the report was a false positive. `# novermin` is Vermin's documented escape hatch for exactly this. Verified the combined `# type: ignore[attr-defined] # novermin` still works: zero mypy errors in the file, and mypy does report unused ignores elsewhere in the same run, so the check has teeth. `targets = 3.6` is left as it is, per the maintainer's decision to keep the 3.6 claim. Recording what that costs, since it was measured and should not have to be re-discovered: after these three markers the report lands on **3.9**, and the remaining floor is real code rather than false positives -- **65 named-expression (walrus) sites across 57 files**, unguarded and at module level, plus a positional-only `/` parameter in `foundation/engines/engine.py`. Walrus is 3.8+, so no amount of marking reaches 3.6. Vermin is not wired into any workflow, so nothing is red either way. Verified: `tests/test_support_helpers.py` plus `tests/utilities` 96 passed, 100 subtests. --- pcapkit/utilities/compat.py | 14 +++- tests/_support.py | 56 +++++++++++++- tests/test_support_helpers.py | 137 ++++++++++++++++++++++++++++++++++ 3 files changed, 202 insertions(+), 5 deletions(-) create mode 100644 tests/test_support_helpers.py diff --git a/pcapkit/utilities/compat.py b/pcapkit/utilities/compat.py index 4395076c0c..84724d7bbf 100644 --- a/pcapkit/utilities/compat.py +++ b/pcapkit/utilities/compat.py @@ -131,10 +131,18 @@ def __get__(self, instance: 'Optional[_S]', List: 'TypeAlias' = list Dict: 'TypeAlias' = dict +# The ``else`` branch of each ``sys.version_info`` guard below imports a name +# that only exists on the newer interpreter, so it never executes on the older +# one the guard is there to support. Vermin's analysis is static and cannot see +# that, so it counts these imports anyway and reports the whole library as +# requiring 3.11 on the strength of three lines that never run below it. +# ``# novermin`` is Vermin's documented escape hatch for precisely this shape. +# It is applied per line rather than by turning on ``lax`` mode, so that an +# *unguarded* use of a new feature anywhere else is still caught. if sys.version_info < (3, 11): from aenum import StrEnum else: - from enum import StrEnum + from enum import StrEnum # novermin if sys.version_info < (3, 8): from typing_extensions import final @@ -188,9 +196,9 @@ def _iter_bits_lsb(num: 'int') -> 'Iterator[int]': def show_flag_values(value: 'IntFlag') -> 'list[int]': return list(_iter_bits_lsb(value)) else: - from enum import show_flag_values # type: ignore[attr-defined] + from enum import show_flag_values # type: ignore[attr-defined] # novermin if sys.version_info < (3, 10): from typing_extensions import TypeAlias else: - from typing import TypeAlias + from typing import TypeAlias # novermin diff --git a/tests/_support.py b/tests/_support.py index f1abf1b5aa..5b275682b0 100644 --- a/tests/_support.py +++ b/tests/_support.py @@ -195,7 +195,59 @@ def purge_modules(prefixes: Iterable[str]) -> None: _reset_abc_caches() +def _close_quietly(target: object) -> None: + """Call ``target.close()``, swallowing anything it raises. + + Args: + target: Object to close, or :data:`None`. Anything without a callable + ``close`` attribute is ignored. + + """ + close = getattr(target, 'close', None) + if not callable(close): + return + try: + close() + except Exception: # pylint: disable=broad-except + # This runs from teardown, where raising would replace the real test + # failure with a secondary error from cleanup and hide what actually + # broke. A half-constructed engine is the common case: the underlying + # handle may never have been opened, so closing it raises rather than + # being a no-op. + pass + + def close_extractor(extractor: object) -> None: + """Release everything an :class:`~pcapkit.foundation.extraction.Extractor` holds. + + Tests that abandon an extractor part-way through a capture never reach + :meth:`Extractor._cleanup ` + or :meth:`Extractor.__exit__ `, + so nothing in the library closes up after them. Both of those close the + input file *and* the engine, and this helper has to do the same: the input + file is not the only resource. The ``pcap_ct`` and ``pypcap`` engines hold a + live :class:`pcap.pcap` handle, and ``pyshark`` holds a temporary file, so + dropping the extractor without closing the engine leaks an OS-level handle + per test. Under a suite that builds hundreds of extractors that accumulates + into a file-descriptor exhaustion whose failure surfaces somewhere unrelated. + + Closes in the same order the library does -- input file, then engine -- and + closes the engine even if closing the input file fails, so one broken + resource cannot strand the other. + + Args: + extractor: The extractor to close. Deliberately typed :obj:`object` and + probed with :func:`getattr`, because teardown also reaches here for + extractors that failed part-way through ``__init__`` (in which case + ``_exeng`` was never assigned) and for test doubles that stand in + for one. + + """ + # ``_exeng`` is read before the input file is touched so that a failure + # closing the stream cannot lose the reference to the engine. stream = getattr(extractor, '_ifile', None) - if stream is not None and hasattr(stream, 'close'): - stream.close() + engine = getattr(extractor, '_exeng', None) + try: + _close_quietly(stream) + finally: + _close_quietly(engine) diff --git a/tests/test_support_helpers.py b/tests/test_support_helpers.py new file mode 100644 index 0000000000..e5aa975324 --- /dev/null +++ b/tests/test_support_helpers.py @@ -0,0 +1,137 @@ +# -*- coding: utf-8 -*- +"""Tests for :func:`tests._support.close_extractor`. + +That helper is teardown machinery: nearly every runtime and integration test +hands it an extractor from ``addCleanup`` or a ``finally`` block. Teardown code +is exactly the code whose bugs stay invisible -- a leak leaks silently, and a +teardown that raises reports itself as a failure in whichever test happened to +be running rather than as a fault in the helper. So the contract is pinned here +rather than left to be inferred from the call sites. + +Three things are worth pinning, and they are the three ways this could rot: + +* both resources are released, not just the input file + (:class:`ClosesBothTests`) -- the original helper closed ``_ifile`` alone and + leaked the engine's :class:`pcap.pcap` handle on every abandoned extractor; +* one broken resource cannot strand the other + (:class:`IndependenceTests`); +* nothing it is handed in teardown makes it raise + (:class:`ToleranceTests`). + +This module is unit-tier: it drives the helper with stand-ins rather than real +extractors, so it reads no sample capture and needs no engine installed. + +""" +from __future__ import annotations + +import unittest + +from tests._support import close_extractor + + +class Closeable: + """A stand-in for a resource that records having been closed. + + Args: + error: Exception to raise from :meth:`close`, or :data:`None` to close + cleanly. A resource that raises on close is the half-constructed + engine case, and is why the helper guards each call. + + """ + + def __init__(self, error: 'BaseException | None' = None) -> None: + self.error = error + self.calls = 0 + + def close(self) -> None: + self.calls += 1 + if self.error is not None: + raise self.error + + +class Extractor: + """A stand-in exposing the two private attributes the helper reads.""" + + def __init__(self, ifile: 'object' = None, exeng: 'object' = None) -> None: + self._ifile = ifile + self._exeng = exeng + + +class ClosesBothTests(unittest.TestCase): + """The engine is closed as well as the input file.""" + + def test_closes_the_input_file_and_the_engine(self) -> None: + stream, engine = Closeable(), Closeable() + + close_extractor(Extractor(stream, engine)) + + self.assertEqual(stream.calls, 1) + # The regression this guards: closing the stream alone leaves the + # engine's OS-level handle open for the rest of the process. + self.assertEqual(engine.calls, 1) + + +class IndependenceTests(unittest.TestCase): + """Neither resource can prevent the other from being released.""" + + def test_engine_is_closed_even_when_the_stream_close_fails(self) -> None: + stream, engine = Closeable(OSError('stream is already gone')), Closeable() + + close_extractor(Extractor(stream, engine)) + + self.assertEqual(engine.calls, 1) + + def test_stream_is_closed_even_when_the_engine_close_fails(self) -> None: + stream, engine = Closeable(), Closeable(AttributeError('_extmp')) + + close_extractor(Extractor(stream, engine)) + + self.assertEqual(stream.calls, 1) + + +class ToleranceTests(unittest.TestCase): + """Nothing teardown can hand the helper makes it raise. + + Each case below is reached in practice: an extractor whose ``__init__`` + failed before assigning ``_exeng``, a test double that is neither, and an + engine whose ``close`` raises because its handle was never opened. + + """ + + def test_absent_attributes_are_ignored(self) -> None: + close_extractor(object()) + + def test_none_valued_attributes_are_ignored(self) -> None: + close_extractor(Extractor(None, None)) + + def test_half_constructed_extractor_without_an_engine(self) -> None: + stream = Closeable() + + # ``_exeng`` is assigned only once the engine has been selected, so an + # extractor that raised before that point has no such attribute at all. + class Partial: + def __init__(self) -> None: + self._ifile = stream + + close_extractor(Partial()) + + self.assertEqual(stream.calls, 1) + + def test_non_callable_close_attribute_is_ignored(self) -> None: + class NotReallyCloseable: + close = 'not a method' + + close_extractor(Extractor(NotReallyCloseable(), NotReallyCloseable())) + + def test_both_closes_raising_is_still_swallowed(self) -> None: + stream = Closeable(OSError('stream')) + engine = Closeable(RuntimeError('engine')) + + close_extractor(Extractor(stream, engine)) + + self.assertEqual(stream.calls, 1) + self.assertEqual(engine.calls, 1) + + +if __name__ == '__main__': + unittest.main() From 15b19422322c73629451ccb87cff1823d0d51f86 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 16 Sep 2026 11:36:45 -0400 Subject: [PATCH 2/2] tests: guard the close lookup itself, and say which exceptions are swallowed Addresses two review findings on #411: - ``getattr(target, 'close', None)`` sat outside the ``try``, so a resource whose ``close`` is a property or resolved through ``__getattr__`` raised from the *lookup* and escaped -- the failure the helper exists to prevent, since it runs from teardown and would replace the real test failure. Measured against the previous code: it raised ``RuntimeError``. The lookup now happens inside the ``try``, and one unreachable resource still does not strand the other. - The docstring said "swallowing anything it raises" of a bare ``except Exception``. It now says :exc:`Exception`, and says why :exc:`BaseException` is left to propagate. Two tests added; 10 pass in the file, 659 in the fixture-free tier. --- tests/_support.py | 15 +++++++++++---- tests/test_support_helpers.py | 36 +++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/tests/_support.py b/tests/_support.py index 5b275682b0..a0e6d5082d 100644 --- a/tests/_support.py +++ b/tests/_support.py @@ -196,17 +196,24 @@ def purge_modules(prefixes: Iterable[str]) -> None: def _close_quietly(target: object) -> None: - """Call ``target.close()``, swallowing anything it raises. + """Call ``target.close()``, swallowing any :exc:`Exception` it raises. + + :exc:`BaseException` is deliberately not caught: a + :exc:`KeyboardInterrupt` or a :exc:`SystemExit` arriving during teardown + should still end the run. Args: target: Object to close, or :data:`None`. Anything without a callable ``close`` attribute is ignored. """ - close = getattr(target, 'close', None) - if not callable(close): - return try: + # Inside the ``try`` because the lookup itself can raise: a test double + # with ``close`` as a property, or a custom ``__getattr__``, fails here + # rather than at the call, and that would defeat the whole point. + close = getattr(target, 'close', None) + if not callable(close): + return close() except Exception: # pylint: disable=broad-except # This runs from teardown, where raising would replace the real test diff --git a/tests/test_support_helpers.py b/tests/test_support_helpers.py index e5aa975324..4b45c0dc16 100644 --- a/tests/test_support_helpers.py +++ b/tests/test_support_helpers.py @@ -132,6 +132,42 @@ def test_both_closes_raising_is_still_swallowed(self) -> None: self.assertEqual(stream.calls, 1) self.assertEqual(engine.calls, 1) + def test_a_close_lookup_that_raises_is_ignored(self) -> None: + """Reaching ``close`` at all can fail, and that is tolerated too. + + ``close`` need not be a plain method: as a property, or resolved through + ``__getattr__``, the *lookup* raises rather than the call. Guarding only + the call would let that escape and mask the real failure. + + """ + class HostileLookup: + @property + def close(self) -> 'object': + raise RuntimeError('lookup') + + engine = Closeable() + + close_extractor(Extractor(HostileLookup(), engine)) + + # And the engine is still closed: one unreachable resource must not + # strand the other, exactly as when the call itself raises. + self.assertEqual(engine.calls, 1) + + +class PropagationTests(unittest.TestCase): + """What the helper deliberately does *not* swallow.""" + + def test_base_exception_is_not_swallowed(self) -> None: + """A :exc:`KeyboardInterrupt` during teardown still ends the run. + + The helper catches :exc:`Exception`, not :exc:`BaseException`, so + interrupting a suite mid-teardown is not quietly absorbed by a cleanup + helper. + + """ + with self.assertRaises(KeyboardInterrupt): + close_extractor(Extractor(Closeable(KeyboardInterrupt()), Closeable())) + if __name__ == '__main__': unittest.main()