From d019b8368429e61c2866e0a075959c2328aaf3e0 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 16 Oct 2026 19:57:27 -0400 Subject: [PATCH 1/2] fix(corekit): quiet get's fallback over an out-of-range value (#1845) - exceptions: add a _CaughtByCaller block, backed by a ContextVar, inside which a BaseError of a named class is constructed quiet; BaseError consults it alongside quiet=True. - enum: EnumLookup.get runs _validate_value and cls(key) in a _CaughtByCaller(ValueError) block when default is usable, so a generated override's loud rejection logs nothing and installs no hook there; with no usable default the miss stays loud. - enum: EnumRegistry.register validates in a block declaring nothing caught, so its rejection stays loud even when reached from inside get. - docs: get's docstring gains a 1.5.0 versionchanged and no longer says the value miss stays loud; registry-protocol.rst says what is reported now. - tests: cover quiet fallback, the loud paths, nesting and per-thread scope. Closes #1845 --- .../conventions/registry-protocol.rst | 42 ++- pcapkit/corekit/enum.py | 88 ++++-- pcapkit/utilities/exceptions.py | 70 ++++- .../test_enum_get_quiet_fallback_1845_unit.py | 259 ++++++++++++++++++ 4 files changed, 425 insertions(+), 34 deletions(-) create mode 100644 tests/corekit/test_enum_get_quiet_fallback_1845_unit.py diff --git a/docs/source/contributing/conventions/registry-protocol.rst b/docs/source/contributing/conventions/registry-protocol.rst index acb90f54a..607d4e4a2 100644 --- a/docs/source/contributing/conventions/registry-protocol.rst +++ b/docs/source/contributing/conventions/registry-protocol.rst @@ -161,7 +161,7 @@ Which branch a key takes, and what each miss ends in: GET -->|"otherwise"| VALUE["_validate_value(key)
then cls(key)"] NAME -->|"miss, no usable default"| KEYERR["EnumKeyError
quiet, derives KeyError"] NAME -->|"hit,
or miss, key is a registered value,
or miss, default is a registered value"| OK["canonical member"] - VALUE -->|"resolves, or \_missing\_ answers,
or ValueError, default is a registered value"| OK + VALUE -->|"resolves, or \_missing\_ answers,
or ValueError, default is a registered value
(that ValueError quiet)"| OK VALUE -->|"ValueError, no usable default"| VALERR["EnumValueError
loud, derives ValueError"] Do not "improve" on the shape by making both misses report identically. Converting @@ -172,19 +172,33 @@ base's :exc:`KeyError` into a :exc:`ValueError`, and :exc:`~pcapkit.utilities.exceptions.EnumValueError` for a name miss, so that the two ways of getting it wrong reported identically. -One asymmetry between the two is deliberate and is **not** visible from the -exception class: the **name** miss is raised quietly -(:class:`~pcapkit.utilities.exceptions.BaseError`'s ``quiet=True``, so nothing is -logged and :data:`sys.tracebacklimit` is left alone) while the **value** miss stays -loud. A name miss is in-library control flow at several call sites, and at -``Method.get`` it is part of a *successful* call -- -that override catches it in order to mint. A loud error there would put a -:data:`logging.CRITICAL` record on every such call and set -:data:`sys.tracebacklimit` to ``0`` process-wide, which is the -:issue:`362` defect ``quiet`` -exists for. So a ``get`` override that catches a name miss as control flow is -following the convention; one that catches a *value* miss that way is silencing a -logged error, and needs a reason. +One more thing is not visible from the exception class, and it is deliberate: +whether a miss is **reported**. A miss that ``get`` answers with ``default`` is +quiet, and so is every **name** miss, while a **value** miss with no usable +``default`` is loud. Quiet means what +:class:`~pcapkit.utilities.exceptions.BaseError`'s ``quiet=True`` means: nothing is +logged and no exception hook is installed. + +* **The name miss** is raised with ``quiet=True``. It is in-library control flow at + several call sites, and at ``Method.get`` it is part of a *successful* call -- + that override catches it in order to mint. +* **The value miss** is quiet only when ``default`` names a registered value. The + owner ruled for that on :issue:`1845`, on the ground that ``get`` is mostly called + from protocol parsing: a value ``get`` answers with ``default`` is input it has + handled, not a fault to report. In that case ``get`` runs ``_validate_value`` and + ``cls(key)`` in a block declaring that it catches :exc:`ValueError` itself, so the + loud :exc:`~pcapkit.utilities.exceptions.EnumValueError` a generated override + raises is constructed quietly without the override passing ``quiet=True``. With + no usable ``default`` the rejection reaches the caller and stays loud, and + :meth:`~pcapkit.corekit.enum.EnumRegistry.register` reports the same rejection + loudly even when reached from inside such a block. + +A loud error on either fallback would put a :data:`logging.CRITICAL` record on every +such call -- the :issue:`362` defect ``quiet`` exists for -- and, outside development +mode, install :mod:`pcapkit`'s exception hooks. So a ``get`` override that catches a +name miss as control flow is following the convention; one that calls ``get`` +without a ``default`` and catches the *value* miss is silencing a logged error, and +should pass a ``default`` instead. ``get`` Override Obligations ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/pcapkit/corekit/enum.py b/pcapkit/corekit/enum.py index db1a9c08c..d29b538c1 100644 --- a/pcapkit/corekit/enum.py +++ b/pcapkit/corekit/enum.py @@ -100,7 +100,7 @@ from aenum import extend_enum from pcapkit.corekit.sentinels import NO_DEFAULT -from pcapkit.utilities.exceptions import BaseError, EnumKeyError, EnumValueError +from pcapkit.utilities.exceptions import BaseError, EnumKeyError, EnumValueError, _CaughtByCaller if TYPE_CHECKING: from typing import Any @@ -198,9 +198,14 @@ def _validate_value(cls, value: 'Any') -> 'None': :meth:`get` path: :exc:`~pcapkit.utilities.exceptions.EnumValueError` subclasses :exc:`ValueError`, so a rejection here is caught by :meth:`get`'s own ``except ValueError`` and falls back to ``default`` - just as any other unresolvable value does. An override raising something - outside that hierarchy would instead propagate past ``default``, which is - a real difference in behaviour rather than a stylistic preference. With + just as any other unresolvable value does -- quietly, since :meth:`get` + calls this hook inside a + :class:`~pcapkit.utilities.exceptions._CaughtByCaller` block whenever it + has a usable ``default``, so an override need not pass ``quiet=True`` + for its rejection to log nothing there (:issue:`1845`). An override + raising something outside that hierarchy would instead propagate past + ``default``, unquieted, which is a real difference in behaviour rather + than a stylistic preference. With no usable ``default``, a rejection from this hook reaches the caller exactly as the override raised it -- :meth:`get` re-raises an in-library ``ValueError`` unchanged rather than re-wrapping it, so the override's own @@ -216,7 +221,10 @@ def _validate_value(cls, value: 'Any') -> 'None': every value present is legal by construction, so there is nothing left to validate. * :meth:`EnumRegistry.register`, before minting -- the one point at which - a caller can introduce a value no member carries yet. + a caller can introduce a value no member carries yet. Its rejection + stays loud even when ``register`` runs inside :meth:`get`'s quiet + block, since a refused mint is a fault to report rather than a + lookup falling back. Of the registries that override :meth:`EnumRegistry.register` to mint through a path of their own, @@ -335,6 +343,17 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': on the same terms; the ``str`` path does not call the hook, for the reason given on :meth:`_validate_value` itself. + When ``default`` names a registered value, the hook and ``cls(key)`` run + inside a :class:`~pcapkit.utilities.exceptions._CaughtByCaller` block for + :exc:`ValueError`, because every :exc:`ValueError` they raise is then + caught here and answered with ``default``. A loud + :exc:`~pcapkit.utilities.exceptions.EnumValueError` an override + constructs there is therefore quiet, though the override never passes + ``quiet=True`` -- the generated overrides cannot know which caller they + serve. An error of any other class still escapes this method, so it + stays loud. Without a usable ``default`` no block is entered, and the + rejection reaches the caller loud, as before. + Both failure paths raise from :mod:`pcapkit.utilities.exceptions` rather than a builtin, per the ruling recorded on GitHub issue :issue:`923`: in-library code raises from ``pcapkit.utilities.exceptions`` @@ -348,20 +367,23 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': These details are worth stating, since the exception type alone does not show them: - * **The name miss is raised quietly** -- - :exc:`~pcapkit.utilities.exceptions.EnumKeyError` with ``quiet=True``, - so nothing is logged and :data:`sys.tracebacklimit` is left alone. - That is not a cosmetic choice: this method's name miss is in-library - control flow, and at + * **A miss answered with** ``default`` **is quiet**, and so is every + name miss: nothing is logged and no exception hook is installed. + The name miss is :exc:`~pcapkit.utilities.exceptions.EnumKeyError` + with ``quiet=True``, because it is in-library control flow -- at :meth:`~pcapkit.const.http.method.Method.get` it is part of a - *successful* call -- that override catches it in order to build an - unregistered member. A loud - error there would put a :data:`logging.CRITICAL` record on every such - call and set :data:`sys.tracebacklimit` to ``0`` process-wide, which - is exactly the GitHub issue :issue:`362` defect + *successful* call, since that override catches it in order to build + an unregistered member. The value miss is quiet through the block + described above, and only when ``default`` is usable, as ruled on + :issue:`1845` on the ground that this method is mostly called from + protocol parsing: a value answered with ``default`` is input handled, + not a fault to report. A value miss with no usable ``default`` + reaches the caller, and stays loud. A loud error on either fallback + would put a :data:`logging.CRITICAL` record on every such call -- + the GitHub issue :issue:`362` defect :class:`~pcapkit.utilities.exceptions.BaseError` documents ``quiet`` - for. The value miss takes no such fallback anywhere in this tree, so - it stays loud. + for -- and, outside development mode, install :mod:`pcapkit`'s + exception hooks. * **An in-library rejection propagates unchanged.** A ``ValueError`` that is already a :exc:`~pcapkit.utilities.exceptions.BaseError` -- typically :exc:`~pcapkit.utilities.exceptions.EnumValueError` from a @@ -398,6 +420,12 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': default. A :exc:`KeyError`, so an ``except KeyError`` caller is unaffected. + .. versionchanged:: 1.5.0 + + A value miss answered with ``default`` is quiet: it logs nothing and + installs no exception hook, while :meth:`EnumRegistry.register` still + reports the same range rejection loudly (:issue:`1845`). + """ if isinstance(key, str): try: @@ -410,6 +438,20 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': quiet=True) from None return cls._value2member_map_[default] try: + usable = default is not NO_DEFAULT and default in cls._value2member_map_ + except TypeError: + # NOTE: an unhashable ``default`` names no member. Left to raise + # here, it would fail a lookup whose ``key`` resolves, which never + # reads ``default`` at all. + usable = False + try: + if usable: + # NOTE: GitHub issue #1845. Every ValueError raised in this block + # is answered with ``default`` below, so a loud one is control + # flow here, whoever constructed it. + with _CaughtByCaller(ValueError): + cls._validate_value(key) + return cls(key) # type: ignore[call-arg] cls._validate_value(key) return cls(key) # type: ignore[call-arg] except ValueError as error: @@ -506,6 +548,13 @@ def register(cls, value: 'Any', name: 'str') -> 'Self': for it than a range complaint would be, and validation is left to guard only the genuinely new value that is about to be minted. + That rejection is always loud. :meth:`~EnumLookup.get` quiets the same + hook when it has a ``default`` to fall back to, but a refused mint has + no fallback, so this calls the hook in a + :class:`~pcapkit.utilities.exceptions._CaughtByCaller` block declaring + that nothing is caught -- loud even if ``register`` is reached from + inside such a ``get`` (:issue:`1845`). + Args: value: Value of the new member. name: Name of the new member. Required rather than derived, which @@ -531,7 +580,10 @@ def register(cls, value: 'Any', name: 'str') -> 'Self': raise ValueError(f'{value!r} is already registered on {cls.__name__} as ' f'{existing.name!r}; use {cls.__name__}.register_alias() ' f'to add a further name for it') - cls._validate_value(value) + # NOTE: GitHub issue #1845. Declares that nothing is caught, so the + # rejection stays loud even inside a ``get`` block that would quiet it. + with _CaughtByCaller(): + cls._validate_value(value) return cls._extend(value, name) @classmethod diff --git a/pcapkit/utilities/exceptions.py b/pcapkit/utilities/exceptions.py index 338d08610..bf7f96de0 100644 --- a/pcapkit/utilities/exceptions.py +++ b/pcapkit/utilities/exceptions.py @@ -14,6 +14,7 @@ .. _tbtrim: https://github.com/gousaiyang/tbtrim """ +import contextvars import inspect import io import os @@ -158,6 +159,64 @@ def stacklevel() -> 'int': ############################################################################## +#: ContextVar[tuple[Type[BaseException], ...]]: The exception classes the +#: :mod:`pcapkit` code running in this context has undertaken to catch itself, +#: as declared by the innermost :class:`_CaughtByCaller` block. A +#: :class:`BaseError` that is an instance of one of them is constructed quiet, +#: whatever its own ``quiet`` says. +#: +#: A :class:`~contextvars.ContextVar` rather than a module global, so that +#: entering a block on one thread never changes what another thread reads, and +#: one thread's lookup cannot silence an unrelated error on another. +_caught_here = contextvars.ContextVar( + 'pcapkit.utilities.exceptions._caught_here', default=(), +) # type: contextvars.ContextVar[tuple[Type[BaseException], ...]] + + +class _CaughtByCaller: + """Construct errors of the given classes quietly for the duration of a block. + + For :mod:`pcapkit` code that is about to call something it does not own -- + a generated :meth:`~pcapkit.corekit.enum.EnumLookup._validate_value` + override, say -- and will catch the named exceptions itself. An error the + callee raises is then the quiet control flow it is at that call site, + without the callee having to pass ``quiet=True``, which it cannot know to + do (:issue:`1845`). Only the named classes are affected: anything else the + callee raises escapes the caller and so stays loud. + + Given no classes, it declares that nothing is caught, which makes errors + loud again inside an enclosing block -- how + :meth:`~pcapkit.corekit.enum.EnumRegistry.register` keeps its rejection + loud whoever calls it. + + Re-entrant: on exit a block restores whatever the enclosing block declared. + An instance holds the token of the one ``with`` it is entered by, so it is + built afresh for each. + + A class rather than :func:`contextlib.contextmanager`, because + :meth:`EnumLookup.get ` enters one on + every call that has a usable ``default``, and the generator machinery + measured at several times the cost of the lookup it would wrap. + + Args: + *types: Exception classes the caller catches. + + """ + + __slots__ = ('_types', '_token') + + def __init__(self, *types: 'Type[BaseException]') -> 'None': + self._types = types + + def __enter__(self) -> 'None': + # NOTE: set here rather than in ``__init__``, since a token exists only + # once the variable has been set, which is what entering the block does. + self._token = _caught_here.set(self._types) # pylint: disable=attribute-defined-outside-init + + def __exit__(self, *exc_info: 'object') -> 'None': + _caught_here.reset(self._token) + + class BaseError(Exception): """Base error class of all kinds. @@ -179,6 +238,12 @@ class BaseError(Exception): still a perfectly ordinary exception, carrying its message for whoever catches it. + An error is also quiet when it is constructed inside a + :class:`_CaughtByCaller` block naming one of its classes, which is how a + :mod:`pcapkit` caller marks as control flow an error raised by code that + does not pass ``quiet=True`` itself. The effect is exactly that of + ``quiet=True``. + Important: * The terseness is deliberately not done by setting @@ -213,8 +278,9 @@ class BaseError(Exception): """ def __init__(self, *args: 'Any', quiet: 'bool' = False, **kwargs: 'Any') -> 'None': - # log error -- a quiet error emits nothing and mutates nothing - if not quiet: + # log error -- a quiet error emits nothing and mutates nothing, and so + # does one a :class:`_CaughtByCaller` block says its caller catches + if not quiet and not isinstance(self, _caught_here.get()): if DEVMODE: logger.critical('%s: %s', type(self).__name__, str(self), exc_info=self if VERBOSE else False, diff --git a/tests/corekit/test_enum_get_quiet_fallback_1845_unit.py b/tests/corekit/test_enum_get_quiet_fallback_1845_unit.py new file mode 100644 index 000000000..155f87a4a --- /dev/null +++ b/tests/corekit/test_enum_get_quiet_fallback_1845_unit.py @@ -0,0 +1,259 @@ +# -*- coding: utf-8 -*- +"""GitHub issue #1845: ``get(value, default)`` falls back quietly; ``register`` stays loud. + +#1823 gave every generated registry a ``_validate_value`` override raising a loud +:exc:`~pcapkit.utilities.exceptions.EnumValueError`, and +:meth:`~pcapkit.corekit.enum.EnumLookup.get` calls that hook before ``cls(key)``. +So ``get(out_of_range, default)`` returned ``default`` but logged a +:data:`logging.CRITICAL` record and installed :mod:`pcapkit`'s exception hooks +on the way. The owner took the quiet option for ``get`` and kept ``register`` +loud. The quiet path lives in :mod:`pcapkit.corekit`, so the generated overrides +are untouched: ``get`` runs them inside a +:class:`~pcapkit.utilities.exceptions._CaughtByCaller` block, which makes a +:exc:`ValueError`-derived :class:`~pcapkit.utilities.exceptions.BaseError` +constructed there quiet. + +The generated registry under test is +:class:`~pcapkit.const.ipv4.tos_pre.ToSPrecedence`, whose override accepts +``0``-``7`` only. ``LLACode``, the registry the issue measured on, is avoided +because one of its members is being renamed in parallel (#1848). + +""" + +from __future__ import annotations + +import contextlib +import sys +import threading +import unittest +from typing import TYPE_CHECKING + +from aenum import IntEnum + +from pcapkit.const.ipv4.tos_pre import ToSPrecedence +from pcapkit.corekit.enum import EnumRegistry +from pcapkit.utilities import exceptions +from pcapkit.utilities.exceptions import EnumValueError, IntError +from pcapkit.utilities.logging import DEVMODE +from pcapkit.utilities.logging import logger as package_logger +from tests.utilities._harness import capture + +if TYPE_CHECKING: + from typing import Any, Iterator + + from tests.utilities._harness import Recorder + +__all__ = ['QuietFallbackTests', 'LoudPathTests', 'MechanismTests'] + +#: A value :class:`ToSPrecedence`'s ``_validate_value`` refuses: the field is 3 bits. +OUT_OF_RANGE = 8 + +#: The one record a loud rejection of :data:`OUT_OF_RANGE` leaves. +LOUD_RECORD = ('CRITICAL', f'EnumValueError: {OUT_OF_RANGE} is not a valid ToSPrecedence') + +#: Set by :class:`_Blocking`'s hook once it is parked inside ``get``. +_INSIDE = threading.Event() +#: Set by the test to let :class:`_Blocking`'s hook reject and return. +_RELEASE = threading.Event() + + +@contextlib.contextmanager +def _records() -> 'Iterator[tuple[Recorder, Recorder]]': + """Capture the package logger and, separately, the exceptions module's own.""" + with capture(package_logger) as package, capture(exceptions.logger) as module: + yield package, module + + +class _Nested(EnumRegistry, IntEnum): + """A registry whose range hook calls ``register`` on another one. + + Contrived on purpose: nothing in :mod:`pcapkit.const` does this, and it is + the only way ``register`` can run while a ``get`` is in progress. + + """ + + zero = 0 + + @classmethod + def _validate_value(cls, value: 'Any') -> 'None': + try: + ToSPrecedence.register(OUT_OF_RANGE, 'Nested_1845') + except ValueError: + pass + if not 0 <= value <= 7: + raise EnumValueError(f'{value!r} is not a valid {cls.__name__}') + + +class _Blocking(EnumRegistry, IntEnum): + """A registry whose range hook parks until told to reject.""" + + zero = 0 + + @classmethod + def _validate_value(cls, value: 'Any') -> 'None': + _INSIDE.set() + _RELEASE.wait(timeout=10) + raise EnumValueError(f'{value!r} is not a valid {cls.__name__}') + + +class _HookGuard(unittest.TestCase): + """Install sentinel exception hooks, and restore the real ones afterwards. + + A loud error outside development mode replaces :data:`sys.excepthook` and + :data:`threading.excepthook`, so the loud controls below would otherwise leak + :mod:`pcapkit`'s hooks into every later test. The sentinels also make "left + the hooks alone" an identity check rather than a guess about what was there. + + """ + + def setUp(self) -> 'None': + saved_sys, saved_threading = sys.excepthook, threading.excepthook + + def _restore() -> 'None': + sys.excepthook = saved_sys + threading.excepthook = saved_threading + + self.addCleanup(_restore) + + def _sys_sentinel(*args: 'Any') -> 'None': + saved_sys(*args) + + def _threading_sentinel(args: 'Any') -> 'None': + saved_threading(args) + + self.sys_sentinel = _sys_sentinel + self.threading_sentinel = _threading_sentinel + sys.excepthook = _sys_sentinel + threading.excepthook = _threading_sentinel + + +class QuietFallbackTests(_HookGuard): + """The fix: an out-of-range value with a usable ``default`` is silent.""" + + def test_returns_the_default(self) -> 'None': + self.assertIs(ToSPrecedence.get(OUT_OF_RANGE, 0), ToSPrecedence.Routine) + self.assertIs(ToSPrecedence.get(-1, ToSPrecedence.Flash.value), ToSPrecedence.Flash) + + def test_logs_nothing(self) -> 'None': + with _records() as (package, module): + ToSPrecedence.get(OUT_OF_RANGE, 0) + self.assertEqual(module.messages, [], + 'get() fell back to default and still logged from ' + 'pcapkit.utilities.exceptions') + self.assertEqual(package.messages, []) + + @unittest.skipIf(DEVMODE, 'a loud error installs no hook in development mode') + def test_leaves_the_exception_hooks_alone(self) -> 'None': + with _records(): + ToSPrecedence.get(OUT_OF_RANGE, 0) + self.assertIs(sys.excepthook, self.sys_sentinel, + 'get() fell back to default and still replaced sys.excepthook') + self.assertIs(threading.excepthook, self.threading_sentinel) + + def test_mints_nothing(self) -> 'None': + names, values = dict(ToSPrecedence._member_map_), dict(ToSPrecedence._value2member_map_) + with _records(): + ToSPrecedence.get(OUT_OF_RANGE, 0) + self.assertEqual(ToSPrecedence._member_map_, names) + self.assertEqual(ToSPrecedence._value2member_map_, values) + + +class LoudPathTests(_HookGuard): + """What stays as it was. These pass before the fix too: they guard against a + fix that quiets more than ``get``'s fallback.""" + + def _assert_one_loud_record(self, module: 'Recorder') -> 'None': + self.assertEqual(module.messages, [LOUD_RECORD]) + if not DEVMODE: + self.assertIs(sys.excepthook, exceptions._excepthook) + + def test_get_without_default_is_loud(self) -> 'None': + with _records() as (_, module): + with self.assertRaises(EnumValueError) as caught: + ToSPrecedence.get(OUT_OF_RANGE) + self.assertEqual(str(caught.exception), f'{OUT_OF_RANGE} is not a valid ToSPrecedence') + self._assert_one_loud_record(module) + + def test_get_with_an_unregistered_default_is_loud(self) -> 'None': + self.assertNotIn(99, ToSPrecedence._value2member_map_) + with _records() as (_, module): + with self.assertRaises(EnumValueError): + ToSPrecedence.get(OUT_OF_RANGE, 99) + self._assert_one_loud_record(module) + + def test_register_is_loud(self) -> 'None': + with _records() as (_, module): + with self.assertRaises(EnumValueError): + ToSPrecedence.register(OUT_OF_RANGE, 'Probe_1845') + self._assert_one_loud_record(module) + self.assertNotIn('Probe_1845', ToSPrecedence.__members__) + + def test_a_non_value_error_from_the_hook_is_loud(self) -> 'None': + """``get`` catches only :exc:`ValueError`, so only that is quieted.""" + + class _TypeRejecting(EnumRegistry, IntEnum): + zero = 0 + + @classmethod + def _validate_value(cls, value: 'Any') -> 'None': + raise IntError(f'{value!r} is not an int-like {cls.__name__}') + + with _records() as (_, module): + with self.assertRaises(IntError): + _TypeRejecting.get(5, 0) + self.assertEqual([level for level, _ in module.messages], ['CRITICAL']) + + +class MechanismTests(_HookGuard): + """The block is re-entrant, per thread, and cannot quiet ``register``.""" + + def test_register_reached_from_inside_get_stays_loud(self) -> 'None': + with _records() as (_, module): + self.assertIs(_Nested.get(99, 0), _Nested.zero) + self.assertEqual(module.messages, [LOUD_RECORD], + 'expected exactly the register() rejection, loud, with the ' + "get() fallback's own rejection quiet") + + def test_register_inside_a_quiet_block_stays_loud(self) -> 'None': + with _records() as (_, module): + with exceptions._CaughtByCaller(ValueError): + with self.assertRaises(EnumValueError): + ToSPrecedence.register(OUT_OF_RANGE, 'Block_1845') + self.assertEqual(module.messages, [LOUD_RECORD]) + + def test_blocks_nest_and_restore(self) -> 'None': + with _records() as (_, module): + with exceptions._CaughtByCaller(ValueError): + EnumValueError('outer, quiet') + with exceptions._CaughtByCaller(): + EnumValueError('inner, loud') + EnumValueError('outer again, quiet') + IntError('not a ValueError, loud') + EnumValueError('outside, loud') + self.assertEqual([message for _, message in module.messages], + ['EnumValueError: inner, loud', 'IntError: not a ValueError, loud', + 'EnumValueError: outside, loud']) + + def test_one_threads_block_does_not_quiet_another_thread(self) -> 'None': + _INSIDE.clear() + _RELEASE.clear() + result = [] # type: list[object] + worker = threading.Thread(target=lambda: result.append(_Blocking.get(5, 0))) + with _records() as (_, module): + worker.start() + try: + self.assertTrue(_INSIDE.wait(timeout=10), 'the worker never reached the hook') + # The worker is now parked inside get()'s quiet block. + EnumValueError('main thread, loud') + finally: + _RELEASE.set() + worker.join(timeout=10) + self.assertFalse(worker.is_alive()) + self.assertEqual(result, [_Blocking.zero]) + self.assertEqual([message for _, message in module.messages], + ['EnumValueError: main thread, loud'], + "the worker's fallback logged, or its block quieted the main thread") + + +if __name__ == '__main__': + unittest.main() From 2da6b1be78bef08a5cceeaf25638b5fef0d91832 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 16 Oct 2026 20:26:31 -0400 Subject: [PATCH 2/2] fix(corekit): make get quiet for an out-of-range value with or without a default (#1845) - enum: EnumLookup.get always runs _validate_value and cls(key) in the _CaughtByCaller(ValueError) block, and converts aenum's own miss into a quiet EnumValueError, so a raised value miss is not logged and installs no hook; register stays loud, including from inside get. - docs: get's docstring, versionchanged and registry-protocol.rst say every get miss is quiet and register is loud. - tests: flip the no-default and unusable-default cases to quiet, add the converted miss, get_all, uncaught-traceback and raising-get-reaches- register cases; the #923 loud control now constructs its error directly. Refs #1845 --- .../conventions/registry-protocol.rst | 56 +++---- pcapkit/corekit/enum.py | 115 +++++++------ pcapkit/utilities/exceptions.py | 2 +- ..._enum_get_exception_provenance_923_unit.py | 30 +++- .../test_enum_get_quiet_fallback_1845_unit.py | 152 +++++++++++++----- .../test_enum_lookup_reparent_930_unit.py | 2 +- 6 files changed, 221 insertions(+), 136 deletions(-) diff --git a/docs/source/contributing/conventions/registry-protocol.rst b/docs/source/contributing/conventions/registry-protocol.rst index 607d4e4a2..7833f7b17 100644 --- a/docs/source/contributing/conventions/registry-protocol.rst +++ b/docs/source/contributing/conventions/registry-protocol.rst @@ -106,13 +106,10 @@ What is easy to get wrong about it: ``get`` re-raises a :exc:`ValueError` that is already a :exc:`~pcapkit.utilities.exceptions.BaseError` exactly as the override raised it, and converts only ``aenum``'s and :mod:`enum`'s own "no member carries this - value". The consequences are the point of the discrimination rather - than side effects: the override's **own message** survives to the caller instead - of being replaced by the base's, and the error is **logged once** rather than - twice, since :class:`~pcapkit.utilities.exceptions.BaseError` logs on - construction and re-wrapping would construct a second one. So an override should - say in its message what it rejected and why; that message is what the caller - sees. + value". The consequence is the point of the discrimination rather than a side + effect: the override's **own message** survives to the caller instead of being + replaced by the base's. So an override should say in its message what it rejected + and why; that message is what the caller sees. * **A** ``str`` **key never reaches it.** That path never calls ``cls(key)``, so it resolves only against already-populated lookup tables, where every value present is legal by construction. ``register`` does call it, after its duplicate check. @@ -161,8 +158,8 @@ Which branch a key takes, and what each miss ends in: GET -->|"otherwise"| VALUE["_validate_value(key)
then cls(key)"] NAME -->|"miss, no usable default"| KEYERR["EnumKeyError
quiet, derives KeyError"] NAME -->|"hit,
or miss, key is a registered value,
or miss, default is a registered value"| OK["canonical member"] - VALUE -->|"resolves, or \_missing\_ answers,
or ValueError, default is a registered value
(that ValueError quiet)"| OK - VALUE -->|"ValueError, no usable default"| VALERR["EnumValueError
loud, derives ValueError"] + VALUE -->|"resolves, or \_missing\_ answers,
or ValueError, default is a registered value"| OK + VALUE -->|"ValueError, no usable default"| VALERR["EnumValueError
quiet, derives ValueError"] Do not "improve" on the shape by making both misses report identically. Converting one into the other is exactly what :issue:`923` retired, and it was retired in several @@ -173,32 +170,35 @@ base's :exc:`KeyError` into a :exc:`ValueError`, and ways of getting it wrong reported identically. One more thing is not visible from the exception class, and it is deliberate: -whether a miss is **reported**. A miss that ``get`` answers with ``default`` is -quiet, and so is every **name** miss, while a **value** miss with no usable -``default`` is loud. Quiet means what +whether a miss is **reported**. **Every miss** ``get`` **reports is quiet** -- by name +or by value, whether ``default`` answers it or it is raised -- while ``register`` +refusing the same value is loud. Quiet means what :class:`~pcapkit.utilities.exceptions.BaseError`'s ``quiet=True`` means: nothing is -logged and no exception hook is installed. +logged and no exception hook is installed. A raised miss is still an ordinary +exception that reaches the caller; left uncaught, it is printed by whichever hook is +installed, the interpreter's own unless an earlier loud error installed +:mod:`pcapkit`'s. * **The name miss** is raised with ``quiet=True``. It is in-library control flow at several call sites, and at ``Method.get`` it is part of a *successful* call -- that override catches it in order to mint. -* **The value miss** is quiet only when ``default`` names a registered value. The - owner ruled for that on :issue:`1845`, on the ground that ``get`` is mostly called - from protocol parsing: a value ``get`` answers with ``default`` is input it has - handled, not a fault to report. In that case ``get`` runs ``_validate_value`` and +* **The value miss** is quiet with or without a usable ``default``. The owner ruled + for that on :issue:`1845`, on the ground that ``get`` is mostly called from + protocol parsing, where a value that does not resolve is input for the caller to + handle rather than a fault to report. ``get`` runs ``_validate_value`` and ``cls(key)`` in a block declaring that it catches :exc:`ValueError` itself, so the loud :exc:`~pcapkit.utilities.exceptions.EnumValueError` a generated override - raises is constructed quietly without the override passing ``quiet=True``. With - no usable ``default`` the rejection reaches the caller and stays loud, and - :meth:`~pcapkit.corekit.enum.EnumRegistry.register` reports the same rejection - loudly even when reached from inside such a block. - -A loud error on either fallback would put a :data:`logging.CRITICAL` record on every -such call -- the :issue:`362` defect ``quiet`` exists for -- and, outside development -mode, install :mod:`pcapkit`'s exception hooks. So a ``get`` override that catches a -name miss as control flow is following the convention; one that calls ``get`` -without a ``default`` and catches the *value* miss is silencing a logged error, and -should pass a ``default`` instead. + raises is constructed quietly without the override passing ``quiet=True``, and + the error ``get`` converts from ``aenum``'s own is raised quiet as well. An error + of any other class is not a lookup miss, and stays loud. +* :meth:`~pcapkit.corekit.enum.EnumRegistry.register` **stays loud**, even when + reached from inside such a block: refusing a mint the caller explicitly asked for + is a fault to report, not a lookup that missed. + +A loud miss would put a :data:`logging.CRITICAL` record on every lookup that misses +-- the :issue:`362` defect ``quiet`` exists for -- and, outside development mode, +install :mod:`pcapkit`'s exception hooks. So catching either miss as control flow, in +a ``get`` override or anywhere else, follows the convention and silences nothing. ``get`` Override Obligations ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/pcapkit/corekit/enum.py b/pcapkit/corekit/enum.py index d29b538c1..4daf1b256 100644 --- a/pcapkit/corekit/enum.py +++ b/pcapkit/corekit/enum.py @@ -198,19 +198,17 @@ def _validate_value(cls, value: 'Any') -> 'None': :meth:`get` path: :exc:`~pcapkit.utilities.exceptions.EnumValueError` subclasses :exc:`ValueError`, so a rejection here is caught by :meth:`get`'s own ``except ValueError`` and falls back to ``default`` - just as any other unresolvable value does -- quietly, since :meth:`get` - calls this hook inside a - :class:`~pcapkit.utilities.exceptions._CaughtByCaller` block whenever it - has a usable ``default``, so an override need not pass ``quiet=True`` - for its rejection to log nothing there (:issue:`1845`). An override - raising something outside that hierarchy would instead propagate past + just as any other unresolvable value does. It is also quiet: + :meth:`get` always calls this hook inside a + :class:`~pcapkit.utilities.exceptions._CaughtByCaller` block for + :exc:`ValueError`, so an override need not pass ``quiet=True`` for its + rejection to log nothing there (:issue:`1845`). An override raising + something outside that hierarchy would instead propagate past ``default``, unquieted, which is a real difference in behaviour rather - than a stylistic preference. With - no usable ``default``, a rejection from this hook reaches the caller - exactly as the override raised it -- :meth:`get` re-raises an in-library - ``ValueError`` unchanged rather than re-wrapping it, so the override's own - message and the single log record it already emitted are what the caller - sees. + than a stylistic preference. With no usable ``default``, a rejection + from this hook reaches the caller exactly as the override raised it -- + :meth:`get` re-raises an in-library ``ValueError`` unchanged rather than + re-wrapping it, so the override's own message is what the caller sees. Called from the places below, and the omissions are deliberate: @@ -224,7 +222,7 @@ def _validate_value(cls, value: 'Any') -> 'None': a caller can introduce a value no member carries yet. Its rejection stays loud even when ``register`` runs inside :meth:`get`'s quiet block, since a refused mint is a fault to report rather than a - lookup falling back. + lookup that missed. Of the registries that override :meth:`EnumRegistry.register` to mint through a path of their own, @@ -343,16 +341,16 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': on the same terms; the ``str`` path does not call the hook, for the reason given on :meth:`_validate_value` itself. - When ``default`` names a registered value, the hook and ``cls(key)`` run - inside a :class:`~pcapkit.utilities.exceptions._CaughtByCaller` block for - :exc:`ValueError`, because every :exc:`ValueError` they raise is then - caught here and answered with ``default``. A loud + The hook and ``cls(key)`` run inside a + :class:`~pcapkit.utilities.exceptions._CaughtByCaller` block for + :exc:`ValueError`, because every :exc:`ValueError` they raise is this + method's own lookup miss: caught here, then answered with ``default`` or + raised to the caller as the miss. A loud :exc:`~pcapkit.utilities.exceptions.EnumValueError` an override constructs there is therefore quiet, though the override never passes ``quiet=True`` -- the generated overrides cannot know which caller they - serve. An error of any other class still escapes this method, so it - stays loud. Without a usable ``default`` no block is entered, and the - rejection reaches the caller loud, as before. + serve. An error of any other class is not a lookup miss and escapes + this method as it was raised, so it stays loud. Both failure paths raise from :mod:`pcapkit.utilities.exceptions` rather than a builtin, per the ruling recorded on GitHub issue @@ -367,30 +365,34 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': These details are worth stating, since the exception type alone does not show them: - * **A miss answered with** ``default`` **is quiet**, and so is every - name miss: nothing is logged and no exception hook is installed. - The name miss is :exc:`~pcapkit.utilities.exceptions.EnumKeyError` - with ``quiet=True``, because it is in-library control flow -- at + * **Every miss is quiet**, by name or by value, whether answered with + ``default`` or raised: nothing is logged and no exception hook is + installed. A raised miss still reaches the caller as an ordinary + exception; left uncaught, it is printed by whichever hook is installed + -- the interpreter's own, unless an earlier loud error installed + :mod:`pcapkit`'s. The name miss is + :exc:`~pcapkit.utilities.exceptions.EnumKeyError` with + ``quiet=True``, because it is in-library control flow -- at :meth:`~pcapkit.const.http.method.Method.get` it is part of a *successful* call, since that override catches it in order to build an unregistered member. The value miss is quiet through the block - described above, and only when ``default`` is usable, as ruled on - :issue:`1845` on the ground that this method is mostly called from - protocol parsing: a value answered with ``default`` is input handled, - not a fault to report. A value miss with no usable ``default`` - reaches the caller, and stays loud. A loud error on either fallback - would put a :data:`logging.CRITICAL` record on every such call -- - the GitHub issue :issue:`362` defect + described above, as ruled on :issue:`1845` on the ground that this + method is mostly called from protocol parsing: a value it cannot + resolve is input for the caller to handle, not a fault to report. A + loud miss would put a :data:`logging.CRITICAL` record on every such + call -- the GitHub issue :issue:`362` defect :class:`~pcapkit.utilities.exceptions.BaseError` documents ``quiet`` for -- and, outside development mode, install :mod:`pcapkit`'s - exception hooks. + exception hooks. :meth:`EnumRegistry.register` refusing the same + value stays loud. * **An in-library rejection propagates unchanged.** A ``ValueError`` that is already a :exc:`~pcapkit.utilities.exceptions.BaseError` -- typically :exc:`~pcapkit.utilities.exceptions.EnumValueError` from a subclass's :meth:`_validate_value` -- is re-raised as it stands rather - than wrapped, so the subclass's own message survives and the error is - logged once instead of twice. Only :mod:`aenum`'s and :mod:`enum`'s - own "no member carries this value" is converted. This is the same + than wrapped, so the subclass's own message survives and no second + error is constructed. Only :mod:`aenum`'s and :mod:`enum`'s own "no + member carries this value" is converted, into a quiet + :exc:`~pcapkit.utilities.exceptions.EnumValueError`. This is the same discrimination :meth:`EnumField.post_process ` already makes for the same reason. @@ -415,16 +417,17 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': rejection reaches the caller as, since that hook is documented to raise this very class and it is passed through rather than re-wrapped. A :exc:`ValueError`, so an - ``except ValueError`` caller is unaffected. + ``except ValueError`` caller is unaffected. Quiet. EnumKeyError: If a name does not resolve and there is no usable default. A :exc:`KeyError`, so an ``except KeyError`` caller is - unaffected. + unaffected. Quiet. .. versionchanged:: 1.5.0 - A value miss answered with ``default`` is quiet: it logs nothing and - installs no exception hook, while :meth:`EnumRegistry.register` still - reports the same range rejection loudly (:issue:`1845`). + A value miss is quiet, whether ``default`` answers it or it is + raised: it logs nothing and installs no exception hook, while + :meth:`EnumRegistry.register` still reports the same range + rejection loudly (:issue:`1845`). """ if isinstance(key, str): @@ -438,27 +441,18 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': quiet=True) from None return cls._value2member_map_[default] try: - usable = default is not NO_DEFAULT and default in cls._value2member_map_ - except TypeError: - # NOTE: an unhashable ``default`` names no member. Left to raise - # here, it would fail a lookup whose ``key`` resolves, which never - # reads ``default`` at all. - usable = False - try: - if usable: - # NOTE: GitHub issue #1845. Every ValueError raised in this block - # is answered with ``default`` below, so a loud one is control - # flow here, whoever constructed it. - with _CaughtByCaller(ValueError): - cls._validate_value(key) - return cls(key) # type: ignore[call-arg] - cls._validate_value(key) - return cls(key) # type: ignore[call-arg] + # NOTE: GitHub issue #1845. Every ValueError raised in this block is + # this method's own lookup miss -- answered with ``default`` below, + # or raised as the miss -- so a loud one is quiet here, whoever + # constructed it. + with _CaughtByCaller(ValueError): + cls._validate_value(key) + return cls(key) # type: ignore[call-arg] except ValueError as error: if default is NO_DEFAULT or default not in cls._value2member_map_: if isinstance(error, BaseError): raise - raise EnumValueError(str(error)) from error + raise EnumValueError(str(error), quiet=True) from error return cls._value2member_map_[default] @classmethod @@ -549,11 +543,12 @@ def register(cls, value: 'Any', name: 'str') -> 'Self': only the genuinely new value that is about to be minted. That rejection is always loud. :meth:`~EnumLookup.get` quiets the same - hook when it has a ``default`` to fall back to, but a refused mint has - no fallback, so this calls the hook in a + hook, because there the rejection is a lookup miss for the caller to + handle; here it refuses a mint the caller explicitly asked for, which + is a fault to report. So this calls the hook in a :class:`~pcapkit.utilities.exceptions._CaughtByCaller` block declaring that nothing is caught -- loud even if ``register`` is reached from - inside such a ``get`` (:issue:`1845`). + inside a ``get`` (:issue:`1845`). Args: value: Value of the new member. diff --git a/pcapkit/utilities/exceptions.py b/pcapkit/utilities/exceptions.py index bf7f96de0..276fa0743 100644 --- a/pcapkit/utilities/exceptions.py +++ b/pcapkit/utilities/exceptions.py @@ -195,7 +195,7 @@ class _CaughtByCaller: A class rather than :func:`contextlib.contextmanager`, because :meth:`EnumLookup.get ` enters one on - every call that has a usable ``default``, and the generator machinery + every call with a non-:class:`str` key, and the generator machinery measured at several times the cost of the lookup it would wrap. Args: diff --git a/tests/corekit/test_enum_get_exception_provenance_923_unit.py b/tests/corekit/test_enum_get_exception_provenance_923_unit.py index 33a51bbde..4e0d80db6 100644 --- a/tests/corekit/test_enum_get_exception_provenance_923_unit.py +++ b/tests/corekit/test_enum_get_exception_provenance_923_unit.py @@ -59,6 +59,7 @@ import pathlib import re import sys +import threading import unittest from typing import TYPE_CHECKING @@ -210,7 +211,7 @@ def test_an_in_library_rejection_is_passed_through_not_rewrapped(self) -> 'None' """A ``_validate_value`` override's own :exc:`~pcapkit.utilities.exceptions.EnumValueError` is what the caller sees, rather than a second one wrapping it -- so the override's message - survives and the error is logged once, not twice.""" + survives and no second error is constructed.""" with self.assertRaises(EnumValueError) as caught: _Ranged.get(99) self.assertIn('is not a valid _Ranged', str(caught.exception)) @@ -248,6 +249,9 @@ class BaseGetQuietnessTests(unittest.TestCase): ``quiet`` for, and :mod:`pcapkit.protocols.protocol` states the same rule inline: *"A pcapkit exception here would log at CRITICAL for something that is handled two lines later."* + + Since GitHub issue #1845 the value miss is quiet as well, so the control + that shows the recorder can see a loud error constructs one directly. """ def setUp(self) -> 'None': @@ -285,13 +289,29 @@ def test_the_minting_override_stays_silent_end_to_end(self) -> 'None': self.assertEqual(recorder.messages, []) self.assertEqual(minted.value, 'QUIET-923-PROBE') - def test_the_value_miss_is_loud_which_is_what_quiet_is_measured_against(self) -> 'None': - """The control. Without it ``recorder.messages == []`` above would also - pass if :class:`~pcapkit.utilities.exceptions.BaseError` had simply - stopped logging, rather than this one raise being quiet.""" + def test_the_value_miss_is_quiet_too(self) -> 'None': + """GitHub issue #1845: the owner ruled the value miss quiet as well, so + this was the loud control until then. It is still raised -- only not + reported.""" with capture(logger) as recorder: with self.assertRaises(EnumValueError): _Closed.get(99) + self.assertEqual(recorder.messages, []) + + def test_a_loud_error_still_logs_which_is_what_quiet_is_measured_against(self) -> 'None': + """The control. Without it ``recorder.messages == []`` above would also + pass if :class:`~pcapkit.utilities.exceptions.BaseError` had simply + stopped logging, rather than these raises being quiet. Constructed + directly, now that no ``get`` miss is loud.""" + saved_sys, saved_threading = sys.excepthook, threading.excepthook + + def _restore() -> 'None': + sys.excepthook = saved_sys + threading.excepthook = saved_threading + + self.addCleanup(_restore) + with capture(logger) as recorder: + EnumValueError('99 is not a valid _Closed') self.assertEqual([level for level, _ in recorder.messages], ['CRITICAL']) self.assertIn('EnumValueError', recorder.messages[0][1]) diff --git a/tests/corekit/test_enum_get_quiet_fallback_1845_unit.py b/tests/corekit/test_enum_get_quiet_fallback_1845_unit.py index 155f87a4a..9fe3ed465 100644 --- a/tests/corekit/test_enum_get_quiet_fallback_1845_unit.py +++ b/tests/corekit/test_enum_get_quiet_fallback_1845_unit.py @@ -1,16 +1,17 @@ # -*- coding: utf-8 -*- -"""GitHub issue #1845: ``get(value, default)`` falls back quietly; ``register`` stays loud. +"""GitHub issue #1845: ``get`` is quiet for a value it cannot resolve; ``register`` is loud. #1823 gave every generated registry a ``_validate_value`` override raising a loud :exc:`~pcapkit.utilities.exceptions.EnumValueError`, and :meth:`~pcapkit.corekit.enum.EnumLookup.get` calls that hook before ``cls(key)``. So ``get(out_of_range, default)`` returned ``default`` but logged a :data:`logging.CRITICAL` record and installed :mod:`pcapkit`'s exception hooks -on the way. The owner took the quiet option for ``get`` and kept ``register`` -loud. The quiet path lives in :mod:`pcapkit.corekit`, so the generated overrides -are untouched: ``get`` runs them inside a -:class:`~pcapkit.utilities.exceptions._CaughtByCaller` block, which makes a -:exc:`ValueError`-derived :class:`~pcapkit.utilities.exceptions.BaseError` +on the way. The owner ruled ``get`` quiet for such a value, with a usable +``default`` or without one -- without, the miss is still raised, just not +reported -- and kept ``register`` loud. The quiet path lives in +:mod:`pcapkit.corekit`, so the generated overrides are untouched: ``get`` runs +them inside a :class:`~pcapkit.utilities.exceptions._CaughtByCaller` block, which +makes a :exc:`ValueError`-derived :class:`~pcapkit.utilities.exceptions.BaseError` constructed there quiet. The generated registry under test is @@ -23,7 +24,10 @@ from __future__ import annotations import contextlib +import os +import subprocess # nosec B404 import sys +import textwrap import threading import unittest from typing import TYPE_CHECKING @@ -31,7 +35,7 @@ from aenum import IntEnum from pcapkit.const.ipv4.tos_pre import ToSPrecedence -from pcapkit.corekit.enum import EnumRegistry +from pcapkit.corekit.enum import EnumLookup, EnumRegistry from pcapkit.utilities import exceptions from pcapkit.utilities.exceptions import EnumValueError, IntError from pcapkit.utilities.logging import DEVMODE @@ -43,13 +47,20 @@ from tests.utilities._harness import Recorder -__all__ = ['QuietFallbackTests', 'LoudPathTests', 'MechanismTests'] +__all__ = ['QuietFallbackTests', 'QuietRaiseTests', 'LoudPathTests', 'MechanismTests'] #: A value :class:`ToSPrecedence`'s ``_validate_value`` refuses: the field is 3 bits. OUT_OF_RANGE = 8 +#: The message every rejection of :data:`OUT_OF_RANGE` carries. +MESSAGE = f'{OUT_OF_RANGE} is not a valid ToSPrecedence' + #: The one record a loud rejection of :data:`OUT_OF_RANGE` leaves. -LOUD_RECORD = ('CRITICAL', f'EnumValueError: {OUT_OF_RANGE} is not a valid ToSPrecedence') +LOUD_RECORD = ('CRITICAL', f'EnumValueError: {MESSAGE}') + +#: Repository root, handed to a child interpreter as ``PYTHONPATH`` so it imports +#: this tree rather than whatever :mod:`pcapkit` is installed. +REPO_ROOT = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) #: Set by :class:`_Blocking`'s hook once it is parked inside ``get``. _INSIDE = threading.Event() @@ -64,6 +75,12 @@ def _records() -> 'Iterator[tuple[Recorder, Recorder]]': yield package, module +class _Sparse(EnumLookup, IntEnum): + """No range hook and no ``_missing_``: a miss is :mod:`aenum`'s own error.""" + + one = 1 + + class _Nested(EnumRegistry, IntEnum): """A registry whose range hook calls ``register`` on another one. @@ -126,29 +143,26 @@ def _threading_sentinel(args: 'Any') -> 'None': sys.excepthook = _sys_sentinel threading.excepthook = _threading_sentinel + def assert_quiet(self, package: 'Recorder', module: 'Recorder') -> 'None': + """Nothing logged anywhere in :mod:`pcapkit`, and both hooks untouched.""" + self.assertEqual(module.messages, [], + 'get() logged from pcapkit.utilities.exceptions') + self.assertEqual(package.messages, []) + self.assertIs(sys.excepthook, self.sys_sentinel, 'get() replaced sys.excepthook') + self.assertIs(threading.excepthook, self.threading_sentinel) + class QuietFallbackTests(_HookGuard): - """The fix: an out-of-range value with a usable ``default`` is silent.""" + """An out-of-range value with a usable ``default`` is answered silently.""" def test_returns_the_default(self) -> 'None': self.assertIs(ToSPrecedence.get(OUT_OF_RANGE, 0), ToSPrecedence.Routine) self.assertIs(ToSPrecedence.get(-1, ToSPrecedence.Flash.value), ToSPrecedence.Flash) - def test_logs_nothing(self) -> 'None': + def test_logs_nothing_and_leaves_the_hooks_alone(self) -> 'None': with _records() as (package, module): ToSPrecedence.get(OUT_OF_RANGE, 0) - self.assertEqual(module.messages, [], - 'get() fell back to default and still logged from ' - 'pcapkit.utilities.exceptions') - self.assertEqual(package.messages, []) - - @unittest.skipIf(DEVMODE, 'a loud error installs no hook in development mode') - def test_leaves_the_exception_hooks_alone(self) -> 'None': - with _records(): - ToSPrecedence.get(OUT_OF_RANGE, 0) - self.assertIs(sys.excepthook, self.sys_sentinel, - 'get() fell back to default and still replaced sys.excepthook') - self.assertIs(threading.excepthook, self.threading_sentinel) + self.assert_quiet(package, module) def test_mints_nothing(self) -> 'None': names, values = dict(ToSPrecedence._member_map_), dict(ToSPrecedence._value2member_map_) @@ -158,38 +172,85 @@ def test_mints_nothing(self) -> 'None': self.assertEqual(ToSPrecedence._value2member_map_, values) -class LoudPathTests(_HookGuard): - """What stays as it was. These pass before the fix too: they guard against a - fix that quiets more than ``get``'s fallback.""" +class QuietRaiseTests(_HookGuard): + """With no usable ``default`` the miss is still raised -- only not reported.""" - def _assert_one_loud_record(self, module: 'Recorder') -> 'None': - self.assertEqual(module.messages, [LOUD_RECORD]) - if not DEVMODE: - self.assertIs(sys.excepthook, exceptions._excepthook) - - def test_get_without_default_is_loud(self) -> 'None': - with _records() as (_, module): + def test_get_without_default_raises_quietly(self) -> 'None': + with _records() as (package, module): with self.assertRaises(EnumValueError) as caught: ToSPrecedence.get(OUT_OF_RANGE) - self.assertEqual(str(caught.exception), f'{OUT_OF_RANGE} is not a valid ToSPrecedence') - self._assert_one_loud_record(module) + self.assertEqual(str(caught.exception), MESSAGE) + self.assert_quiet(package, module) - def test_get_with_an_unregistered_default_is_loud(self) -> 'None': + def test_get_with_an_unregistered_default_raises_quietly(self) -> 'None': self.assertNotIn(99, ToSPrecedence._value2member_map_) - with _records() as (_, module): - with self.assertRaises(EnumValueError): + with _records() as (package, module): + with self.assertRaises(EnumValueError) as caught: ToSPrecedence.get(OUT_OF_RANGE, 99) - self._assert_one_loud_record(module) + self.assertEqual(str(caught.exception), MESSAGE) + self.assert_quiet(package, module) + + def test_get_all_raises_quietly(self) -> 'None': + with _records() as (package, module): + with self.assertRaises(EnumValueError): + ToSPrecedence.get_all(OUT_OF_RANGE) + self.assert_quiet(package, module) + + def test_a_converted_enumeration_miss_is_quiet(self) -> 'None': + """``get`` converts :mod:`aenum`'s own miss, outside the block, so the + error it constructs has to be quiet in its own right.""" + with _records() as (package, module): + with self.assertRaises(EnumValueError) as caught: + _Sparse.get(5) + self.assertNotIsInstance(caught.exception.__cause__, exceptions.BaseError) + self.assertIsInstance(caught.exception.__cause__, ValueError) + self.assert_quiet(package, module) + + def test_an_uncaught_miss_prints_the_interpreters_own_traceback(self) -> 'None': + """Quiet installs no hook, so the default one prints the full traceback. + + Run in a fresh interpreter, because an uncaught exception is the one + thing a test cannot raise in its own process. :mod:`pcapkit`'s stderr + handler is attached there so that a loud error would also show up as a + ``[CRITICAL]`` line. + + """ + code = textwrap.dedent(f''' + import pcapkit + assert pcapkit.__file__.startswith({REPO_ROOT!r}), pcapkit.__file__ + from pcapkit.utilities.logging import handler, logger + logger.addHandler(handler) + from pcapkit.const.ipv4.tos_pre import ToSPrecedence + ToSPrecedence.get({OUT_OF_RANGE}) + ''') + env = dict(os.environ, PCAPKIT_DEVMODE='0', PYTHONSAFEPATH='1', PYTHONPATH=REPO_ROOT) + proc = subprocess.run([sys.executable, '-c', code], capture_output=True, # nosec B603 + text=True, env=env, timeout=60, check=False) + lines = proc.stderr.strip().splitlines() + self.assertEqual(proc.returncode, 1, proc.stderr) + self.assertNotIn('[CRITICAL]', proc.stderr, 'the miss was logged') + self.assertEqual(lines[0], 'Traceback (most recent call last):', + "pcapkit's terse hook printed the miss, so one was installed") + self.assertTrue(any('corekit' in line and 'enum.py' in line for line in lines), + 'the traceback has no frames inside get()') + self.assertEqual(lines[-1], f'pcapkit.utilities.exceptions.EnumValueError: {MESSAGE}') + + +class LoudPathTests(_HookGuard): + """What stays loud. These pass before the fix too: they guard against a fix + that quiets more than ``get``'s own lookup miss.""" def test_register_is_loud(self) -> 'None': with _records() as (_, module): with self.assertRaises(EnumValueError): ToSPrecedence.register(OUT_OF_RANGE, 'Probe_1845') - self._assert_one_loud_record(module) + self.assertEqual(module.messages, [LOUD_RECORD]) + if not DEVMODE: + self.assertIs(sys.excepthook, exceptions._excepthook) self.assertNotIn('Probe_1845', ToSPrecedence.__members__) def test_a_non_value_error_from_the_hook_is_loud(self) -> 'None': - """``get`` catches only :exc:`ValueError`, so only that is quieted.""" + """``get`` catches only :exc:`ValueError`, so only that is a lookup miss.""" class _TypeRejecting(EnumRegistry, IntEnum): zero = 0 @@ -214,6 +275,15 @@ def test_register_reached_from_inside_get_stays_loud(self) -> 'None': 'expected exactly the register() rejection, loud, with the ' "get() fallback's own rejection quiet") + def test_register_reached_from_a_raising_get_stays_loud(self) -> 'None': + with _records() as (_, module): + with self.assertRaises(EnumValueError) as caught: + _Nested.get(99) + self.assertEqual(str(caught.exception), '99 is not a valid _Nested') + self.assertEqual(module.messages, [LOUD_RECORD], + 'expected exactly the register() rejection, loud, with the ' + "miss get() raised quiet") + def test_register_inside_a_quiet_block_stays_loud(self) -> 'None': with _records() as (_, module): with exceptions._CaughtByCaller(ValueError): diff --git a/tests/corekit/test_enum_lookup_reparent_930_unit.py b/tests/corekit/test_enum_lookup_reparent_930_unit.py index 213724054..4c54d535d 100644 --- a/tests/corekit/test_enum_lookup_reparent_930_unit.py +++ b/tests/corekit/test_enum_lookup_reparent_930_unit.py @@ -623,7 +623,7 @@ class NonCanonicalKeyConvergenceTests(unittest.TestCase): (:mod:`pcapkit.corekit.enum`) branches on ``isinstance(key, str)`` instead, so the same key falls through to :meth:`~pcapkit.corekit.enum.EnumLookup._validate_value` and the - constructor, reaching :meth:`_missing_` and answering with a loud + constructor, reaching :meth:`_missing_` and answering with an :exc:`~pcapkit.utilities.exceptions.EnumValueError`. Deleting the overrides removes the branch that disagreed, so all seven now answer both keys the same way -- measured here rather than assumed, since