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

Use pytest fixtures for tmp dir creation in tests #1988

Merged

Conversation

jenshnielsen
Copy link
Collaborator

@jenshnielsen jenshnielsen commented Apr 20, 2020

This should prevent the unable to unlink tmpdir because pytest has a reference to it failures that we see from time to time. https://docs.pytest.org/en/latest/tmpdir.html#temporary-directories-and-files

There are 3 context managers that I have still not changed. These should probably eventually be rewritten as pytest fixtures.

The pytest fixtures will only create one tmp_path pr test so in tests that requires 2 distinct directories I have used the factory functions to generate another one

@astafan8
Copy link
Contributor

There are 3 context managers that I have still not changed. These should probably eventually be rewritten as pytest fixtures.

in test_config.py? i just looked at them, i think that they can all be safely be rewritten with tmp_path. the more interesting one, config_file_context, that one seems to be a candidate for a "fixture factory" (if i remember correctly, pytest supports that)

@jenshnielsen
Copy link
Collaborator Author

There are 3 context managers that I have still not changed. These should probably eventually be rewritten as pytest fixtures.

in test_config.py? i just looked at them, i think that they can all be safely be rewritten with tmp_path. the more interesting one, config_file_context, that one seems to be a candidate for a "fixture factory" (if i remember correctly, pytest supports that)

Maybe but they all failed the first time round. I suggest doing them in a separarate pr since this one is working now

@codecov
Copy link

codecov bot commented Apr 20, 2020

Codecov Report

Merging #1988 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1988   +/-   ##
=======================================
  Coverage   70.59%   70.59%           
=======================================
  Files         148      148           
  Lines       19248    19248           
=======================================
  Hits        13588    13588           
  Misses       5660     5660           

@jenshnielsen jenshnielsen merged commit c59d56a into microsoft:master Apr 20, 2020
@jenshnielsen jenshnielsen deleted the use_pytest_fixtures_for_tmp_dir branch April 20, 2020 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants