Skip to content

Fix windows unit tests #3294

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

Merged
merged 9 commits into from
Jan 17, 2016
Merged

Conversation

xavfernandez
Copy link
Member

Review on Reviewable

@pfmoore
Copy link
Member

pfmoore commented Dec 5, 2015

Well, the error is

        with _secure_open_write(lock.path, self.filemode) as fh:
            fh.write(value)
            TypeError: 'str' does not support the buffer interface

and value is a string:

    cache = SafeFileCache(cache_dir)
    cache.set("foo", "bar")

But see https://github.com/pypa/pip/blob/develop/pip/_vendor/cachecontrol/caches/file_cache.py#L44 - _secure_open_write opens the file in binary mode.

This seems like it's a simple bug, nothing to do with Windows (although maybe there's some oddity in Unix that means the error isn't getting picked up there?)

@xavfernandez
Copy link
Member Author

This seems like it's a simple bug, nothing to do with Windows (although maybe there's some oddity in Unix that means the error isn't getting picked up there?)

Yes it seems simple but there should be no difference for unix/windows :'(

@pfmoore
Copy link
Member

pfmoore commented Dec 5, 2015

Agreed. But I'd focus on working out why the test doesn't fail on Unix first, as I think it should. Even if you confirm that the Unix behaviour is correct, then knowing why should help work out what's wrong on Windows.

@xavfernandez xavfernandez force-pushed the fix_windows_unit_tests branch 3 times, most recently from 6367bad to 0771a19 Compare December 17, 2015 21:40
@xavfernandez xavfernandez force-pushed the fix_windows_unit_tests branch 2 times, most recently from b738f79 to 3242170 Compare January 4, 2016 21:11
@xavfernandez xavfernandez added this to the 8.0 milestone Jan 4, 2016
@xavfernandez
Copy link
Member Author

@pfmoore The test actually makes the directory unavailable before calling cache.set("foo", "bar") making it a no-op: the test does not even attempt to write.

But on Windows os.chmod does not make the directory unavailable, meaning the cache is used and the tests crashed.

"Fixed" it via a063654

@pfmoore
Copy link
Member

pfmoore commented Jan 4, 2016

Ah, I see. Yes, chmod doesn't do that on Windows (indeed, chmod is largely meaningless on Windows, the Windows permission model is very different from the Unix one). Nice catch.

@xavfernandez
Copy link
Member Author

Also caught an issue with os.path.samefile thanks to appveyor :)

But for the failing tests with python3.5 it looks more like a pytest issue: https://ci.appveyor.com/project/xavfernandez/pip/build/job/vb2mx08p8rkjo056

So I think I'll merge it as is. Maybe we could add a hook on pypa/pip for the unit tests on windows without 3.5 ? (This would have caught the samefile issue).

@xavfernandez
Copy link
Member Author

It looks like it was fixed in pytest-dev/pytest@b18e643 so pytest-2.7.3 .

And of course we are pinned to 2.7.2 due to pytest-dev/pytest#1083 😐

@xavfernandez xavfernandez force-pushed the fix_windows_unit_tests branch from a063654 to dda0379 Compare January 5, 2016 09:11
@xavfernandez xavfernandez force-pushed the fix_windows_unit_tests branch 2 times, most recently from dc1d896 to f159871 Compare January 15, 2016 09:19
@xavfernandez xavfernandez force-pushed the fix_windows_unit_tests branch from f159871 to b998ab6 Compare January 17, 2016 21:40
xavfernandez added a commit that referenced this pull request Jan 17, 2016
@xavfernandez xavfernandez merged commit 2f51af2 into pypa:develop Jan 17, 2016
@xavfernandez
Copy link
Member Author

Merged this as is, to have xavfernandez@b47c144 in pip 8.0.
The unit tests seem to pass, it would be good to add an appveyor link when we will be able to unpin pytest and have the unit tests pass on all supported versions.

@xavfernandez xavfernandez deleted the fix_windows_unit_tests branch January 17, 2016 22:10
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 3, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants