fix(pytest+unittest): add safety check for Windows paths (#7627)

[CIVIS-8034

](https://datadoghq.atlassian.net/browse/CIVIS-8034?atlOrigin=eyJpIjoiNTVhNTFmZjkxYWRkNGUwOThiYzExYjhkZTkxZjU5NzQiLCJwIjoiaiJ9)

## Motivation

This PR aims to fix a GitHub issue regarding ValueErrors on Windows
machines when using different path drives.

## What does this PR change?

Adds a few type safety checks around `os.path.relpath` calls to ensure
that the plugin will continue to work smoothly when encountering these
edge cases.

## 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] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [X] Change is maintainable (easy to change, telemetry, documentation).
- [X] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [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))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly 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)
- [x] If this PR touches code that signs or publishes builds or
packages, or handles credentials of any kind, I've requested a review
from `@DataDog/security-design-and-guidance`.
- [x] This PR doesn't touch any of that.

---------

Co-authored-by: Romain Komorn <136473744+romainkomorndatadog@users.noreply.github.com>
Co-authored-by: Brett Langdon <brett.langdon@datadoghq.com>
(cherry picked from commit 1c9765f)

ddtrace/contrib/pytest/plugin.py

@@ -47,6 +47,7 @@
 from ddtrace.internal.ci_visibility.coverage import _initialize_coverage
 from ddtrace.internal.ci_visibility.coverage import build_payload as build_coverage_payload
 from ddtrace.internal.ci_visibility.utils import _add_start_end_source_file_path_data_to_span
+from ddtrace.internal.ci_visibility.utils import get_relative_or_absolute_path_for_path
 from ddtrace.internal.constants import COMPONENT
 from ddtrace.internal.logger import get_logger
 
@@ -642,7 +643,7 @@ def pytest_runtest_protocol(item, nextitem):
             _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)
+            _add_start_end_source_file_path_data_to_span(span, test_method_object, test_name, 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
@@ -806,8 +807,7 @@ def pytest_ddtrace_get_item_suite_name(item):
     if test_module_path:
         if not pytest_module_item.nodeid.startswith(test_module_path):
             log.warning("Suite path is not under module path: '%s' '%s'", pytest_module_item.nodeid, test_module_path)
-        suite_path = os.path.relpath(pytest_module_item.nodeid, start=test_module_path)
-        return suite_path
+        return get_relative_or_absolute_path_for_path(pytest_module_item.nodeid, test_module_path)
     return pytest_module_item.nodeid
 
 

ddtrace/contrib/unittest/patch.py

@@ -31,6 +31,7 @@
 from ddtrace.internal.ci_visibility.coverage import _initialize_coverage
 from ddtrace.internal.ci_visibility.coverage import build_payload as build_coverage_payload
 from ddtrace.internal.ci_visibility.utils import _add_start_end_source_file_path_data_to_span
+from ddtrace.internal.ci_visibility.utils import get_relative_or_absolute_path_for_path
 from ddtrace.internal.constants import COMPONENT
 from ddtrace.internal.logger import get_logger
 from ddtrace.internal.utils.formats import asbool
@@ -258,7 +259,14 @@ def _extract_test_file_name(item) -> str:
 
 def _extract_module_file_path(item) -> str:
     if _is_test(item):
-        return os.path.relpath(inspect.getfile(item.__class__))
+        try:
+            test_module_object = inspect.getfile(item.__class__)
+        except TypeError:
+            log.debug(
+                "Tried to collect module file path but it is a built-in Python function",
+            )
+            return ""
+        return get_relative_or_absolute_path_for_path(test_module_object, os.getcwd())
 
     return ""
 
@@ -537,14 +545,18 @@ def add_xpass_test_wrapper(func, instance, args: tuple, kwargs: dict):
 def _mark_test_as_unskippable(obj):
     test_name = obj.__name__
     test_suite_name = str(obj).split(".")[0].split()[1]
-    test_module_path = os.path.relpath(obj.__code__.co_filename)
+    test_module_path = get_relative_or_absolute_path_for_path(obj.__code__.co_filename, os.getcwd())
     test_module_suite_name = _generate_module_suite_test_path(test_module_path, test_suite_name, test_name)
     _CIVisibility._unittest_data["unskippable_tests"].add(test_module_suite_name)
     return obj
 
 
+def _using_unskippable_decorator(args, kwargs):
+    return args[0] is False and _extract_skip_if_reason(args, kwargs) == ITR_UNSKIPPABLE_REASON
+
+
 def skip_if_decorator(func, instance, args: tuple, kwargs: dict):
-    if args[0] is False and _extract_skip_if_reason(args, kwargs) == ITR_UNSKIPPABLE_REASON:
+    if _using_unskippable_decorator(args, kwargs):
         return _mark_test_as_unskippable
     return func(*args, **kwargs)
 
@@ -789,7 +801,7 @@ def _start_test_span(instance, test_suite_span: ddtrace.Span) -> ddtrace.Span:
 
     _CIVisibility.set_codeowners_of(_extract_test_file_name(instance), span=span)
 
-    _add_start_end_source_file_path_data_to_span(span, test_method_object, test_name)
+    _add_start_end_source_file_path_data_to_span(span, test_method_object, test_name, os.getcwd())
 
     _store_test_span(instance, span)
     return span

ddtrace/internal/ci_visibility/utils.py

@@ -10,12 +10,25 @@
 log = get_logger(__name__)
 
 
-def get_source_file_path_for_test_method(test_method_object) -> typing.Union[str, None]:
+def get_relative_or_absolute_path_for_path(path: str, start_directory: str):
     try:
-        source_file_path = os.path.relpath(inspect.getfile(test_method_object))
+        relative_path = os.path.relpath(path, start=start_directory)
+    except ValueError:
+        log.debug(
+            "Tried to collect relative path but it is using different drive paths on Windows, "
+            "using absolute path instead",
+        )
+        return os.path.abspath(path)
+    return relative_path
+
+
+def get_source_file_path_for_test_method(test_method_object, repo_directory: str) -> typing.Union[str, None]:
+    try:
+        file_object = inspect.getfile(test_method_object)
     except TypeError:
-        return None
-    return source_file_path
+        return ""
+
+    return get_relative_or_absolute_path_for_path(file_object, repo_directory)
 
 
 def get_source_lines_for_test_method(
@@ -30,16 +43,21 @@ def get_source_lines_for_test_method(
     return start_line, end_line
 
 
-def _add_start_end_source_file_path_data_to_span(span: ddtrace.Span, test_method_object, test_name: str):
+def _add_start_end_source_file_path_data_to_span(
+    span: ddtrace.Span, test_method_object, test_name: str, repo_directory: str
+):
     if not test_method_object:
         log.debug(
             "Tried to collect source start/end lines for test method %s but test method object could not be found",
             test_name,
         )
         return
-    source_file_path = get_source_file_path_for_test_method(test_method_object)
+    source_file_path = get_source_file_path_for_test_method(test_method_object, repo_directory)
     if not source_file_path:
-        log.debug("Tried to collect file path for test %s but it is a built-in Python function", test_name)
+        log.debug(
+            "Tried to collect file path for test %s but it is a built-in Python function",
+            test_name,
+        )
         return
     start_line, end_line = get_source_lines_for_test_method(test_method_object)
     if not start_line or not end_line:

releasenotes/notes/fix-path-issue-windows-d17b5d3ae2294834.yaml

@@ -0,0 +1,4 @@
+---
+fixes:
+  - |
+    CI Visibility: Fixes an issue where a ``ValueError`` was raised when using different path drives on Windows

tests/ci_visibility/test_utils.py

@@ -0,0 +1,17 @@
+import os.path
+from unittest import mock
+
+from ddtrace.internal.ci_visibility.utils import get_relative_or_absolute_path_for_path
+
+
+def test_gets_relative_path():
+    actual_output = get_relative_or_absolute_path_for_path("ddtrace/contrib", start_directory=os.getcwd())
+
+    assert not os.path.isabs(actual_output)
+
+
+def test_gets_absolute_path_with_exception():
+    with mock.patch("os.path.relpath", side_effect=ValueError()):
+        actual_output = get_relative_or_absolute_path_for_path("ddtrace/contrib", start_directory=os.getcwd())
+
+        assert os.path.isabs(actual_output)