Add support for sync integration tests that span restarts

Historically, sync integration tests were incompatible with testing an
actual browser restarts, leading to (best case) weird workarounds or
(more often) lack of test coverage for very basic functionality (e.g.
loading of persisted data).

Browser tests do support a mechanism for this, which involves prefixing
test names with PRE_, designed to reuse the very same profile path in
multiple tests and hence carry over state.

The only reason why this didn't work for sync integration tests is that
we created random directories on every test run: instead, this patch
makes the profile path deterministic.

In this patch, a couple of tests are migrated to the new scheme, as
a proof-of-concept.

Bug: 856696
Change-Id: I6bf33bcb78d834984ff5230f7119cb79eb8bab55
Reviewed-on: https://chromium-review.googlesource.com/c/1341996
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610152}

chrome/browser/extensions/api/identity/identity_apitest.cc

@@ -501,6 +501,7 @@ class IdentityTestWithSignin : public AsyncExtensionBrowserTest {
 
   std::string SeedAccountInfo(const std::string& email) {
     std::string gaia = "gaia_id_for_" + email;
+    std::replace(gaia.begin(), gaia.end(), '@', '_');
     AccountTrackerService* account_tracker =
         AccountTrackerServiceFactory::GetForProfile(profile());
     return account_tracker->SeedAccountInfo(gaia, email);
@@ -624,17 +625,17 @@ IN_PROC_BROWSER_TEST_F(IdentityGetAccountsFunctionTest,
 IN_PROC_BROWSER_TEST_F(IdentityGetAccountsFunctionTest,
                        PrimaryAccountSignedIn) {
   SignIn("primary@example.com");
-  EXPECT_TRUE(ExpectGetAccounts({"gaia_id_for_primary@example.com"}));
+  EXPECT_TRUE(ExpectGetAccounts({"gaia_id_for_primary_example.com"}));
 }
 
 IN_PROC_BROWSER_TEST_F(IdentityGetAccountsFunctionTest, TwoAccountsSignedIn) {
   SignIn("primary@example.com");
   AddAccount("secondary@example.com");
   if (!id_api()->AreExtensionsRestrictedToPrimaryAccount()) {
-    EXPECT_TRUE(ExpectGetAccounts({"gaia_id_for_primary@example.com",
-                                   "gaia_id_for_secondary@example.com"}));
+    EXPECT_TRUE(ExpectGetAccounts({"gaia_id_for_primary_example.com",
+                                   "gaia_id_for_secondary_example.com"}));
   } else {
-    EXPECT_TRUE(ExpectGetAccounts({"gaia_id_for_primary@example.com"}));
+    EXPECT_TRUE(ExpectGetAccounts({"gaia_id_for_primary_example.com"}));
   }
 }
 
@@ -661,7 +662,7 @@ IN_PROC_BROWSER_TEST_F(IdentityOldProfilesGetAccountsFunctionTest,
                        TwoAccountsSignedIn) {
   SignIn("primary@example.com");
   AddAccount("secondary@example.com");
-  EXPECT_TRUE(ExpectGetAccounts({"gaia_id_for_primary@example.com"}));
+  EXPECT_TRUE(ExpectGetAccounts({"gaia_id_for_primary_example.com"}));
 }
 
 class IdentityGetProfileUserInfoFunctionTest : public IdentityTestWithSignin {
@@ -704,7 +705,7 @@ IN_PROC_BROWSER_TEST_F(IdentityGetProfileUserInfoFunctionTest, SignedIn) {
   std::unique_ptr<api::identity::ProfileUserInfo> info =
       RunGetProfileUserInfoWithEmail();
   EXPECT_EQ("president@example.com", info->email);
-  EXPECT_EQ("gaia_id_for_president@example.com", info->id);
+  EXPECT_EQ("gaia_id_for_president_example.com", info->id);
 }
 
 IN_PROC_BROWSER_TEST_F(IdentityGetProfileUserInfoFunctionTest,
@@ -1785,7 +1786,7 @@ IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest,
   on_access_token_requested_ = run_loop.QuitClosure();
   RunFunctionAsync(
       func.get(),
-      "[{\"account\": { \"id\": \"gaia_id_for_primary@example.com\" } }]");
+      "[{\"account\": { \"id\": \"gaia_id_for_primary_example.com\" } }]");
   run_loop.Run();
 
   std::string primary_account_access_token =
@@ -1819,7 +1820,7 @@ IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest,
   on_access_token_requested_ = run_loop.QuitClosure();
   RunFunctionAsync(
       func.get(),
-      "[{\"account\": { \"id\": \"gaia_id_for_secondary@example.com\" } }]");
+      "[{\"account\": { \"id\": \"gaia_id_for_secondary_example.com\" } }]");
   run_loop.Run();
 
   std::string secondary_account_access_token =
@@ -1864,7 +1865,7 @@ IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest,
   func->set_mint_token_result(TestOAuth2MintTokenFlow::MINT_TOKEN_FAILURE);
   std::string error = utils::RunFunctionAndReturnError(
       func.get(),
-      "[{\"account\": { \"id\": \"gaia_id_for_secondary@example.com\" } }]",
+      "[{\"account\": { \"id\": \"gaia_id_for_secondary_example.com\" } }]",
       browser());
   EXPECT_TRUE(base::StartsWith(error, errors::kAuthFailure,
                                base::CompareCase::INSENSITIVE_ASCII));
@@ -1886,7 +1887,7 @@ IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest,
   func->set_login_access_token_result(false);
   std::string error = utils::RunFunctionAndReturnError(
       func.get(),
-      "[{\"account\": { \"id\": \"gaia_id_for_secondary@example.com\" } }]",
+      "[{\"account\": { \"id\": \"gaia_id_for_secondary_example.com\" } }]",
       browser());
   EXPECT_TRUE(base::StartsWith(error, errors::kAuthFailure,
                                base::CompareCase::INSENSITIVE_ASCII));
@@ -1907,7 +1908,7 @@ IN_PROC_BROWSER_TEST_F(GetAuthTokenFunctionTest,
   func->set_scope_ui_failure(GaiaWebAuthFlow::WINDOW_CLOSED);
   std::string error = utils::RunFunctionAndReturnError(
       func.get(),
-      "[{\"account\": { \"id\": \"gaia_id_for_secondary@example.com\" }, "
+      "[{\"account\": { \"id\": \"gaia_id_for_secondary_example.com\" }, "
       "\"interactive\": "
       "true}]",
       browser());
@@ -2341,7 +2342,7 @@ class OnSignInChangedEventTest : public IdentityTestWithSignin {
 // Test that an event is fired when the primary account signs in.
 IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest, FireOnPrimaryAccountSignIn) {
   api::identity::AccountInfo account_info;
-  account_info.id = "gaia_id_for_primary@example.com";
+  account_info.id = "gaia_id_for_primary_example.com";
   AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, true));
 
   // Sign in and verify that the callback fires.
@@ -2354,7 +2355,7 @@ IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest, FireOnPrimaryAccountSignIn) {
 // Test that an event is fired when the primary account signs out.
 IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest, FireOnPrimaryAccountSignOut) {
   api::identity::AccountInfo account_info;
-  account_info.id = "gaia_id_for_primary@example.com";
+  account_info.id = "gaia_id_for_primary_example.com";
   AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, true));
 
   SignIn("primary@example.com");
@@ -2373,7 +2374,7 @@ IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest, FireOnPrimaryAccountSignOut) {
 IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest,
                        FireOnPrimaryAccountRefreshTokenRevoked) {
   api::identity::AccountInfo account_info;
-  account_info.id = "gaia_id_for_primary@example.com";
+  account_info.id = "gaia_id_for_primary_example.com";
   AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, true));
 
   std::string primary_account_id = SignIn("primary@example.com");
@@ -2391,7 +2392,7 @@ IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest,
 IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest,
                        FireOnPrimaryAccountRefreshTokenAvailable) {
   api::identity::AccountInfo account_info;
-  account_info.id = "gaia_id_for_primary@example.com";
+  account_info.id = "gaia_id_for_primary_example.com";
   AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, true));
 
   std::string primary_account_id = SignIn("primary@example.com");
@@ -2399,7 +2400,7 @@ IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest,
   AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, false));
   token_service_->RevokeCredentials(primary_account_id);
 
-  account_info.id = "gaia_id_for_primary@example.com";
+  account_info.id = "gaia_id_for_primary_example.com";
   AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, true));
 
   // Make the primary account's refresh token available and check that the
@@ -2412,11 +2413,11 @@ IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest,
 // Test that an event is fired for changes to a secondary account.
 IN_PROC_BROWSER_TEST_F(OnSignInChangedEventTest, FireForSecondaryAccount) {
   api::identity::AccountInfo account_info;
-  account_info.id = "gaia_id_for_primary@example.com";
+  account_info.id = "gaia_id_for_primary_example.com";
   AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, true));
   SignIn("primary@example.com");
 
-  account_info.id = "gaia_id_for_secondary@example.com";
+  account_info.id = "gaia_id_for_secondary_example.com";
   AddExpectedEvent(api::identity::OnSignInChanged::Create(account_info, true));
 
   // Make a secondary account's refresh token available and check that the

chrome/browser/sync/test/integration/enable_disable_test.cc

@@ -277,6 +277,24 @@ IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, EnableDisable) {
   }
 }
 
+IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, PRE_EnableAndRestart) {
+  SetupTest(/*all_types_enabled=*/true);
+}
+
+IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, EnableAndRestart) {
+  ASSERT_TRUE(SetupClients());
+
+  EXPECT_TRUE(GetClient(0)->AwaitEngineInitialization());
+
+  // Proxy types don't really run.
+  const ModelTypeSet non_proxy_types =
+      Difference(selectable_types_, ProxyTypes());
+
+  for (ModelType type : non_proxy_types) {
+    EXPECT_TRUE(ModelTypeExists(type)) << " for " << ModelTypeToString(type);
+  }
+}
+
 IN_PROC_BROWSER_TEST_F(EnableDisableSingleClientTest, FastEnableDisableEnable) {
   SetupTest(/*all_types_enabled=*/false);
 

chrome/browser/sync/test/integration/single_client_polling_sync_test.cc

@@ -106,14 +106,17 @@ IN_PROC_BROWSER_TEST_F(SingleClientPollingSyncTest,
 // Note, that there's a more realistic (and more complex) test for this in
 // two_client_polling_sync_test.cc too.
 IN_PROC_BROWSER_TEST_F(SingleClientPollingSyncTest,
-                       ShouldPollWhenIntervalExpiredAcrossRestarts) {
+                       PRE_ShouldPollWhenIntervalExpiredAcrossRestarts) {
   base::Time start = base::Time::Now();
 
   ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
 
   SyncPrefs remote_prefs(GetProfile(0)->GetPrefs());
-  remote_prefs.SetShortPollInterval(base::TimeDelta::FromMinutes(10));
-  remote_prefs.SetLongPollInterval(base::TimeDelta::FromMinutes(10));
+  // Set small polling intervals to make random delays introduced in
+  // SyncSchedulerImpl::ComputeLastPollOnStart() negligible, but big enough to
+  // avoid periodic polls during a test run.
+  remote_prefs.SetLongPollInterval(base::TimeDelta::FromSeconds(300));
+  remote_prefs.SetShortPollInterval(base::TimeDelta::FromSeconds(300));
 
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
@@ -130,15 +133,24 @@ IN_PROC_BROWSER_TEST_F(SingleClientPollingSyncTest,
   EXPECT_THAT(remote_prefs.GetLastPollTime(), Ge(start));
   EXPECT_THAT(remote_prefs.GetLastPollTime(), Le(base::Time::Now()));
 
-  // Make sure no extra sync cycles get triggered by test infrastructure and
-  // stop sync.
-  StopConfigurationRefresher();
-  GetClient(0)->StopSyncService(syncer::SyncService::KEEP_DATA);
-
-  // Simulate elapsed time so that the poll interval expired.
+  // Simulate elapsed time so that the poll interval expired upon restart.
   remote_prefs.SetLastPollTime(base::Time::Now() -
-                               base::TimeDelta::FromMinutes(11));
-  ASSERT_TRUE(GetClient(0)->StartSyncService()) << "SetupSync() failed.";
+                               remote_prefs.GetLongPollInterval());
+}
+
+IN_PROC_BROWSER_TEST_F(SingleClientPollingSyncTest,
+                       ShouldPollWhenIntervalExpiredAcrossRestarts) {
+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+#if defined(CHROMEOS)
+  // identity::SetRefreshTokenForPrimaryAccount() is needed on ChromeOS in order
+  // to get a non-empty refresh token on startup.
+  GetClient(0)->SignInPrimaryAccount();
+#endif  // defined(CHROMEOS)
+  ASSERT_TRUE(GetClient(0)->AwaitEngineInitialization());
+
+  SyncPrefs remote_prefs(GetProfile(0)->GetPrefs());
+  ASSERT_FALSE(remote_prefs.GetLastPollTime().is_null());
+
   // After the start, the last sync cycle snapshot should be empty.
   // Once a sync request happened (e.g. by a poll), that snapshot is populated.
   // We use the following checker to simply wait for an non-empty snapshot.

chrome/browser/sync/test/integration/single_client_user_consents_sync_test.cc

@@ -31,7 +31,7 @@ std::string GetAccountId() {
   // impossible to discover otherwise.
   return "user@gmail.com";
 #else
-  return "gaia_id_for_user@gmail.com";
+  return "gaia_id_for_user_gmail.com";
 #endif
 }
 

chrome/browser/sync/test/integration/single_client_user_events_sync_test.cc

@@ -41,7 +41,7 @@ std::string GetAccountId() {
   // impossible to discover otherwise.
   return "user@gmail.com";
 #else
-  return "gaia_id_for_user@gmail.com";
+  return "gaia_id_for_user_gmail.com";
 #endif
 }
 

chrome/browser/sync/test/integration/sync_test.cc

@@ -336,9 +336,11 @@ bool SyncTest::CreateGaiaAccount(const std::string& username,
 void SyncTest::BeforeSetupClient(int index) {}
 
 bool SyncTest::CreateProfile(int index) {
+  base::FilePath profile_path;
+
   base::ScopedAllowBlockingForTesting allow_blocking;
-  tmp_profile_paths_[index] = new base::ScopedTempDir();
   if (UsingExternalServers() && num_clients_ > 1) {
+    scoped_temp_dirs_.push_back(std::make_unique<base::ScopedTempDir>());
     // For multi profile UI signin, profile paths should be outside user data
     // dir to allow signing-in multiple profiles to same account. Otherwise, we
     // get an error that the profile has already signed in on this device.
@@ -346,23 +348,26 @@ bool SyncTest::CreateProfile(int index) {
     // user data dir. We violate that assumption here, which can lead to weird
     // issues, see https://crbug.com/801569 and the workaround in
     // TearDownOnMainThread.
-    if (!tmp_profile_paths_[index]->CreateUniqueTempDir()) {
+    if (!scoped_temp_dirs_.back()->CreateUniqueTempDir()) {
       ADD_FAILURE();
       return false;
     }
+
+    profile_path = scoped_temp_dirs_.back()->GetPath();
   } else {
-    // Create new profiles in user data dir so that other profiles can know
-    // about it. This is needed in tests such as supervised user cases which
-    // assume browser->profile() as the custodian profile.
     base::FilePath user_data_dir;
     base::PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
-    if (!tmp_profile_paths_[index]->CreateUniqueTempDirUnderPath(
-            user_data_dir)) {
-      ADD_FAILURE();
-      return false;
-    }
+
+    // Create new profiles in user data dir so that other profiles can know
+    // about it. This is needed in tests such as supervised user cases which
+    // assume browser->profile() as the custodian profile. Instead of creating
+    // a new directory, we use a deterministic name such that PRE_ tests (i.e.
+    // test that span browser restarts) can reuse the same directory and carry
+    // over state.
+    profile_path = user_data_dir.AppendASCII(
+        base::StringPrintf("SyncIntegrationTestClient%d", index));
   }
-  base::FilePath profile_path = tmp_profile_paths_[index]->GetPath();
+
   if (UsingExternalServers()) {
     // If running against an EXTERNAL_LIVE_SERVER, we signin profiles using real
     // GAIA server. This requires creating profiles with no test hooks.
@@ -539,7 +544,6 @@ bool SyncTest::SetupClients() {
   // Create the required number of sync profiles, browsers and clients.
   profiles_.resize(num_clients_);
   profile_delegates_.resize(num_clients_ + 1);  // + 1 for the verifier.
-  tmp_profile_paths_.resize(num_clients_);
   clients_.resize(num_clients_);
   invalidation_forwarders_.resize(num_clients_);
   sync_refreshers_.resize(num_clients_);
@@ -904,10 +908,6 @@ void SyncTest::TearDownOnMainThread() {
         previous_profile_->GetPath().BaseName().MaybeAsASCII());
   }
 
-  for (size_t i = 0; i < clients_.size(); ++i) {
-    clients_[i]->service()->RequestStop(ProfileSyncService::CLEAR_DATA);
-  }
-
   // Closing all browsers created by this test. The calls here block until
   // they are closed. Other browsers created outside SyncTest setup should be
   // closed by the creator of that browser.

chrome/browser/sync/test/integration/sync_test.h

@@ -447,10 +447,9 @@ class SyncTest : public InProcessBrowserTest {
   // time.
   std::vector<std::unique_ptr<Profile::Delegate>> profile_delegates_;
 
-  // Collection of profile paths used by a test. Each profile has a unique path
-  // which should be cleaned up at test shutdown. Profile paths inside the
-  // default user data dir gets deleted at InProcessBrowserTest teardown.
-  std::vector<base::ScopedTempDir*> tmp_profile_paths_;
+  // List of temporary directories that need to be deleted when the test is
+  // completed, used for two-client tests with external server.
+  std::vector<std::unique_ptr<base::ScopedTempDir>> scoped_temp_dirs_;
 
   // Collection of pointers to the browser objects used by a test. One browser
   // instance is created for each sync profile. Browser object lifetime is

services/identity/public/cpp/identity_test_utils.cc

@@ -4,11 +4,15 @@
 
 #include "services/identity/public/cpp/identity_test_utils.h"
 
+#include <utility>
+#include <vector>
+
 #include "base/run_loop.h"
 #include "base/strings/string_split.h"
 #include "components/signin/core/browser/account_tracker_service.h"
 #include "components/signin/core/browser/fake_gaia_cookie_manager_service.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
+#include "google_apis/gaia/gaia_auth_util.h"
 #include "services/identity/public/cpp/identity_manager.h"
 
 namespace identity {
@@ -124,6 +128,15 @@ void UpdateRefreshTokenForAccount(ProfileOAuth2TokenService* token_service,
   run_loop.Run();
 }
 
+std::string GetTestGaiaIdForEmail(const std::string& email) {
+  std::string gaia_id =
+      std::string("gaia_id_for_") + gaia::CanonicalizeEmail(email);
+  // Avoid character '@' in the gaia ID string as there is code in the codebase
+  // that asserts that a gaia ID does not contain a "@" character.
+  std::replace(gaia_id.begin(), gaia_id.end(), '@', '_');
+  return gaia_id;
+}
+
 }  // namespace
 
 AccountInfo SetPrimaryAccount(IdentityManager* identity_manager,
@@ -133,7 +146,7 @@ AccountInfo SetPrimaryAccount(IdentityManager* identity_manager,
   SigninManagerBase* signin_manager = identity_manager->GetSigninManager();
   DCHECK(!signin_manager->IsAuthenticated());
 
-  std::string gaia_id = "gaia_id_for_" + email;
+  std::string gaia_id = GetTestGaiaIdForEmail(email);
 
 #if defined(OS_CHROMEOS)
   // ChromeOS has no real notion of signin, so just plumb the information
@@ -253,7 +266,7 @@ AccountInfo MakeAccountAvailable(IdentityManager* identity_manager,
   DCHECK(account_tracker_service);
   DCHECK(account_tracker_service->FindAccountInfoByEmail(email).IsEmpty());
 
-  std::string gaia_id = "gaia_id_for_" + email;
+  std::string gaia_id = GetTestGaiaIdForEmail(email);
   account_tracker_service->SeedAccountInfo(gaia_id, email);
 
   AccountInfo account_info =