From 8cb69d27b589ecdff6dec20a7c4996c3d44f937e Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sun, 27 Sep 2026 19:18:03 -0400 Subject: [PATCH 1/2] fix(corekit): stop EnumRegistry.get's default from minting (#864) Both `get`'s `cls(default)` sites -- the `str` branch's and the value branch's -- reached `_missing_` for a `default` that fell inside a still-minting registry's own range, growing the registry as a side effect of resolving `default` rather than `key`. - Replace both `cls(default)` calls with a plain `_value2member_map_` lookup (owner's ruling, option 1): `default` can no longer mint by construction, and a `default` naming no registered member now falls through to the same lookup error `key` itself would have raised. - `key` resolution is unchanged: `EtherType.get(0x0888)` still mints `Xyplex_0x0888` through `_missing_`, per the ruling's own exception -- landing in both lookup tables, exactly as `register` would leave it, since that is #775's own deliberately-kept-minting exception, not something `get` itself does. - Update the docstring's "It never mints" caveat (now qualified: true for `default`, not for `key` on the three still-minting registries), the now-false claim that an unregistered default still reaches `cls(default)`, and the Args/Raises text to match. - Add `GetDefaultNoMintTests` pinning the issue's own repro, the preserved key-path mint, default resolving to a registered value (int and str), and the exception type per path. Update two `NoDefaultSentinelTests` assertions that asserted the old `cls(default)` failure mode for an unregistered `-1`/`-1.0` default -- their docstrings now say plainly that they coincidentally pass on the pre-#857 tree too, and point at the test that still discriminates it. - Update four assertions in `test_const_enum_get.py` and `test_const_enum_builtin_parity.py` that encoded the superseded contract (an unresolvable/unregistered default failing with its own named error, or resolving into a declared-but-unassigned range) -- each retargeted to the new contract rather than weakened. The FilterType test's three dropped resolve/identity assertions are restored through its unaffected `key` path and the method renamed to match; it and its comment cross-reference are updated together. Owner's rulings, verbatim: - "Take (b). Only register can mint. get should not mint unless it falls through the _missing_'s minted ranges." - Choosing option 1 of three proposed: "I think 1 is correct mechanism we'd like." Build/tests, under plain unittest, methods not subTest records: test_const_registry_protocol.py 74/74, test_const_enum_get.py 8/8, test_const_enum_builtin_parity.py 33/33, tests/vendor/ 86/86 -- no regressions. tests/const/ as a whole carries 6 records: 3 methods inherited from #866 (fixed in #867) plus 1 further out-of-scope failure in test_const_enum_no_mint.py (contended with #865, handed over rather than edited here). --- pcapkit/corekit/enum.py | 54 +++-- tests/const/test_const_enum_builtin_parity.py | 26 ++- tests/const/test_const_enum_get.py | 124 +++++++++--- tests/const/test_const_registry_protocol.py | 186 ++++++++++++++++-- 4 files changed, 327 insertions(+), 63 deletions(-) diff --git a/pcapkit/corekit/enum.py b/pcapkit/corekit/enum.py index 15a3e6ecad..6e2f6c479b 100644 --- a/pcapkit/corekit/enum.py +++ b/pcapkit/corekit/enum.py @@ -264,15 +264,25 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': returns the member the alias points at, not a separate object -- so two names for one assignment resolve to one enum. - It never mints -- see #864 for the ``default`` path, which does and - should not. Registering a member is :meth:`register`'s job and - nobody else's, which is the ruling #775 exists to carry out: *"so that - we dont create registered enums out of unrecognised/unregistered - values, unless user/caller explicitly created them"*. A value inside a - registry's declared-but-unassigned range still resolves, through that - registry's own ``_missing_`` and :meth:`_unregistered_member`, to a - member that is deliberately absent from the lookup tables -- for a - non-``str`` key; the ``str`` case is qualified below. + It never mints while resolving ``default``; ``key`` may still mint + through a ``_missing_`` that GitHub issue #775's ruling deliberately + kept minting, on three registries (``EtherType``, ``Socket``, + ``CGAType``). Registering a member any other way is + :meth:`register`'s job and nobody else's, which is the ruling #775 + exists to carry out: *"so that we dont create registered enums out + of unrecognised/unregistered values, unless user/caller explicitly + created them"*. A value inside a registry's declared-but-unassigned + range still resolves, through that registry's own ``_missing_`` and + :meth:`_unregistered_member`, to a member that is deliberately + absent from the lookup tables -- true outside the three registries + named above, where such a value instead lands in *both* tables, + exactly as :meth:`register` would leave it -- for a non-``str`` key; + the ``str`` case is qualified below. Both describe ``key`` resolution + only. ``default`` never reaches ``_missing_`` on either branch: a + declared-but-unassigned ``default`` does not resolve to an + unregistered member the way such a ``key`` does -- it simply does + not resolve, and the lookup error ``key`` itself would have raised + propagates instead. For a ``str`` key, a name match wins over a value match -- the two are checked in that order, so a string that happens to be both a member's @@ -286,9 +296,11 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': mint a permanent member where it previously just raised. Restricting the value side of ``key`` to an already-registered value keeps *that side* non-minting on every ``str``-valued registry, not only the ones - without a minting ``_missing_`` -- it is a claim about the value side - alone, not about this method as a whole: an unregistered ``default`` - still reaches ``cls(default)`` below, and can mint there just the same. + without a minting ``_missing_``. Since #864, that is no longer merely + a claim about the value side alone: ``default`` resolves through the + same kind of ``_value2member_map_`` lookup rather than + ``cls(default)``, so for a ``str`` key every path through this + method -- name, value and ``default`` alike -- is non-minting. That restriction has a cost the paragraph above glosses over: a *declared-but-unassigned* value -- the case resolved there through @@ -304,9 +316,13 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': Args: key: Name or value to look up. - default: Value to fall back to when ``key`` does not resolve. - :data:`NO_DEFAULT` stands for *no default*, in which case the - lookup error propagates instead. + default: An already-registered value to fall back to when + ``key`` does not resolve. Resolved through a plain + ``_value2member_map_`` lookup, never through + ``cls(default)``, so it cannot mint -- see #864. + :data:`NO_DEFAULT` stands for *no default*; that and a + ``default`` naming no registered member both fall through to + the same lookup error ``key`` itself would have raised. Returns: The canonical member for ``key``, or for ``default``. @@ -324,15 +340,15 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': except KeyError: if key in cls._value2member_map_: return cls._value2member_map_[key] - if default is NO_DEFAULT: + if default is NO_DEFAULT or default not in cls._value2member_map_: raise - return cls(default) # type: ignore[call-arg] + return cls._value2member_map_[default] try: return cls(key) # type: ignore[call-arg] except ValueError: - if default is NO_DEFAULT: + if default is NO_DEFAULT or default not in cls._value2member_map_: raise - return cls(default) # type: ignore[call-arg] + return cls._value2member_map_[default] @classmethod def get_all(cls, key: 'Any') -> 'tuple[Self, ...]': diff --git a/tests/const/test_const_enum_builtin_parity.py b/tests/const/test_const_enum_builtin_parity.py index 0162092eff..5f6101bd5d 100644 --- a/tests/const/test_const_enum_builtin_parity.py +++ b/tests/const/test_const_enum_builtin_parity.py @@ -676,6 +676,23 @@ def test_the_guard_leaves_the_other_lookup_paths_alone(self) -> None: and the integer path reaches it inside a ``try``. Both are exercised here for the four modules GitHub issue #647 touched, so a guard that had broken either would fail rather than merely go unmeasured. + + This used to also probe the fallback with ``Flags.get(UNRESOLVABLE, + 0)``, expecting it to resolve -- ``0`` is in-bounds for ``Flags``'s + own ``_missing_`` (``0 <= value <= 0xFFFF``), so before GitHub issue + #864 it reached ``cls(0)`` and resolved via the converted pseudo-member + path, uncaught. It is not a *registered* ``Flags`` member, though + (``Flags``'s smallest declared value is ``1 << 4``), so under #864's + restriction of ``default`` to a plain ``_value2member_map_`` lookup it + no longer resolves at all -- the same accepted cost as an unassigned + integer range, here on an :class:`~aenum.IntFlag` registry instead. + That is why the prior version of this assertion errored rather than + failed: the ``ValueError`` from the *key* (``UNRESOLVABLE``) now + propagates uncaught through a bare ``assertEqual`` that expected + success. Replaced with the same two-part check used elsewhere in this + batch: a *registered* default (``1 << 14``, ``SYN``) is still + genuinely consulted and resolves, while the unregistered ``0`` now + raises the same original-key error as no default at all. """ from pcapkit.const.ftp.command import Command from pcapkit.const.http.method import Method @@ -694,7 +711,14 @@ def test_the_guard_leaves_the_other_lookup_paths_alone(self) -> None: # And the fallback the guard's ValueError is what triggers: GitHub issue # #584's ``get(key, default)``, which an EnumError would have walked past. - self.assertEqual(int(Flags.get(UNRESOLVABLE, 0)), 0) + # A registered default is still genuinely consulted and resolves. + self.assertIs(Flags.get(UNRESOLVABLE, 1 << 14), Flags.SYN) + # Since #864: ``0`` is not a registered ``Flags`` member, so it no + # longer resolves either -- the original key's error propagates, the + # same as omitting the default outright. + with self.assertRaises(ValueError) as caught: + Flags.get(UNRESOLVABLE, 0) + self.assertIn(str(UNRESOLVABLE), str(caught.exception)) with self.assertRaises(ValueError): Flags.get(UNRESOLVABLE) diff --git a/tests/const/test_const_enum_get.py b/tests/const/test_const_enum_get.py index 8064c69f27..0238b1f9b8 100644 --- a/tests/const/test_const_enum_get.py +++ b/tests/const/test_const_enum_get.py @@ -120,7 +120,7 @@ #: value at all that ``obj(fallback)`` would resolve to a *cached* identity #: for. Excused from the main sweep for that reason and covered by its own #: test instead, :meth:`ConstEnumGetDefaultTests. -#: test_filter_type_default_is_consulted_without_a_cached_fallback`. +#: test_filter_type_default_never_resolves_but_key_path_still_uses_an_uncached_member`. EXPECTED_WITHOUT_A_CACHEABLE_FALLBACK = frozenset({ 'pcapkit.const.pcapng.filter_type.FilterType', }) @@ -191,18 +191,27 @@ def test_the_reported_case_returns_the_default(self) -> None: from pcapkit.const.arp.hardware import Hardware # Before the fix this raised ValueError('99999 is not a valid Hardware'), - # dropping the caller's default entirely. + # dropping the caller's default entirely. Both ``0`` (``Reserved_0``) + # and ``1`` (``Ethernet``) are genuinely registered members, so #864 + # leaves this half of the repro untouched: a registered default still + # resolves, exactly as #584 asked for. self.assertIs(Hardware.get(99999, 0), Hardware(0)) self.assertIs(Hardware.get(99999, 1), Hardware.Ethernet) - # The default is genuinely consulted rather than merely swallowing the - # error: a default that is itself unresolvable is now what fails, and - # the message names it rather than the original key. #584 passed a - # ``str`` where the signature says ``int``, and that is still invalid. + # GitHub issue #864 changes what an *unresolvable* default does, + # though: #584 passed a ``str`` where the signature says ``int``, + # and ``'X'`` is not a registered value either way -- before #864 + # that failed with ``cls('X')``'s own error, naming ``'X'``; #864's + # ruling ("get should not mint unless it falls through the + # _missing_'s minted ranges") replaces ``cls(default)`` with a plain + # ``_value2member_map_`` lookup for exactly this reason, so an + # unresolvable default no longer gets an attempt of its own to fail + # from -- it now falls through to the *original* key lookup's own + # error instead, same as if no default had been supplied at all. with self.assertRaises(ValueError) as caught: Hardware.get(99999, 'X') - self.assertIn('X', str(caught.exception)) - self.assertNotIn('99999', str(caught.exception)) + self.assertIn('99999', str(caught.exception)) + self.assertNotIn('X', str(caught.exception)) # A resolvable key is untouched. self.assertIs(Hardware.get(1), Hardware.Ethernet) @@ -216,16 +225,35 @@ def test_the_reported_case_returns_the_default(self) -> None: def test_omitting_the_default_still_raises_for_the_original_key(self) -> None: """GitHub issue #857: ``-1`` used to be compared with ``==`` against :data:`~pcapkit.corekit.enum.NO_DEFAULT`, so a caller-supplied ``-1`` - was silently read as *no default was supplied* -- "explicitly passing - the placeholder is the same as omitting it", as this test used to - assert. :data:`~pcapkit.corekit.enum.NO_DEFAULT` is now an exported - instance of the dedicated :class:`~pcapkit.corekit.enum.NoDefaultType` - compared with ``is``, so ``-1`` is a genuine default like any other: - omitting ``default`` entirely still raises for the *original* key - (unchanged, pinned below), while explicitly passing ``-1`` now raises - for the *attempted fallback* ``cls(-1)`` instead -- no longer the same - error, since every registry's domain here starts at ``0`` and ``-1`` - is never a legitimate value. + was silently read as *no default was supplied*. + :data:`~pcapkit.corekit.enum.NO_DEFAULT` is now an exported instance + of the dedicated :class:`~pcapkit.corekit.enum.NoDefaultType`, + compared with ``is`` rather than ``==`` -- so a caller-supplied + ``-1`` is never confused with the sentinel. That fact is pinned + directly, independent of ``get()``, by + :class:`~tests.const.test_const_registry_protocol.NoDefaultSentinelTests + .test_no_default_is_not_equal_to_any_plausible_caller_value`. + + Before GitHub issue #864, that distinction was *also* observable + through ``get()`` itself: a supplied-but-unresolvable ``-1`` failed + with its own name (``cls(-1)`` raising directly), differently from + the omitted case's original-key error. #864's ruling ("get should + not mint unless it falls through the _missing_'s minted ranges") + removes that particular observation for an *unregistered* default + specifically: ``default`` no longer reaches ``cls(default)`` at + all, and ``-1`` is not a registered value on either registry here + (both domains start at ``0``), so supplying it now converges on + exactly the *same* original-key error as omitting it outright, + rather than a distinguishable one of its own. The test's own title + is, if anything, more true after #864 than before: *omitting* the + default raises for the original key, and now so does supplying an + unregistered one -- pinned below for both. + + The genuinely-consulted half of #857's claim is repinned with a + *registered* default instead (``0``, ``Reserved_0`` on both + registries), which still resolves rather than raising -- the + observable proof #864 leaves available once ``-1`` no longer + provides it. """ from pcapkit.const.arp.hardware import Hardware from pcapkit.const.arp.operation import Operation @@ -236,12 +264,18 @@ def test_omitting_the_default_still_raises_for_the_original_key(self) -> None: enum.get(99999) self.assertIn('99999', str(omitted.exception)) - # No longer "the same as omitting it": this now names the - # failed fallback (``-1``), not the original key (``99999``). + # Since #864: an unregistered default (neither registry here + # has a member at ``-1``) converges on the *same* + # original-key error as omission, rather than a distinct one + # naming ``-1``. with self.assertRaises(ValueError) as supplied: enum.get(99999, -1) - self.assertIn('-1', str(supplied.exception)) - self.assertNotIn('99999', str(supplied.exception)) + self.assertIn('99999', str(supplied.exception)) + self.assertNotIn('-1', str(supplied.exception)) + + # A *registered* default, by contrast, is still genuinely + # consulted rather than raising at all. + self.assertIs(enum.get(99999, 0), enum(0)) def test_the_two_unverified_enums_from_the_issue(self) -> None: """#584 named ``Operation`` and ``LinkType`` but verified only ``Hardware``.""" @@ -343,20 +377,54 @@ def test_the_always_resolving_registries_have_nothing_to_fall_back_to(self) -> N # every time, and the registry never grows for it. self.assertIsNot(resolved, obj.get(UNRESOLVABLE, 0)) - def test_filter_type_default_is_consulted_without_a_cached_fallback(self) -> None: + def test_filter_type_default_never_resolves_but_key_path_still_uses_an_uncached_member(self) -> None: """:class:`~pcapkit.const.pcapng.filter_type.FilterType`'s own replacement for the sweep above -- see - :data:`EXPECTED_WITHOUT_A_CACHEABLE_FALLBACK` for why it needs one.""" + :data:`EXPECTED_WITHOUT_A_CACHEABLE_FALLBACK` for why it needs one. + + FilterType declares *no* static members at all, so its own + ``_value2member_map_`` is permanently empty -- not ``0``, not + anything. Before GitHub issue #864, ``default=0`` still resolved + because ``get`` called ``cls(0)`` directly, reaching ``_missing_`` + and its converted, uncached pseudo-member path. #864's ruling + closes exactly that path for ``default``: it is restricted to a + plain ``_value2member_map_`` lookup, and FilterType's is + permanently empty, so *no* default can ever resolve for this + registry any more -- the strongest instance of the ruling's own + accepted cost, since it is not one range that stops resolving but + the registry's entire domain. + + ``key`` resolution is untouched by #864, though, and it is where + the uncached pseudo-member path this test was written to cover + still lives: ``FilterType.get(0)`` -- ``0`` as the *key*, no + default at all -- still reaches ``cls(0)`` -> ``_missing_`` + directly, resolving to a fresh, equal-but-not-identical + pseudo-member every call, exactly as before. Restored here through + the path #864 does not touch, now that ``default`` can no longer + demonstrate it. + """ from pcapkit.const.pcapng.filter_type import FilterType with self.assertRaises(ValueError): FilterType.get(UNRESOLVABLE) - # 0 is in-bounds (0x00-0xFF) but not a real member either -- FilterType - # declares none -- so this resolves via the same converted pseudo-member - # path as UNRESOLVABLE-with-a-default does, not via a cached identity. + # No default can resolve on FilterType any more -- it has no + # registered members at all -- so even a value that is in-bounds + # (0x00-0xFF) for the *key* path, like ``0``, now falls through to + # the *original* ``UNRESOLVABLE`` error rather than resolving. before = len(FilterType.__members__) - resolved = FilterType.get(UNRESOLVABLE, 0) + with self.assertRaises(ValueError) as caught: + FilterType.get(UNRESOLVABLE, 0) + self.assertIn(str(UNRESOLVABLE), str(caught.exception)) + self.assertEqual(len(FilterType.__members__), before) + + # The uncached pseudo-member path itself is unaffected: as a *key* + # (no default supplied), ``0`` still resolves via ``cls(0)`` -> + # ``_missing_``, returning a fresh, equal-but-not-identical member + # every time rather than a cached identity -- the strength this + # test was written to add over the generic sweep, now demonstrated + # through ``key`` instead of ``default``. + resolved = FilterType.get(0) self.assertEqual(int(resolved), 0) self.assertEqual(resolved, FilterType(0)) self.assertIsNot(resolved, FilterType(0)) diff --git a/tests/const/test_const_registry_protocol.py b/tests/const/test_const_registry_protocol.py index 3e5b5c086e..f28bac0ee3 100644 --- a/tests/const/test_const_registry_protocol.py +++ b/tests/const/test_const_registry_protocol.py @@ -1291,36 +1291,65 @@ def test_identity_survives_an_ordinary_second_import(self) -> None: self.assertIs(module_again.NO_DEFAULT, first_import) def test_missing_name_with_default_negative_one_is_now_a_real_default(self) -> None: - """Fails on the pre-#857 tree: ``ExtensionHeader.get(, -1)`` - raised :exc:`KeyError` there, because ``-1 == NO_DEFAULT`` read the - caller's ``-1`` as *no default* and re-raised the name lookup's own - error. On this head it raises :exc:`ValueError` instead, for the - *attempted fallback* ``ExtensionHeader(-1)`` -- ``-1`` is now a - genuine default, and every registry's domain starts at ``0``, so it - is itself unresolvable. + """Coincidentally passes on the pre-#857 tree too, not just this one: + ``ExtensionHeader.get(, -1)`` raised :exc:`KeyError` there + as well, because ``-1 == NO_DEFAULT`` read the caller's ``-1`` as *no + default* and re-raised the name lookup's own error -- the same + exception type and content this test asserts, but for the wrong + reason (silently discarding an explicit default rather than + genuinely attempting and failing to resolve it). This test no longer + discriminates that defect; the pin for it is + :meth:`NoDefaultSentinelTests.test_no_default_is_not_equal_to_any_plausible_caller_value`, + which checks the sentinel's identity comparison directly, never + touches :meth:`~pcapkit.corekit.enum.EnumRegistry.get`, and so is + unaffected by #864 -- look there, not here, for that discrimination. + + ``-1`` is a genuine default here, not a rediscovered sentinel; it is + simply not a *resolvable* one, since #864 restricts + ``default`` to :attr:`~pcapkit.corekit.enum.EnumRegistry._value2member_map_`, + every registry's domain starts at ``0``, and there is no + ``cls(default)`` fallback left to attempt and raise a fresh + :exc:`ValueError` from. So the original name lookup's own + :exc:`KeyError` propagates instead, exactly as it would with no + default at all. Updated from the pre-#864 tree, which asserted + :exc:`ValueError` mentioning ``-1`` and not the original key -- that + assertion described ``cls(-1)`` failing, a code path #864 removes; it + is not weakened here, it is retargeted at the deliberate replacement + contract, and still pins that ``-1`` never mints anything. """ from pcapkit.const.ipv6.extension_header import ExtensionHeader before = len(ExtensionHeader.__members__) - with self.assertRaises(ValueError) as caught: + with self.assertRaises(KeyError) as caught: ExtensionHeader.get('Definitely-Not-A-Member', -1) - self.assertIn('-1', str(caught.exception)) - self.assertNotIn('Definitely-Not-A-Member', str(caught.exception)) + self.assertIn('Definitely-Not-A-Member', str(caught.exception)) self.assertEqual(before, len(ExtensionHeader.__members__)) def test_missing_name_with_default_negative_one_float_is_now_a_real_default(self) -> None: """The float case that actually motivates #857: ``-1.0 == -1`` is ``True``, so the old ``==`` comparison could not tell a caller's - ``-1.0`` apart from the ``-1`` marker either. Fails on the pre-#857 - tree with :exc:`KeyError` for the same reason as the ``-1`` case - above; raises :exc:`ValueError` here, for ``ExtensionHeader(-1.0)``. + ``-1.0`` apart from the ``-1`` marker either. Coincidentally passes + on the pre-#857 tree too, for the same reason its sibling test above + explains -- the old comparison read ``-1.0`` as the sentinel there, + producing the very same :exc:`KeyError` this asserts, for the wrong + reason. The discriminating pin for #857 lives in + :meth:`NoDefaultSentinelTests.test_no_default_is_not_equal_to_any_plausible_caller_value` + instead, not here -- see that sibling test's docstring for why. + + Raises :exc:`KeyError` here, post-#864, for the reason its sibling + test above explains: ``-1.0`` is not a registered value, ``default`` + no longer reaches ``cls(default)`` to fail on its own terms, and the + original name lookup's error propagates instead. Updated from the + pre-#864 tree, which asserted :exc:`ValueError` mentioning ``-1.0`` + for ``ExtensionHeader(-1.0)`` -- that path no longer exists; this is + the same no-weaker retargeting as above. """ from pcapkit.const.ipv6.extension_header import ExtensionHeader before = len(ExtensionHeader.__members__) - with self.assertRaises(ValueError) as caught: + with self.assertRaises(KeyError) as caught: ExtensionHeader.get('Definitely-Not-A-Member', -1.0) - self.assertIn('-1.0', str(caught.exception)) + self.assertIn('Definitely-Not-A-Member', str(caught.exception)) self.assertEqual(before, len(ExtensionHeader.__members__)) def test_missing_name_without_default_still_raises_on_an_int_enum(self) -> None: @@ -1348,5 +1377,132 @@ def test_missing_name_without_default_still_raises_on_an_int_flag(self) -> None: self.assertEqual(before, len(BindingUpdateFlag.__members__)) +class GetDefaultNoMintTests(unittest.TestCase): + """GitHub issue #864. Both ``cls(default)`` sites -- the ``str`` branch's + and the value branch's -- reached ``_missing_`` for a ``default`` that + fell inside a still-minting registry's own range, growing the registry + as a side effect of resolving ``default`` rather than ``key``. The + owner's ruling, verbatim: *"Take (b). Only register can mint. get should + not mint unless it falls through the ``_missing_``'s minted ranges."*, + and on the implementation, choosing option 1 of three: *"I think 1 is + correct mechanism we'd like."* -- ``default`` now resolves through a + plain ``_value2member_map_`` lookup only, so it cannot mint by + construction, while ``key`` resolution -- and whatever it lets + ``_missing_`` do -- is deliberately unchanged. + """ + + def setUp(self) -> None: + snapshot = snapshot_modules(ISOLATED_PREFIXES) + purge_modules(['pcapkit']) + self.addCleanup(restore_modules, snapshot, ISOLATED_PREFIXES) + + def test_the_issues_own_case_no_longer_mints_and_raises_instead(self) -> None: + """The exact reproduction from #864, on the real shipped registry. + Measured live on ``main`` before this fix:: + + >>> len(EtherType.__members__) + 160 + >>> EtherType.get(0x1234, 0x0888) + + >>> len(EtherType.__members__) + 161 + + ``0x1234`` (4660) falls in no ``_missing_`` range, so the lookup + falls to ``default``; ``0x0888`` (2184) falls inside the ``Xyplex`` + range #775's ruling deliberately kept minting -- but only when + reached as a ``key``. Fails on the pre-#864 tree for that reason; + passes here because ``default`` no longer reaches ``cls(default)`` + at all, so ``0x0888`` not being an *already-registered* value + propagates the ``0x1234`` lookup's own failure instead of minting + anything.""" + from pcapkit.const.reg.ethertype import EtherType + + before = len(EtherType.__members__) + self.assertEqual(before, 160) # the issue's own measured baseline + self.assertNotIn('Xyplex_0x0888', EtherType.__members__) + + with self.assertRaises(ValueError) as caught: + EtherType.get(0x1234, 0x0888) + self.assertIn('4660', str(caught.exception)) # 0x1234 -- the key + self.assertNotIn('2184', str(caught.exception)) # 0x0888 -- the default + + self.assertEqual(before, len(EtherType.__members__)) + self.assertNotIn('Xyplex_0x0888', EtherType.__members__) + + def test_key_path_minting_is_preserved(self) -> None: + """No-change guard: ``EtherType.get(0x0888)`` -- no ``default`` at + all -- still mints ``Xyplex_0x0888`` through its own ``_missing_``, + exactly as before. Ruling one permits this explicitly (*"get should + not mint unless it falls through the _missing_'s minted ranges"*), + and #864 closes only the ``default`` path, so this passes on both + the pre- and post-#864 tree -- ``key`` resolution is untouched.""" + from pcapkit.const.reg.ethertype import EtherType + + before = len(EtherType.__members__) + self.assertNotIn('Xyplex_0x0888', EtherType.__members__) + + result = EtherType.get(0x0888) + + self.assertEqual(result.name, 'Xyplex_0x0888') + self.assertEqual(result.value, 0x0888) + self.assertIn('Xyplex_0x0888', EtherType.__members__) + self.assertEqual(before + 1, len(EtherType.__members__)) + + def test_default_still_resolves_when_registered_int(self) -> None: + """The ruling's other half: a ``default`` that *is* already a member + still resolves, on an ``int``-valued (and, here, still-minting) + registry -- #864 restricts ``default`` to + ``_value2member_map_``, it does not disable it.""" + from pcapkit.const.reg.ethertype import EtherType + + target = EtherType.Internet_Protocol_version_4 + before = len(EtherType.__members__) + + result = EtherType.get(0x1234, target.value) + + self.assertIs(result, target) + self.assertEqual(before, len(EtherType.__members__)) + + def test_default_still_resolves_when_registered_str(self) -> None: + """As above, on a ``str``-valued registry -- synthetic, since no + shipped :class:`~aenum.StrEnum` registry mixes in the base yet (see + :data:`EXCLUDED_STILL_BESPOKE`).""" + class _Str(EnumRegistry, StrEnum): + KNOWN = 'known-value' + OTHER = 'other-value' + + result = _Str.get('totally-unknown-name', 'other-value') + + self.assertIs(result, _Str.OTHER) + self.assertEqual({'KNOWN', 'OTHER'}, set(_Str.__members__)) + + def test_default_naming_no_registered_member_raises_the_original_error(self) -> None: + """Ruling one's accepted cost, made concrete: a ``default`` inside a + declared-but-unassigned range used to resolve via ``cls(default)`` -> + ``_missing_``; now it simply does not resolve, on either branch, and + the *original* lookup error -- the one ``key`` itself would have + raised -- propagates rather than a new error about ``default``.""" + from pcapkit.const.reg.ethertype import EtherType + + before = len(EtherType.__members__) + + # Non-``str`` branch: the original error is ValueError, about ``key``. + with self.assertRaises(ValueError) as caught_value: + EtherType.get(0x1234, 0x0888) + self.assertIn('4660', str(caught_value.exception)) + self.assertNotIn('2184', str(caught_value.exception)) + + # ``str`` branch: the original error is KeyError, about ``key``. + class _Str(EnumRegistry, StrEnum): + KNOWN = 'known-value' + + with self.assertRaises(KeyError) as caught_key: + _Str.get('totally-unknown-name', 'also-not-a-member') + self.assertIn('totally-unknown-name', str(caught_key.exception)) + self.assertNotIn('also-not-a-member', str(caught_key.exception)) + + self.assertEqual(before, len(EtherType.__members__)) + + if __name__ == '__main__': unittest.main() From f23e9ed4d85f8cf735b2cbdc482103ddd2f08bb7 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sun, 27 Sep 2026 20:50:54 -0400 Subject: [PATCH 2/2] test(corekit): retarget the last #864 default-in-unassigned-range assertion tests/const/test_const_enum_no_mint.py's GetNoLongerMintsTests.test_unresolvable_string_key_with_default_in_unassigned_range was contended with #865 (both touched pcapkit/corekit/enum.py-adjacent const/vendor territory) and deferred to this follow-up commit; #865 has now merged, so it lands here rather than being applied separately. Hardware.get('Definitely-Not-A-Member', 40) used to resolve 40 -- a value in Hardware's declared-but-unassigned 39-255 range, not a registered member -- via cls(default) -> _missing_ to an unregistered pseudo-member. #864 restricts default to a plain _value2member_map_ lookup, so it no longer resolves: the original KeyError for the key propagates instead, exactly the accepted cost the owner's ruling names. Re-measured fresh on the current tree (after #866/#867 and #862/#865 both landed): Hardware.get('Definitely-Not-A-Member', 40) raises KeyError: 'Definitely-Not-A-Member', members unchanged at 42 before and after. Checked for overlap with #865's own additions to this file (ETHERTYPE_UNASSIGNED_PROBES, EtherTypeMixedMintTests, the retired test_masked_old_xerox_row_converts_by_source) -- none: those are all EtherType key-path minting-order probes, unrelated to Hardware's default-path resolution this test covers. GetNoLongerMintsTests remains the right class. --- tests/const/test_const_enum_no_mint.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/tests/const/test_const_enum_no_mint.py b/tests/const/test_const_enum_no_mint.py index 25ac037eaf..1aa7c66a36 100644 --- a/tests/const/test_const_enum_no_mint.py +++ b/tests/const/test_const_enum_no_mint.py @@ -657,15 +657,21 @@ def test_unresolvable_string_key_with_default_falls_back_by_value(self) -> None: self.assertEqual(before, len(Hardware.__members__)) def test_unresolvable_string_key_with_default_in_unassigned_range(self) -> None: - """The fallback is itself a value lookup, so a default landing in a - bounded-unassigned range returns the same kind of pseudo-member the - int path does -- rather than minting a member literally named after - the caller's unresolved key, which is what this used to do.""" + """The fallback is itself a value lookup (GitHub issue #864), so a + default landing in a bounded-unassigned range no longer returns the + pseudo-member the int path does -- it simply does not resolve, and + the original key's own error propagates instead. Before #864 this + resolved via ``cls(default)`` -> ``_missing_`` to an unregistered + pseudo-member; that is exactly the cost the owner's ruling accepts + ("a default naming a value with no registered member stops + resolving, on every registry... today such a default returns an + unregistered member via _missing_").""" from pcapkit.const.arp.hardware import Hardware before = len(Hardware.__members__) - result = Hardware.get('Definitely-Not-A-Member', 40) - self.assertEqual(result.value, 40) + with self.assertRaises(KeyError) as caught: + Hardware.get('Definitely-Not-A-Member', 40) + self.assertIn('Definitely-Not-A-Member', str(caught.exception)) self.assertEqual(before, len(Hardware.__members__)) self.assertNotIn('Definitely-Not-A-Member', Hardware.__members__)