fix[pytest]: Default JSON encoding for non-serializable types to __repr__ (backport #2660) (#2671)

* fix[pytest]: Default JSON encoding for non-serializable types to __repr__ (#2660)

* Default JSON encode to __repr__ for all non-serializable cases.

* Added MagicMock circular reference test case

* Added custom circular reference object for test case.

* Spelling wordlist addition for changelog

* Made test compatible with py2 and py3

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 5e6fc1b)

# Conflicts:
#	ddtrace/contrib/pytest/plugin.py
#	tests/contrib/pytest/test_pytest.py

* Merge conflict resolution for backport

Co-authored-by: Yun Kim <[email protected]>
Co-authored-by: Yun Kim <[email protected]>

Author: mergify[bot]

ddtrace/contrib/pytest/plugin.py

@@ -1,6 +1,4 @@
 import json
-from typing import Any
-from typing import Dict
 
 import pytest
 
@@ -32,20 +30,6 @@ def _store_span(item, span):
     setattr(item, "_datadog_span", span)
 
 
-def _json_encode(params):
-    # type: (Dict[str, Any]) -> str
-    """JSON encode parameters. If complex object show inner values, otherwise default to string representation."""
-
-    def inner_encode(obj):
-        try:
-            obj_dict = getattr(obj, "__dict__", None)
-            return obj_dict if obj_dict else repr(obj)
-        except Exception as e:
-            return repr(e)
-
-    return json.dumps(params, default=inner_encode)
-
-
 PATCH_ALL_HELP_MSG = "Call ddtrace.patch_all before running tests."
 
 
@@ -124,7 +108,7 @@ def pytest_runtest_protocol(item, nextitem):
         # Pytest docs: https://docs.pytest.org/en/6.2.x/reference.html#pytest.Function
         if getattr(item, "callspec", None):
             params = {"arguments": item.callspec.params, "metadata": {}}
-            span.set_tag(test.PARAMETERS, _json_encode(params))
+            span.set_tag(test.PARAMETERS, json.dumps(params, default=repr))
 
         markers = [marker.kwargs for marker in item.iter_markers(name="dd_tags")]
         for tags in markers:

docs/spelling_wordlist.txt

@@ -64,6 +64,7 @@ iPython
 initializer
 integration
 integrations
+JSON
 jinja
 kombu
 kubernetes
@@ -111,6 +112,7 @@ runnable
 runtime
 runtime-id
 sanic
+serializable
 sql
 sqlalchemy
 sqlite

releasenotes/notes/fix-pytest-parameterized-606fbc3a574c67c6.yaml

@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    Fixed JSON encoding errors in the pytest plugin for parameterized tests with complex Python object parameters.
+    The pytest plugin now defaults to encoding the string representations of non-JSON serializable test parameters.

tests/contrib/pytest/test_pytest.py

@@ -2,12 +2,9 @@
 import os
 import sys
 
-from hypothesis import given
-from hypothesis import strategies as st
 import pytest
 
 from ddtrace import Pin
-from ddtrace.contrib.pytest.plugin import _json_encode
 from ddtrace.ext import test
 from tests.utils import TracerTestCase
 
@@ -106,11 +103,36 @@ def test_ini(ddspan):
         assert len(spans) == 1
 
     def test_parameterize_case(self):
-        """Test parametrize case."""
+        """Test parametrize case with simple objects."""
         py_file = self.testdir.makepyfile(
             """
             import pytest
 
+
+            @pytest.mark.parametrize('item', [1, 2, 3, 4, pytest.param([1, 2, 3], marks=pytest.mark.skip)])
+            class Test1(object):
+                def test_1(self, item):
+                    assert item in {1, 2, 3}
+        """
+        )
+        file_name = os.path.basename(py_file.strpath)
+        rec = self.inline_run("--ddtrace", file_name)
+        rec.assertoutcome(passed=3, failed=1, skipped=1)
+        spans = self.pop_spans()
+
+        expected_params = [1, 2, 3, 4, [1, 2, 3]]
+        assert len(spans) == 5
+        for i in range(len(expected_params)):
+            extracted_params = json.loads(spans[i].meta[test.PARAMETERS])
+            assert extracted_params == {"arguments": {"item": expected_params[i]}, "metadata": {}}
+
+    def test_parameterize_case_complex_objects(self):
+        """Test parametrize case with complex objects."""
+        py_file = self.testdir.makepyfile(
+            """
+            from mock import MagicMock
+            import pytest
+
             class A:
                 def __init__(self, name, value):
                     self.name = name
@@ -119,19 +141,19 @@ def __init__(self, name, value):
             def item_param():
                 return 42
 
+            circular_reference = A("circular_reference", A("child", None))
+            circular_reference.value.value = circular_reference
+
             @pytest.mark.parametrize(
-                'item',
-                [
-                    1,
-                    2,
-                    3,
-                    4,
-                    pytest.param(A("test_name", "value"), marks=pytest.mark.skip),
-                    pytest.param(A("test_name", A("inner_name", "value")), marks=pytest.mark.skip),
-                    pytest.param({"a": A("test_name", "value"), "b": [1, 2, 3]}, marks=pytest.mark.skip),
-                    pytest.param([1, 2, 3], marks=pytest.mark.skip),
-                    pytest.param(item_param, marks=pytest.mark.skip)
-                ]
+            'item',
+            [
+                pytest.param(A("test_name", "value"), marks=pytest.mark.skip),
+                pytest.param(A("test_name", A("inner_name", "value")), marks=pytest.mark.skip),
+                pytest.param(item_param, marks=pytest.mark.skip),
+                pytest.param({"a": A("test_name", "value"), "b": [1, 2, 3]}, marks=pytest.mark.skip),
+                pytest.param(MagicMock(value=MagicMock()), marks=pytest.mark.skip),
+                pytest.param(circular_reference, marks=pytest.mark.skip),
+            ]
             )
             class Test1(object):
                 def test_1(self, item):
@@ -140,24 +162,22 @@ def test_1(self, item):
         )
         file_name = os.path.basename(py_file.strpath)
         rec = self.inline_run("--ddtrace", file_name)
-        rec.assertoutcome(passed=3, failed=1, skipped=5)
+        rec.assertoutcome(skipped=6)
         spans = self.pop_spans()
 
-        expected_params = [
-            1,
-            2,
-            3,
-            4,
-            {"name": "test_name", "value": "value"},
-            {"name": "test_name", "value": {"name": "inner_name", "value": "value"}},
-            {"a": {"name": "test_name", "value": "value"}, "b": [1, 2, 3]},
-            [1, 2, 3],
+        # Since object will have arbitrary addresses, only need to ensure that
+        # the params string contains most of the string representation of the object.
+        expected_params_contains = [
+            "test_parameterize_case_complex_objects.A",
+            "test_parameterize_case_complex_objects.A",
+            "<function item_param at 0x",
+            '"a": "<test_parameterize_case_complex_objects.A',
+            "<MagicMock id=",
+            "test_parameterize_case_complex_objects.A",
         ]
-        assert len(spans) == 9
-        for i in range(len(spans) - 1):
-            extracted_params = json.loads(spans[i].meta[test.PARAMETERS])
-            assert extracted_params == {"arguments": {"item": expected_params[i]}, "metadata": {}}
-        assert "<function item_param at 0x" in json.loads(spans[8].meta[test.PARAMETERS])["arguments"]["item"]
+        assert len(spans) == 6
+        for i in range(len(expected_params_contains)):
+            assert expected_params_contains[i] in spans[i].meta[test.PARAMETERS]
 
     def test_skip(self):
         """Test parametrize case."""
@@ -329,68 +349,3 @@ def test_service(ddspan):
         file_name = os.path.basename(py_file.strpath)
         rec = self.subprocess_run("--ddtrace", file_name)
         assert 0 == rec.ret
-
-
-class A(object):
-    def __init__(self, name, value):
-        self.name = name
-        self.value = value
-
-
-simple_types = [st.none(), st.booleans(), st.text(), st.integers(), st.floats(allow_infinity=False, allow_nan=False)]
-complex_types = [st.functions(), st.dates(), st.decimals(), st.builds(A, name=st.text(), value=st.integers())]
-
-
-@given(
-    st.dictionaries(
-        st.text(),
-        st.one_of(
-            st.lists(st.one_of(*simple_types)), st.dictionaries(st.text(), st.one_of(*simple_types)), *simple_types
-        ),
-    )
-)
-def test_custom_json_encoding_simple_types(obj):
-    """Ensures the _json.encode helper encodes simple objects."""
-    encoded = _json_encode(obj)
-    decoded = json.loads(encoded)
-    assert obj == decoded
-
-
-@given(
-    st.dictionaries(
-        st.text(),
-        st.one_of(
-            st.lists(st.one_of(*complex_types)), st.dictionaries(st.text(), st.one_of(*complex_types)), *complex_types
-        ),
-    )
-)
-def test_custom_json_encoding_python_objects(obj):
-    """Ensures the _json_encode helper encodes complex objects into dicts of inner values or a string representation."""
-    encoded = _json_encode(obj)
-    obj = json.loads(
-        json.dumps(obj, default=lambda x: getattr(x, "__dict__", None) if getattr(x, "__dict__", None) else repr(x))
-    )
-    decoded = json.loads(encoded)
-    assert obj == decoded
-
-
-def test_custom_json_encoding_side_effects():
-    """Ensures the _json_encode helper encodes objects with side effects (getattr, repr) without raising exceptions."""
-    dict_side_effect = Exception("side effect __dict__")
-    repr_side_effect = Exception("side effect __repr__")
-
-    class B(object):
-        def __getattribute__(self, item):
-            if item == "__dict__":
-                raise dict_side_effect
-            raise AttributeError()
-
-    class C(object):
-        def __repr__(self):
-            raise repr_side_effect
-
-    obj = {"b": B(), "c": C()}
-    encoded = _json_encode(obj)
-    decoded = json.loads(encoded)
-    assert decoded["b"] == repr(dict_side_effect)
-    assert decoded["c"] == repr(repr_side_effect)