Refactoring MediaSessionVisibility browser tests

content/browser/media/session/media_session_visibility_browsertest.cc

Index: content/browser/media/session/media_session_visibility_browsertest.cc
diff --git a/content/browser/media/session/media_session_visibility_browsertest.cc b/content/browser/media/session/media_session_visibility_browsertest.cc
index 70ddbfbf4c1ec4b7d81f950601091b2edaac9027..0684ff754666f2457eb060f2bdb94c5371032b3e 100644
--- a/content/browser/media/session/media_session_visibility_browsertest.cc
+++ b/content/browser/media/session/media_session_visibility_browsertest.cc
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <string>
+
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/logging.h"
@@ -24,6 +26,14 @@ static const char kStartPlayerScript[] =
     "document.getElementById('long-video').play()";
 static const char kPausePlayerScript[] =
     "document.getElementById('long-video').pause()";
+
+struct VisibilityTestData {
+  bool is_enable_media_suspend;

[whywhat, 2016/09/02 19:49:26]

nit: rename is_enable_xxx to xxx_enabled (or is_xxx_enabled) for better
readability?

[Zhiqiang Zhang (Slow), 2016/09/05 12:21:53]

I'm using enums now, and the problem got addressed

+  bool is_enable_unified_pipeline;
+  bool is_enable_background_resuming_video;
+  std::string test_method_name;
+};
+
 }
 
 
@@ -34,7 +44,8 @@ static const char kPausePlayerScript[] =
 // include required tests. See
 // media_session_visibility_browsertest_instances.cc for examples.
 class MediaSessionVisibilityBrowserTest
-    : public ContentBrowserTest {
+    : public ContentBrowserTest,
+      public ::testing::WithParamInterface<VisibilityTestData> {
  public:
   MediaSessionVisibilityBrowserTest() = default;
   ~MediaSessionVisibilityBrowserTest() override = default;
@@ -79,14 +90,32 @@ class MediaSessionVisibilityBrowserTest
   }
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
-    EnableDisableResumingBackgroundVideos(false);
-
     command_line->AppendSwitch(
         switches::kDisableGestureRequirementForMediaPlayback);
 #if !defined(OS_ANDROID)
     command_line->AppendSwitch(
         switches::kEnableDefaultMediaSession);
 #endif  // !defined(OS_ANDROID)
+
+    VisibilityTestData params = GetParam();
+
+    if (params.is_enable_media_suspend)
+      command_line->AppendSwitch(switches::kEnableMediaSuspend);
+    else
+      command_line->AppendSwitch(switches::kDisableMediaSuspend);
+
+#if defined(OS_ANDROID)
+    if (!params.is_enable_unified_pipeline)
+      command_line->AppendSwitch(switches::kDisableUnifiedMediaPipeline);
+#endif  // defined(OS_ANDROID)
+
+    if (params.is_enable_background_resuming_video) {
+      command_line->AppendSwitchASCII(switches::kEnableFeatures,
+                                      media::kResumeBackgroundVideo.name);
+    } else {
+      command_line->AppendSwitchASCII(switches::kDisableFeatures,
+                                      media::kResumeBackgroundVideo.name);
+    }
   }
 
   void LoadTestPage() {
@@ -247,110 +276,51 @@ class MediaSessionVisibilityBrowserTest
   DISALLOW_COPY_AND_ASSIGN(MediaSessionVisibilityBrowserTest);
 };
 
-// Helper macro to include tests from the base class.
-#define INCLUDE_TEST_FROM_BASE_CLASS(test_fixture, test_name)   \
-  IN_PROC_BROWSER_TEST_F(test_fixture, test_name) {             \
-    test_name();                                                \
-  }
-
-///////////////////////////////////////////////////////////////////////////////
-// Configuration instances.
-
-// UnifiedPipeline + SuspendOnHide
-class MediaSessionVisibilityBrowserTest_UnifiedPipeline_SuspendOnHide :
-      public MediaSessionVisibilityBrowserTest {
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    MediaSessionVisibilityBrowserTest::SetUpCommandLine(command_line);
-#if !defined(OS_ANDROID)
-    command_line->AppendSwitch(switches::kEnableMediaSuspend);
-#endif  // defined(OS_ANDROID)
+IN_PROC_BROWSER_TEST_P(MediaSessionVisibilityBrowserTest,
+                       TestEntryPoint) {
+  std::string test_method_name = GetParam().test_method_name;
+  LOG(INFO) << "Running test: " << test_method_name;
+
+  if (test_method_name == "TestSessionInactiveWhenHiddenAfterContentPause") {

[whywhat, 2016/09/02 19:49:26]

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

[Zhiqiang Zhang (Slow), 2016/09/05 12:21:53]

Done in another way. Please check the changes.

+    TestSessionInactiveWhenHiddenAfterContentPause();
+  } else if (test_method_name == "TestSessionInactiveWhenHiddenWhilePlaying") {
+    TestSessionInactiveWhenHiddenWhilePlaying();
+  } else if (test_method_name == "TestSessionSuspendedWhenHiddenWhilePlaying") {
+    TestSessionSuspendedWhenHiddenWhilePlaying();
+  } else if (test_method_name ==
+             "TestSessionSuspendedWhenHiddenAfterContentPause") {
+    TestSessionSuspendedWhenHiddenAfterContentPause();
+  } else if (test_method_name == "TestSessionActiveWhenHiddenWhilePlaying") {
+    TestSessionActiveWhenHiddenWhilePlaying();
+  } else {
+    FAIL() << "unrecognized test method";
   }
-};
-
-INCLUDE_TEST_FROM_BASE_CLASS(
-    MediaSessionVisibilityBrowserTest_UnifiedPipeline_SuspendOnHide,
-    TestSessionInactiveWhenHiddenAfterContentPause)
-INCLUDE_TEST_FROM_BASE_CLASS(
-    MediaSessionVisibilityBrowserTest_UnifiedPipeline_SuspendOnHide,
-    TestSessionInactiveWhenHiddenWhilePlaying)
-
-IN_PROC_BROWSER_TEST_F(
-    MediaSessionVisibilityBrowserTest_UnifiedPipeline_SuspendOnHide,
-    TestSessionSuspendedWhenHiddenWhilePlaying) {
-  EnableDisableResumingBackgroundVideos(true);
-  TestSessionSuspendedWhenHiddenWhilePlaying();
 }
 
-IN_PROC_BROWSER_TEST_F(
-    MediaSessionVisibilityBrowserTest_UnifiedPipeline_SuspendOnHide,
-    TestSessionSuspendedWhenHiddenAfterContentPause) {
-  EnableDisableResumingBackgroundVideos(true);
-  TestSessionSuspendedWhenHiddenAfterContentPause();
-}
-
-// UnifiedPipeline + NosuspendOnHide
-class MediaSessionVisibilityBrowserTest_UnifiedPipeline_NosuspendOnHide :
-      public MediaSessionVisibilityBrowserTest {
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    MediaSessionVisibilityBrowserTest::SetUpCommandLine(command_line);
-#if defined(OS_ANDROID)
-    command_line->AppendSwitch(switches::kDisableMediaSuspend);
-#endif  // defined(OS_ANDROID)
-  }
-};
+namespace {
 
-INCLUDE_TEST_FROM_BASE_CLASS(
-    MediaSessionVisibilityBrowserTest_UnifiedPipeline_NosuspendOnHide,
-    TestSessionSuspendedWhenHiddenAfterContentPause)
-INCLUDE_TEST_FROM_BASE_CLASS(
-    MediaSessionVisibilityBrowserTest_UnifiedPipeline_NosuspendOnHide,
-    TestSessionActiveWhenHiddenWhilePlaying)
-
-IN_PROC_BROWSER_TEST_F(
-    MediaSessionVisibilityBrowserTest_UnifiedPipeline_NosuspendOnHide,
-    TestSessionActiveWhenHiddenWhilePlayingWithResume) {
-  EnableDisableResumingBackgroundVideos(true);
-  TestSessionActiveWhenHiddenWhilePlaying();
-}
+VisibilityTestData kTestParams[] = {
+  { true, true, false, "TestSessionInactiveWhenHiddenAfterContentPause" },

[whywhat, 2016/09/02 19:49:26]

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 }

[Zhiqiang Zhang (Slow), 2016/09/05 12:21:53]

Now WMPI and WMPA have the same instances.

+  { true, true, false, "TestSessionInactiveWhenHiddenWhilePlaying" },
+  { true, true, true, "TestSessionSuspendedWhenHiddenWhilePlaying" },
+  { true, true, true, "TestSessionSuspendedWhenHiddenAfterContentPause" },
+  { false, true, false, "TestSessionSuspendedWhenHiddenAfterContentPause" },
+  { false, true, false, "TestSessionActiveWhenHiddenWhilePlaying" },
+  { false, true, true, "TestSessionActiveWhenHiddenWhilePlaying" },
 
 #if defined(OS_ANDROID)
-// AndroidPipeline + SuspendOnHide
-class MediaSessionVisibilityBrowserTest_AndroidPipeline_SuspendOnHide :
-      public MediaSessionVisibilityBrowserTest {
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    MediaSessionVisibilityBrowserTest::SetUpCommandLine(command_line);
-    command_line->AppendSwitch(switches::kDisableUnifiedMediaPipeline);
-  }
-};
+  { true, false, false, "TestSessionInactiveWhenHiddenAfterContentPause" },
+  { true, false, false, "TestSessionInactiveWhenHiddenWhilePlaying" },
+  { false, false, false, "TestSessionSuspendedWhenHiddenAfterContentPause" },
+  { false, false, false, "TestSessionActiveWhenHiddenWhilePlaying" },

[whywhat, 2016/09/02 19:49:26]

These tests never check that background video can be resumed on WMPA path, but
they should.

[Zhiqiang Zhang (Slow), 2016/09/05 12:21:53]

Now WMPI and WMPA have the same instances.

+#endif  // defined(OS_ANDROID)
 
-// The following tests are flaky. Re-enabling with logging to see what's
-// happening on the bots. See crbug.com/619096.
-INCLUDE_TEST_FROM_BASE_CLASS(
-    MediaSessionVisibilityBrowserTest_AndroidPipeline_SuspendOnHide,
-    TestSessionInactiveWhenHiddenAfterContentPause)
-INCLUDE_TEST_FROM_BASE_CLASS(
-    MediaSessionVisibilityBrowserTest_AndroidPipeline_SuspendOnHide,
-    TestSessionInactiveWhenHiddenWhilePlaying)
-
-// AndroidPipeline + NosuspendOnHide
-class MediaSessionVisibilityBrowserTest_AndroidPipeline_NosuspendOnHide :
-      public MediaSessionVisibilityBrowserTest {
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    MediaSessionVisibilityBrowserTest::SetUpCommandLine(command_line);
-    command_line->AppendSwitch(switches::kDisableUnifiedMediaPipeline);
-    command_line->AppendSwitch(switches::kDisableMediaSuspend);
-  }
 };
 
-// The following tests are flaky. Re-enabling with logging to see what's
-// happening on the bots. See crbug.com/619096.
-INCLUDE_TEST_FROM_BASE_CLASS(
-    MediaSessionVisibilityBrowserTest_AndroidPipeline_NosuspendOnHide,
-    TestSessionSuspendedWhenHiddenAfterContentPause)
-INCLUDE_TEST_FROM_BASE_CLASS(
-   MediaSessionVisibilityBrowserTest_AndroidPipeline_NosuspendOnHide,
-   TestSessionActiveWhenHiddenWhilePlaying)
+}  // anonymous namespace
 
-#endif  // defined(OS_ANDROID)
+INSTANTIATE_TEST_CASE_P(MediaSessionVisibilityBrowserTestInstances,
+                        MediaSessionVisibilityBrowserTest,
+                        testing::ValuesIn(kTestParams));
 
 }  // namespace content