From 203e89489f1e698a127317dc57b01e9d6467e859 Mon Sep 17 00:00:00 2001 From: Romain Komorn <136473744+romainkomorndatadog@users.noreply.github.com> Date: Wed, 26 Jun 2024 14:00:31 +0100 Subject: [PATCH] fix(ci_visibility): properly resolve decorated functions for test source file information (#9586) Fixes an issue where decorated test functions could resolve to the wrong location when certain decorators (eg: @mock.patch) were used. This also fixes the fact that source file info paths might not always be relative to the current repo root. The test added in this PR verifies the above by using a variety of decorators as well as executing from the `nested_dir` directory instead of the git repo root, with the source file info properly showing the path as `nested_dir/test_mydecorators.py`. ## Checklist - [x] Change(s) are motivated and described in the PR description - [x] Testing strategy is described if automated tests are not included in the PR - [x] Risks are described (performance impact, potential for breakage, maintainability) - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed or label `changelog/no-changelog` is set - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)) - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) - [x] If this PR changes the public interface, I've notified `@DataDog/apm-tees`. ## Reviewer Checklist - [x] Title is accurate - [x] All changes are related to the pull request's stated goal - [x] Description motivates each change - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - [x] Testing strategy adequately addresses listed risks - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] Release note makes sense to a user of the library - [x] Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - [x] Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) --- ddtrace/contrib/pytest/_plugin_v1.py | 22 +++- ddtrace/internal/ci_visibility/utils.py | 2 +- ...rly_unwrap_functions-7c631b68720adab2.yaml | 5 + tests/contrib/pytest/test_pytest.py | 115 +++++++++++++++++- 4 files changed, 138 insertions(+), 6 deletions(-) create mode 100644 releasenotes/notes/fix-ci_visibility-properly_unwrap_functions-7c631b68720adab2.yaml diff --git a/ddtrace/contrib/pytest/_plugin_v1.py b/ddtrace/contrib/pytest/_plugin_v1.py index 894f12b8a84..7046e00cdbd 100644 --- a/ddtrace/contrib/pytest/_plugin_v1.py +++ b/ddtrace/contrib/pytest/_plugin_v1.py @@ -37,6 +37,7 @@ from ddtrace.contrib.unittest import unpatch as unpatch_unittest from ddtrace.ext import SpanTypes from ddtrace.ext import test +from ddtrace.ext.git import extract_workspace_path from ddtrace.internal.ci_visibility import CIVisibility as _CIVisibility from ddtrace.internal.ci_visibility.constants import EVENT_TYPE as _EVENT_TYPE from ddtrace.internal.ci_visibility.constants import ITR_CORRELATION_ID_TAG_NAME @@ -67,6 +68,7 @@ from ddtrace.internal.coverage.code import ModuleCodeCollector from ddtrace.internal.logger import get_logger from ddtrace.internal.utils.formats import asbool +from ddtrace.internal.utils.inspection import undecorated log = get_logger(__name__) @@ -97,7 +99,7 @@ def _is_pytest_cov_enabled(config) -> bool: nocov_option = config.getoption("--no-cov", default=False) if nocov_option is True: return False - if type(cov_option) == list and cov_option == [True] and not nocov_option: + if isinstance(cov_option, list) and cov_option == [True] and not nocov_option: return True return cov_option @@ -449,6 +451,14 @@ def pytest_sessionstart(session): log.debug("CI Visibility enabled - starting test session") global _global_skipped_elements _global_skipped_elements = 0 + try: + workspace_path = extract_workspace_path() + except ValueError: + log.debug("Couldn't extract workspace path from git, reverting to config rootdir") + workspace_path = session.config.rootdir + + session._dd_workspace_path = workspace_path + test_session_span = _CIVisibility._instance.tracer.trace( "pytest.test_session", service=_CIVisibility._instance._service, @@ -661,8 +671,14 @@ def pytest_runtest_protocol(item, nextitem): if item.location and item.location[0]: _CIVisibility.set_codeowners_of(item.location[0], span=span) if hasattr(item, "_obj"): - test_method_object = item._obj - _add_start_end_source_file_path_data_to_span(span, test_method_object, test_name, item.config.rootdir) + item_path = Path(item.path if hasattr(item, "path") else item.fspath) + test_method_object = undecorated(item._obj, item.name, item_path) + _add_start_end_source_file_path_data_to_span( + span, + test_method_object, + test_name, + getattr(item.session, "_dd_workspace_path", item.config.rootdir), + ) # We preemptively set FAIL as a status, because if pytest_runtest_makereport is not called # (where the actual test status is set), it means there was a pytest error diff --git a/ddtrace/internal/ci_visibility/utils.py b/ddtrace/internal/ci_visibility/utils.py index 1944ec847b9..fd05c2c37d7 100644 --- a/ddtrace/internal/ci_visibility/utils.py +++ b/ddtrace/internal/ci_visibility/utils.py @@ -79,7 +79,7 @@ def _add_pct_covered_to_span(coverage_data: dict, span: ddtrace.Span): log.warning("Tried to add total covered percentage to session span but no data was found") return lines_pct_value = coverage_data[PCT_COVERED_KEY] - if type(lines_pct_value) != float: + if not isinstance(lines_pct_value, float): log.warning("Tried to add total covered percentage to session span but the format was unexpected") return span.set_tag(test.TEST_LINES_PCT, lines_pct_value) diff --git a/releasenotes/notes/fix-ci_visibility-properly_unwrap_functions-7c631b68720adab2.yaml b/releasenotes/notes/fix-ci_visibility-properly_unwrap_functions-7c631b68720adab2.yaml new file mode 100644 index 00000000000..300f778ca87 --- /dev/null +++ b/releasenotes/notes/fix-ci_visibility-properly_unwrap_functions-7c631b68720adab2.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + CI Visibility: fixes source file information that would be incorrect in certain decorated / wrapped scenarios and forces + paths to be relative to the repository root, if present. diff --git a/tests/contrib/pytest/test_pytest.py b/tests/contrib/pytest/test_pytest.py index 46a2c7dbecb..526477188eb 100644 --- a/tests/contrib/pytest/test_pytest.py +++ b/tests/contrib/pytest/test_pytest.py @@ -67,7 +67,7 @@ def subprocess_run(self, *args): def test_and_emit_get_version(self): version = get_version() - assert type(version) == str + assert isinstance(version, str) assert version != "" emit_integration_and_version_to_test_agent("pytest", version) @@ -3494,7 +3494,7 @@ def test_add_two_number_list(): lines_pct_value = test_session_span.get_metric("test.code_coverage.lines_pct") assert lines_pct_value is not None - assert type(lines_pct_value) == float + assert isinstance(lines_pct_value, float) assert test_module_span.get_metric("test.code_coverage.lines_pct") is None assert test_suite_span.get_metric("test.code_coverage.lines_pct") is None assert test_span.get_metric("test.code_coverage.lines_pct") is None @@ -3663,3 +3663,114 @@ def test_add_two_number_list(): assert test_module_span.get_metric("test.code_coverage.lines_pct") is None assert test_suite_span.get_metric("test.code_coverage.lines_pct") is None assert test_span.get_metric("test.code_coverage.lines_pct") is None + + def test_pytest_reports_correct_source_info(self): + """Tests that decorated functions are reported with correct source file information and with relative to + repo root + """ + os.chdir(self.git_repo) + os.mkdir("nested_dir") + os.chdir("nested_dir") + with open("my_decorators.py", "w+") as fd: + fd.write( + textwrap.dedent( + ( + """ + def outer_decorator(func): + def wrapper(*args, **kwargs): + return func(*args, **kwargs) + return wrapper + + @outer_decorator + def inner_decorator(func): + def wrapper(*args, **kwargs): + return func(*args, **kwargs) + return wrapper + """ + ) + ) + ) + + with open("test_mydecorators.py", "w+") as fd: + fd.write( + textwrap.dedent( + ( + """ + # this comment is line 2 and if you didn't know that it'd be easy to miscount below + from my_decorators import outer_decorator, inner_decorator + from unittest.mock import patch + + def local_decorator(func): + def wrapper(*args, **kwargs): + return func(*args, **kwargs) + return wrapper + + def test_one_decorator(): # line 11 + str1 = "string 1" + str2 = "string 2" + assert str1 != str2 + + @local_decorator # line 16 + def test_local_decorated(): + str1 = "string 1" + str2 = "string 2" + assert str1 == str2 + + @patch("ddtrace.config._potato", "potato") # line 22 + def test_patched_undecorated(): + str1 = "string 1" + str2 = "string 2" + assert str1 != str2 + + @patch("ddtrace.config._potato", "potato") # line 28 + @inner_decorator + def test_patched_single_decorated(): + str1 = "string 1" + str2 = "string 2" + assert str1 == str2 + + @patch("ddtrace.config._potato", "potato") # line 35 + @outer_decorator + def test_patched_double_decorated(): + str1 = "string 1" + str2 = "string 2" + assert str1 != str2 + + @outer_decorator # line 42 + @patch("ddtrace.config._potato", "potato") + @local_decorator + def test_grand_slam(): + str1 = "string 1" + str2 = "string 2" + assert str1 == str2 + """ + ) + ) + ) + + self.inline_run("--ddtrace") + + spans = self.pop_spans() + assert len(spans) == 9 + test_names_to_source_info = { + span.get_tag("test.name"): ( + span.get_tag("test.source.file"), + span.get_metric("test.source.start"), + span.get_metric("test.source.end"), + ) + for span in spans + if span.get_tag("type") == "test" + } + assert len(test_names_to_source_info) == 6 + + expected_path = "nested_dir/test_mydecorators.py" + expected_source_info = { + "test_one_decorator": (expected_path, 11, 15), + "test_local_decorated": (expected_path, 16, 21), + "test_patched_undecorated": (expected_path, 22, 27), + "test_patched_single_decorated": (expected_path, 28, 34), + "test_patched_double_decorated": (expected_path, 35, 41), + "test_grand_slam": (expected_path, 42, 49), + } + + assert expected_source_info == test_names_to_source_info