Skip ARC initial screen when everything is set up by policy

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 23bfe6d9d8986fd7b9789010d37c3a567f425683..a9bdaacd67e87610bee9e5fc4898a5c27d091f59 100644
--- a/chrome/browser/chromeos/arc/arc_session_manager_unittest.cc
+++ b/chrome/browser/chromeos/arc/arc_session_manager_unittest.cc
@@ -11,10 +11,12 @@
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/observer_list.h"
 #include "base/run_loop.h"
+#include "base/values.h"
 #include "chrome/browser/chromeos/arc/arc_optin_uma.h"
 #include "chrome/browser/chromeos/arc/arc_session_manager.h"
 #include "chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h"
@@ -42,6 +44,7 @@
 #include "components/arc/arc_util.h"
 #include "components/arc/test/fake_arc_session.h"
 #include "components/prefs/pref_service.h"
+#include "components/prefs/testing_pref_service.h"
 #include "components/signin/core/account_id/account_id.h"
 #include "components/sync/model/fake_sync_change_processor.h"
 #include "components/sync/model/sync_error_factory_mock.h"
@@ -442,6 +445,62 @@ TEST_F(ArcSessionManagerTest, SignInStatus) {
   arc_session_manager()->Shutdown();
 }
 
+TEST_F(ArcSessionManagerTest, SkippingTermsDueToPolicies) {

[hidehiko, 2017/02/09 15:28:04]

Could you use parameterized test so that what failed is easily identified.

[emaxx, 2017/02/10 15:40:16]

Quick question: do you think this will be better, given that the debug
information is currently printed by the test code too? (Note the last EXPECT_EQ
in this function - all other EXPECT* statements are actually assertions which
should hold regardless of the tested pref values.)
I'm just concerned a little bit about bloating the test code.

[hidehiko, 2017/02/14 11:51:36]

I do not think using WithParamInterface bloats the code. It'd be something like;

class ArcSessionManagerPolicyTest : public ArcSessionManagerTest, public
testing::WithParamInterface<std::tuple<base::Value, base::Value>> {

  const base::Value& GetBackupRestorePref() {
    return std::get<0>(GetParam());
  }
  const base::Value& GetLocationServicePref() {
    return std::get<1>(GetParam());
  }
};

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))));

So that you can flatten all the contents in the nested for stmt.
Additional benefit to the logs is, all inflight test state is reset for each
case, so even if some are failed, others won't be affected.
WDYT?

Note: you may want/need to rebase on top of
https://codereview.chromium.org/2691213002/, to avoid name conflicting.

[emaxx, 2017/02/15 03:40:59]

Done. Thanks for the detailed comment!

+  sync_preferences::TestingPrefServiceSyncable* const prefs =
+      profile()->GetTestingPrefService();
+
+  // Values of the ArcBackupRestoreEnabled pref to be tested.
+  const std::vector<base::Value> test_backup_pref_values = {
+      base::Value(), base::Value(false), base::Value(true)};
+  // Values of the ArcLocationServiceEnabled pref to be tested.
+  const std::vector<base::Value> test_location_pref_values = {
+      base::Value(), base::Value(false), base::Value(true)};
+
+  for (const auto& backup_pref_value : test_backup_pref_values) {
+    for (const auto& location_pref_value : test_location_pref_values) {
+      EXPECT_FALSE(prefs->GetBoolean(prefs::kArcSignedIn));
+      EXPECT_FALSE(prefs->GetBoolean(prefs::kArcTermsAccepted));
+
+      // Set ARC to be managed.
+      prefs->SetManagedPref(prefs::kArcEnabled,
+                            base::MakeUnique<base::Value>(true).release());
+
+      // Assign test values to the prefs.
+      if (backup_pref_value.is_bool()) {
+        prefs->SetManagedPref(prefs::kArcBackupRestoreEnabled,
+                              backup_pref_value.DeepCopy());
+      } else {
+        prefs->RemoveManagedPref(prefs::kArcBackupRestoreEnabled);
+      }
+      if (location_pref_value.is_bool()) {
+        prefs->SetManagedPref(prefs::kArcLocationServiceEnabled,
+                              location_pref_value.DeepCopy());
+      } else {
+        prefs->RemoveManagedPref(prefs::kArcLocationServiceEnabled);
+      }
+
+      arc_session_manager()->OnPrimaryUserProfilePrepared(profile());
+      EXPECT_TRUE(arc_session_manager()->IsArcEnabled());
+      EXPECT_TRUE(arc_session_manager()->IsArcManaged());
+
+      // Terms of Service should be shown if either ArcBackupRestoreEnabled or
+      // ArcLocationServiceEnabled is unmanaged.
+      const ArcSessionManager::State expected_state =
+          backup_pref_value.is_bool() && location_pref_value.is_bool()
+              ? ArcSessionManager::State::ACTIVE
+              : ArcSessionManager::State::SHOWING_TERMS_OF_SERVICE;
+      EXPECT_EQ(expected_state, arc_session_manager()->state())
+          << "ArcBackupRestoreEnabled pref is " << backup_pref_value
+          << "ArcLocationServiceEnabled pref is " << location_pref_value;
+
+      // Stop ARC and shutdown the service.
+      prefs->RemoveManagedPref(prefs::kArcEnabled);
+      WaitForDataRemoved(ArcSessionManager::State::STOPPED);
+      arc_session_manager()->Shutdown();
+    }
+  }
+}
+
 TEST_F(ArcSessionManagerTest, DisabledForDeviceLocalAccount) {
   PrefService* const prefs = profile()->GetPrefs();
   EXPECT_FALSE(prefs->GetBoolean(prefs::kArcSignedIn));