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

Clean up files written to tmpdir during tests #2505

Merged
merged 6 commits into from Jul 14, 2021

Conversation

freddyaboulton
Copy link
Contributor

@freddyaboulton freddyaboulton commented Jul 13, 2021

Pull Request Description

Fixes #2337

build_conda_pkg has passed three times already. It used to fail consistently when test_save_png_file was enabled. I ran locally with --basetemp mydir for main and --basetemp mydir-new-fixture for this branch. The total space on this branch is 0 bytes compared to 22 mb on main.

image


After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of docs/source/release_notes.rst to include this pull request by adding :pr:123.

@codecov
Copy link

codecov bot commented Jul 13, 2021

Codecov Report

Merging #2505 (eca6ea3) into main (e30c30c) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #2505     +/-   ##
=======================================
+ Coverage   99.7%   99.7%   +0.1%     
=======================================
  Files        283     283             
  Lines      25585   25589      +4     
=======================================
+ Hits       25483   25488      +5     
+ Misses       102     101      -1     
Impacted Files Coverage Δ
evalml/tests/conftest.py 98.3% <100.0%> (+0.1%) ⬆️
evalml/tests/pipeline_tests/test_graphs.py 100.0% <100.0%> (+0.8%) ⬆️

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 e30c30c...eca6ea3. Read the comment docs.



@pytest.fixture
def tmpdir_with_cleanup(tmpdir):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are other ways to clean up files after a test that don't involve using a new fixture (see here) but this seems simpler to me

Copy link
Contributor

Choose a reason for hiding this comment

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

why not just use one fixture instead of having one for the tmpdir and one for cleaning up? Are there some tests we don't want the clean up functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion. I don't think we ever want to not clean up after the test. I thought about this but tbh

@pytest.fixture
def tmpdir(tmpdir):
    yield tmpdir
    tmpdir.remove(ignore_errors=True)

looked weird to me. That being said, I will try the following:

@pytest.fixture
def tmpdir(tmp_path):
    dir = py.path.local(tmp_path)
    yield dir
    dir.remove(ignore_errors=True)

which I think will work and will involve less code changes!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed this up! Thanks for the suggestion - it's working nicely

@freddyaboulton freddyaboulton marked this pull request as ready for review July 14, 2021 14:36
@freddyaboulton freddyaboulton self-assigned this Jul 14, 2021
@@ -6,10 +6,6 @@ on:
- main
pull_request:
types: [ opened, synchronize ]
paths:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add back before merge

@pytest.fixture
def tmpdir_with_cleanup(tmpdir):
yield tmpdir
tmpdir.remove(ignore_errors=True)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested locally that this removes the directory even if the test fails.

Copy link
Contributor

@jeremyliweishih jeremyliweishih 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! just one question.



@pytest.fixture
def tmpdir_with_cleanup(tmpdir):
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just use one fixture instead of having one for the tmpdir and one for cleaning up? Are there some tests we don't want the clean up functionality?

Copy link
Contributor

@angela97lin angela97lin left a comment

Choose a reason for hiding this comment

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

I'm surprised that this isn't something built into tmpdir :( but looks good, thank you!

@freddyaboulton freddyaboulton merged commit 86e7a02 into main Jul 14, 2021
@freddyaboulton freddyaboulton deleted the 2337-enable-test-saving-png-file branch July 14, 2021 20:40
@chukarsten chukarsten mentioned this pull request Jul 22, 2021
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.

Allow test_saving_png_file to run during build_conda_pkg job
3 participants