[Sync] Simplify sync startup behavior.

components/sync_driver/startup_controller_unittest.cc

Index: components/sync_driver/startup_controller_unittest.cc
diff --git a/components/sync_driver/startup_controller_unittest.cc b/components/sync_driver/startup_controller_unittest.cc
index e11678dbd0b6e2ad216154b4f3e262dc44e0227f..d63fe8e968b3f438a315e415e37368b4c6d0c7bd 100644
--- a/components/sync_driver/startup_controller_unittest.cc
+++ b/components/sync_driver/startup_controller_unittest.cc
@@ -55,7 +55,7 @@ class StartupControllerTest : public testing::Test {
     token_service_.reset(new FakeProfileOAuth2TokenService());
     signin_.reset(new FakeSigninManagerWrapper());
 
-    SetUpController(AUTO_START);
+    SetUpController();
   }
 
   void TearDown() override {
@@ -67,13 +67,12 @@ class StartupControllerTest : public testing::Test {
     started_ = false;
   }
 
-  void SetUpController(ProfileSyncServiceStartBehavior start_behavior) {
+  void SetUpController() {
     started_ = false;
     base::Closure fake_start_backend = base::Bind(
         &StartupControllerTest::FakeStartBackend, base::Unretained(this));
-    controller_.reset(new StartupController(start_behavior, token_service(),
-                                            sync_prefs_.get(), signin_.get(),
-                                            fake_start_backend));
+    controller_.reset(new StartupController(token_service(), sync_prefs_.get(),
+                                            signin_.get(), fake_start_backend));
     controller_->Reset(syncer::UserTypes());
     controller_->OverrideFallbackTimeoutForTest(
         base::TimeDelta::FromSeconds(0));
@@ -83,6 +82,27 @@ class StartupControllerTest : public testing::Test {
     started_ = true;
   }
 
+  void ExpectStarted() {
+    EXPECT_TRUE(started());
+    EXPECT_EQ(kStateStringStarted,
+              controller()->GetBackendInitializationStateString());
+  }
+
+  void ExpectStartDeferred() {
+    const bool deferred_start =
+        !base::CommandLine::ForCurrentProcess()->HasSwitch(
+            switches::kSyncDisableDeferredStartup);
+    EXPECT_EQ(!deferred_start, started());
+    EXPECT_EQ(deferred_start ? kStateStringDeferred : kStateStringStarted,
+              controller()->GetBackendInitializationStateString());
+  }
+
+  void ExpectNotStarted() {
+    EXPECT_FALSE(started());
+    EXPECT_EQ(kStateStringNotStarted,
+              controller()->GetBackendInitializationStateString());
+  }
+
   bool started() const { return started_; }
   void clear_started() { started_ = false; }
   StartupController* controller() { return controller_.get(); }
@@ -105,63 +125,46 @@ class StartupControllerTest : public testing::Test {
 // Test that sync doesn't start until all conditions are met.
 TEST_F(StartupControllerTest, Basic) {
   controller()->TryStart();
-  EXPECT_FALSE(started());
-  sync_prefs()->SetFirstSetupComplete();
-  controller()->TryStart();
-  EXPECT_FALSE(started());
+  ExpectNotStarted();
+
   signin()->set_account_id(kTestUser);
   controller()->TryStart();
-  EXPECT_FALSE(started());
+  ExpectNotStarted();
+
   token_service()->UpdateCredentials(kTestUser, kTestToken);
-  const bool deferred_start =
-      !base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kSyncDisableDeferredStartup);
   controller()->TryStart();
-  EXPECT_EQ(!deferred_start, started());
-  std::string state(controller()->GetBackendInitializationStateString());
-  EXPECT_TRUE(deferred_start ? state == kStateStringDeferred :
-                               state == kStateStringStarted);
+  ExpectStartDeferred();
 }
 
 // Test that sync doesn't start when not requested even if all other
 // conditons are met.
 TEST_F(StartupControllerTest, NotRequested) {
-  sync_prefs()->SetFirstSetupComplete();
   sync_prefs()->SetSyncRequested(false);
   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
-  EXPECT_FALSE(started());
-  EXPECT_EQ(kStateStringNotStarted,
-            controller()->GetBackendInitializationStateString());
+  ExpectNotStarted();
 }
 
 // Test that sync doesn't when managed even if all other conditons are met.
 TEST_F(StartupControllerTest, Managed) {
-  sync_prefs()->SetFirstSetupComplete();
   sync_prefs()->SetManagedForTest(true);
   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
-  EXPECT_FALSE(started());
-  EXPECT_EQ(kStateStringNotStarted,
-            controller()->GetBackendInitializationStateString());
+  ExpectNotStarted();
 }
 
 // Test that sync doesn't start until all conditions are met and a
 // data type triggers sync startup.
 TEST_F(StartupControllerTest, DataTypeTriggered) {
-  sync_prefs()->SetFirstSetupComplete();
   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
-  EXPECT_FALSE(started());
-  EXPECT_EQ(kStateStringDeferred,
-            controller()->GetBackendInitializationStateString());
+  ExpectStartDeferred();
+
   controller()->OnDataTypeRequestsSyncStartup(syncer::SESSIONS);
-  EXPECT_TRUE(started());
-  EXPECT_EQ(kStateStringStarted,
-            controller()->GetBackendInitializationStateString());
+  ExpectStarted();
 
   // The fallback timer shouldn't result in another invocation of the closure
   // we passed to the StartupController.
@@ -173,13 +176,13 @@ TEST_F(StartupControllerTest, DataTypeTriggered) {
 // Test that the fallback timer starts sync in the event all
 // conditions are met and no data type requests sync.
 TEST_F(StartupControllerTest, FallbackTimer) {
-  sync_prefs()->SetFirstSetupComplete();
   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
-  EXPECT_FALSE(started());
+  ExpectStartDeferred();
+
   base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(started());
+  ExpectStarted();
 }
 
 // Test that we start immediately if sessions is disabled.
@@ -193,61 +196,44 @@ TEST_F(StartupControllerTest, NoDeferralWithoutSessionsSync) {
   sync_prefs()->SetKeepEverythingSynced(false);
   sync_prefs()->SetPreferredDataTypes(syncer::UserTypes(), types);
   controller()->Reset(syncer::UserTypes());
-  sync_prefs()->SetFirstSetupComplete();
+
   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
-  EXPECT_TRUE(started());
+  ExpectStarted();
 }
 
 // Sanity check that the fallback timer doesn't fire before startup
 // conditions are met.
 TEST_F(StartupControllerTest, FallbackTimerWaits) {
   controller()->TryStart();
-  EXPECT_FALSE(started());
+  ExpectNotStarted();
   base::RunLoop().RunUntilIdle();
-  EXPECT_FALSE(started());
+  ExpectNotStarted();
 }
 
-// Test that sync starts without the user having to explicitly ask for
-// setup when AUTO_START is the startup behavior requested.
-TEST_F(StartupControllerTest, FirstSetupWithAutoStart) {
+TEST_F(StartupControllerTest, NoDeferralWithSetupInProgress) {

[Nicolas Zea, 2016/03/10 21:21:39]

nit: would be good to keep the comments that give more detail about the
rationale behind each test

[maxbogue, 2016/03/11 00:35:34]

Done.

   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
-  controller()->TryStart();
-  EXPECT_TRUE(started());
-}
-
-// Test that sync starts only after user explicitly asks for setup when
-// MANUAL_START is the startup behavior requested.
-TEST_F(StartupControllerTest, FirstSetupWithManualStart) {
-  signin()->set_account_id(kTestUser);
-  token_service()->UpdateCredentials(kTestUser, kTestToken);
-  SetUpController(MANUAL_START);
-  controller()->TryStart();
-  EXPECT_FALSE(started());
   controller()->set_setup_in_progress(true);
   controller()->TryStart();
-  EXPECT_TRUE(started());
+  ExpectStarted();
 }
 
-TEST_F(StartupControllerTest, Reset) {
-  sync_prefs()->SetFirstSetupComplete();
+TEST_F(StartupControllerTest, NoDeferralOnRestart) {

[Nicolas Zea, 2016/03/10 21:21:39]

nit: Shouldn't the name of the test be the opposite?

[maxbogue, 2016/03/11 00:35:34]

Done.

   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
   controller()->TryStart();
-  const bool deferred_start =
-      !base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kSyncDisableDeferredStartup);
-  EXPECT_EQ(!deferred_start, started());
+  ExpectStartDeferred();
   controller()->OnDataTypeRequestsSyncStartup(syncer::SESSIONS);
-  EXPECT_TRUE(started());
+  ExpectStarted();
+
   clear_started();
   controller()->Reset(syncer::UserTypes());
-  EXPECT_FALSE(started());
+  ExpectNotStarted();
   controller()->TryStart();
   // Restart is not deferred.
-  EXPECT_TRUE(started());
+  ExpectStarted();

[Nicolas Zea, 2016/03/10 21:21:39]

I think this should be deferred. I'm pretty sure the test was previously
checking for the reset during the backup/normal transition. We should always
defer on restart. Is this test passing?

[maxbogue, 2016/03/11 00:35:34]

It's not passing in this patch, no. I was letting the bots find all the broken
tests for me because I thought more than just StartupControllerTests would
break, but I guess not.

It WAS passing before the patch, which I believe was due to first_start? That
would explain why I was so confused about the intended behavior in this case.
PTAL, should be fixed now.

 }
 
 // Test that setup-in-progress tracking is persistent across a Reset.