Prevent FD leak in RVsession tests #638
Conversation
* Added test for creating an rv session file with the sample file above
…tead of `start_frame` for frame substitution base
…and `tearDown()` methods to prevent fd leak
* * Added initial support for `ImageSequenceReference` * * Added sample otio file with `ImageSequenceReference` * Added test for creating an rv session file with the sample file above * * Use the `frame_zero_padding` from the `ImageSequenceReference` in stead of `start_frame` for frame substitution base * * renamed _in and _out variables * * lint fix
Codecov Report
@@ Coverage Diff @@
## imagesequence #638 +/- ##
==============================================
Coverage 83.04% 83.04%
==============================================
Files 74 74
Lines 2860 2860
==============================================
Hits 2375 2375
Misses 485 485
Continue to review full report at Codecov.
|
reinecke
left a comment
reinecke
left a comment
There was a problem hiding this comment.
Is it possible to avoid putting this in setUp and tearDown? It creates an implicit behavior of giving every test a tempfile whether or not it's using it.
Would the NamedTemporaryFile do what you're looking for:
with tempfile.NamedTemporaryFile("w+t", suffix=".rv") as temp:
otio.adapters.write_to_file(timeline, temp.name)
temp.seek(0)
test_data = temp.read()
This keeps the code easier to reason about locally and allows more complex tests to operate on multiple temporary files.
|
I'll update my comment because I just caught up with the thread on the original PR. |
|
OK, I'll give it a go. |
|
Hmm. |
reinecke
left a comment
reinecke
left a comment
There was a problem hiding this comment.
Given that the context managers are each painful in their own respect, I think your approach is reasonable.
I had just a few nitpicks but overall looks good!
contrib/opentimelineio_contrib/adapters/tests/test_rvsession.py
Outdated
Show resolved
Hide resolved
contrib/opentimelineio_contrib/adapters/tests/test_rvsession.py
Outdated
Show resolved
Hide resolved
contrib/opentimelineio_contrib/adapters/tests/test_rvsession.py
Outdated
Show resolved
Hide resolved
* Removed redundant assertions as suggested in PR comments
reinecke
left a comment
reinecke
left a comment
There was a problem hiding this comment.
Looks good to me! Thanks for resolving this!
* * create and delete temp file and file descriptors through `setUp()` and `tearDown()` methods to prevent fd leak * * Close tmp file descriptor in `setUp()` to avoid trouble in Windows * Removed redundant assertions as suggested in PR comments
5 participants
I moved the creation and deletion of the temp files and corresponding file descriptors in
setUp()andtearDown()methods.I suspect I've done something stupid in my merging of the upstream code, thus all the previous commits are tagging along in this PR. Please let me know if I should close this PR and start over. I think I did my previous change directly in the imagesequence branch and not a feature branch based on it. That might be what's causing this.
Apologize if this causes head aches.