diff --git a/changelog/1372.bugfix.rst b/changelog/1372.bugfix.rst new file mode 100644 index 00000000..5c69317a --- /dev/null +++ b/changelog/1372.bugfix.rst @@ -0,0 +1,3 @@ +Warnings crossing to the controller now keep the state they carry beside their ``args``. + +Only ``args`` was transferred, and the warning was rebuilt by calling its class with them, so a warning deriving its message from its own fields arrived rendered from defaults -- reading like a legitimate message while saying something else. Whatever the instance keeps, in its dictionary or in ``__slots__``, is transferred now, and the instance is rebuilt without re-running ``__init__``; when that state cannot be serialized, the class is dropped rather than rebuilt without it. diff --git a/src/xdist/remote.py b/src/xdist/remote.py index 4d7c9267..2b6d3e89 100644 --- a/src/xdist/remote.py +++ b/src/xdist/remote.py @@ -324,6 +324,43 @@ def pytest_warning_recorded( ) +def _serializable_warning_state( + message: Warning, +) -> tuple[dict[str, Any] | None, bool]: + """Return `(state, lost)` for everything the instance keeps beside its `args`. + + Read from the instance rather than from `__reduce__`. `BaseException.__reduce__` + reports only `self.__dict__`, so a `__slots__` class reduces to a two-tuple and + its slots would read as "no state"; and a class that replaces `__reduce__` + would read as "state we cannot send" even when it keeps nothing at all. + + `lost` is the half that matters: rebuilding the class without the attributes + its own `__str__` reads raises `AttributeError` when the controller renders + the warning, so the caller must be able to tell "keeps nothing" from "keeps + something that will not cross". + """ + state: dict[str, Any] = {} + state.update(getattr(message, "__dict__", None) or {}) + for klass in type(message).__mro__: + slots = getattr(klass, "__slots__", ()) + if isinstance(slots, str): + slots = (slots,) + for name in slots: + if name in ("__dict__", "__weakref__"): + continue + try: + state[name] = getattr(message, name) + except AttributeError: + continue + if not state: + return None, False + try: + execnet.dumps(state) + except execnet.DumpError: + return None, True + return state, False + + def serialize_warning_message( warning_message: warnings.WarningMessage, ) -> dict[str, Any]: @@ -339,11 +376,19 @@ def serialize_warning_message( message_args = None else: message_args = warning_message.message.args + # `args` alone does not describe a warning. Sending only `args` drops + # whatever the instance keeps beside them, and the controller then rebuilds + # something that renders plausibly and is not the same warning. + message_state, message_state_lost = _serializable_warning_state( + warning_message.message + ) else: message_str = warning_message.message message_module = None message_class_name = None message_args = None + message_state = None + message_state_lost = False if warning_message.category: category_module = warning_message.category.__module__ category_class_name = warning_message.category.__name__ @@ -356,6 +401,8 @@ def serialize_warning_message( "message_module": message_module, "message_class_name": message_class_name, "message_args": message_args, + "message_state": message_state, + "message_state_lost": message_state_lost, "category_module": category_module, "category_class_name": category_class_name, } diff --git a/src/xdist/workermanage.py b/src/xdist/workermanage.py index c54b18fb..3e1947ff 100644 --- a/src/xdist/workermanage.py +++ b/src/xdist/workermanage.py @@ -483,11 +483,20 @@ def unserialize_warning_message(data: dict[str, Any]) -> warnings.WarningMessage mod = importlib.import_module(data["message_module"]) cls = getattr(mod, data["message_class_name"]) message = None - if data["message_args"] is not None: + if data["message_args"] is not None and not data["message_state_lost"]: + # Rebuilt without running `__init__`: a warning is free to derive its + # message from its own fields, and calling the class with the args would + # hand it back its own rendered text as if it were input. try: - message = cls(*data["message_args"]) - except TypeError: - pass + message = cls.__new__(cls) + BaseException.__init__(message, *data["message_args"]) + state = data["message_state"] + if state is not None: + # `__setstate__` sets one attribute per key, which reaches + # slots as well as the instance dictionary. + message.__setstate__(state) + except Exception: + message = None if message is None: # could not recreate the original warning instance; # create a generic Warning instance with the original diff --git a/testing/acceptance_test.py b/testing/acceptance_test.py index 4bd7039e..fdaa8705 100644 --- a/testing/acceptance_test.py +++ b/testing/acceptance_test.py @@ -867,6 +867,73 @@ def test_func(request): result = pytester.runpytest(n) result.stdout.fnmatch_lines(["*MyWarning*", "*1 passed, 1 warning*"]) + @pytest.mark.parametrize("n", ["-n0", "-n1"]) + def test_state_beyond_args_survives( + self, pytester: pytest.Pytester, n: str + ) -> None: + """A warning keeping state beside its args must report the same either way. + + `-n0` is the reference: no serialization happens, so whatever it prints is + what the warning says. `-n1` must match it. The class below renders its + message from `self.code`, which `args` does not carry, so rebuilding it by + calling the class reports the default instead — and reads like a real + message while doing it. + """ + pytester.makepyfile( + """ + import warnings + + class CodeWarning(UserWarning): + + def __init__(self, code=None): + self.code = code + super().__init__() + + def __str__(self): + return "code {} tripped".format(self.code or "unknown") + + def test_func(): + warnings.warn(CodeWarning(42)) + """ + ) + pytester.syspathinsert() + result = pytester.runpytest(n) + result.stdout.fnmatch_lines(["*code 42 tripped*", "*1 passed, 1 warning*"]) + result.stdout.no_fnmatch_line("*code unknown tripped*") + + @pytest.mark.parametrize("n", ["-n0", "-n1"]) + def test_state_in_slots_survives(self, pytester: pytest.Pytester, n: str) -> None: + """A warning keeping its state in slots must report the same either way. + + `BaseException.__reduce__` reports only the instance dictionary, so slots + read as "this warning keeps nothing". Rebuilding the class without running + `__init__` then leaves the slot unset, and the `AttributeError` lands where + the controller renders the warning -- an INTERNALERROR, not a wrong message. + """ + pytester.makepyfile( + """ + import warnings + + class SlotsWarning(UserWarning): + + __slots__ = ("code",) + + def __init__(self, code=None): + self.code = code + super().__init__() + + def __str__(self): + return "code {} tripped".format(self.code or "unknown") + + def test_func(): + warnings.warn(SlotsWarning(11)) + """ + ) + pytester.syspathinsert() + result = pytester.runpytest(n) + result.stdout.fnmatch_lines(["*code 11 tripped*", "*1 passed, 1 warning*"]) + result.stdout.no_fnmatch_line("*INTERNALERROR*") + @pytest.mark.parametrize("n", ["-n0", "-n1"]) def test_unserializable_arguments(self, pytester: pytest.Pytester, n: str) -> None: """Check that warnings with unserializable arguments are handled correctly (#349).""" diff --git a/testing/test_workermanage.py b/testing/test_workermanage.py index 4b393150..a355604d 100644 --- a/testing/test_workermanage.py +++ b/testing/test_workermanage.py @@ -1,8 +1,10 @@ from __future__ import annotations +from collections.abc import Callable from pathlib import Path import shutil import textwrap +from typing import Any import warnings import execnet @@ -497,6 +499,145 @@ class MyWarning2(UserWarning): assert v1 == v2 +class WarningWithFieldFirst(UserWarning): + """First argument is a field; the message is rendered from it.""" + + def __init__(self, resource: str) -> None: + self.resource = resource + super().__init__(f"{resource!r} is not available") + + +class WarningWithSlots(UserWarning): + """Keeps its state in slots, which `BaseException.__reduce__` never reports.""" + + __slots__ = ("code",) + + def __init__(self, code: int | None = None) -> None: + self.code = code + super().__init__() + + def __str__(self) -> str: + return f"code {self.code or 'unknown'} tripped" + + +class WarningWithoutArgs(UserWarning): + """No args at all; the text is derived from state.""" + + def __init__(self, code: int | None = None) -> None: + self.code = code + super().__init__() + + def __str__(self) -> str: + return f"code {self.code or 'unknown'} tripped" + + +def _rebuild_with_own_callable(code: int) -> WarningWithCustomReduce: + return WarningWithCustomReduce(code) + + +class WarningWithCustomReduce(UserWarning): + """Replaces `__reduce__`, and carries its state inside the *args*.""" + + def __init__(self, code: int | None = None) -> None: + self.code = code + super().__init__() + + def __str__(self) -> str: + return f"code {self.code or 'unknown'} tripped" + + def __reduce__(self) -> tuple[Any, ...]: + return (_rebuild_with_own_callable, (self.code,)) + + +@pytest.mark.parametrize( + ("factory", "expected_text", "attribute", "expected_value"), + [ + ( + lambda: WarningWithFieldFirst("gpu"), + "'gpu' is not available", + "resource", + "gpu", + ), + (lambda: WarningWithoutArgs(42), "code 42 tripped", "code", 42), + (lambda: WarningWithSlots(11), "code 11 tripped", "code", 11), + (lambda: WarningWithCustomReduce(7), "code 7 tripped", "code", 7), + ], + ids=["field-first", "state-only", "slots", "custom-reduce"], +) +def test_warning_state_survives_the_round_trip( + factory: Callable[[], UserWarning], + expected_text: str, + attribute: str, + expected_value: object, +) -> None: + """The rebuilt warning must be the same warning, not one that reads like it. + + Calling the class with its own `args` re-runs `__init__`, which for these two + shapes either renders around the rendered text or falls back to a default. + Both survive `pickle` and `copy` untouched, so the loss was ours. + """ + with pytest.warns(UserWarning) as w: + warnings.warn(factory()) + + assert len(w) == 1 + w_msg = w[0] + assert str(w_msg.message) == expected_text + + data = serialize_warning_message(w_msg) + rebuilt = unserialize_warning_message(data).message + + assert type(rebuilt) is type(w_msg.message) + assert str(rebuilt) == expected_text + assert getattr(rebuilt, attribute) == expected_value + + +class WarningWithUntransferableState(UserWarning): + """Keeps something `execnet.dumps` will not carry.""" + + def __init__(self, code: int | None = None) -> None: + self.code = code + self.handle = lambda: None + super().__init__() + + def __str__(self) -> str: + return f"code {self.code or 'unknown'} tripped" + + +def test_state_that_cannot_transfer_falls_back_to_a_plain_warning() -> None: + """Rebuilding the class without its state is worse than not rebuilding it. + + This shape keeps state its own `__str__` reads, and that state is not + serializable. Rebuilding the class and applying no state produces an instance + missing the attribute, and the controller raises `AttributeError` the moment it + renders the warning -- a failure introduced while fixing a rendering defect. + So the class is dropped and the text is reported once. + """ + with pytest.warns(UserWarning) as w: + warnings.warn(WarningWithUntransferableState(9)) + + original = w[0].message + rebuilt = unserialize_warning_message(serialize_warning_message(w[0])).message + + assert type(rebuilt) is Warning + assert str(original) in str(rebuilt) + + +def test_a_warning_keeping_no_state_is_still_rebuilt_exactly() -> None: + """The fallback must not widen to warnings that have nothing to lose. + + A warning holding nothing beside its `args` reduces to a two-tuple. There is + no state, so there is nothing to fail to transfer, and dropping its class + would cost the caller its type for no reason. + """ + with pytest.warns(UserWarning) as w: + warnings.warn(UserWarning("plain text")) + + rebuilt = unserialize_warning_message(serialize_warning_message(w[0])).message + + assert type(rebuilt) is UserWarning + assert str(rebuilt) == "plain text" + + class MyWarningUnknown(UserWarning): # Changing the __module__ attribute is only safe if class can be imported # from there