[Sync] Simplify sync startup behavior.

components/sync_driver/startup_controller_unittest.cc

 // Copyright 2014 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 "components/sync_driver/startup_controller.h"
 
 #include <string>
 
 #include "base/command_line.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 37 matching lines @@
 class StartupControllerTest : public testing::Test {
  public:
   StartupControllerTest() : started_(false) {}
 
   void SetUp() override {
     sync_driver::SyncPrefs::RegisterProfilePrefs(pref_service_.registry());
     sync_prefs_.reset(new sync_driver::SyncPrefs(&pref_service_));
     token_service_.reset(new FakeProfileOAuth2TokenService());
     signin_.reset(new FakeSigninManagerWrapper());
 
-    SetUpController(AUTO_START);
+    SetUpController();
   }
 
   void TearDown() override {
     controller_.reset();
     signin_.reset();
     token_service_->Shutdown();
     token_service_.reset();
     sync_prefs_.reset();
     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));
   }
 
   void FakeStartBackend() {
     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(); }
   FakeSigninManagerWrapper* signin() { return signin_.get(); }
   FakeProfileOAuth2TokenService* token_service() {
     return token_service_.get();
   }
   sync_driver::SyncPrefs* sync_prefs() { return sync_prefs_.get(); }
 
  private:
   bool started_;
   base::MessageLoop message_loop_;
   syncable_prefs::TestingPrefServiceSyncable pref_service_;
   scoped_ptr<StartupController> controller_;
   scoped_ptr<FakeSigninManagerWrapper> signin_;
   scoped_ptr<FakeProfileOAuth2TokenService> token_service_;
   scoped_ptr<sync_driver::SyncPrefs> sync_prefs_;
 };
 
 // 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.
   clear_started();
   base::RunLoop().RunUntilIdle();
   EXPECT_FALSE(started());
 }
 
 // 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.
 TEST_F(StartupControllerTest, NoDeferralWithoutSessionsSync) {
   syncer::ModelTypeSet types(syncer::UserTypes());
   // Disabling sessions means disabling 4 types due to groupings.
   types.Remove(syncer::SESSIONS);
   types.Remove(syncer::PROXY_TABS);
   types.Remove(syncer::TYPED_URLS);
   types.Remove(syncer::SUPERVISED_USER_SETTINGS);
   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()->set_setup_in_progress(true);
+  controller()->TryStart();
+  ExpectStarted();
+}
+
+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();
-  EXPECT_TRUE(started());
-}
+  ExpectStartDeferred();
+  controller()->OnDataTypeRequestsSyncStartup(syncer::SESSIONS);
+  ExpectStarted();
 
-// 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());
-}
-
-TEST_F(StartupControllerTest, Reset) {
-  sync_prefs()->SetFirstSetupComplete();
-  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());
-  controller()->OnDataTypeRequestsSyncStartup(syncer::SESSIONS);
-  EXPECT_TRUE(started());
   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.
 TEST_F(StartupControllerTest, ResetDuringSetup) {
   signin()->set_account_id(kTestUser);
   token_service()->UpdateCredentials(kTestUser, kTestToken);
 
   // Simulate UI telling us setup is in progress.
   controller()->set_setup_in_progress(true);
 
   // This could happen if the UI triggers a stop-syncing permanently call.
   controller()->Reset(syncer::UserTypes());
 
   // From the UI's point of view, setup is still in progress.
   EXPECT_TRUE(controller()->IsSetupInProgress());
 }
 
 }  // namespace browser_sync