Move internet tests in single file #3579
Conversation
|
There are still 3 tests that I need to port: those that use Naively porting these tests would require importing their base class I would suggest to just download the 2 test images and put them in the assets directory instead, so that these tests don't even make any internet call. Alternatively, I can extract the |
Codecov Report
@@ Coverage Diff @@
## master #3579 +/- ##
=======================================
Coverage 79.07% 79.07%
=======================================
Files 105 105
Lines 9787 9787
Branches 1572 1572
=======================================
Hits 7739 7739
Misses 1567 1567
Partials 481 481 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
- Just FYI: Everything in
test/test_datasets_download.pyalso requires internet connection, but these tests are already ignored in our normal runs
- Although I like the idea behind this change, I think it is problematic that these tests are now grouped by requirements. If we could depend on
pytestwe could simply use@pytest.mark.internetmarker on these tests and add a--skip-internetcommand line option. This can be done without any additional plugins. For context, I also use@pytest.mark.slowfor slow tests (runtime > 1 s) to be able to run a large portion of the test suite significantly faster.
test/test_internet.py
Outdated
| @unittest.skipIf(_HAS_VIDEO_OPT is False, "Didn't compile with ffmpeg") | ||
| @PY39_SKIP | ||
| class VideoAPITester(unittest.TestCase): | ||
|
|
||
| VIDEO_DIR = os.path.join(os.path.dirname(os.path.abspath(__file__)), "assets", "videos") | ||
|
|
||
| def _fate(self, name, path="."): | ||
| """Download and return a path to a sample from the FFmpeg test suite. | ||
| See the `FFmpeg Automated Test Environment <https://www.ffmpeg.org/fate.html>`_ | ||
| """ | ||
|
|
||
| file_name = name.split("/")[1] | ||
| utils.download_url("http://fate.ffmpeg.org/fate-suite/" + name, path, file_name) | ||
| return os.path.join(path, file_name) | ||
|
|
||
| def test_fate_suite(self): | ||
| video_path = self._fate("sub/MovText_capability_tester.mp4", self.VIDEO_DIR) | ||
| vr = VideoReader(video_path) | ||
| metadata = vr.get_metadata() | ||
|
|
||
| self.assertTrue(metadata["subtitles"]["duration"] is not None) | ||
| os.remove(video_path) |
There was a problem hiding this comment.
Let's leave this test where it was before. We currently don't run those tests in FB, and I'm unsure about the license for those videos. cc @bjuncek
|
@pmeier skipped tests don't play well with our internal infra, so using markers wouldn't work. And we don't run |
Summary: * Put internet tests in single file * adding the file * edit contirbuting guide * restore videoapi test Reviewed By: fmassa Differential Revision: D27127990 fbshipit-source-id: bb68aba1c422d31f158390d963e683116ca35858
This PR moves all internet-related tests in a single file. This will make ignoring these tests easier on fb internals.
To make sure I didn't miss any tests, I used https://github.com/miketheman/pytest-socket to disable all internet access, and ported those that failed