[sync] Add Sessions integration tests

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

Index: chrome/browser/sync/test/integration/sync_test.cc
diff --git a/chrome/browser/sync/test/integration/sync_test.cc b/chrome/browser/sync/test/integration/sync_test.cc
index f40d1f98fd44fbfb4f595c9aa98392743c0720e2..ced355a29747c443c1d9ef6f90ba49cc1f44845e 100644
--- a/chrome/browser/sync/test/integration/sync_test.cc
+++ b/chrome/browser/sync/test/integration/sync_test.cc
@@ -445,6 +445,24 @@ Browser* SyncTest::GetBrowser(int index) {
   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.";
@@ -487,7 +505,6 @@ bool SyncTest::SetupClients() {
   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_);
@@ -528,11 +545,7 @@ 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