Refactoring MediaSessionVisibility browser tests

content/browser/media/session/media_session_visibility_browsertest.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // 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"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "content/browser/media/session/media_session.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test.h"
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/shell/browser/shell.h"
 #include "media/base/media_switches.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace content {
 namespace {
 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;
+};
+
 }
 
 
 // Base class of MediaSession visibility tests. The class is intended
 // to be used to run tests under different configurations. Tests
 // should inheret from this class, set up their own command line per
 // their configuration, and use macro INCLUDE_TEST_FROM_BASE_CLASS to
 // 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;
 
   void SetUpOnMainThread() override {
     ContentBrowserTest::SetUpOnMainThread();
     web_contents_ = shell()->web_contents();
     media_session_ = MediaSession::Get(web_contents_);
 
     media_session_state_loop_runners_[MediaSession::State::ACTIVE] =
@@ skipping 24 matching lines @@
       disabled_features = media::kResumeBackgroundVideo.name;
 
     std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
     feature_list->InitializeFromCommandLine(
         enabled_features, disabled_features);
     base::FeatureList::ClearInstanceForTesting();
     base::FeatureList::SetInstance(std::move(feature_list));
   }
 
   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() {
     TestNavigationObserver navigation_observer(shell()->web_contents(), 1);
     shell()->LoadURL(GetTestUrl("media/session", "media-session.html"));
     navigation_observer.Wait();
   }
 
   void RunScript(const std::string& script) {
     ASSERT_TRUE(ExecuteScript(web_contents_->GetMainFrame(), script));
@@ skipping 140 matching lines @@
   // state change after play().
   std::map<MediaSession::State, scoped_refptr<MessageLoopRunner> >
   media_session_state_loop_runners_;
   std::unique_ptr<base::CallbackList<void(MediaSession::State)>::Subscription>
       media_session_state_callback_subscription_;
 
  private:
   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();                                                \
+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";
   }
+}
 
-///////////////////////////////////////////////////////////////////////////////
-// Configuration instances.
+namespace {
 
-// 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);
+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)
+  { 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)
-  }
+
 };
 
-INCLUDE_TEST_FROM_BASE_CLASS(
-    MediaSessionVisibilityBrowserTest_UnifiedPipeline_SuspendOnHide,
-    TestSessionInactiveWhenHiddenAfterContentPause)
-INCLUDE_TEST_FROM_BASE_CLASS(
-    MediaSessionVisibilityBrowserTest_UnifiedPipeline_SuspendOnHide,
-    TestSessionInactiveWhenHiddenWhilePlaying)
+}  // anonymous namespace
 
-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)
-  }
-};
-
-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();
-}
-
-#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);
-  }
-};
-
-// 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)
-
-#endif  // defined(OS_ANDROID)
+INSTANTIATE_TEST_CASE_P(MediaSessionVisibilityBrowserTestInstances,
+                        MediaSessionVisibilityBrowserTest,
+                        testing::ValuesIn(kTestParams));
 
 }  // namespace content