pytest-datadir icon indicating copy to clipboard operation
pytest-datadir copied to clipboard

Published 20 hours ago • verified publisher icon gabrielcnr

Add fixes for _win32_longpath

Open bilderbuchi opened this issue 5 years ago • 9 comments

This should fix #33.

I am not sure how I should test for this, as it depends on being on Windows and having UNC paths around.

bilderbuchi avatar Oct 14 '20 19:10 bilderbuchi

I first push a test that should fail on Appveyor before adding a fix.

bilderbuchi avatar Oct 14 '20 19:10 bilderbuchi

I am not sure how I should test for this, as it depends on being on Windows and having UNC paths around.

A unittest like the one you did looks sufficient. 👍

nicoddemus avatar Oct 14 '20 19:10 nicoddemus

Ha, inspiration strikes, I just realised that the path does not have to exist to check it.

Do you know what's up with this test failure, that does not seem to be related to me?

bilderbuchi avatar Oct 14 '20 19:10 bilderbuchi

OK, I am done, and it seems to work.

That CI failure from py35 onwards seems to be caused by a file in the test harness having an overly long name, which Windows/Python does not deal gracefully with. The fact that isfile() fails seems to support this. Dunno why this was not a problem before, or why not at lower Python versions?

bilderbuchi avatar Oct 14 '20 20:10 bilderbuchi

Could the test collection failure be a pytest problem? I see it working with pytest-4.6.11, and failing with pytest-6.1.1 where it was previously working with pytest-5.2.1.

bilderbuchi avatar Oct 15 '20 06:10 bilderbuchi

Btw, I just realised you also have Travis tests, but they are not displayed in this PR checks field. I think there was a necessary change in the Travis-Github authentication that you had to do so that does not break. I think the Travis account owner needs to do this.

bilderbuchi avatar Oct 15 '20 07:10 bilderbuchi

Hi @bilderbuchi - thanks for submitting this PR. We very much appreciate. I will take a look at this later this week and let you know about it. Stay tuned ;-)

gabrielcnr avatar Oct 26 '20 12:10 gabrielcnr

I looked here and the change and the tests look good. Thanks @bilderbuchi

I don't work on Windows, so it took me some effort to test this on Windows. The first thing is actually cloning the git repo on Windows failed because of the Windows path length limit/constraint. @nicoddemus I don't think we should have the test file committed there. Probably it would be worth generating it dynamically on the test. This may help to explain the problems on AppVeyor too.

image

gabrielcnr avatar Oct 27 '20 10:10 gabrielcnr

OK, thanks! Do you need anything from me here to proceed?

Btw, you'll probably have to test that "dynamically creating" the files actually works correctly (e.g. in a separate PR), I expect you will run into problems because Windows will also refuse to create those files. :-P

bilderbuchi avatar Nov 09 '20 20:11 bilderbuchi