-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 collection of short paths on Windows #11936
Conversation
1024598
to
f52d637
Compare
The initial commit was used just to demonstrate the failing test on CI, however we should squash/merge this in the end. |
Each call to |
Should be possible, I will look into it, however I think it will change the resulting node ids. |
1186137
to
7ed44fb
Compare
Done, it was a bit more code but definitely won't impact performance. |
@bluetech just stumbled into this: Lines 595 to 605 in 7690a0d
This suggests we might be not considering other cases if we try to just expand the short names during collection. Perhaps we should bite the bullet and just go with the stat solution? Less code and will be the safest one, while I understand there's the performance concern. |
@bluetech what do you think regarding my last point? |
@nicoddemus I'll try to make an informed review, but if I take too long (like before 8.0.2) then please don't wait for me, I don't mean to block this. |
I used https://github.com/nicoddemus/collection-test-bed to measure the performance so we can have some numbers to decide. Using that configuration we have 10.5k tests 4 levels deep, with test files containing simple parametrized tests, here are the numbers in my Windows using
So indeed the Testing in a more realist test suite from a project I have around with 1.3k tests, I couldn't measure any difference between the So while we can see some difference in a synthetic test suite, with 10k tests but without any other imports, using a realistic test suite, with 1.3k tests and which imports application code, the difference is not perceptible at all. I'm leaning towards the stat solution as it is simpler and also more correct in my opinion. |
Passing a short path in the command line was causing the matchparts check to fail, because ``Path(short_path) != Path(long_path)``. Using ``os.path.samefile`` as fallback ensures the comparsion works on Windows when comparing short/long paths. Fix pytest-dev#11895
This reverts commit 2e8f957.
7b04075
to
abb43a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, and I also favor the stat
solution - correctness is definitely worth it IMO.
Going ahead and merge this then, we can revisit later if needed. 👍 |
The check for short paths under Windows via os.path.samefile, introduced in pytest-dev#11936, also found similar tests in symlinked tests in the GH Actions CI. This checks additionally that one of the files is not a symlink. Fixes pytest-dev#12039.
The check for short paths under Windows via os.path.samefile, introduced in pytest-dev#11936, also found similar tests in symlinked tests in the GH Actions CI. This checks additionally that one of the files is not a symlink. Fixes pytest-dev#12039.
The check for short paths under Windows via os.path.samefile, introduced in pytest-dev#11936, also found similar tests in symlinked tests in the GH Actions CI. This checks additionally that one of the files is not a symlink. Fixes pytest-dev#12039.
The check for short paths under Windows via os.path.samefile, introduced in pytest-dev#11936, also found similar tests in symlinked tests in the GH Actions CI. This checks additionally that one of the files is not a symlink. Fixes pytest-dev#12039.
The check for short paths under Windows via os.path.samefile, introduced in #11936, also found similar tests in symlinked tests in the GH Actions CI. Fixes #12039. Co-authored-by: Bruno Oliveira <[email protected]>
Passing a short path in the command line was causing the matchparts check to fail, because ``Path(short_path) != Path(long_path)``. Using ``os.path.samefile`` as fallback ensures the comparsion works on Windows when comparing short/long paths. Fix pytest-dev#11895
The check for short paths under Windows via os.path.samefile, introduced in pytest-dev#11936, also found similar tests in symlinked tests in the GH Actions CI. Fixes pytest-dev#12039. Co-authored-by: Bruno Oliveira <[email protected]>
Passing a short path in the command line was causing the matchparts check to fail, because
Path(short_path) != Path(long_path)
.Using
os.path.samefile
as fallback ensures the comparison works on Windows when comparing short/long paths.Fix #11895