From 3254fc0193f9bd76718879ca2759b1df893d2751 Mon Sep 17 00:00:00 2001 From: Diego Prada Date: Sun, 16 Aug 2026 00:53:45 -0600 Subject: [PATCH 1/5] Do not rebuild a warning whose first argument is not its message `unserialize_warning_message` recreates the warning on the controller as `cls(*message_args)`, where `message_args` is the original instance's `args`. That assumes the first argument is the message. A `Warning` subclass is free not to do that: it may name a field of its own and build the message out of it, a common shape in libraries that render diagnostics from structured data. For such a class the rebuilt instance is a different warning. The rendered text goes back into whatever field the first argument names, and `__str__` renders around it a second time: class ResourceWarning(UserWarning): def __init__(self, resource): super().__init__(f"{resource!r} is not available") # controller: ResourceWarning("'gpu' is not available") # reports: "'gpu' is not available" is not available A subclass whose parameters all have defaults is worse: it rebuilds without error and reports the *default* message, so the text is wrong without looking wrong. Only subclasses that reject the call are handled today, by the `except TypeError` fallback. Keep the rebuilt instance only when it still says what the original said, and otherwise take the existing "could not recreate the original warning instance" path, which reports the original text once. `category` is unaffected: it is rebuilt from its own fields. Found when a test suite run under `-n 12` reported warning text nested inside itself while the same suite run serially did not. --- src/xdist/workermanage.py | 9 +++++++++ testing/test_workermanage.py | 37 ++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/src/xdist/workermanage.py b/src/xdist/workermanage.py index c54b18fb..cafc838e 100644 --- a/src/xdist/workermanage.py +++ b/src/xdist/workermanage.py @@ -488,6 +488,15 @@ def unserialize_warning_message(data: dict[str, Any]) -> warnings.WarningMessage message = cls(*data["message_args"]) except TypeError: pass + else: + # `cls(*args)` assumes the first argument is the message, which a + # Warning subclass is free not to do: it may name a field of its + # own and build the message out of it. Rebuilding such a warning + # feeds the rendered text back into that field, so the instance + # renders around its own output. Keep the rebuilt instance only + # when it still says what the original said. + if str(message) != data["message_str"]: + 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/test_workermanage.py b/testing/test_workermanage.py index 4b393150..fcc383d1 100644 --- a/testing/test_workermanage.py +++ b/testing/test_workermanage.py @@ -497,6 +497,43 @@ class MyWarning2(UserWarning): assert v1 == v2 +class WarningWithOwnFirstArgument(UserWarning): + """A warning whose first argument is a field, not the message. + + Libraries that build their text from structured data do this: the caller + passes the datum and the class renders the sentence. + """ + + def __init__(self, resource: str) -> None: + self.resource = resource + super().__init__(f"{resource!r} is not available") + + +def test_unserialize_warning_msg_that_cannot_round_trip() -> None: + """A warning that does not take its message first must not be re-rendered. + + Rebuilding it as ``cls(*args)`` puts the rendered sentence back into the + field the first argument names, so the instance renders around its own + output. Fall back to the generic form, which at least reports the original + text once. + """ + with pytest.warns(UserWarning) as w: + warnings.warn(WarningWithOwnFirstArgument("gpu")) + + assert len(w) == 1 + w_msg = w[0] + assert str(w_msg.message) == "'gpu' is not available" + + data = serialize_warning_message(w_msg) + w_msg2 = unserialize_warning_message(data) + + text = str(w_msg2.message) + assert "'gpu' is not available" in text + assert text.count("is not available") == 1, text + # The category survives regardless; it is rebuilt from its own fields. + assert w_msg2.category is WarningWithOwnFirstArgument + + class MyWarningUnknown(UserWarning): # Changing the __module__ attribute is only safe if class can be imported # from there From 6d7e8aad63472adf329ee1c1320fddd6ba63635c Mon Sep 17 00:00:00 2001 From: Diego Prada Date: Sun, 16 Aug 2026 01:00:59 -0600 Subject: [PATCH 2/5] Add changelog fragment for #1372 --- changelog/1372.bugfix.rst | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/1372.bugfix.rst diff --git a/changelog/1372.bugfix.rst b/changelog/1372.bugfix.rst new file mode 100644 index 00000000..4fa47850 --- /dev/null +++ b/changelog/1372.bugfix.rst @@ -0,0 +1,5 @@ +A warning whose first constructor argument is not its message is no longer +rebuilt incorrectly on the controller. Such a warning was recreated as +``cls(*args)``, which fed its own rendered text back into the field that +argument names, so the reported message rendered around itself. The rebuilt +instance is now kept only when it still says what the original said. From 3fea6f6a2a9e597a22d735f8bd14fe56a20f8a85 Mon Sep 17 00:00:00 2001 From: Diego Prada Date: Mon, 17 Aug 2026 00:01:06 -0600 Subject: [PATCH 3/5] Transfer warning state, and rebuild without re-running __init__ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the check added in the first version of this branch. That one compared `str()` against the transferred text and discarded the rebuilt instance when they differed, which detected that the transfer was lossy without making it any less so, and paid for the detection by dropping the message class and prefixing the text. The loss is in what we send and how we rebuild it. `serialize_warning_message` sends only `args`; `BaseException.__reduce__` carries the instance dictionary as a third element and `BaseException.__setstate__` applies it, and we transfer neither. So a warning keeping anything beside its `args` arrives without it, and one deriving its message from that state arrives rendered from its defaults — reading like a real message rather than failing. `-n0` serializes nothing and is therefore the reference for what a warning says. A class rendering its message from `self.code` reports `code 42 tripped` there and `code unknown tripped` under `-n1`, while round-tripping correctly under both `pickle` and `copy`. - the reduce state travels alongside `args`, each gated on what `execnet.dumps` can carry, so a payload that cannot cross is reported absent rather than half-sent; - the instance is rebuilt with `__new__` + `BaseException.__init__` + `__setstate__`, so a re-rendering `__init__` never runs; - the `(dict, slots)` two-tuple that `__slots__` classes reduce to is applied by hand, since `BaseException.__setstate__` does not take it; - a class-provided `__reduce__` whose callable is not the class itself is left alone and falls back, because `execnet` cannot carry that callable; - the generic `Warning` fallback is reached only when the state will not transfer. `test_state_beyond_args_survives` covers it end to end over `-n0`/`-n1`, and two unit tests cover the field-first and state-only shapes, asserting the resulting type, the exact text and the restored attribute. All three fail without this change. --- changelog/1372.bugfix.rst | 11 ++++---- src/xdist/remote.py | 35 +++++++++++++++++++++++ src/xdist/workermanage.py | 49 ++++++++++++++++++++++++-------- testing/acceptance_test.py | 32 +++++++++++++++++++++ testing/test_workermanage.py | 54 +++++++++++++++++++++++------------- 5 files changed, 144 insertions(+), 37 deletions(-) diff --git a/changelog/1372.bugfix.rst b/changelog/1372.bugfix.rst index 4fa47850..665963f6 100644 --- a/changelog/1372.bugfix.rst +++ b/changelog/1372.bugfix.rst @@ -1,5 +1,6 @@ -A warning whose first constructor argument is not its message is no longer -rebuilt incorrectly on the controller. Such a warning was recreated as -``cls(*args)``, which fed its own rendered text back into the field that -argument names, so the reported message rendered around itself. The rebuilt -instance is now kept only when it still says what the original said. +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..3eaa19ae 100644 --- a/src/xdist/remote.py +++ b/src/xdist/remote.py @@ -324,6 +324,33 @@ 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 +366,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 +390,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 cafc838e..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,19 +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 - else: - # `cls(*args)` assumes the first argument is the message, which a - # Warning subclass is free not to do: it may name a field of its - # own and build the message out of it. Rebuilding such a warning - # feeds the rendered text back into that field, so the instance - # renders around its own output. Keep the rebuilt instance only - # when it still says what the original said. - if str(message) != data["message_str"]: - message = None + 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..fe9a056f 100644 --- a/testing/acceptance_test.py +++ b/testing/acceptance_test.py @@ -867,6 +867,38 @@ 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 fcc383d1..e846463f 100644 --- a/testing/test_workermanage.py +++ b/testing/test_workermanage.py @@ -497,41 +497,55 @@ class MyWarning2(UserWarning): assert v1 == v2 -class WarningWithOwnFirstArgument(UserWarning): - """A warning whose first argument is a field, not the message. - - Libraries that build their text from structured data do this: the caller - passes the datum and the class renders the sentence. - """ +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") -def test_unserialize_warning_msg_that_cannot_round_trip() -> None: - """A warning that does not take its message first must not be re-rendered. +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" + - Rebuilding it as ``cls(*args)`` puts the rendered sentence back into the - field the first argument names, so the instance renders around its own - output. Fall back to the generic form, which at least reports the original - text once. +@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, 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(WarningWithOwnFirstArgument("gpu")) + warnings.warn(factory()) assert len(w) == 1 w_msg = w[0] - assert str(w_msg.message) == "'gpu' is not available" + assert str(w_msg.message) == expected_text data = serialize_warning_message(w_msg) - w_msg2 = unserialize_warning_message(data) + rebuilt = unserialize_warning_message(data).message - text = str(w_msg2.message) - assert "'gpu' is not available" in text - assert text.count("is not available") == 1, text - # The category survives regardless; it is rebuilt from its own fields. - assert w_msg2.category is WarningWithOwnFirstArgument + assert type(rebuilt) is type(w_msg.message) + assert str(rebuilt) == expected_text + assert getattr(rebuilt, attribute) == expected_value class MyWarningUnknown(UserWarning): From 12176ac6719e31f0b194a472fba8ce293e9ab3c3 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 17 Aug 2026 06:02:57 +0000 Subject: [PATCH 4/5] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/xdist/remote.py | 3 +-- testing/acceptance_test.py | 4 +++- testing/test_workermanage.py | 7 ++++++- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/xdist/remote.py b/src/xdist/remote.py index 3eaa19ae..b8e31f0d 100644 --- a/src/xdist/remote.py +++ b/src/xdist/remote.py @@ -324,8 +324,6 @@ def pytest_warning_recorded( ) - - def _serializable_reduce_state(message: Warning) -> Any | None: """Return the state `__reduce__` carries, when it can cross the wire. @@ -351,6 +349,7 @@ def _serializable_reduce_state(message: Warning) -> Any | None: return None return state + def serialize_warning_message( warning_message: warnings.WarningMessage, ) -> dict[str, Any]: diff --git a/testing/acceptance_test.py b/testing/acceptance_test.py index fe9a056f..9197aa37 100644 --- a/testing/acceptance_test.py +++ b/testing/acceptance_test.py @@ -868,7 +868,9 @@ def test_func(request): 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: + 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 diff --git a/testing/test_workermanage.py b/testing/test_workermanage.py index e846463f..fec9507e 100644 --- a/testing/test_workermanage.py +++ b/testing/test_workermanage.py @@ -519,7 +519,12 @@ def __str__(self) -> str: @pytest.mark.parametrize( ("factory", "expected_text", "attribute", "expected_value"), [ - (lambda: WarningWithFieldFirst("gpu"), "'gpu' is not available", "resource", "gpu"), + ( + lambda: WarningWithFieldFirst("gpu"), + "'gpu' is not available", + "resource", + "gpu", + ), (lambda: WarningWithoutArgs(42), "code 42 tripped", "code", 42), ], ids=["field-first", "state-only"], From 1dcec70f2a15bcc4204dee551591d35bdd5ea8ac Mon Sep 17 00:00:00 2001 From: Diego Prada Date: Mon, 17 Aug 2026 00:13:09 -0600 Subject: [PATCH 5/5] Annotate the parametrized factory `mypy` rejected the new parametrized test: its `factory` parameter carried no annotation, and `no-untyped-def` applies to `testing/` as well as `src/`. Typed as `Callable[[], UserWarning]`, which is what both ids pass. The formatting half of this was already handled by pre-commit.ci. Checked with the hook's own dependency set: mypy clean over 27 files, ruff and ruff format clean, unit tests passing. --- testing/test_workermanage.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/testing/test_workermanage.py b/testing/test_workermanage.py index fec9507e..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 @@ -530,7 +531,10 @@ def __str__(self) -> str: ids=["field-first", "state-only"], ) def test_warning_state_survives_the_round_trip( - factory, expected_text: str, attribute: str, expected_value: object + 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.