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

Close Temporary file handles in tests #1363

Merged

Conversation

markreidvfx
Copy link
Contributor

I noticed these file handles were leaking when running tests on mingw64.
I matched the style that's in the files, but I think it would be better to use a TemporaryDirectory and cleanup when the test is done. Some tests are doing that, but it is a bit mixed.

Signed-off-by: Mark Reid <mindmark@gmail.com>
@codecov-commenter
Copy link

Codecov Report

Merging #1363 (7359ac7) into main (8847e90) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1363   +/-   ##
=======================================
  Coverage   86.27%   86.27%           
=======================================
  Files         196      196           
  Lines       19865    19871    +6     
  Branches     2309     2309           
=======================================
+ Hits        17138    17144    +6     
  Misses       2161     2161           
  Partials      566      566           
Flag Coverage Δ
py-unittests 86.27% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
tests/test_otiod.py 96.92% <100.00%> (+0.14%) ⬆️
tests/test_otioz.py 98.01% <100.00%> (+0.06%) ⬆️

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 8847e90...7359ac7. Read the comment docs.

@jminor
Copy link
Collaborator

jminor commented Jul 26, 2022

This looks like a good step forward to me. I always find Python's scoping rules for file handles confusing, and in this case the code really just wants a unique filename, not an actual file handle. Is there a standard pattern for cleaning up temp files in unit tests that we should be using here?

@markreidvfx
Copy link
Contributor Author

Currently some tests use unittest.TestCase setUp and tearDown methods

    def setUp(self):
        self.tmpDir = tempfile.mkdtemp()

    def tearDown(self):
        shutil.rmtree(self.tmpDir)

Some tests are using tempfile.TemporaryDirectory

with tempfile.TemporaryDirectory() as temp_dir:
    temp_file = os.path.join(temp_dir, "test_basic.otio")

Some tests are not cleaning up at all.

In pyaaf2 I write all test results to known location instead of temp. I also don't delete them so you can inspect them if a failure happens.

@JeanChristopheMorinPerso
Copy link
Member

In pyaaf2 I write all test results to known location instead of temp. I also don't delete them so you can inspect them if a failure happens.

If we were using Pytest (and here not just as a runner), we would get all these features. But that's for another day.

@jminor
Copy link
Collaborator

jminor commented Aug 1, 2022

There is some other discussion about using pytest here: #1364

I'm fine accepting this PR as-is, and then we can look into the improvements that pytest could give us as a separate change.

@reinecke reinecke merged commit d6ad4eb into AcademySoftwareFoundation:main Aug 18, 2022
@reinecke reinecke added this to the Public Beta 15 milestone Aug 18, 2022
MichaelPlug pushed a commit to MichaelPlug/OpenTimelineIO that referenced this pull request Aug 5, 2023
Signed-off-by: Mark Reid <mindmark@gmail.com>
Signed-off-by: Michele Spina <michelespina96@gmail.com>
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

5 participants