[sync] Add Sessions integration tests

chrome/browser/sync/test/integration/two_client_sessions_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 <string>
 
 #include "base/guid.h"
 #include "base/macros.h"
 #include "base/strings/stringprintf.h"
 #include "build/build_config.h"
 #include "chrome/browser/sessions/session_service.h"
 #include "chrome/browser/sync/test/integration/passwords_helper.h"
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
 #include "chrome/browser/sync/test/integration/sessions_helper.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "components/sync/engine/cycle/sync_cycle_snapshot.h"
 
 using sessions_helper::CheckInitialState;
 using sessions_helper::DeleteForeignSession;
 using sessions_helper::GetLocalWindows;
 using sessions_helper::GetSessionData;
+using sessions_helper::NavigateTab;
 using sessions_helper::OpenTabAndGetLocalWindows;
+using sessions_helper::OpenTab;
+using sessions_helper::OpenTabAtIndex;
 using sessions_helper::ScopedWindowMap;
 using sessions_helper::SessionWindowMap;
 using sessions_helper::SyncedSessionVector;
 using sessions_helper::WindowsMatch;
 
 class TwoClientSessionsSyncTest : public SyncTest {
  public:
   TwoClientSessionsSyncTest() : SyncTest(TWO_CLIENT) {}
   ~TwoClientSessionsSyncTest() override {}
 
+  void WaitForWindowsInForeignSession(int profile_index,
+                                      ScopedWindowMap windows) {
+    std::vector<ScopedWindowMap> expected_windows(1);

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

Wow, you really seem to like reserving space. What about using an initializer
list instead?

EXPECT_TRUE(ForeignSessionsMatchChecker(profile_index, {std::move(windows)}));

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

I actually tried this. You can't (trivially) put unique_ptrs into initializer
lists like that unfortunately. 

[skym, 2017/02/27 19:12:27]

Why?

[skym, 2017/02/27 19:42:49]

Okay, you're right, and things get confusing really fast.

http://stackoverflow.com/questions/8468774/can-i-list-initialize-a-vector-of-...
http://stackoverflow.com/questions/7231351/initializer-list-constructing-a-ve...

[Patrick Noland, 2017/02/27 19:44:50]

initializer lists don't work with move only types. They provide read only
sequences, and the proposal explicitly for them explicitly states the desire to
share identical initializer_list sequences by reusing the same instance. As far
as I can tell this isn't an intentional feature, just the result of designing
initializer lists well before move semantics were formalized. There is some
background here: http://open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4166.pdf

+    expected_windows[0] = std::move(windows);
+    ASSERT_TRUE(

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

EXPECT_TRUE?

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

Done.

+        ForeignSessionsMatchChecker(profile_index, expected_windows).Wait());
+  }
+
  private:
   DISALLOW_COPY_AND_ASSIGN(TwoClientSessionsSyncTest);
 };
 
 static const char* kURL1 = "http://127.0.0.1/bubba1";
 static const char* kURL2 = "http://127.0.0.1/bubba2";
+static const char* kURL3 = "http://127.0.0.1/foobar";
+static const char* kURL4 = "http://127.0.0.1/barbaz";
 
 // TODO(zea): Test each individual session command we care about separately.
 // (as well as multi-window). We're currently only checking basic single-window/
 // single-tab functionality.
 
 
 IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest,
                        E2E_ENABLED(SingleClientChanged)) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
@@ skipping 85 matching lines @@
 }
 
 IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, BothChanged) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
   ASSERT_TRUE(CheckInitialState(0));
   ASSERT_TRUE(CheckInitialState(1));
 
   // Open tabs on both clients and retain window information.
   ScopedWindowMap client0_windows;
   ASSERT_TRUE(OpenTabAndGetLocalWindows(0, GURL(kURL2), &client0_windows));

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

This pattern seems fairly awkward. It shows up over and over here.

ScopedWindowMap client0_windows;
ASSERT_TRUE(OpenTabAndGetLocalWindows(0, GURL(kURL2), &client0_windows));
WaitForWindowsInForeignSession(1, std::move(client0_windows));

I'd prefer to see something like

EXPECT_TRUE(OpenTab(0, GURL(kURL2));
WaitForForeignSessionsToSync({0, 1}, FROM_HERE);

Don't feel like you have to do this though.

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

I've gone ahead and added a helper to do this, although I'm not sure what the
FOR_HERE is for?

[skym, 2017/02/27 19:12:27]

So, we have a problem with our integ test checkers, in that when you fail a
build, often there isn't enough information to figure out what went wrong. I
actually made this problem a bunch worse when I moved the critical portions of
waiting into a base class in https://codereview.chromium.org/2379433002/ . I'd
really like test failures to contain exactly which wait they were, and my
thoughts were that FROM_HERE would facilitate that. It's still a half baked
idea, I haven't tried any of this yet. Feel free to ignore for now.

   ScopedWindowMap client1_windows;
   ASSERT_TRUE(OpenTabAndGetLocalWindows(1, GURL(kURL1), &client1_windows));
 
-  // Wait for sync.
-  ASSERT_TRUE(AwaitQuiescence());
+  WaitForWindowsInForeignSession(1, std::move(client0_windows));
+  WaitForWindowsInForeignSession(0, std::move(client0_windows));

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

You're moving client0_windows twice.

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

Done.

 
-  // Get foreign session data from client 0 and 1.
-  SyncedSessionVector sessions0;
-  SyncedSessionVector sessions1;
-  ASSERT_TRUE(GetSessionData(0, &sessions0));
-  ASSERT_TRUE(GetSessionData(1, &sessions1));
-
-  // Verify client 1's foreign session matches client 0's current window and
-  // vice versa.
-  ASSERT_EQ(1U, sessions0.size());
-  ASSERT_EQ(1U, sessions1.size());
-  ASSERT_TRUE(WindowsMatch(sessions1[0]->windows, client0_windows));
-  ASSERT_TRUE(WindowsMatch(sessions0[0]->windows, client1_windows));
+  // Check that a navigation is reflected on the other client.
+  NavigateTab(0, GURL(kURL3));
+  ASSERT_TRUE(GetLocalWindows(0, &client0_windows));
+  WaitForWindowsInForeignSession(1, std::move(client0_windows));
 }
 
 IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, DeleteIdleSession) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
   ASSERT_TRUE(CheckInitialState(0));
   ASSERT_TRUE(CheckInitialState(1));
 
   // Client 0 opened some tabs then went idle.
   ScopedWindowMap client0_windows;
   ASSERT_TRUE(OpenTabAndGetLocalWindows(0, GURL(kURL1), &client0_windows));
 
-  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
+  WaitForWindowsInForeignSession(1, std::move(client0_windows));
 
   // Get foreign session data from client 1.
   SyncedSessionVector sessions1;
   ASSERT_TRUE(GetSessionData(1, &sessions1));
 
-  // Verify client 1's foreign session matches client 0 current window.
-  ASSERT_EQ(1U, sessions1.size());
-  ASSERT_TRUE(WindowsMatch(sessions1[0]->windows, client0_windows));
-
   // Client 1 now deletes client 0's tabs. This frees the memory of sessions1.
   DeleteForeignSession(1, sessions1[0]->session_tag);
   ASSERT_TRUE(GetClient(1)->AwaitMutualSyncCycleCompletion(GetClient(0)));
   ASSERT_FALSE(GetSessionData(1, &sessions1));
 }
 
 // Fails all release trybots. crbug.com/263369.
 IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest,
                        DeleteActiveSession) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
   ASSERT_TRUE(CheckInitialState(0));
   ASSERT_TRUE(CheckInitialState(1));
 
   // Client 0 opened some tabs then went idle.
   ScopedWindowMap client0_windows;
   ASSERT_TRUE(OpenTabAndGetLocalWindows(0, GURL(kURL1), &client0_windows));
 
-  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
+  WaitForWindowsInForeignSession(1, std::move(client0_windows));
+
   SyncedSessionVector sessions1;
   ASSERT_TRUE(GetSessionData(1, &sessions1));
   ASSERT_EQ(1U, sessions1.size());
-  ASSERT_TRUE(WindowsMatch(sessions1[0]->windows, client0_windows));
 
   // Client 1 now deletes client 0's tabs. This frees the memory of sessions1.
   DeleteForeignSession(1, sessions1[0]->session_tag);
   ASSERT_TRUE(GetClient(1)->AwaitMutualSyncCycleCompletion(GetClient(0)));
   ASSERT_FALSE(GetSessionData(1, &sessions1));
 
   // Client 0 becomes active again with a new tab.
   ASSERT_TRUE(OpenTabAndGetLocalWindows(0, GURL(kURL2), &client0_windows));
-  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
-  ASSERT_TRUE(GetSessionData(1, &sessions1));
-  ASSERT_EQ(1U, sessions1.size());
-  ASSERT_TRUE(WindowsMatch(sessions1[0]->windows, client0_windows));
+  WaitForWindowsInForeignSession(1, std::move(client0_windows));
 }
+
+IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, MultipleWindowsMultipleTabs) {
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+
+  ASSERT_TRUE(CheckInitialState(0));
+  ASSERT_TRUE(CheckInitialState(1));
+
+  ASSERT_TRUE(OpenTab(0, GURL(kURL1)));
+  ASSERT_TRUE(OpenTabAtIndex(0, 1, GURL(kURL2)));
+
+  // Add a second browser for profile 0. This browser ends up in index 2.
+  AddBrowser(0);
+  ASSERT_TRUE(OpenTab(2, GURL(kURL3)));
+  ASSERT_TRUE(OpenTabAtIndex(2, 2, GURL(kURL4)));
+
+  ScopedWindowMap client0_windows;
+  ASSERT_TRUE(GetLocalWindows(2, &client0_windows));
+  EXPECT_EQ(client0_windows.size(), 2u);

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

https://github.com/google/googletest/blob/master/googletest/docs/Primer.md

"Historical note: Before February 2016 *_EQ had a convention of calling it as
ASSERT_EQ(expected, actual), so lots of existing code uses this order. Now *_EQ
treats both parameters in the same way."

I guess they don't care anymore, I liked it the way things were!

+
+  WaitForWindowsInForeignSession(1, std::move(client0_windows));
+}