Skip to content

Fix issue with slashes being turned into backslashes on Windows #12760

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Sep 8, 2024
Merged

Fix issue with slashes being turned into backslashes on Windows #12760

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
88cd113
Fix issue with slashes being turned into backslashes on Windows
nauman897 Sep 1, 2024
edf7b11
updated changelog
nauman897 Sep 1, 2024
0a08fe9
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Sep 1, 2024
fb5c935
Add tests for nodeid handling
nauman897 Sep 3, 2024
abf7413
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Sep 3, 2024
b5e2f0f
Improved `nodeid` handling logic
nauman897 Sep 4, 2024
688a530
test: Reproduce use case scenarios using E2E
dongfangtianyu Sep 6, 2024
03e5e41
Merge pull request #1 from dongfangtianyu/windows-slash-handling
nauman897 Sep 6, 2024
a9f2e99
Added encoding type while calling `write_text`
nauman897 Sep 6, 2024
5b33322
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Sep 6, 2024
b9f6557
Merge branch 'main' into fix/windows-slash-handling
nauman897 Sep 6, 2024
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
1 change: 1 addition & 0 deletions AUTHORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,7 @@ mrbean-bremen
Nathan Goldbaum
Nathaniel Compton
Nathaniel Waisbrot
Nauman Ahmed
Ned Batchelder
Neil Martin
Neven Mundar
Expand Down
1 change: 1 addition & 0 deletions changelog/12745.bugfix.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixed an issue with backslashes being incorrectly converted in nodeid paths on Windows, ensuring consistent path handling across environments.
8 changes: 6 additions & 2 deletions src/_pytest/config/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1178,8 +1178,12 @@
def cwd_relative_nodeid(self, nodeid: str) -> str:
# nodeid's are relative to the rootpath, compute relative to cwd.
if self.invocation_params.dir != self.rootpath:
fullpath = self.rootpath / nodeid
nodeid = bestrelpath(self.invocation_params.dir, fullpath)
base_path_part, *nodeid_part = nodeid.split("::")

Check warning on line 1181 in src/_pytest/config/__init__.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/config/__init__.py#L1181 

Added line #L1181 was not covered by tests
# Only process path part
fullpath = self.rootpath / base_path_part
relative_path = bestrelpath(self.invocation_params.dir, fullpath)

Check warning on line 1184 in src/_pytest/config/__init__.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/config/__init__.py#L1183-L1184 

Added lines #L1183 - L1184 were not covered by tests

nodeid = "::".join([relative_path, *nodeid_part])

Check warning on line 1186 in src/_pytest/config/__init__.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/config/__init__.py#L1186 

Added line #L1186 was not covered by tests
return nodeid

@classmethod
Expand Down
25 changes: 25 additions & 0 deletions testing/test_terminal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3067,3 +3067,28 @@ def test_pass():
"*= 1 xpassed in * =*",
]
)


class TestNodeIDHandling:
def test_nodeid_handling_windows_paths(self, pytester: Pytester) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

I just tested this on Windows, on the main branch, and it passes... does this fail only on Linux?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @nicoddemus ,
The problem specifically occurs under these circumstances as mentioned in the issue:
The IDs show up correctly in --collect-only, and they show correctly when test_x.py is not in a subfolder, or when pytest is not invoked in the subfolder.

Copy link
Member

Choose a reason for hiding this comment

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

But then the test case should make sure to replicate that scenario, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I'll fix that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe @dongfangtianyu has already taken care of it: nauman897#1

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @nauman897 ,I have submitted a PR to your repository, hoping that you can review, verify, and merge this modification. It’s not difficult, you can give it a try.

Ideally:

  1. The test case will fail when pytest runs without your patch being merged.
  2. The test case will pass after merging your patch into pytest.
    

If the actual result is different, it indicates there are still issues with our code, and it should not be merged into pytest’s main branch.

Copy link
Contributor Author

@nauman897 nauman897 Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @dongfangtianyu for the help with these tests.
I can confirm that:

  1. The tests fail with the previous logic in cwd_relative_nodeid:
FAILED test_foo.py::test_x[x\y] - assert False
FAILED test_foo.py::test_x[C:\path] - assert False
FAILED test_foo.py::test_x[\] - assert False
FAILED test_foo.py::test_x[C:\path] - assert False
FAILED test_foo.py::test_x[a::b\] - assert False
  1. The tests pass after the patch is applied.
test_foo.py::test_x[x/y] FAILED                                          [ 20%]
test_foo.py::test_x[C:/path] FAILED                                      [ 40%]
test_foo.py::test_x[\\] FAILED                                           [ 60%]
test_foo.py::test_x[C:\\path] FAILED                                     [ 80%]
test_foo.py::test_x[a::b/] FAILED                                        [100%]

I had to make a minor adjustment to the new tests, as Python 3.11 and later requires passing the encoding_type param while calling the write_text method.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's cool @nauman897 !

  1. The tests pass after the patch is applied.
test_terminal.py::TestNodeIDHandling::test_nodeid_handling_windows_paths PASSED [100%]

This is a more intuitive output:

test_foo.py::test_x[x/y] FAILED                                          [ 20%]
test_foo.py::test_x[C:/path] FAILED                                      [ 40%]
test_foo.py::test_x[\\] FAILED                                           [ 60%]
test_foo.py::test_x[C:\\path] FAILED                                     [ 80%]
test_foo.py::test_x[a::b/] FAILED                                        [100%]

"""Test the correct handling of Windows-style paths with backslashes."""
pytester.makepyfile(
"""
import pytest

@pytest.mark.parametrize("a", ["x/y", "C:/path", "\\\\", "C:\\\\path", "a::b/"])
def test_x(a):
assert False
"""
)
result = pytester.runpytest("-v")

result.stdout.re_match_lines(
[
r".*test_nodeid_handling_windows_paths.py::test_x\[x/y\] .*FAILED.*",
r".*test_nodeid_handling_windows_paths.py::test_x\[C:/path\] .*FAILED.*",
r".*test_nodeid_handling_windows_paths.py::test_x\[\\\\\] .*FAILED.*",
r".*test_nodeid_handling_windows_paths.py::test_x\[C:\\\\path\] .*FAILED.*",
r".*test_nodeid_handling_windows_paths.py::test_x\[a::b/\] .*FAILED.*",
]
)
Loading