Start ARC and sign in after Chrome OS login

chrome/browser/chromeos/arc/arc_session_manager_unittest.cc

Index: chrome/browser/chromeos/arc/arc_session_manager_unittest.cc
diff --git a/chrome/browser/chromeos/arc/arc_session_manager_unittest.cc b/chrome/browser/chromeos/arc/arc_session_manager_unittest.cc
index efdfe629e5ffb024eff224ebad20fb3453129379..42d18a230ddd0d4b2d8d95eab674a4ceb929ca41 100644
--- a/chrome/browser/chromeos/arc/arc_session_manager_unittest.cc
+++ b/chrome/browser/chromeos/arc/arc_session_manager_unittest.cc
@@ -35,6 +35,7 @@
 #include "chrome/browser/prefs/pref_service_syncable_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_list_prefs.h"
+#include "chrome/browser/ui/app_list/arc/arc_app_test.h"
 #include "chrome/browser/ui/ash/multi_user/multi_user_util.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_profile.h"
@@ -64,6 +65,11 @@ namespace arc {
 
 namespace {
 
+enum ArcAlwaysStartOption : bool {
+  ON = true,
+  OFF = false,
+};
+
 class FakeLoginDisplayHost : public chromeos::LoginDisplayHost {
  public:
   FakeLoginDisplayHost() {
@@ -203,12 +209,14 @@ class ArcSessionManagerTestBase : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(ArcSessionManagerTestBase);
 };
 
-class ArcSessionManagerTest : public ArcSessionManagerTestBase {
+// Intermediate class so that the children can inject test parameter freely.
+class AbstractArcSessionManagerTest : public ArcSessionManagerTestBase {

[hidehiko, 2017/03/06 05:37:20]

IIUC, except skipping ToS, ASM should run same for (current) ARC and pARC,
right?
If so, could you get rid of param test now, instead, set the flag in
BaseWorkflowWithArcAlwaysStart only?

Also, even in future, it'd be nice if ASM works (almost) same for ARC and pARC.
If something big diff is needed, maybe doing it in a (conceptually delegated)
another class will be better maintenance, IMHO. So, no need of param test, in
future, too?

WDYT?

(off topic) Just a random idea for future work. Also, if ToS is the only matter,
we may want to extract ToS negotiation stuff from ASM, so that ASM works exactly
same for both ARC and pARC? I guess this is something we chatted offline last
weekend. WDYT?

[victorhsieh, 2017/03/06 15:46:56]

Yes in the future, but no for now.  No because I haven't implemented full ASM
related logic like opt-in, opt-out (including data removal).

I'm fine if you prefer to only test BaseWorkflowWithArcAlwaysStart, and ignore
other test cases, until all features are implemented.  WDYT?
Given that we will eventually turn the flag on by default and make this the
standard ARC, I'm not sure if delegating to another class would be an over kill
at this point.  But we'll see.
Yes, I'm working on this now.  Data removal would be another difference, btw.

[hidehiko, 2017/03/06 17:10:43]

I personally prefer testing BaseWorkflowWithArcAlwaysStart only now.
And, adding tests for opt-in/-out cases with its implementation on that ToT,
considering you, Yusuke, and I will touch the code at the same time.
Yes, it depends on the change. Let's see the coming change.

[victorhsieh, 2017/03/06 18:24:56]

SG.  Removed many others, too.

  public:
-  ArcSessionManagerTest() = default;
-
+  AbstractArcSessionManagerTest() = default;
   void SetUp() override {
     ArcSessionManagerTestBase::SetUp();
+    if (ShouldArcAlwaysStartInTest() == ArcAlwaysStartOption::ON)
+      SetArcAlwaysStartForTesting();
 
     const AccountId account_id(AccountId::FromUserEmailGaiaId(
         profile()->GetProfileUserName(), "1234567890"));
@@ -220,11 +228,37 @@ class ArcSessionManagerTest : public ArcSessionManagerTestBase {
     ASSERT_TRUE(arc_session_manager()->IsSessionStopped());
   }
 
+ protected:
+  virtual ArcAlwaysStartOption ShouldArcAlwaysStartInTest() = 0;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(AbstractArcSessionManagerTest);
+};
+
+class ArcSessionManagerTest
+    : public AbstractArcSessionManagerTest,
+      public testing::WithParamInterface<ArcAlwaysStartOption> {
+ public:
+  ArcSessionManagerTest() = default;
+
+ protected:
+  ArcAlwaysStartOption ShouldArcAlwaysStartInTest() override {
+    return GetParam();
+  }
+
  private:
   DISALLOW_COPY_AND_ASSIGN(ArcSessionManagerTest);
 };
 
-TEST_F(ArcSessionManagerTest, BaseWorkflow) {
+INSTANTIATE_TEST_CASE_P(,
+                        ArcSessionManagerTest,
+                        testing::Values(ArcAlwaysStartOption::OFF,
+                                        ArcAlwaysStartOption::ON));
+
+TEST_P(ArcSessionManagerTest, BaseWorkflow) {
+  if (ShouldArcAlwaysStart())
+    return;
+
   EXPECT_TRUE(arc_session_manager()->sign_in_start_time().is_null());
   EXPECT_TRUE(arc_session_manager()->arc_start_time().is_null());
 
@@ -252,7 +286,40 @@ TEST_F(ArcSessionManagerTest, BaseWorkflow) {
   arc_session_manager()->Shutdown();
 }
 
-TEST_F(ArcSessionManagerTest, CancelFetchingDisablesArc) {
+TEST_P(ArcSessionManagerTest, BaseWorkflowWithArcAlwaysStart) {
+  if (!ShouldArcAlwaysStart())
+    return;
+
+  // TODO(victorhsieh): Consider also tracking sign-in activity, which is
+  // initiated from the Android side.
+  EXPECT_TRUE(arc_session_manager()->arc_start_time().is_null());
+
+  arc_session_manager()->SetProfile(profile());
+
+  // By default ARC is not enabled.
+  EXPECT_EQ(ArcSessionManager::State::STOPPED, arc_session_manager()->state());
+
+  // When ARC is always started, ArcSessionManager should always be in ACTIVE
+  // state.
+  arc_session_manager()->RequestEnable();
+  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(ArcSessionManager::State::ACTIVE, arc_session_manager()->state());
+
+  arc_session_manager()->StartArcForTesting();
+
+  EXPECT_FALSE(arc_session_manager()->arc_start_time().is_null());
+
+  ASSERT_EQ(ArcSessionManager::State::ACTIVE, arc_session_manager()->state());
+  ASSERT_TRUE(arc_session_manager()->IsSessionRunning());
+
+  arc_session_manager()->Shutdown();
+}
+
+TEST_P(ArcSessionManagerTest, CancelFetchingDisablesArc) {
+  // TODO(victorhsieh): Implement opt-in flow on Persistent ARC.
+  if (ShouldArcAlwaysStart())
+    return;
+
   SetArcPlayStoreEnabledForProfile(profile(), true);
 
   // Starts ARC.
@@ -278,7 +345,11 @@ TEST_F(ArcSessionManagerTest, CancelFetchingDisablesArc) {
   arc_session_manager()->Shutdown();
 }
 
-TEST_F(ArcSessionManagerTest, CloseUIKeepsArcEnabled) {
+TEST_P(ArcSessionManagerTest, CloseUIKeepsArcEnabled) {
+  // TODO(victorhsieh): Implement opt-in flow.
+  if (ShouldArcAlwaysStart())
+    return;
+
   // Starts ARC.
   SetArcPlayStoreEnabledForProfile(profile(), true);
   arc_session_manager()->SetProfile(profile());
@@ -298,7 +369,7 @@ TEST_F(ArcSessionManagerTest, CloseUIKeepsArcEnabled) {
   arc_session_manager()->Shutdown();
 }
 
-TEST_F(ArcSessionManagerTest, Provisioning_Success) {
+TEST_P(ArcSessionManagerTest, Provisioning_Success) {
   PrefService* const prefs = profile()->GetPrefs();
 
   EXPECT_TRUE(arc_session_manager()->sign_in_start_time().is_null());
@@ -309,12 +380,17 @@ TEST_F(ArcSessionManagerTest, Provisioning_Success) {
 
   arc_session_manager()->SetProfile(profile());
   arc_session_manager()->RequestEnable();
-  ASSERT_EQ(ArcSessionManager::State::SHOWING_TERMS_OF_SERVICE,
-            arc_session_manager()->state());
+  if (ShouldArcAlwaysStart()) {
+    ASSERT_EQ(ArcSessionManager::State::ACTIVE, arc_session_manager()->state());
+  } else {
+    ASSERT_EQ(ArcSessionManager::State::SHOWING_TERMS_OF_SERVICE,
+              arc_session_manager()->state());
+  }
 
   // Emulate to accept the terms of service.
   prefs->SetBoolean(prefs::kArcTermsAccepted, true);
-  arc_session_manager()->StartArcForTesting();
+  if (!ShouldArcAlwaysStart())
+    arc_session_manager()->StartArcForTesting();
   EXPECT_EQ(ArcSessionManager::State::ACTIVE, arc_session_manager()->state());
   EXPECT_TRUE(arc_session_manager()->IsSessionRunning());
 
@@ -332,7 +408,7 @@ TEST_F(ArcSessionManagerTest, Provisioning_Success) {
   EXPECT_TRUE(arc_session_manager()->IsPlaystoreLaunchRequestedForTesting());
 }
 
-TEST_F(ArcSessionManagerTest, Provisioning_Restart) {
+TEST_P(ArcSessionManagerTest, Provisioning_Restart) {
   // Set up the situation that provisioning is successfully done in the
   // previous session.
   PrefService* const prefs = profile()->GetPrefs();
@@ -359,12 +435,17 @@ TEST_F(ArcSessionManagerTest, Provisioning_Restart) {
   arc_session_manager()->Shutdown();
 }
 
-TEST_F(ArcSessionManagerTest, RemoveDataDir) {
+TEST_P(ArcSessionManagerTest, RemoveDataDir) {
+  // TODO(victorhsieh): Implement data removal on Persistent ARC.
+  if (ShouldArcAlwaysStart())
+    return;
+
   // Emulate the situation where the initial Google Play Store enabled
   // preference is false for managed user, i.e., data dir is being removed at
   // beginning.
   arc_session_manager()->SetProfile(profile());
   arc_session_manager()->RemoveArcData();
+
   EXPECT_TRUE(
       profile()->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
   EXPECT_EQ(ArcSessionManager::State::REMOVING_DATA_DIR,
@@ -398,7 +479,11 @@ TEST_F(ArcSessionManagerTest, RemoveDataDir) {
       profile()->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
 }
 
-TEST_F(ArcSessionManagerTest, RemoveDataDir_Restart) {
+TEST_P(ArcSessionManagerTest, RemoveDataDir_Restart) {
+  // TODO(victorhsieh): Implement data removal on Persistent ARC.
+  if (ShouldArcAlwaysStart())
+    return;
+
   // Emulate second sign-in. Data should be removed first and ARC started after.
   PrefService* const prefs = profile()->GetPrefs();
   prefs->SetBoolean(prefs::kArcDataRemoveRequested, true);
@@ -414,7 +499,7 @@ TEST_F(ArcSessionManagerTest, RemoveDataDir_Restart) {
   arc_session_manager()->Shutdown();
 }
 
-TEST_F(ArcSessionManagerTest, IgnoreSecondErrorReporting) {
+TEST_P(ArcSessionManagerTest, IgnoreSecondErrorReporting) {
   arc_session_manager()->SetProfile(profile());
   arc_session_manager()->RequestEnable();
   arc_session_manager()->StartArcForTesting();
@@ -435,8 +520,9 @@ TEST_F(ArcSessionManagerTest, IgnoreSecondErrorReporting) {
 }
 
 class ArcSessionManagerPolicyTest
-    : public ArcSessionManagerTest,
-      public testing::WithParamInterface<std::tuple<base::Value, base::Value>> {
+    : public AbstractArcSessionManagerTest,
+      public testing::WithParamInterface<
+          std::tuple<base::Value, base::Value, ArcAlwaysStartOption>> {
  public:
   const base::Value& backup_restore_pref_value() const {
     return std::get<0>(GetParam());
@@ -445,9 +531,18 @@ class ArcSessionManagerPolicyTest
   const base::Value& location_service_pref_value() const {
     return std::get<1>(GetParam());
   }
+
+ protected:
+  ArcAlwaysStartOption ShouldArcAlwaysStartInTest() override {
+    return std::get<2>(GetParam());
+  }
 };
 
 TEST_P(ArcSessionManagerPolicyTest, SkippingTerms) {
+  // TODO(victorhsieh): Implement opt-in flow.
+  if (ShouldArcAlwaysStart())
+    return;
+
   sync_preferences::TestingPrefServiceSyncable* const prefs =
       profile()->GetTestingPrefService();
 
@@ -510,11 +605,12 @@ TEST_P(ArcSessionManagerPolicyTest, SkippingTerms) {
 }
 
 INSTANTIATE_TEST_CASE_P(
-    ArcSessionManagerPolicyTest,
+    ,
     ArcSessionManagerPolicyTest,
     testing::Combine(
         testing::Values(base::Value(), base::Value(false), base::Value(true)),
-        testing::Values(base::Value(), base::Value(false), base::Value(true))));
+        testing::Values(base::Value(), base::Value(false), base::Value(true)),
+        testing::Values(ArcAlwaysStartOption::OFF, ArcAlwaysStartOption::ON)));
 
 class ArcSessionManagerKioskTest : public ArcSessionManagerTestBase {
  public:
@@ -550,11 +646,20 @@ TEST_F(ArcSessionManagerKioskTest, AuthFailure) {
   EXPECT_TRUE(terminated);
 }
 
-class ArcSessionOobeOptInTest : public ArcSessionManagerTest {
+// This class takes two test parameters because both itself and its child need
+// to be parameterized.  Having redundant parameter here avoid the trouble to
+// deal with multiple inheritance from WithParamInterface instances.
+class ArcSessionOobeOptInTest : public AbstractArcSessionManagerTest,
+                                public testing::WithParamInterface<
+                                    std::tuple<ArcAlwaysStartOption, bool>> {
  public:
   ArcSessionOobeOptInTest() = default;
 
  protected:
+  ArcAlwaysStartOption ShouldArcAlwaysStartInTest() override {
+    return std::get<0>(GetParam());
+  }
+
   void CreateLoginDisplayHost() {
     fake_login_display_host_ = base::MakeUnique<FakeLoginDisplayHost>();
   }
@@ -572,7 +677,14 @@ class ArcSessionOobeOptInTest : public ArcSessionManagerTest {
   DISALLOW_COPY_AND_ASSIGN(ArcSessionOobeOptInTest);
 };
 
-TEST_F(ArcSessionOobeOptInTest, OobeOptInActive) {
+INSTANTIATE_TEST_CASE_P(
+    ,
+    ArcSessionOobeOptInTest,
+    testing::Combine(testing::Values(ArcAlwaysStartOption::OFF,
+                                     ArcAlwaysStartOption::ON),
+                     testing::Values(false) /* dummy */));
+
+TEST_P(ArcSessionOobeOptInTest, OobeOptInActive) {
   // OOBE OptIn is active in case of OOBE is started for new user and ARC OOBE
   // is enabled by switch.
   EXPECT_FALSE(ArcSessionManager::IsOobeOptInActive());
@@ -593,8 +705,7 @@ TEST_F(ArcSessionOobeOptInTest, OobeOptInActive) {
 
 class ArcSessionOobeOptInNegotiatorTest
     : public ArcSessionOobeOptInTest,
-      public chromeos::ArcTermsOfServiceScreenView,
-      public testing::WithParamInterface<bool> {
+      public chromeos::ArcTermsOfServiceScreenView {
  public:
   ArcSessionOobeOptInNegotiatorTest() = default;
 
@@ -636,7 +747,11 @@ class ArcSessionOobeOptInNegotiatorTest
   }
 
  protected:
-  bool IsManagedUser() { return GetParam(); }
+  ArcAlwaysStartOption ShouldArcAlwaysStartInTest() override {
+    return std::get<0>(GetParam());
+  }
+
+  bool IsManagedUser() { return std::get<1>(GetParam()); }
 
   void ReportResult(bool accepted) {
     for (auto& observer : observer_list_) {
@@ -683,9 +798,12 @@ class ArcSessionOobeOptInNegotiatorTest
   DISALLOW_COPY_AND_ASSIGN(ArcSessionOobeOptInNegotiatorTest);
 };
 
-INSTANTIATE_TEST_CASE_P(ArcSessionOobeOptInNegotiatorTestImpl,
-                        ArcSessionOobeOptInNegotiatorTest,
-                        ::testing::Values(true, false));
+// TODO(victorhsieh): Add test to cover when ARC always start
+INSTANTIATE_TEST_CASE_P(
+    ,
+    ArcSessionOobeOptInNegotiatorTest,
+    testing::Combine(testing::Values(ArcAlwaysStartOption::OFF),
+                     testing::Bool() /* managed user */));
 
 TEST_P(ArcSessionOobeOptInNegotiatorTest, OobeTermsAccepted) {
   view()->Show();