Pytest for test_videoapi.py and test_video_reader.py #4233
Conversation
NicolasHug
left a comment
NicolasHug
left a comment
There was a problem hiding this comment.
Thanks for the PR @vmoens ! I made a few minor comments, but this looks good!
Also there are a few linting issue, which you can find by looking at the ci/circleci: python_lint job below.
You did well getting rid of TestCase, I'm glad we're finally done with it :)
test/test_videoapi.py
Outdated
| if __name__ == "__main__": | ||
| unittest.main() | ||
| if __name__ == '__main__': | ||
| pytest.main([__file__]) |
There was a problem hiding this comment.
Looks like this is missing a blank line at the end of the file
test/test_video_reader.py
Outdated
| vtimebase[0].item(), vtimebase[1].item() | ||
| ) | ||
| self.assertAlmostEqual(video_duration, config.duration, delta=0.5) | ||
| assert abs(video_duration-config.duration)<=0.5 |
There was a problem hiding this comment.
have you tried using pytest.approx here? https://docs.pytest.org/en/6.2.x/reference.html#pytest-approx
I think we should be able to use it everywhere assertAlmostEqual was used
test/test_video_reader.py
Outdated
| def test_probe_video_from_memory_script(self): | ||
| scripted_fun = torch.jit.script(io._probe_video_from_memory) | ||
| self.assertIsNotNone(scripted_fun) | ||
| assert scripted_fun |
There was a problem hiding this comment.
in order to be slightly more explicit about what we don't want scripted_fun to be, let's use assert scripted_fun is not None
test/test_video_reader.py
Outdated
| full_path, start_offset, end_offset, pts_unit='pts') | ||
| self.assertGreaterEqual(audio.shape[0], 1) | ||
| self.assertGreaterEqual(audio.shape[1], 1) | ||
| assert all(audio.shape[:2]) |
There was a problem hiding this comment.
here as well it will probably be more obvious to include the >= 1 part in the assertion
test/test_video_reader.py
Outdated
| full_path, start_offset, end_offset, pts_unit='sec') | ||
| self.assertGreaterEqual(audio.shape[0], 1) | ||
| self.assertGreaterEqual(audio.shape[1], 1) | ||
| assert all(audio.shape[:2]) |
test/test_video_reader.py
Outdated
| if __name__ == "__main__": | ||
| unittest.main() | ||
| if __name__ == '__main__': | ||
| pytest.main([__file__]) |
There was a problem hiding this comment.
needs a new line here as well :)
test/test_videoapi.py
Outdated
|
|
||
| self.assertTrue(middle_num_frames < num_frames) | ||
| self.assertAlmostEqual(middle_num_frames, num_frames // 2, delta=1) | ||
| assert (middle_num_frames < num_frames) |
There was a problem hiding this comment.
Here and in various places, we don't need the surrounding parenthesis. It's a nit though, feel free not to address!
NicolasHug
left a comment
NicolasHug
left a comment
There was a problem hiding this comment.
Thanks a lot @vmoens !
I made 2 minor comments but LGTM, feel free to self merge once addressed
test/test_video_reader.py
Outdated
|
|
||
| scripted_fun = torch.jit.script(io._read_video_from_memory) | ||
| self.assertIsNotNone(scripted_fun) | ||
| assert scripted_fun |
There was a problem hiding this comment.
| assert scripted_fun | |
| assert scripted_fun is not None |
test/test_videoapi.py
Outdated
| lb = duration / 2 - 1 / md[stream]["fps"][0] | ||
| ub = duration / 2 + 1 / md[stream]["fps"][0] | ||
| self.assertTrue((lb <= frame["pts"]) & (ub >= frame["pts"])) | ||
| assert (lb <= frame["pts"]) & (ub >= frame["pts"]) |
There was a problem hiding this comment.
It's not originally from this PR, but should this be and instead of &?
|
Hey @vmoens! You merged this PR, but no labels were added. |
Summary: * test_video_reader pytest refactoring * pytest refactoring of test_videoapi.py * test_video_reader pytest refactoring * pytest refactoring of test_videoapi.py * using pytest.approx for test_video_reader.py * using pytest.approx for test_videoapi.py * Fixing minor comments * linting fixes * minor comments Reviewed By: NicolasHug Differential Revision: D30069951 fbshipit-source-id: 26ef4fe4c8373231c764cf8d557b770447563edc Co-authored-by: Vincent Moens <[email protected]>
3 participants
I also removed the common_utils.py/TestCase class which -- as far as i can tell - wasn't used anywhere. This is probably the most contentious point of this PR and needs proper reviewing!