From eb164421a83d694e884fcf67a00c0b7c0357a6bc Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 23 Sep 2026 00:30:06 -0400 Subject: [PATCH] fix(tests): derive the nameless-flag probe from each registry's own width (#702) `tests/dumpkit/test_nameless_enum_rendering_unit.py` swept the literal `NAMELESS_VALUES = (0, 1, 8, 9, 65536)` against `pcapkit.const.tcp.flags.Flags`, and `65536` is `0x10000` -- one bit past the sixteen-bit field that registry's `_missing_` bounds itself to. The guard is right, so the probe was what had to move: a value a registry is correct to refuse cannot also be a value the dumper is expected to render. `main` has been failing on the `library='aenum'` subtest since #670 introduced the file, and every PR whose Unit Tests job reached it failed for reasons that were not its own. Swept all seven flag registries under `pcapkit/const/` before choosing a direction, since aborting at the first failure hid the rest: * All seven bound `_missing_` to their own field, at four distinct widths -- three bits for `CommandType`, four for `TransportProtocol`, eight for `BindingACKFlag`, `HandoverACKFlag` and `HandoverInitiateFlag`, sixteen for `BindingUpdateFlag` and `Flags`. Exempting the width-guarded registries would therefore have exempted every one of them and left this half of the sweep with nothing in it, which is why the probe is derived per registry instead. * `65536` is out of range for all seven, not only for `Flags`. And `8` and `9` are *named* in `BindingACKFlag` and `TransportProtocol` rather than nameless, so one fixed literal was never right for more than one registry. * For all seven the bound is exactly the smallest all-ones mask covering every declared bit, which `TransportProtocol` already spells for itself as `max(cls.__members__.values()) * 2 - 1`. `_field_mask` derives that, so it needs no table to maintain and cannot drift from the guard. * `_nameless_values` returns the values a registry admits that no member names: no bits at all, each undeclared bit alone, and every undeclared bit at once. The last takes over `9`'s multi-bit role and is the largest nameless value the field holds, which is what the out-of-range literal was reaching for. * `test_no_flag_registry_renders_the_literal_none` sweeps those values rather than `registry(0)` alone. It was broad across registries and one value deep in each, so the four Mobility Header registries only ever saw a single value. * `test_a_value_past_the_field_is_refused_rather_than_rendered` asserts that rejection instead of tripping over it, and pins the derivation itself: the widest in-field value is accepted and the next one up is not, which holds only if the derived mask is the bound each registry declared. No `pcapkit` line changed. The file goes from `1 failed, 5 passed, 16 subtests passed` to `6 passed, 64 subtests passed`, and `tests/dumpkit/` is 15 passed. Coverage rises rather than falls, which deleting the literal on its own would not have managed: `tcp/flags.py:91` was reached only by this file failing on it, and the same `raise` in the other six registries was reached by nothing at all. All seven are now covered -- `ftp/command.py:76`, `mh/binding_ack_flag.py:72`, `mh/binding_update_flag.py:87`, `mh/handover_ack_flag.py:60`, `mh/handover_initiate_flag.py:63`, `reg/apptype.py:65` and `tcp/flags.py:91` -- for six fewer missed statements and six fewer partially-covered branches across those modules. Against the #648 guard reverted the sweep fails 27 subtests and the scalar test 21; against `Flags`' width widened to `0xFFFFFFFF` the new test fails on `value=65536`, which is the trade #702 warns against. Fixes #702 --- .../test_nameless_enum_rendering_unit.py | 297 +++++++++++++++--- 1 file changed, 249 insertions(+), 48 deletions(-) diff --git a/tests/dumpkit/test_nameless_enum_rendering_unit.py b/tests/dumpkit/test_nameless_enum_rendering_unit.py index b4ec54589..b63bcbc73 100644 --- a/tests/dumpkit/test_nameless_enum_rendering_unit.py +++ b/tests/dumpkit/test_nameless_enum_rendering_unit.py @@ -5,18 +5,21 @@ import importlib.util import pkgutil import unittest +from typing import TYPE_CHECKING from tests._support import purge_modules +if TYPE_CHECKING: + import aenum + + #: Either enumeration library's flag class. :class:`aenum.Flag` is *not* an + #: :class:`enum.Flag` subclass -- which is why the sweep below tests for + #: both of them -- so neither one alone annotates a registry. + FlagRegistry = type[enum.Flag] | type[aenum.Flag] + RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) -#: Flag values made up *entirely* of bits no member declares. Every one of these -#: has ``name is None``, which is the whole point of #648: the defect is a -#: property of "no declared bits", not of the number zero, so a guard written -#: against ``value == 0`` would fix the first of these and leave the rest. -NAMELESS_VALUES = (0, 1, 8, 9, 65536) - class StdFlags(enum.IntFlag): """A stdlib replica of :class:`pcapkit.const.tcp.flags.Flags`' declared bits. @@ -95,6 +98,16 @@ class NamelessEnumRenderingTests(unittest.TestCase): * It needs no special case for zero, which matters because the defect never was about zero. + The values probed are derived from each enumeration's own declared bits + rather than written down, which is #702. This file used to sweep the literal + ``(0, 1, 8, 9, 65536)``, and ``65536`` is ``0x10000`` -- one bit past the + sixteen-bit field :class:`~pcapkit.const.tcp.flags.Flags` bounds its + :meth:`~enum.Enum._missing_` to. That guard is correct, so the probe was + what had to move: a value the registry is right to refuse cannot also be a + value the dumper renders. Every one of the library's seven flag registries + carries such a guard, at four distinct widths, so exempting the guarded ones + instead would have left this half of the sweep with nothing in it at all. + """ def setUp(self) -> None: @@ -103,9 +116,14 @@ def setUp(self) -> None: def test_scalar_return_renders_a_nameless_member_as_its_value(self) -> None: """The plain ``return`` at the end of the enumeration branch. - Covers the nameless value *and* four non-zero ones, because a fix that - special-cased zero would pass on ``Flags(0)`` alone and still emit - ``'Flags::8 [8]'``'s predecessor ``'Flags::None [8]'``. + Covers the nameless zero *and* every non-zero nameless value the + registry admits, because a fix that special-cased zero would pass on + ``Flags(0)`` alone and still emit ``'Flags::8 [8]'``'s predecessor + ``'Flags::None [8]'``. + + Both libraries are swept from the same derivation, so this is still the + demonstration that the rendering is not an :mod:`aenum` quirk -- stdlib + :class:`enum.IntFlag` spells a wholly-undeclared value the same way. """ from pcapkit.const.tcp.flags import Flags @@ -113,23 +131,24 @@ def test_scalar_return_renders_a_nameless_member_as_its_value(self) -> None: dumper = make_dumper(BaseDumper)() - for value in NAMELESS_VALUES: - with self.subTest(library='aenum', value=value): - member = Flags(value) - # The premise: there is no name to interpolate. - self.assertIsNone(member.name) - rendered = dumper.object_hook(member) - self.assertEqual(rendered, f'Flags::{value} [{value}]') - # The defect, stated as what must no longer appear. Asserted - # separately from the equality above so a future change to the - # rendering cannot quietly reintroduce the literal. - self.assertNotIn('None', rendered) - - with self.subTest(library='enum', value=value): - # Not an ``aenum`` quirk -- stdlib behaves identically. - std = StdFlags(value) - self.assertIsNone(std.name) - self.assertEqual(dumper.object_hook(std), f'StdFlags::{value} [{value}]') + for registry, library in ((Flags, 'aenum'), (StdFlags, 'enum')): + values = _nameless_values(registry) + # Without this the loop below could sweep nothing at all and still + # report a pass, which is how a derived probe set rots silently + # where a written-down one would not. + self.assertGreater(len(values), 1, (library, values)) + + for value in values: + with self.subTest(library=library, value=value): + member = registry(value) + # The premise: there is no name to interpolate. + self.assertIsNone(member.name) + rendered = dumper.object_hook(member) + self.assertEqual(rendered, f'{registry.__name__}::{value} [{value}]') + # The defect, stated as what must no longer appear. Asserted + # separately from the equality above so a future change to + # the rendering cannot quietly reintroduce the literal. + self.assertNotIn('None', rendered) def test_named_members_are_untouched(self) -> None: """The control. Only the name half of a *nameless* member changes. @@ -227,24 +246,17 @@ def test_no_flag_registry_renders_the_literal_none(self) -> None: Mobility Header flag registries. Naming them here would rot the moment an eighth is added, so they are discovered. - """ - import aenum + Since #702 the sweep is over every nameless value each registry admits + rather than over ``registry(0)`` alone. Before that, the four Mobility + Header registries were only ever exercised at the one value they share, + so the sweep was broad across registries and one value deep in each. + """ from pcapkit.dumpkit.common import make_dumper dumper = make_dumper(BaseDumper)() - registries = {} - for module in pkgutil.walk_packages(_const_path(), prefix='pcapkit.const.'): - try: - imported = importlib.import_module(module.name) - except ImportError: # pragma: no cover - a registry that cannot import - continue - for attribute in vars(imported).values(): - if not isinstance(attribute, type) or attribute.__module__ != module.name: - continue - if issubclass(attribute, (enum.Flag, aenum.Flag)): - registries[f'{module.name}.{attribute.__name__}'] = attribute + registries = _flag_registries() # A guard on the sweep itself: an empty mapping would make every # assertion below vacuous, and that is how this test would rot silently. @@ -252,20 +264,85 @@ def test_no_flag_registry_renders_the_literal_none(self) -> None: nameless = [] for label, registry in sorted(registries.items()): - with self.subTest(registry=label): - member = registry(0) - rendered = dumper.object_hook(member) - self.assertNotIn('::None [', rendered) - if member.name is None: - nameless.append(label) - self.assertEqual(rendered, f'{registry.__name__}::0 [0]') - else: - self.assertEqual(rendered, f'{registry.__name__}::{member.name} [0]') + values = _nameless_values(registry) + if values: + nameless.append(label) + + # ``0`` is rendered whether or not it is nameless. For the two + # registries that declare it -- ``CommandType`` and + # ``TransportProtocol``, the two with no undeclared bits left in + # their field -- it is the only thing there is to render, and it is + # the control showing the fix keys on the name and not on the value. + for value in dict.fromkeys((0, *values)): + with self.subTest(registry=label, value=value): + member = registry(value) + rendered = dumper.object_hook(member) + self.assertNotIn('::None [', rendered) + if member.name is None: + # Cross-checks the derivation as well as the rendering: + # a nameless value the helper did not return would mean + # the sweep is skipping part of the field. + self.assertIn(value, values) + self.assertEqual(rendered, f'{registry.__name__}::{value} [{value}]') + else: + self.assertEqual(rendered, + f'{registry.__name__}::{member.name} [{value}]') # Not an incidental detail: if this ever drops to zero the test above # stops exercising the fix at all and would pass on unfixed code. self.assertGreaterEqual(len(nameless), 5, nameless) + def test_a_value_past_the_field_is_refused_rather_than_rendered(self) -> None: + """#702 -- the probe that was wrong, asserted the right way round. + + All seven flag registries bound their :meth:`~enum.Enum._missing_` to + the width of their own field, and the widths differ: three bits for + :class:`~pcapkit.const.ftp.command.CommandType`, four for + :class:`~pcapkit.const.reg.apptype.TransportProtocol`, eight for three + of the Mobility Header flags, sixteen for ``BindingUpdateFlag`` and + :class:`~pcapkit.const.tcp.flags.Flags`. One literal therefore cannot + mean "past the field" for all of them, which is the whole reason + :func:`_field_mask` derives it per registry. + + This also pins the derivation without parsing anybody's source: the + widest in-field value is accepted and the next one up is refused, which + holds only if the mask :func:`_field_mask` computes is exactly the bound + each registry wrote down for itself. + + ``65536`` survives here, as the bound of the two sixteen-bit registries + -- asserted as the rejection it always was, rather than as a value the + dumper was expected to render. Which also keeps the ``raise`` in those + guards covered: six of the seven were never reached by any test, and the + seventh was reached only by this file failing on it. + + """ + registries = _flag_registries() + self.assertGreaterEqual(len(registries), 7, registries) + + widths = set() + for label, registry in sorted(registries.items()): + mask = _field_mask(registry) + widths.add(mask.bit_length()) + + with self.subTest(registry=label, value=mask): + # The widest value the field holds is valid, named or not. + self.assertEqual(registry(mask).value, mask) + + with self.subTest(registry=label, value=mask + 1): + with self.assertRaises(ValueError) as caught: + registry(mask + 1) + # ``enum`` requires ``ValueError`` from a refused lookup, and + # #677 brought the last six divergent registries onto the bare + # built-in the rest of :mod:`pcapkit.const` already raised, so + # this asserts the message rather than a type of its own. + self.assertIn(str(mask + 1), str(caught.exception)) + self.assertIn(registry.__name__, str(caught.exception)) + + # The literal could not have been right for all of them, and this is the + # measurement that says so: several distinct widths, and ``65536`` is + # outside every single one of them. + self.assertGreaterEqual(len(widths), 4, widths) + def _const_path() -> 'list[str]': """The filesystem path of :mod:`pcapkit.const`, for :func:`pkgutil.walk_packages`. @@ -282,5 +359,129 @@ def _const_path() -> 'list[str]': return list(pcapkit.const.__path__) +def _flag_registries() -> 'dict[str, FlagRegistry]': + """Every flag enumeration declared under :mod:`pcapkit.const`. + + Discovered rather than listed, so an eighth registry is swept the day it + lands instead of the day somebody remembers to add it here. Keyed by + ``.`` so a failing subtest names the registry it came from. + + Both libraries are matched because the two are unrelated types -- + :class:`aenum.Flag` is not an :class:`enum.Flag` subclass -- and the + generated registries use :mod:`aenum` while the stand-ins in this file use + the stdlib. + + Returns: + The discovered registries, keyed by dotted path. + + """ + import aenum + + registries: 'dict[str, FlagRegistry]' = {} + for module in pkgutil.walk_packages(_const_path(), prefix='pcapkit.const.'): + try: + imported = importlib.import_module(module.name) + except ImportError: # pragma: no cover - a registry that cannot import + continue + for attribute in vars(imported).values(): + if not isinstance(attribute, type) or attribute.__module__ != module.name: + continue + if issubclass(attribute, (enum.Flag, aenum.Flag)): + registries[f'{module.name}.{attribute.__name__}'] = attribute + return registries + + +def _declared_bits(registry: 'FlagRegistry') -> int: + """The union of every bit *registry* declares a member for. + + Iteration over a flag enumeration yields the canonical single-bit members, + skipping a zero member and any alias, which is what makes this the registry's + *declared bits* rather than a member count: ``Flags`` declares twelve members + from ``1 << 4`` upwards, so this is ``0xFFF0``. + + Args: + registry: The flag enumeration to inspect. + + Returns: + The bitwise OR of every member's value. + + """ + declared = 0 + for member in registry: + declared |= member.value + return declared + + +def _field_mask(registry: 'FlagRegistry') -> int: + """The width of *registry*'s field, as an all-ones mask. + + Every flag registry in the library bounds its :meth:`~enum.Enum._missing_` + to the field its declared bits live in, and for all seven of them that bound + is exactly the smallest all-ones mask covering every declared bit -- + ``0xFFFF`` for ``Flags``' ``1 << 4 .. 1 << 15``, ``0xFF`` for the eight-bit + Mobility Header flags, ``0x07`` for the three-bit + :class:`~pcapkit.const.ftp.command.CommandType`. + :class:`~pcapkit.const.reg.apptype.TransportProtocol` already spells it that + way in its own source, as ``max(cls.__members__.values()) * 2 - 1``, because + it extends itself at runtime and cannot hard-code a bound. + + Derived rather than read off the source, so it cannot drift from the guard + and needs no table to maintain. That it does not drift is itself asserted, by + :meth:`NamelessEnumRenderingTests.test_a_value_past_the_field_is_refused_rather_than_rendered`. + + Args: + registry: The flag enumeration to inspect. + + Returns: + An all-ones mask as wide as the registry's field. + + """ + return (1 << _declared_bits(registry).bit_length()) - 1 + + +def _nameless_values(registry: 'FlagRegistry') -> 'tuple[int, ...]': + """The in-field values of *registry* that no member names. + + A flag value made up *entirely* of bits no member declares has + ``name is None``, which is the whole point of #648: the defect is a property + of "no declared bits", not of the number zero, so a guard written against + ``value == 0`` would fix the first of these and leave the rest. + + Three kinds of value come back and no more -- no bits at all, each single + undeclared bit on its own, and every undeclared bit at once. The last is the + multi-bit case, and it is the largest value the field admits with no declared + bit in it, which is what the old literal's ``9`` and its out-of-range + ``65536`` were each reaching for. Returning every subset would be exhaustive + and is not worth 8192 subtests for :class:`StdFlags`' thirteen undeclared + bits. + + Nothing out of range is returned, which is #702: the values are all masked + into the registry's own field, so a registry that legitimately refuses + ``0x10000`` is never asked to mint a pseudo-member for it. + + A registry whose declared bits fill its field has no nameless value at all + and yields an empty tuple. :class:`~pcapkit.const.ftp.command.CommandType` + and :class:`~pcapkit.const.reg.apptype.TransportProtocol` are both of that + shape, each declaring ``0`` as ``undefined``, which is why five of the seven + registries are nameless at zero rather than all seven. + + Args: + registry: The flag enumeration to inspect. + + Returns: + The registry's nameless values, smallest first. + + """ + undeclared = _field_mask(registry) & ~_declared_bits(registry) + singles = [1 << index for index in range(undeclared.bit_length()) + if undeclared >> index & 1] + + candidates = [0, *singles] + if len(singles) > 1: + candidates.append(undeclared) + + return tuple(value for value in candidates if registry(value).name is None) + + if __name__ == '__main__': unittest.main()