Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2299203005:
Refactoring MediaSessionVisibility browser tests (Closed)
|
Created:
4 years, 3 months ago by Zhiqiang Zhang (Slow) Modified:
4 years, 3 months ago Reviewers:
whywhat CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactoring MediaSessionVisibility browser tests
Using parameterized GTests to run MediaSessionVisibility browser tests
to help make further modification easier.
BUG=632767
Committed: https://crrev.com/cc32190deee8678fbc1b1be901cca5e9da9f0297
Cr-Commit-Position: refs/heads/master@{#416939}
Patch Set 1 #
Total comments: 8
Patch Set 2 : addressed Anton's comments #
Total comments: 4
Patch Set 3 : addressed more comments #Messages
Total messages: 18 (9 generated)
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
zqzhang@chromium.org changed reviewers: + avayvod@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2299203005/diff/1/content/browser/media/sessi... File content/browser/media/session/media_session_visibility_browsertest.cc (right): https://codereview.chromium.org/2299203005/diff/1/content/browser/media/sessi... content/browser/media/session/media_session_visibility_browsertest.cc:31: bool is_enable_media_suspend; nit: rename is_enable_xxx to xxx_enabled (or is_xxx_enabled) for better readability? https://codereview.chromium.org/2299203005/diff/1/content/browser/media/sessi... content/browser/media/session/media_session_visibility_browsertest.cc:284: if (test_method_name == "TestSessionInactiveWhenHiddenAfterContentPause") { Can we generalize to what commands to run on the player and what to expect? Seems like all 11 test cases check the session state after the player was hidden either while playing or after pause. We should be able to pass two values (expected_session_state = {inactive, active, suspended} and player_state = { playing, paused }; E.g. TestSessionState(); // + Android only ones for WMPA #if defined(OS_ANDROID) TestSessionStateUsingWMPA() { // disable unified pipeline, call TestSessionState(); } #endif https://codereview.chromium.org/2299203005/diff/1/content/browser/media/sessi... content/browser/media/session/media_session_visibility_browsertest.cc:303: { true, true, false, "TestSessionInactiveWhenHiddenAfterContentPause" }, The combination list is incomplete. You definitely miss background resume for WMPA and { false, true, true } case for paused state. This would become something like: { NoSuspend, NoResume, Active, Playing } { NoSuspend, NoResume, Suspended, Paused } { NoSuspend, Resume, Active, Playing } { NoSuspend, Resume, Suspended, Paused } { Suspend, NoResume, Inactive, Playing } { Suspend, NoResume, Inactive, Paused } { Suspend, Resume, Suspended, Playing } { Suspend, Resume, Suspended, Paused } https://codereview.chromium.org/2299203005/diff/1/content/browser/media/sessi... content/browser/media/session/media_session_visibility_browsertest.cc:315: { false, false, false, "TestSessionActiveWhenHiddenWhilePlaying" }, These tests never check that background video can be resumed on WMPA path, but they should.
Patchset #2 (id:20001) has been deleted
PTAL. https://codereview.chromium.org/2299203005/diff/1/content/browser/media/sessi... File content/browser/media/session/media_session_visibility_browsertest.cc (right): https://codereview.chromium.org/2299203005/diff/1/content/browser/media/sessi... content/browser/media/session/media_session_visibility_browsertest.cc:31: bool is_enable_media_suspend; On 2016/09/02 19:49:26, whywhat wrote: > nit: rename is_enable_xxx to xxx_enabled (or is_xxx_enabled) for better > readability? I'm using enums now, and the problem got addressed https://codereview.chromium.org/2299203005/diff/1/content/browser/media/sessi... content/browser/media/session/media_session_visibility_browsertest.cc:284: if (test_method_name == "TestSessionInactiveWhenHiddenAfterContentPause") { On 2016/09/02 19:49:26, whywhat wrote: > Can we generalize to what commands to run on the player and what to expect? > > Seems like all 11 test cases check the session state after the player was hidden > either while playing or after pause. We should be able to pass two values > (expected_session_state = {inactive, active, suspended} and player_state = { > playing, paused }; > > E.g. > > TestSessionState(); > > // + Android only ones for WMPA > > #if defined(OS_ANDROID) > > TestSessionStateUsingWMPA() { // disable unified pipeline, call > TestSessionState(); } > > #endif Done in another way. Please check the changes. https://codereview.chromium.org/2299203005/diff/1/content/browser/media/sessi... content/browser/media/session/media_session_visibility_browsertest.cc:303: { true, true, false, "TestSessionInactiveWhenHiddenAfterContentPause" }, On 2016/09/02 19:49:26, whywhat wrote: > The combination list is incomplete. You definitely miss background resume for > WMPA and { false, true, true } case for paused state. > > This would become something like: > > { NoSuspend, NoResume, Active, Playing } > { NoSuspend, NoResume, Suspended, Paused } > { NoSuspend, Resume, Active, Playing } > { NoSuspend, Resume, Suspended, Paused } > { Suspend, NoResume, Inactive, Playing } > { Suspend, NoResume, Inactive, Paused } > { Suspend, Resume, Suspended, Playing } > { Suspend, Resume, Suspended, Paused } Now WMPI and WMPA have the same instances. https://codereview.chromium.org/2299203005/diff/1/content/browser/media/sessi... content/browser/media/session/media_session_visibility_browsertest.cc:315: { false, false, false, "TestSessionActiveWhenHiddenWhilePlaying" }, On 2016/09/02 19:49:26, whywhat wrote: > These tests never check that background video can be resumed on WMPA path, but > they should. Now WMPI and WMPA have the same instances.
lgtm with some comments. overall I think we're abusing the test templates as we actually change the behavior tested by the template depending on the params (while in the examples at least, the test verifies that the same behavior is observed independent of the params passed). So WMPI/WMPA is a good fit to use the template instantiation, everything else is probably not. Let's hope we'll remove most of these flags sooner or later which should simplify the tests here. https://codereview.chromium.org/2299203005/diff/40001/content/browser/media/s... File content/browser/media/session/media_session_visibility_browsertest.cc (right): https://codereview.chromium.org/2299203005/diff/40001/content/browser/media/s... content/browser/media/session/media_session_visibility_browsertest.cc:272: { MediaSuspend::DISABLED, Pipeline::WMPI, BackgroundResuming::ENABLED, What about the same case where the initial state is suspended? What about the inactive session state? We could've used the Combine generator if we didn't have to pass the result state of the test to the test :( https://codereview.chromium.org/2299203005/diff/40001/content/browser/media/s... content/browser/media/session/media_session_visibility_browsertest.cc:278: { MediaSuspend::ENABLED, Pipeline::WMPA, BackgroundResuming::DISABLED, For WMPA we need to make sure that the same set of tests is run. I'd still prefer having a second test template that would set the WMPA flag and call the first test. Then instantiate it with the same list as the non-WMPA template. Both could be #ifdef-ed with OS_ANDROID.
https://codereview.chromium.org/2299203005/diff/40001/content/browser/media/s... File content/browser/media/session/media_session_visibility_browsertest.cc (right): https://codereview.chromium.org/2299203005/diff/40001/content/browser/media/s... content/browser/media/session/media_session_visibility_browsertest.cc:272: { MediaSuspend::DISABLED, Pipeline::WMPI, BackgroundResuming::ENABLED, On 2016/09/06 17:37:29, whywhat wrote: > What about the same case where the initial state is suspended? Added tests for this > What about the inactive session state? I think the case for inactive state is less useful. > We could've used the Combine generator if we didn't have to pass the result > state of the test to the test :( https://codereview.chromium.org/2299203005/diff/40001/content/browser/media/s... content/browser/media/session/media_session_visibility_browsertest.cc:278: { MediaSuspend::ENABLED, Pipeline::WMPA, BackgroundResuming::DISABLED, On 2016/09/06 17:37:29, whywhat wrote: > For WMPA we need to make sure that the same set of tests is run. > > I'd still prefer having a second test template that would set the WMPA flag and > call the first test. Then instantiate it with the same list as the non-WMPA > template. Both could be #ifdef-ed with OS_ANDROID. Done by separating out Pipeline from the Params, and use testing::Combine to do this :)
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org Link to the patchset: https://codereview.chromium.org/2299203005/#ps60001 (title: "addressed more comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thanks, that looks neat now!
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Refactoring MediaSessionVisibility browser tests Using parameterized GTests to run MediaSessionVisibility browser tests to help make further modification easier. BUG=632767 ========== to ========== Refactoring MediaSessionVisibility browser tests Using parameterized GTests to run MediaSessionVisibility browser tests to help make further modification easier. BUG=632767 Committed: https://crrev.com/cc32190deee8678fbc1b1be901cca5e9da9f0297 Cr-Commit-Position: refs/heads/master@{#416939} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/cc32190deee8678fbc1b1be901cca5e9da9f0297 Cr-Commit-Position: refs/heads/master@{#416939} |
