reland of [sync] Add Sessions integration tests

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

Index: chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
diff --git a/chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc b/chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
index 69c51bc0f108aa593ab06e87307ff52c5bd97e41..ea43256b06c5cbf61ae977d199de0c3162d0e1ea 100644
--- a/chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
+++ b/chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
@@ -3,21 +3,22 @@
 // found in the LICENSE file.
 
 #include "base/macros.h"
+#include "base/path_service.h"
 #include "base/test/histogram_tester.h"
 #include "chrome/browser/sessions/session_service.h"
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
+#include "chrome/browser/sync/test/integration/session_hierarchy_match_checker.h"
 #include "chrome/browser/sync/test/integration/sessions_helper.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/browser/sync/test/integration/typed_urls_helper.h"
 #include "chrome/browser/sync/test/integration/updated_progress_marker_checker.h"
-#include "chrome/common/chrome_switches.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/common/url_constants.h"
 #include "components/browser_sync/profile_sync_service.h"
 #include "components/history/core/browser/history_types.h"
 #include "components/sessions/core/session_types.h"
 #include "components/sync/base/time.h"
 #include "components/sync/driver/sync_driver_switches.h"
-#include "components/sync/test/fake_server/fake_server_verifier.h"
 #include "components/sync/test/fake_server/sessions_hierarchy.h"
 
 #if defined(OS_CHROMEOS)
@@ -32,7 +33,12 @@ using sessions_helper::CheckInitialState;
 using sessions_helper::GetLocalWindows;
 using sessions_helper::GetSessionData;
 using sessions_helper::ModelAssociatorHasTabWithUrl;
-using sessions_helper::OpenTabAndGetLocalWindows;
+using sessions_helper::MoveTab;
+using sessions_helper::NavigateTab;
+using sessions_helper::NavigateTabBack;
+using sessions_helper::NavigateTabForward;
+using sessions_helper::OpenTab;
+using sessions_helper::OpenTabAtIndex;
 using sessions_helper::ScopedWindowMap;
 using sessions_helper::SessionWindowMap;
 using sessions_helper::SyncedSessionVector;
@@ -49,7 +55,7 @@ void ExpectUniqueSampleGE(const HistogramTester& histogram_tester,
   std::unique_ptr<HistogramSamples> samples =
       histogram_tester.GetHistogramSamplesSinceCreation(name);
   int sample_count = samples->GetCount(sample);
-  EXPECT_GE(expected_inclusive_lower_bound, sample_count);
+  EXPECT_GE(sample_count, expected_inclusive_lower_bound);

[skym, 2017/03/02 19:56:11]

When trying to always put the expected value on the left, goes wrong. Thanks for
fixing my mistake.

[Patrick Noland, 2017/03/02 21:57:44]

Acknowledged.

   EXPECT_EQ(sample_count, samples->TotalCount());
 }
 
@@ -60,25 +66,41 @@ class SingleClientSessionsSyncTest : public SyncTest {
   SingleClientSessionsSyncTest() : SyncTest(SINGLE_CLIENT) {}
   ~SingleClientSessionsSyncTest() override {}
 
-  void SetUpCommandLine(base::CommandLine* cl) override {
-    // This is a hacky override of the switches set in
-    // SyncTest::SetUpCommandLine() to avoid the switch that speeds up nudge
-    // delays. CookieJarMismatch asserts exact histogram counts assuming that
-    // sync is relatively slow, so we preserve that assumption.
-    if (!cl->HasSwitch(switches::kDisableBackgroundNetworking))
-      cl->AppendSwitch(switches::kDisableBackgroundNetworking);
+  void SetUpOnMainThread() override {
+    base::FilePath test_data_dir;
+    PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir);
+    embedded_test_server()->ServeFilesFromDirectory(test_data_dir);
+    ASSERT_TRUE(embedded_test_server()->Start());
+  }
 
-    if (!cl->HasSwitch(switches::kSyncShortInitialRetryOverride))
-      cl->AppendSwitch(switches::kSyncShortInitialRetryOverride);
+  void ExpectNavigationChain(const std::vector<GURL>& urls) {
+    ScopedWindowMap windows;
+    ASSERT_TRUE(GetLocalWindows(0, &windows));
+    ASSERT_EQ(windows.begin()->second->tabs.size(), 1u);
+    sessions::SessionTab* tab = windows.begin()->second->tabs[0].get();
+
+    int index = 0;
+    EXPECT_EQ(urls.size(), tab->navigations.size());
+    for (auto it = tab->navigations.begin(); it != tab->navigations.end();
+         ++it, ++index) {
+      EXPECT_EQ(urls[index], it->virtual_url());
+    }
+  }
 
-#if defined(OS_CHROMEOS)
-    // kIgnoreUserProfileMappingForTests will let UserManager always return
-    // active user. If this switch is not set, sync test's profile will not
-    // match UserManager's active user, then UserManager won't return active
-    // user to our tests.
-    if (!cl->HasSwitch(chromeos::switches::kIgnoreUserProfileMappingForTests))
-      cl->AppendSwitch(chromeos::switches::kIgnoreUserProfileMappingForTests);
-#endif
+  // Block until the expected hierarchy is recorded on the FakeServer for
+  // profile 0. This will time out if the hierarchy is never
+  // recorded.
+  void WaitForHierarchyOnServer(
+      const fake_server::SessionsHierarchy& hierarchy) {
+    SessionHierarchyMatchChecker checker(hierarchy, GetSyncService(0),
+                                         GetFakeServer());
+    EXPECT_TRUE(checker.Wait());
+  }
+
+  // Shortcut to call WaitForHierarchyOnServer for only |url| in a single
+  // window.
+  void WaitForURLOnServer(const GURL& url) {
+    WaitForHierarchyOnServer({{url.spec()}});
   }
 
  private:
@@ -93,7 +115,8 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, Sanity) {
   // Add a new session to client 0 and wait for it to sync.
   ScopedWindowMap old_windows;
   GURL url = GURL("http://127.0.0.1/bubba");
-  ASSERT_TRUE(OpenTabAndGetLocalWindows(0, url, &old_windows));
+  ASSERT_TRUE(OpenTab(0, url));
+  ASSERT_TRUE(GetLocalWindows(0, &old_windows));
   ASSERT_TRUE(UpdatedProgressMarkerChecker(GetSyncService(0)).Wait());
 
   // Get foreign session data from client 0.
@@ -106,18 +129,13 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, Sanity) {
   ASSERT_TRUE(GetLocalWindows(0, &new_windows));
   ASSERT_TRUE(WindowsMatch(old_windows, new_windows));
 
-  fake_server::FakeServerVerifier fake_server_verifier(GetFakeServer());
-  SessionsHierarchy expected_sessions;
-  expected_sessions.AddWindow(url.spec());
-  ASSERT_TRUE(fake_server_verifier.VerifySessions(expected_sessions));
+  WaitForURLOnServer(url);
 }
 
 IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, NoSessions) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
-  fake_server::FakeServerVerifier fake_server_verifier(GetFakeServer());
-  SessionsHierarchy expected_sessions;
-  ASSERT_TRUE(fake_server_verifier.VerifySessions(expected_sessions));
+  WaitForHierarchyOnServer(SessionsHierarchy());
 }
 
 IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, ChromeHistory) {
@@ -125,17 +143,8 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, ChromeHistory) {
 
   ASSERT_TRUE(CheckInitialState(0));
 
-  // Add a new session to client 0 and wait for it to sync.
-  ScopedWindowMap old_windows;
-  ASSERT_TRUE(OpenTabAndGetLocalWindows(0, GURL(chrome::kChromeUIHistoryURL),
-                                        &old_windows));
-  std::vector<GURL> urls;
-  urls.push_back(GURL(chrome::kChromeUIHistoryURL));
-  ASSERT_TRUE(WaitForTabsToLoad(0, urls));
-
-  // Verify the chrome history page synced.
-  ASSERT_TRUE(ModelAssociatorHasTabWithUrl(0,
-                                           GURL(chrome::kChromeUIHistoryURL)));
+  ASSERT_TRUE(OpenTab(0, GURL(chrome::kChromeUIHistoryURL)));
+  WaitForURLOnServer(GURL(chrome::kChromeUIHistoryURL));
 }
 
 IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, TimestampMatchesHistory) {
@@ -147,7 +156,8 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, TimestampMatchesHistory) {
   const GURL url("data:text/html,<html><title>Test</title></html>");
 
   ScopedWindowMap windows;
-  ASSERT_TRUE(OpenTabAndGetLocalWindows(0, url, &windows));
+  ASSERT_TRUE(OpenTab(0, url));
+  ASSERT_TRUE(GetLocalWindows(0, &windows));
 
   int found_navigations = 0;
   for (auto it = windows.begin(); it != windows.end(); ++it) {
@@ -178,7 +188,8 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, ResponseCodeIsPreserved) {
   const GURL url("data:text/html,<html><title>Test</title></html>");
 
   ScopedWindowMap windows;
-  ASSERT_TRUE(OpenTabAndGetLocalWindows(0, url, &windows));
+  ASSERT_TRUE(OpenTab(0, url));
+  ASSERT_TRUE(GetLocalWindows(0, &windows));
 
   int found_navigations = 0;
   for (auto it = windows.begin(); it != windows.end(); ++it) {
@@ -194,6 +205,133 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, ResponseCodeIsPreserved) {
   ASSERT_EQ(1, found_navigations);
 }
 
+IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, FragmentURLNavigation) {
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(CheckInitialState(0));
+
+  GURL url = embedded_test_server()->GetURL("/fragment.html");

[skym, 2017/03/02 19:56:11]

Per our discussion offline, lets use the data: URI thing.

[Patrick Noland, 2017/03/02 21:57:45]

Done.

+  ASSERT_TRUE(OpenTab(0, url));
+  WaitForURLOnServer(url);
+
+  embedded_test_server();

[skym, 2017/03/02 19:56:11]

Should this line be removed?

[Patrick Noland, 2017/03/02 21:57:44]

Done.

+  GURL fragment_url = embedded_test_server()->GetURL("/fragment.html#fragment");
+  NavigateTab(0, fragment_url);
+  WaitForURLOnServer(fragment_url);
+}
+
+IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest,
+                       NavigationChainForwardBack) {
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(CheckInitialState(0));
+
+  GURL first_url = GURL("http://127.0.0.1/foobar");
+  ASSERT_TRUE(OpenTab(0, first_url));

[skym, 2017/03/02 19:56:11]

Since this is apparently flaky, can we completely remove the usage of
non-resolving urls from sync integ tests? Also, lets comment on it somewhere,
maybe in the helper utils file.

[Patrick Noland, 2017/03/02 21:57:45]

Done.

+  WaitForURLOnServer(first_url);
+
+  GURL second_url = GURL("http://127.0.0.1/barbaz");
+  NavigateTab(0, second_url);
+  WaitForURLOnServer(second_url);
+
+  NavigateTabBack(0);
+  WaitForURLOnServer(first_url);
+
+  ExpectNavigationChain({first_url, second_url});
+
+  NavigateTabForward(0);
+  WaitForURLOnServer(second_url);
+
+  ExpectNavigationChain({first_url, second_url});
+}
+
+IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest,
+                       NavigationChainAlteredDestructively) {
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(CheckInitialState(0));
+
+  GURL base_url = GURL("http://127.0.0.1/bubba");
+  ASSERT_TRUE(OpenTab(0, base_url));
+  WaitForURLOnServer(base_url);
+
+  GURL first_url = GURL("http://127.0.0.1/foobar");
+  ASSERT_TRUE(NavigateTab(0, first_url));
+  WaitForURLOnServer(first_url);
+
+  // Check that the navigation chain matches the above sequence of {base_url,
+  // first_url}.
+  ExpectNavigationChain({base_url, first_url});
+
+  NavigateTabBack(0);
+  WaitForURLOnServer(base_url);
+
+  GURL second_url = GURL("http://127.0.0.1/barbaz");
+  NavigateTab(0, second_url);
+  WaitForURLOnServer(second_url);
+
+  NavigateTabBack(0);
+  WaitForURLOnServer(base_url);
+
+  // Check that the navigation chain contains second_url where first_url was
+  // before.
+  ExpectNavigationChain({base_url, second_url});
+}
+
+IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, OpenNewTab) {
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(CheckInitialState(0));
+
+  GURL base_url = GURL("http://127.0.0.1/bubba");
+  ASSERT_TRUE(OpenTabAtIndex(0, 0, base_url));
+
+  WaitForURLOnServer(base_url);
+
+  GURL new_tab_url = GURL("http://127.0.0.1/foobar");
+  ASSERT_TRUE(OpenTabAtIndex(0, 1, new_tab_url));
+
+  WaitForHierarchyOnServer(
+      SessionsHierarchy({{base_url.spec(), new_tab_url.spec()}}));
+}
+
+IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, OpenNewWindow) {
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(CheckInitialState(0));
+
+  GURL base_url = GURL("http://127.0.0.1/bubba");
+  ASSERT_TRUE(OpenTab(0, base_url));
+
+  WaitForURLOnServer(base_url);
+
+  GURL new_window_url = GURL("http://127.0.0.1/foobar");
+  AddBrowser(0);
+  ASSERT_TRUE(OpenTab(1, new_window_url));
+
+  WaitForHierarchyOnServer(
+      SessionsHierarchy({{base_url.spec()}, {new_window_url.spec()}}));
+}
+
+IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, TabMovedToOtherWindow) {
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(CheckInitialState(0));
+
+  GURL base_url = GURL("http://127.0.0.1/bubba");
+  GURL moved_tab_url = GURL("http://127.0.0.1/foobar");
+
+  ASSERT_TRUE(OpenTab(0, base_url));
+  ASSERT_TRUE(OpenTabAtIndex(0, 1, moved_tab_url));
+
+  GURL new_window_url = GURL("http://127.0.0.1/barbaz");
+  AddBrowser(0);
+  ASSERT_TRUE(OpenTab(1, new_window_url));
+
+  WaitForHierarchyOnServer(SessionsHierarchy(
+      {{base_url.spec(), moved_tab_url.spec()}, {new_window_url.spec()}}));
+
+  // Move tab 1 in browser 0 to browser 1.
+  MoveTab(0, 1, 1);
+
+  WaitForHierarchyOnServer(SessionsHierarchy(
+      {{base_url.spec()}, {new_window_url.spec(), moved_tab_url.spec()}}));
+}
+
 IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, CookieJarMismatch) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
@@ -208,7 +346,7 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, CookieJarMismatch) {
 
     // Add a new session to client 0 and wait for it to sync.
     GURL url = GURL("http://127.0.0.1/bubba");
-    ASSERT_TRUE(OpenTabAndGetLocalWindows(0, url, &old_windows));
+    ASSERT_TRUE(OpenTab(0, url));
     TriggerSyncForModelTypes(0, syncer::ModelTypeSet(syncer::SESSIONS));

[skym, 2017/03/02 19:56:11]

Why the change? Reading this code, it isn't clear what's being done here.

[Patrick Noland, 2017/03/02 21:57:45]

WaitForURLOnServer, which was used here previously, only waited until the
FakeServer saw a window with the given URL. For this test, we don't care about
that; we want to wait until a sync cycle completes, since that's the event that
triggers the histogram emission. 

[skym, 2017/03/02 22:59:43]

Discussed this offline, I would still prefer the WaitForURLOnServer approach.

[Patrick Noland, 2017/03/02 23:32:48]

Done.

     ASSERT_TRUE(UpdatedProgressMarkerChecker(GetSyncService(0)).Wait());
 
@@ -219,7 +357,7 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, CookieJarMismatch) {
     ASSERT_TRUE(message.commit().config_params().cookie_jar_mismatch());
 
     // It is possible that multiple sync cycles occured during the call to
-    // OpenTabAndGetLocalWindows, which would cause multiple identical samples.
+    // OpenTab, which would cause multiple identical samples.
     ExpectUniqueSampleGE(histogram_tester, "Sync.CookieJarMatchOnNavigation",
                          false, 1);
     ExpectUniqueSampleGE(histogram_tester, "Sync.CookieJarEmptyOnMismatch",
@@ -242,7 +380,8 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, CookieJarMismatch) {
 
     // Trigger a sync and wait for it.
     GURL url = GURL("http://127.0.0.1/bubba2");
-    ASSERT_TRUE(OpenTabAndGetLocalWindows(0, url, &old_windows));
+    ASSERT_TRUE(OpenTab(0, url));
+    ASSERT_TRUE(GetLocalWindows(0, &old_windows));
     TriggerSyncForModelTypes(0, syncer::ModelTypeSet(syncer::SESSIONS));
     ASSERT_TRUE(UpdatedProgressMarkerChecker(GetSyncService(0)).Wait());