[sync] Add Sessions integration tests

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

 // Copyright (c) 2012 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 "chrome/browser/sync/test/integration/sync_test.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <limits>
@@ skipping 402 matching lines @@
   }
 
   Profile* profile =
       Profile::CreateProfile(profile_path, profile_delegates_[index].get(),
                              Profile::CREATE_MODE_SYNCHRONOUS);
   return profile;
 }
 
 Profile* SyncTest::GetProfile(int index) {
   if (profiles_.empty())
     LOG(FATAL) << "SetupClients() has not yet been called.";

[skym, 2017/02/24 16:20:17]

Can these be EXPECT_* and not LOG(FATAL)?

[Patrick Noland, 2017/02/27 18:53:26]

Done.

   if (index < 0 || index >= static_cast<int>(profiles_.size()))
     LOG(FATAL) << "GetProfile(): Index is out of bounds.";
   return profiles_[index];

[skym, 2017/02/24 16:20:17]

What would happen if you checked for nullptr here? Instead of at every call
site.

[Patrick Noland, 2017/02/27 18:53:26]

Done.

 }
 
 std::vector<Profile*> SyncTest::GetAllProfiles() {
   std::vector<Profile*> profiles;
   if (use_verifier()) {
     profiles.push_back(verifier());
   }
   for (int i = 0; i < num_clients(); ++i) {
     profiles.push_back(GetProfile(i));
   }
   return profiles;
 }
 
 Browser* SyncTest::GetBrowser(int index) {
   if (browsers_.empty())
     LOG(FATAL) << "SetupClients() has not yet been called.";
   if (index < 0 || index >= static_cast<int>(browsers_.size()))
     LOG(FATAL) << "GetBrowser(): Index is out of bounds.";
   return browsers_[index];

[skym, 2017/02/24 16:20:17]

Same, can we check for nullptr?

[Patrick Noland, 2017/02/27 18:53:26]

Done.

 }
 
+Browser* SyncTest::AddBrowser(int profile_index) {
+  Profile* profile = GetProfile(profile_index);
+  EXPECT_NE(nullptr, profile)
+      << "No profile found at profile_index: " << profile_index;
+
+  // Add the new browser at the end of the list.
+  int index = browsers_.size();
+  browsers_.resize(browsers_.size() + 1);

[skym, 2017/02/24 00:14:09]

why not browsers_.push_back(new Browser(...)); ?

[Patrick Noland, 2017/02/24 01:00:40]

Because I'm copying the bizarre pattern used elsewhere in this file. I'll fix
it.

+  profiles_.resize(profiles_.size() + 1);
+  browsers_[index] = new Browser(Browser::CreateParams(profile, true));
+
+  Browser* browser = GetBrowser(index);

[skym, 2017/02/24 00:14:09]

But. What? You just made this thing, why fetch it again?

[Patrick Noland, 2017/02/24 01:00:40]

Because I'm copying the bizarre pattern used elsewhere in this file. I'll fix
it.

[skym, 2017/02/24 16:20:17]

...Is this a memory leak? Who owns this Browser object?

[Patrick Noland, 2017/02/27 18:53:26]

According to sync_test.h "Browser object lifetime is managed by BrowserList, so
we don't use a ScopedVector here."

+  EXPECT_NE(nullptr, browser) << "Could not create Browser " << index;

[skym, 2017/02/24 00:14:09]

So when does this happen?

[Patrick Noland, 2017/02/24 01:00:40]

Good question. The old code seemed to think it was possible so I'm loath to
remove it.

[skym, 2017/02/24 16:20:17]

This is test code, worst case a test fails. Remove it!

+  profiles_[index] = profile;

[skym, 2017/02/24 00:14:09]

I'm confused, is browsers_.size() allowed to != profiles_.size() or not?

[Patrick Noland, 2017/02/24 01:00:40]

No, they should always match. There's a longwinded explanation above, but the
tldr is we copy the profile pointer so that the correspondence in size and
position is maintained. 

+
+  return browser;
+}
+
 ProfileSyncServiceHarness* SyncTest::GetClient(int index) {
   if (clients_.empty())
     LOG(FATAL) << "SetupClients() has not yet been called.";
   if (index < 0 || index >= static_cast<int>(clients_.size()))
     LOG(FATAL) << "GetClient(): Index is out of bounds.";
   return clients_[index];
 }
 
 ProfileSyncService* SyncTest::GetSyncService(int index) {
   return ProfileSyncServiceFactory::GetForProfile(GetProfile(index));
@@ skipping 22 matching lines @@
 bool SyncTest::SetupClients() {
   if (num_clients_ <= 0)
     LOG(FATAL) << "num_clients_ incorrectly initialized.";
   if (!profiles_.empty() || !browsers_.empty() || !clients_.empty())
     LOG(FATAL) << "SetupClients() has already been called.";
 
   // 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_);
-  browsers_.resize(num_clients_);
   clients_.resize(num_clients_);
   invalidation_forwarders_.resize(num_clients_);
   sync_refreshers_.resize(num_clients_);
   fake_server_invalidation_services_.resize(num_clients_);
 
   if (create_gaia_account_at_runtime_) {
     CHECK(UsingExternalServers()) <<
         "Cannot create Gaia accounts without external authentication servers";
     if (!CreateGaiaAccount(username_, password_))
       LOG(FATAL) << "Could not create Gaia account.";
@@ skipping 20 matching lines @@
   // Error cases are all handled by LOG(FATAL) messages. So there is not really
   // a case that returns false.  In case we failed to create a verifier profile,
   // any call to the verifier() would fail.
   return true;
 }
 
 void SyncTest::InitializeProfile(int index, Profile* profile) {
   DCHECK(profile);
   profiles_[index] = profile;
 
-  // CheckInitialState() assumes that no windows are open at startup.
-  browsers_[index] =
-      new Browser(Browser::CreateParams(GetProfile(index), true));
-
-  EXPECT_NE(nullptr, GetBrowser(index)) << "Could not create Browser " << index;
+  AddBrowser(index);
 
   // Make sure the ProfileSyncService has been created before creating the
   // ProfileSyncServiceHarness - some tests expect the ProfileSyncService to
   // already exist.
   ProfileSyncService* profile_sync_service =
       ProfileSyncServiceFactory::GetForProfile(GetProfile(index));
 
   if (server_type_ == IN_PROCESS_FAKE_SERVER) {
     // TODO(pvalenzuela): Run the fake server via EmbeddedTestServer.
     profile_sync_service->OverrideNetworkResourcesForTest(
@@ skipping 586 matching lines @@
 
 void SyncTest::TriggerSyncForModelTypes(int index,
                                         syncer::ModelTypeSet model_types) {
   GetSyncService(index)->TriggerRefresh(model_types);
 }
 
 void SyncTest::SetPreexistingPreferencesFileContents(
     const std::string& contents) {
   preexisting_preferences_file_contents_ = contents;
 }