diff --git a/changelog/1372.bugfix.rst b/changelog/1372.bugfix.rst new file mode 100644 index 00000000..665963f6 --- /dev/null +++ b/changelog/1372.bugfix.rst @@ -0,0 +1,6 @@ +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 +the class with them, so a warning whose message is derived from its own fields +arrived rendered from defaults — reading like a real message rather than failing. +The reduce state is now transferred as well, and the instance is rebuilt without +re-running ``__init__``. diff --git a/src/xdist/remote.py b/src/xdist/remote.py index 4d7c9267..b8e31f0d 100644 --- a/src/xdist/remote.py +++ b/src/xdist/remote.py @@ -324,6 +324,32 @@ def pytest_warning_recorded( ) +def _serializable_reduce_state(message: Warning) -> Any | None: + """Return the state `__reduce__` carries, when it can cross the wire. + + `None` means the state could not be transferred and the controller should + not pretend otherwise. A class that replaces `__reduce__` with its own + callable is left alone: rebuilding it here would run something we do not + understand, and its own protocol already round-trips under pickle. + """ + try: + reduced = message.__reduce__() + except Exception: + return None + if not isinstance(reduced, tuple) or len(reduced) < 3: + return None + if reduced[0] is not type(message): + return None + state = reduced[2] + if state is None: + return None + try: + execnet.dumps(state) + except execnet.DumpError: + return None + return state + + def serialize_warning_message( warning_message: warnings.WarningMessage, ) -> dict[str, Any]: @@ -339,11 +365,18 @@ def serialize_warning_message( message_args = None else: message_args = warning_message.message.args + # `args` alone does not describe a warning. `BaseException.__reduce__` + # carries the instance dictionary as a third element, and a class may + # replace `__reduce__` entirely. 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 = _serializable_reduce_state(warning_message.message) else: message_str = warning_message.message message_module = None message_class_name = None message_args = None + message_state = None if warning_message.category: category_module = warning_message.category.__module__ category_class_name = warning_message.category.__name__ @@ -356,6 +389,7 @@ def serialize_warning_message( "message_module": message_module, "message_class_name": message_class_name, "message_args": message_args, + "message_state": message_state, "category_module": category_module, "category_class_name": category_class_name, } diff --git a/src/xdist/workermanage.py b/src/xdist/workermanage.py index c54b18fb..20fdd1f1 100644 --- a/src/xdist/workermanage.py +++ b/src/xdist/workermanage.py @@ -476,6 +476,30 @@ def process_from_remote( self.notify_inproc("errordown", node=self, error=excinfo) +def _restore_warning_state(message: BaseException, state: Any) -> None: + """Apply the state `__reduce__` carried, in the two shapes it comes in. + + `BaseException` implements `__setstate__` as a `setattr` per key, which is + the branch pickle takes for exceptions. A class using `__slots__` reduces to + a `(dict, slots)` two-tuple instead, and that one has to be applied by hand. + """ + setstate = getattr(message, "__setstate__", None) + if isinstance(state, tuple) and len(state) == 2: + instance_dict, slot_state = state + if instance_dict: + if setstate is not None: + setstate(instance_dict) + else: + message.__dict__.update(instance_dict) + for key, value in (slot_state or {}).items(): + setattr(message, key, value) + return + if setstate is not None: + setstate(state) + else: + message.__dict__.update(state) + + def unserialize_warning_message(data: dict[str, Any]) -> warnings.WarningMessage: import importlib @@ -484,10 +508,20 @@ def unserialize_warning_message(data: dict[str, Any]) -> warnings.WarningMessage cls = getattr(mod, data["message_class_name"]) message = None if data["message_args"] is not None: + # 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 rendered text as if it were input: the + # instance then renders around its own output, or silently falls back + # to whatever its defaults say. `BaseException.__setstate__` restores + # the rest, which is the half we used not to send at all. try: - message = cls(*data["message_args"]) - except TypeError: - pass + message = cls.__new__(cls) + BaseException.__init__(message, *data["message_args"]) + state = data.get("message_state") + if state is not None: + _restore_warning_state(message, 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 814c8c09..9197aa37 100644 --- a/testing/acceptance_test.py +++ b/testing/acceptance_test.py @@ -867,6 +867,40 @@ 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_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..842b8331 100644 --- a/testing/test_workermanage.py +++ b/testing/test_workermanage.py @@ -1,5 +1,6 @@ from __future__ import annotations +from collections.abc import Callable from pathlib import Path import shutil import textwrap @@ -497,6 +498,65 @@ 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 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" + + +@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), + ], + ids=["field-first", "state-only"], +) +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 MyWarningUnknown(UserWarning): # Changing the __module__ attribute is only safe if class can be imported # from there