Skip to content
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.

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

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

Merged
merged 11 commits into from
Sep 8, 2024

Conversation

nauman897
Copy link
Contributor

Fix slash handling in nodeid paths for Windows environments.

@nauman897
Copy link
Contributor Author

I’ve tackled the issue with backslashes getting messed up in nodeid paths on Windows by isolating the path and nodeid parts, handling them separately, and then putting them back together. This should take care of the problem, including a few tricky edge cases with mixed slashes and special characters.

I haven’t added unit tests yet—figured I’d wait to see if the approach looks good to you all. If it does, I’ll go ahead and write the tests to make sure everything’s covered.

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Sep 3, 2024
Co-authored-by: dongfangtianyu <7629022+dongfangtianyu@users.noreply.github.com>
Copy link
Contributor

@dongfangtianyu dongfangtianyu left a comment

Choose a reason for hiding this comment

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

Thank you @nauman897 for this PR.
I have debugged and verified it locally, and everything is fine.



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

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%]

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks folks, we greatly appreciate it!

@nicoddemus nicoddemus added the backport 8.3.x apply to PRs at any point; backports the changes to the 8.3.x branch label Sep 8, 2024
@nicoddemus nicoddemus merged commit d35b802 into pytest-dev:main Sep 8, 2024
29 checks passed
Copy link

patchback bot commented Sep 8, 2024

Backport to 8.3.x: 💚 backport PR created

✅ Backport PR branch: patchback/backports/8.3.x/d35b802805b210c6d5281864d663f53c6a72f153/pr-12760

Backported as #12787

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Sep 8, 2024
nicoddemus pushed a commit that referenced this pull request Sep 8, 2024
…) (#12787)

Fix #12745

(cherry picked from commit d35b802)

Co-authored-by: Nauman Ahmed <90570675+nauman897@users.noreply.github.com>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 8.3.x apply to PRs at any point; backports the changes to the 8.3.x branch bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slashes in parametrization values get turned into backslashes on Windows
4 participants