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 test_extract_(zip|tar|tar_xz|gzip) on windows #3542

Merged
merged 3 commits into from
Mar 11, 2021

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Mar 10, 2021

These tests were disabled before, because they would fail on Windows due to this construct

with tempfile.NamedTemporaryFile(suffix='.zip') as f:
with zipfile.ZipFile(f, 'w') as zf:
zf.writestr('file.tst', 'this is the content')

On Windows this raised a PermissionsError because zipfile.ZipFile tried to open an already open file. You can find a similar construct in all the disabled tests.

Given that all this already happens in a temporary directory that will be deleted after the test is complete, it is unnecessarily complicated to also create the names of the files at random. This PR fixes this which makes the tests more legible as well as able to run on Windows.

@pmeier pmeier requested a review from fmassa March 10, 2021 15:40
Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Thanks!

@fmassa fmassa merged commit 1a46ec9 into pytorch:master Mar 11, 2021
@pmeier pmeier deleted the reenable-extract-tests-win branch March 12, 2021 06:45
facebook-github-bot pushed a commit that referenced this pull request Mar 18, 2021
Summary:
* fix test_extract_(zip|tar|tar_xz|gzip) on windows

* lint

Reviewed By: fmassa

Differential Revision: D27127988

fbshipit-source-id: 62394146aef72ca5baf86ae86d52cf82f77c07aa

Co-authored-by: Francisco Massa <fvsmassa@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants