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

remove generated dataset folders after download tests #3376

Merged
merged 2 commits into from
Feb 11, 2021

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Feb 11, 2021

While the actual download and extraction logic is mocked, some datasets still create an extra directory in the root directory:

os.makedirs(self.raw_folder, exist_ok=True)
os.makedirs(self.processed_folder, exist_ok=True)

os.makedirs(self.root, exist_ok=True)

With this, all tests use a temporary directory instead of the current directory as root for the datasets. This temporary directory is simply removed after all tests are run.

Another option would be to mock the function that creates the extra directory (here os.makedirs). While not complicated, we would than need to manually check during code review that either

  • only the already mocked functions are used to create directories, or
  • the new function is added to the list of mocked functions.

@codecov
Copy link

codecov bot commented Feb 11, 2021

Codecov Report

Merging #3376 (5669c78) into master (5905de0) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3376   +/-   ##
=======================================
  Coverage   74.80%   74.80%           
=======================================
  Files         105      105           
  Lines        9716     9716           
  Branches     1561     1561           
=======================================
  Hits         7268     7268           
  Misses       1961     1961           
  Partials      487      487           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5905de0...5669c78. Read the comment docs.

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

Looks good, I just left one comment. If you could provide a few more information so that we have history of how this works that would be awesome.



@pytest.fixture(scope="module", autouse=True)
def root():
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that you are using pytest specific functionalities here but could you elaborate on when this is triggered?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The autouse=True flag tells pytest to trigger this automatically even if it is not explicitly used. The scope="module" tells pytest to run this once per module. In general, everything before the yield is the setup whereas everything after it is the teardown.

Here, pytest triggers this fixture the first time any test in this module is run and finishes it when the last test is run.

@datumbox datumbox merged commit 5acd5c9 into pytorch:master Feb 11, 2021
@pmeier pmeier deleted the cleanup-download-tests branch February 11, 2021 14:07
facebook-github-bot pushed a commit that referenced this pull request Feb 12, 2021
Summary:
* remove generated dataset folders after download tests

* revert unnecessary style changes

Reviewed By: mthrok

Differential Revision: D26422439

fbshipit-source-id: e24fff8a183768ecd5bac57cc073d27f40e7f3e8
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.

3 participants