Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog/13963.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fixed subtests running with `pytest-xdist <https://github.com/pytest-dev/pytest>`__ when their contexts contain non-standard objects.

Fixes `pytest-dev/pytest-xdist#1273 <https://github.com/pytest-dev/pytest-xdist/issues/1273>`__.
5 changes: 4 additions & 1 deletion src/_pytest/subtests.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,10 @@ class SubtestContext:
kwargs: Mapping[str, Any]

def _to_json(self) -> dict[str, Any]:
return dataclasses.asdict(self)
result = dataclasses.asdict(self)
# Brute-force the returned kwargs dict to be JSON serializable (pytest-dev/pytest-xdist#1273).
result["kwargs"] = {k: saferepr(v) for (k, v) in result["kwargs"].items()}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should strive to have the property that from_json(to_json(report)) == report. In this case it means that we should have self.kwargs itself should be serializable, not just in to_json.

Copy link
Member Author

@nicoddemus nicoddemus Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should strive to have the property that from_json(to_json(report)) == report.

I agree this is desirable, but I don't see how we can accomplish this in a general manner, for any type of object.

In unittest subtests, kwargs is permitted to contain any type of object, because it is not serialized to anything and used only for reporting.

I don't see how we can support that in functions that serialize to JSON.

Perhaps I'm missing something, could you elaborate a bit?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm thinking is that kwargs itself contain strings, not the objects themselves, that would of course make it serializable. It is not exposed in the API so should be fine? Unless the original objects are needed for some reason?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well once they are serialized and come back unserialized on the other side, unfortunately they might not be useful depending on the intent. For reporting this should be fine, but if a plugin is using the serialization hooks and expecting the actual objects to be returned this would be a problem.

We can perhaps try to keep the concrete types for objects we know are safe (int, float, etc.), but for other objects I don't see another solution.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can perhaps try to keep the concrete types for objects we know are safe (int, float, etc.)

Implemented that, take a look.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we'll be getting an arbitration :) So please go ahead with the change (I'll approve).

Copy link
Member Author

@nicoddemus nicoddemus Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be wrong in my point of view, it would be nice to get other's opinions. 😁

I will wait a few more days.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really have a clear opinion on this, I think. Some considerations:

I do agree pickle kind of gives me the ick. I feel like more of those could be relevant, and I don't think the security argument is necessarily moot. Take this with a grain of salt because I haven't worked with custom reporting much, but where are the hooks coming from vs. where is the JSON data coming from?

I feel like it's not uncommon to have the hooks from a trusted source (e.g. a custom plugin), but reading JSON data from an untrusted source? Say e.g. I build a "fake pytest result viewer" which takes a JSON report from someone and shows me a fake terminal session of how the run looked. It seems like quite a surprising responsibility if my (trusted) viewer runs arbitrary code when I don't carefully audit the (intransparent) binary data in the JSON report, no?

I thought the idea of having JSON reports is precisely that they can be used by third-party tools and not just pytest itself (and pytest-xdist), no?

Copy link
Member Author

@nicoddemus nicoddemus Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a confusion here regarding what it means by "JSON-like" returned by pytest_report_to_serializable. It was originally created just as a way to customize report serialization with pytest-subtests, specially for SubTestReport from pytest-subtests.

It is meant to be the counterpart of pytest_report_from_serializable, meaning that users shouldn't even rely on what pytest_report_to_serializable returns, it is only meant to be passed along to pytest_report_from_serializable to obtain the restored report. The return value is not meant to be looked at or consumed in any way, and is subject to change.

But I needed to document what pytest_report_from_serializable should return, so I went with "JSON-like" which might have been unfortunate because it is not what I intended.

In other words, the intended usage of those hooks is:

data = pytest_report_to_serializable(sub_report)
loaded = pytest_report_from_serializable(data)
assert sub_report == loaded

Having said all that, it seems I failed to convince my POV on how this should work, so I will go ahead and convert the kwargs on input and use saferepr as suggested.

This will result in:

data = pytest_report_to_serializable(sub_report)
loaded = pytest_report_from_serializable(data)
assert sub_report != loaded  # loaded.kwargs is not a dict, but a saferepr() of the original sub_report.kwargs.

This won't matter for pytest-xdist which just prints it, but might affect user code or some other plugins that expected the first property (sub_report == loaded) to hold.

Ultimately, I think this is the wrong call, because we will be effectively throwing away user data on the off-chance that we might eventually serialize it with pytest_report_to_serializable hook, but I don't seem to be able to convince folks so I defer to your judgment call on this one (I'm not a big fan of pickle either, but can't think of a generic way to save a generic dictionary).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying you're wrong here, for what it's worth. I haven't really looked at how reports or those hooks work in detail personally, so take what I said with a grain of salt. I think it does make sense with your explanation, and perhaps the issue you mention could be solved by making it more clear in the documentation?

return result

@classmethod
def _from_json(cls, d: dict[str, Any]) -> Self:
Expand Down
38 changes: 36 additions & 2 deletions testing/test_subtests.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
from __future__ import annotations

from enum import Enum
import sys
from typing import Literal

from _pytest._io.saferepr import saferepr
from _pytest.subtests import SubtestContext
from _pytest.subtests import SubtestReport
import pytest
Expand Down Expand Up @@ -958,20 +960,52 @@ def test(subtests):


def test_serialization() -> None:
"""Ensure subtest's kwargs are serialized using `saferepr` (pytest-dev/pytest-xdist#1273)."""
from _pytest.subtests import pytest_report_from_serializable
from _pytest.subtests import pytest_report_to_serializable

class MyEnum(Enum):
A = "A"

report = SubtestReport(
"test_foo::test_foo",
("test_foo.py", 12, ""),
keywords={},
outcome="passed",
when="call",
longrepr=None,
context=SubtestContext(msg="custom message", kwargs=dict(i=10)),
context=SubtestContext(msg="custom message", kwargs=dict(i=10, a=MyEnum.A)),
)
data = pytest_report_to_serializable(report)
assert data is not None
new_report = pytest_report_from_serializable(data)
assert new_report is not None
assert new_report.context == SubtestContext(msg="custom message", kwargs=dict(i=10))
assert new_report.context == SubtestContext(
msg="custom message", kwargs=dict(i=saferepr(10), a=saferepr(MyEnum.A))
)


def test_serialization_xdist(pytester: pytest.Pytester) -> None:
"""Regression test for pytest-dev/pytest-xdist#1273."""
pytest.importorskip("xdist")
pytester.makepyfile(
"""
from enum import Enum
import unittest

class MyEnum(Enum):
A = "A"

def test(subtests):
with subtests.test(a=MyEnum.A):
pass

class T(unittest.TestCase):

def test(self):
with self.subTest(a=MyEnum.A):
pass
"""
)
result = pytester.runpytest("-n1", "-pxdist.plugin")
result.assert_outcomes(passed=2)