capture_unittests should fail if no camera is found |
||||||||
Issue descriptionCapture_unittests currently returns when no camera is found in test cases which require camera devices (e.g. [1]). So, capture_unittests cannot catch bugs in the feature for enumerating devices. However, just changing them into failures won't work because capture_unittests is running even on devices that have no camera indeed (e.g. Chromebox). To resolve this problem, we should split capture_unittests to 2 suites; one that doesn't require a camera and one that does. The latter suite will run only on devices that has cameras. [1] https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device_unittest.cc?l=505&rcl=e89480287d6a22f042d96c54586004e6ef861a2a
,
Oct 9
,
Oct 9
chfremer@, I assign this issue to you for now. When we decide how we will work on this, let's re-assign. I'm not sure how we should split the current_unittests. Should they be different binaries? Or, should we use command line parameters to specify which tests run? Any idea?
,
Oct 9
I think a command-line parameter or a test case name prefix would be suitable. We already started using the prefix UsingRealWebcam_ in several places to distinguish tests that try to access real capture devices in order to make sure we run them sequentially and never in parallel. Maybe a prefix RequiresRealCamera_ could be used here. Another question: Is the only reason we want to do this split to be able to get coverage of code relevant for enumerating devices? If that is the case, would we be able to get such coverage without requiring real cameras, e.g. by mocking out a user-space API as done in https://cs.chromium.org/chromium/src/media/capture/video/linux/video_capture_device_factory_linux_unittest.cc?dr=CSs&g=0&l=91 ?
,
Oct 10
Using a prefix sounds reasonable. Then, we can filter them by gtest's command-line option. But, I wonder if we should distinguish "UsingRealWebcam_" and "RequiresRealCamera_". Is there any special reason for this? "RequiresRealCamera_" tests also need to be run sequentially, I think. -- Another reason for splitting tests is that I don't think tests should say "PASS" when they are not actually performed. For example, a test "TakePhoto" passes when no camera is found *although* any photo feature is not exercised. It's confusing. Also, we can save time and resources in test infra by splitting test suites, because we can avoid running "UsingRealWebcam" tests on devices that have no camera.
,
Oct 10
Just to build on top of what Keiichi said, we generally want to avoid cases where a part of the underlying software stack has a bug which leads the camera not to be detected, but the tests keep passing, because the test setup just invisibly skips them if no camera is present.
,
Oct 10
I agree, we don't need two different prefixes, as long as we can make sure to correctly configure the gtest_filter command-line option everywhere where such tests are run. And I also agree it is problematic to have tests quietly skipping. I have to admit that I don't think I know all those places, though. The Chromium CQ is certainly one important place, and the other is the webrtc builders, i.e. buildbot/chromium.webrtc.fyi/* and buildbot/chromium.webrtc/* on https://ci.chromium.org/p/chromium/builders. But there may be others that I am not aware of.
,
Oct 11
+oprypin@ and phoglund@, who are owners of buildbot/chromium.webrtc/. Note that this discussion came from [1] in go/cros-camera-waterfall-test. --- I took a look at /src/testing/buildbot/test_suites.pyl and found that capture_unittests are used in three tests: - chromeos_gtests [2] - chromium_gtests [3] - webrtc_chromium_baremetal_gtests [4] "chromeos_gtests" is the one that I'm working on. Since I'm planning to use vivid (virtual video input device) for it, we will be able to run "UsingRealWebcam_" tests there. In "chromium_gtests", capture_unittests is called without "--test-launcher-jobs=1", which is required when a real webcam is used IIUC. So, I guess chromium_gtests isn't used for platforms that have real cameras. Then, we should exclude "UsingRealWebcam_" tests here. For "webrtc_chromium_baremetal_gtests", we can run "UsingRealWebcam_" tests because the comment says that their bots have real webcams. According to the search result [5], waterfalls.pyl also includes some "capture_unittests". However, it seems that they just build and don't run capture_unittests because they are in "additional_compile_targets". --- So, I think we can achieve this task by the following way: * Add the prefix "UsingRealWebcam_" to test cases in capture_unittests that use real webcams * Make "UsingRealWebcam_" tests fail when no camera is found * Add a command-line option to exclude "UsingRealWebcam_" tests for "chromium_gtests". Does it make sense? [1] https://docs.google.com/a/google.com/document/d/1h8BEmDdUzfbaIR6_4WiBBh_gNOb8gVdJi1kX8SbdoTk/edit?disco=AAAACPba4BA [2] https://cs.chromium.org/chromium/src/testing/buildbot/test_suites.pyl?l=368&rcl=f34485ffdec1a7ac058941794747fbec1c83ece6 [3] https://cs.chromium.org/chromium/src/testing/buildbot/test_suites.pyl?l=567&rcl=f34485ffdec1a7ac058941794747fbec1c83ece6 [4] https://cs.chromium.org/chromium/src/testing/buildbot/test_suites.pyl?l=2280&rcl=f34485ffdec1a7ac058941794747fbec1c83ece6 [5] https://cs.chromium.org/search/?q=capture_unittest+file:pyl+file:%5Esrc/testing/buildbot/+package:%5Echromium$&type=cs
,
Oct 11
I create a CL: https://chromium-review.googlesource.com/c/chromium/src/+/1276407
,
Oct 11
Note to self: After this change, we will need to exclude CaptureMjpeg when we run capture_unittests on ChromeOS VM with vivid.
,
Oct 11
re #8: Thanks for the thorough analysis. Yes I think this makes sense.
,
Oct 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dfeab488cdc95233b19b9edeedef3d0b7581b17e commit dfeab488cdc95233b19b9edeedef3d0b7581b17e Author: Keiichi Watanabe <keiichiw@chromium.org> Date: Fri Oct 12 04:50:26 2018 [Video Capture, Test] Make tests requiring real webcam fail if it's not found Use prefix "UsingRealWebcam_" for names of tests that require real webcam and add checking whether webcam is found or not. Accordingly, exclude such test cases from test suites for devices that have no camera. Bug: 893494 Test: Ran capture_unittests including UsingRealWebcam_ tests on Chromebox (guado) and it failed. Test: Ran capture_unittests excluding UsingRealWebcam_ tests on Chromebox (guado) and it passed. Test: Ran capture_unittests including UsingRealWebcam_ tests on Chromebook (kevin) and it passed. Change-Id: I5c91380b7674aeace6a7d1764f7fad31f02f8af7 Reviewed-on: https://chromium-review.googlesource.com/c/1276407 Commit-Queue: Keiichi Watanabe <keiichiw@chromium.org> Reviewed-by: Christian Fremerey <chfremer@chromium.org> Reviewed-by: Ben Pastene <bpastene@chromium.org> Cr-Commit-Position: refs/heads/master@{#599097} [modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/media/capture/video/video_capture_device_unittest.cc [modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/testing/buildbot/chromium.android.fyi.json [modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/testing/buildbot/chromium.android.json [modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/testing/buildbot/chromium.chromiumos.json [modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/testing/buildbot/chromium.clang.json [modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/testing/buildbot/chromium.fyi.json [modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/testing/buildbot/chromium.linux.json [modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/testing/buildbot/chromium.mac.json [modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/testing/buildbot/chromium.memory.json [modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/testing/buildbot/chromium.win.json [modify] https://crrev.com/dfeab488cdc95233b19b9edeedef3d0b7581b17e/testing/buildbot/test_suites.pyl
,
Oct 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7bb43df173039894cf46c8b3476913f0f20ae6d1 commit 7bb43df173039894cf46c8b3476913f0f20ae6d1 Author: Henrik Grunell <grunell@chromium.org> Date: Fri Oct 12 06:57:19 2018 Revert "[Video Capture, Test] Make tests requiring real webcam fail if it's not found" This reverts commit dfeab488cdc95233b19b9edeedef3d0b7581b17e. Reason for revert: Tests fail on Chromium WebRTC bots. Example: https://ci.chromium.org/buildbot/chromium.webrtc/Mac%20Tester/84091 Original change's description: > [Video Capture, Test] Make tests requiring real webcam fail if it's not found > > Use prefix "UsingRealWebcam_" for names of tests that require real webcam and > add checking whether webcam is found or not. > Accordingly, exclude such test cases from test suites for devices that have no > camera. > > Bug: 893494 > Test: Ran capture_unittests including UsingRealWebcam_ tests on Chromebox (guado) > and it failed. > Test: Ran capture_unittests excluding UsingRealWebcam_ tests on Chromebox (guado) > and it passed. > Test: Ran capture_unittests including UsingRealWebcam_ tests on Chromebook (kevin) > and it passed. > > Change-Id: I5c91380b7674aeace6a7d1764f7fad31f02f8af7 > Reviewed-on: https://chromium-review.googlesource.com/c/1276407 > Commit-Queue: Keiichi Watanabe <keiichiw@chromium.org> > Reviewed-by: Christian Fremerey <chfremer@chromium.org> > Reviewed-by: Ben Pastene <bpastene@chromium.org> > Cr-Commit-Position: refs/heads/master@{#599097} TBR=bpastene@chromium.org,chfremer@chromium.org,keiichiw@chromium.org Change-Id: I282dc00ffad2ff6614e7dfab1a9404da85cf032a No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 893494 Reviewed-on: https://chromium-review.googlesource.com/c/1278272 Reviewed-by: Henrik Grunell <grunell@chromium.org> Commit-Queue: Henrik Grunell <grunell@chromium.org> Cr-Commit-Position: refs/heads/master@{#599123} [modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/media/capture/video/video_capture_device_unittest.cc [modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/testing/buildbot/chromium.android.fyi.json [modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/testing/buildbot/chromium.android.json [modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/testing/buildbot/chromium.chromiumos.json [modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/testing/buildbot/chromium.clang.json [modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/testing/buildbot/chromium.fyi.json [modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/testing/buildbot/chromium.linux.json [modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/testing/buildbot/chromium.mac.json [modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/testing/buildbot/chromium.memory.json [modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/testing/buildbot/chromium.win.json [modify] https://crrev.com/7bb43df173039894cf46c8b3476913f0f20ae6d1/testing/buildbot/test_suites.pyl
,
Oct 12
The CL in #12 caused failures on Win/Mac builders in chromium.webrtc and was reverted by #13. There is a discussion on https://chromium-review.googlesource.com/c/chromium/src/+/1278272/1/testing/buildbot/test_suites.pyl#2282. I submitted the fixed version of CL: crrev.com/c/1278432
,
Oct 12
The webrtc bots seem to have bots called "... Builder", e.g. https://ci.chromium.org/buildbot/chromium.webrtc/Linux%20Builder, and bots called "... Tester", e.g. https://ci.chromium.org/buildbot/chromium.webrtc/Linux%20Tester. I assume that the ones called Builder do not have webcams, but the ones called Tester do. We need to make sure that the tests keep running on the ones called Tester. Otherwise we lose crucial test coverage.
,
Oct 12
#15: IIUC, while Builders just do build and don't run tests, Testers do actually tests. For example, this is my reverted CL's log on Mac Builder: https://ci.chromium.org/buildbot/chromium.webrtc/Mac%20Builder/104344 As far as I see, it only built files and capture_unittests didn't run here. You can see Builders as "bld" and Testers as "tst" here. https://ci.chromium.org/p/chromium/g/chromium.webrtc/console So, I think we only need to consider Testers for this issue. In my new CL (crrev.com/c/1278612), I added a filter in "AddTestSpec", which is used for Testers' settings.
,
Oct 12
I found a document about Buiders and Testers, which seems to help my comment #16: https://www.chromium.org/developers/testing/chromium-build-infrastructure/tour-of-the-chromium-buildbot#TOC-Builders-vs-Testers
,
Oct 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/build/+/ac1ce788ac4d015fa6ce23d3438337ab12ac07ce commit ac1ce788ac4d015fa6ce23d3438337ab12ac07ce Author: Keiichi Watanabe <keiichiw@chromium.org> Date: Mon Oct 15 07:09:16 2018 Stop running capture unittests requiring real webcams in chromium.webrtc We decided to make capture_unittests's tests fail if no webcam is found. Since real webcams seem unavailable on Windows/Mac builders (cf. CL:1278272), we exclude such test cases there in chromium.webrtc. Bug: 893494 Change-Id: I98a6e107a230726fdd9dd54c89e4a0048bdfd14d Reviewed-on: https://chromium-review.googlesource.com/c/1278612 Reviewed-by: Oleh Prypin <oprypin@chromium.org> Reviewed-by: Patrik Höglund <phoglund@chromium.org> Commit-Queue: Patrik Höglund <phoglund@chromium.org> Auto-Submit: Keiichi Watanabe <keiichiw@chromium.org> [modify] https://crrev.com/ac1ce788ac4d015fa6ce23d3438337ab12ac07ce/scripts/slave/recipe_modules/chromium_tests/chromium_webrtc.py
,
Oct 15
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b332e81bd632b21d2cbb4634f30e08768b687bb5 commit b332e81bd632b21d2cbb4634f30e08768b687bb5 Author: Keiichi Watanabe <keiichiw@chromium.org> Date: Mon Oct 15 09:25:34 2018 [Video Capture, Test] Make tests requiring real webcam fail if it's not found Use prefix "UsingRealWebcam_" for names of tests that require real webcam and add checking whether webcam is found or not. Accordingly, exclude such test cases from test suites for devices that have no camera. and it failed. and it passed. and it passed. Bug: 893494 Test: Ran capture_unittests including UsingRealWebcam_ tests on Chromebox (guado) Test: Run capture_unittests excluding UsingRealWebcam_ tests on Chromebox (guado) Test: Run capture_unittests including UsingRealWebcam_ tests on Chromebook (kevin) Change-Id: I9ba9657b47afadf6f3128cdcb91e74e1793040b4 Reviewed-on: https://chromium-review.googlesource.com/c/1278432 Reviewed-by: Christian Fremerey <chfremer@chromium.org> Reviewed-by: Ben Pastene <bpastene@chromium.org> Commit-Queue: Keiichi Watanabe <keiichiw@chromium.org> Cr-Commit-Position: refs/heads/master@{#599596} [modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/media/capture/video/video_capture_device_unittest.cc [modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/testing/buildbot/chromium.android.fyi.json [modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/testing/buildbot/chromium.android.json [modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/testing/buildbot/chromium.chromiumos.json [modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/testing/buildbot/chromium.clang.json [modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/testing/buildbot/chromium.fyi.json [modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/testing/buildbot/chromium.linux.json [modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/testing/buildbot/chromium.mac.json [modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/testing/buildbot/chromium.memory.json [modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/testing/buildbot/chromium.win.json [modify] https://crrev.com/b332e81bd632b21d2cbb4634f30e08768b687bb5/testing/buildbot/test_suites.pyl
,
Oct 15
The updated capture_unittests passed also on Win/Mac testers. Win 10: https://ci.chromium.org/buildbot/chromium.webrtc/Win10%20Tester/32937 Mac: https://ci.chromium.org/buildbot/chromium.webrtc/Mac%20Tester/84196 So, I'll mark this issue fixed.
,
Oct 29
Looks like the tests are flaky on Windows: ../../media/capture/video/video_capture_device_unittest.cc(642): error: Value of: device_descriptor Most of the tests in https://logs.chromium.org/logs/chromium/bb/chromium.webrtc/Win8_Tester/45883/+/recipes/steps/capture_unittests/0/logs/VideoCaptureDeviceTests_VideoCaptureDeviceTest.UsingRealWebcam_CaptureMjpeg_1/0 pass but not all of them. See https://ci.chromium.org/buildbot/chromium.webrtc/Win8%20Tester/45883.
,
Nov 1
Hmm. After Build #45883, capture_unittests hasn't failed until the current latest build(#45950) on Win8 Tester. https://ci.chromium.org/buildbot/chromium.webrtc/Win8%20Tester/?limit=200 Though we might want to keep watching it for more a few days, I'm not sure we should call it "flaky". > error: Value of: device_descriptor This message means that no real camera is available. I think there is no way to avoid this kind of problems completely as long as we use real devices. So, perhaps, I guess we should not care this kind of errors if it doesn't happen frequently. How do you think?
,
Nov 1
Yeah, I'm not sure if they're actually flaky, rather they're just broken. Using real devices has been working fine on other platforms and our webcam browsertest appears to work: https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/webrtc_webcam_browsertest.cc?sq=package:chromium&g=0 On the other hand, they just skip rather than fail if there is no real device, so it's possible we just haven't seen the errors. I'll disable the capture_unittests tests on Win for now and enable for the other platforms on the WebRTC bots.
,
Nov 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f2309c614d50ff8fe8b433e75a2bdcfc7cbd106f commit f2309c614d50ff8fe8b433e75a2bdcfc7cbd106f Author: Patrik Höglund <phoglund@chromium.org> Date: Wed Nov 07 14:17:50 2018 Re-enable capture_unittests on WebRTC bots except windows. The tests are clearly broken on Windows if a webcam is plugged in. They pass on win10 but only because they're disabled there. See bug for example breakages on Windows. Bug: 893494 Change-Id: I8ea43cc4e3ef23082246ff23ec635f2ec5bfbbe2 Reviewed-on: https://chromium-review.googlesource.com/c/1309789 Reviewed-by: Emircan Uysaler <emircan@chromium.org> Reviewed-by: Kenneth Russell <kbr@chromium.org> Commit-Queue: Patrik Höglund <phoglund@chromium.org> Cr-Commit-Position: refs/heads/master@{#606038} [modify] https://crrev.com/f2309c614d50ff8fe8b433e75a2bdcfc7cbd106f/media/capture/video/video_capture_device_unittest.cc [modify] https://crrev.com/f2309c614d50ff8fe8b433e75a2bdcfc7cbd106f/testing/buildbot/chromium.webrtc.json [modify] https://crrev.com/f2309c614d50ff8fe8b433e75a2bdcfc7cbd106f/testing/buildbot/test_suites.pyl
,
Nov 8
Can we close this issue? Or, do you want to keep it open until we re-enable it on windows? If it's the latter case, we perhaps might want to change the summary of this issue. Anyway, let me change the owner to phoglund@.
,
Nov 8
I don't plan to work more on this. I think we can close it even though there's a TODO pointing to it. Perhaps some kind soul in the future will take up fixing the tests on win...
,
Nov 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c02508024cd68c9c350777a4d0d70af9f0ce731d commit c02508024cd68c9c350777a4d0d70af9f0ce731d Author: Patrik Höglund <phoglund@chromium.org> Date: Thu Nov 08 12:23:04 2018 Correct disable statement for CaptureWithSize. I missed 4 (!) cases when disabling this test for win, so let's fix that. Bug: 893494 Change-Id: I6ebfe7d2db4462b4da378aa6a39497b60025f060 Tbr: emircan@chromium.org Reviewed-on: https://chromium-review.googlesource.com/c/1326146 Reviewed-by: Patrik Höglund <phoglund@chromium.org> Commit-Queue: Patrik Höglund <phoglund@chromium.org> Cr-Commit-Position: refs/heads/master@{#606420} [modify] https://crrev.com/c02508024cd68c9c350777a4d0d70af9f0ce731d/media/capture/video/video_capture_device_unittest.cc
,
Nov 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/35b85773cd9e66481ad68b8210118435cf7891a8 commit 35b85773cd9e66481ad68b8210118435cf7891a8 Author: Patrik Höglund <phoglund@chromium.org> Date: Thu Nov 08 14:31:21 2018 Make CaptureWithSize actually be disabled for real this time. Bug: 893494 Change-Id: Ia1d28a678058ce949726e71f511f536482c2eee9 Tbr: emircan@chromium.org Reviewed-on: https://chromium-review.googlesource.com/c/1326461 Reviewed-by: Patrik Höglund <phoglund@chromium.org> Commit-Queue: Patrik Höglund <phoglund@chromium.org> Cr-Commit-Position: refs/heads/master@{#606458} [modify] https://crrev.com/35b85773cd9e66481ad68b8210118435cf7891a8/media/capture/video/video_capture_device_unittest.cc |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by keiichiw@chromium.org
, Oct 9