Remove remaining tests from collection #1142
Conversation
meta-cla
bot
added
the
CLA Signed
| # This just validates that we can decode 10-bit videos. | ||
| # TODO validate against the ref that the decoded frames are correct | ||
|
|
||
| if device == "cuda:beta" and asset is H264_10BITS: |
There was a problem hiding this comment.
no longer applicable because the BETA interface fallsback to the CPU now
Dan-Flores
left a comment
Dan-Flores
left a comment
There was a problem hiding this comment.
Thanks for working on this @mollyxu! Lets focus these changes on the cases where we can simply swap out pytest.skip. I left some comments on the changes where I think the value tradeoff for increased complexity is less clear.
NicolasHug
left a comment
NicolasHug
left a comment
There was a problem hiding this comment.
Thanks Molly, I made a quick pass, this is good! I agree with Daniel's point above that the extra complexity in some places is probably not worth the added benefit.
test/conftest.py
Outdated
| # Current we skip all marked tests internally, whether they are marked | ||
| # with needs_ffmpeg_cli, skip, or skipif. Future skipped tests | ||
| # should follow the pattern of needs_cuda and needs_ffmpeg_cli if we are | ||
| # skipping a test because of a dependency. |
There was a problem hiding this comment.
I have a bit of trouble understanding this comment, can you clarify what we mean by "follow the pattern of needs_cuda and needs_ffmpeg_cli if we are skipping a test because of a dependency" ?
I think that part of the reason I'm confused is that this mentioned needs_cuda, which isn't used within this if in_fbcode() block - maybe this comment was meant to be somewhere else?
There was a problem hiding this comment.
Thanks for the review! The original comment was a bit confusing so I clarified it and moved it to a better section. The intention was to encourage people to follow the decorator pattern for needs_cuda and needs_ffmpeg_cli rather than individual skipwhen we are conditionally skipping tests due to dependencies.
test/test_decoders.py
Outdated
| assert_frames_equal(ref_frame3, frames[1].data) | ||
| assert_frames_equal(ref_frame5, frames[2].data) | ||
|
|
||
| @pytest.mark.skip(reason="TODO: Need video with no pts values.") |
There was a problem hiding this comment.
Make sure to also move the related comment block here as well. Starting with "The test video we have ...".
|
Thanks @Dan-Flores @NicolasHug for the review! |
3 participants
Remove remaining skipped tests from collection in internal environment
Prevent internal CI failures from skipped tests by excluding skipped tests from collection in internal environment.
TorchCodec tests were flaky / broken internal because:
pytest.skip()calls, so they were collected but then skipped during executionFollow up to #1068
Changes Made
1. Replaced most instances of
pytest.skip()with@pytest.mark.skipif()2. Updated
conftest.pyto document our test collection logic3. Removed skip that was no longer applicable in
test_10bit_videos