test: Updated asserts in test_io #1496
Conversation
Updated all raw asserts to corresponding unittest.TestCase.assert. See pytorch#1483
test/test_io.py
Outdated
| @@ -53,8 +53,8 @@ def _create_video_frames(num_frames, height, width): | |||
| @contextlib.contextmanager | |||
| def temp_video(num_frames, height, width, fps, lossless=False, video_codec=None, options=None): | |||
| if lossless: | |||
| assert video_codec is None, "video_codec can't be specified together with lossless" | |||
| assert options is None, "options can't be specified together with lossless" | |||
| self.assertIsInstance(video_codec, type(None), "video_codec can't be specified together with lossless") | |||
There was a problem hiding this comment.
This is not correct, temp_video is not a class method but a free function, so it doesn't have a self .
There was a problem hiding this comment.
My bad, I had the same issue in the first place but didn't commit the right version of it
In order to use unittest.TestCase for asserts in temp_video, the object has to be passed itself to temp_video.
test/test_io.py
Outdated
| @@ -51,10 +51,10 @@ def _create_video_frames(num_frames, height, width): | |||
|
|
|||
|
|
|||
| @contextlib.contextmanager | |||
| def temp_video(num_frames, height, width, fps, lossless=False, video_codec=None, options=None): | |||
| def temp_video(tester, num_frames, height, width, fps, lossless=False, video_codec=None, options=None): | |||
There was a problem hiding this comment.
I don't think this is a good idea. This is a function that is independent on the tester, so I think it should stay free from it.
From my view, if you want to remove the assert here (which is outside of the test btw), I'd do something like
if video_codec is not None:
raise ValueError(...)There was a problem hiding this comment.
Good idea, I'll update the PR. Two quick questions:
- would you prefer a
AssertionErrorrorValueError? - was it for error clarity that both verifications are not performed at once?
There was a problem hiding this comment.
I would prefer ValueError actually.
This is not a test case, but a helper function, which could be used outside of testing, so having a more precise error type would be helpful
Switched initial raw assert to AssertionError.
Codecov Report
@@ Coverage Diff @@
## master #1496 +/- ##
==========================================
+ Coverage 64.46% 64.49% +0.03%
==========================================
Files 83 83
Lines 6421 6428 +7
Branches 982 984 +2
==========================================
+ Hits 4139 4146 +7
Misses 1992 1992
Partials 290 290
Continue to review full report at Codecov.
|
This PR updates all raw
assertto the correspondingunittest.TestCaseassert method for tests related totorchvision.ioas suggested by #1483.The specific test was run locally with success, any feedback is welcome!