[sync] Add Sessions integration tests

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

Index: chrome/browser/sync/test/integration/sessions_helper.cc
diff --git a/chrome/browser/sync/test/integration/sessions_helper.cc b/chrome/browser/sync/test/integration/sessions_helper.cc
index 98e0936e214d9914a5802d3d8427b0ce63d44bab..cc0abe316587e2a22f6e9be866db594bc5d25ddf 100644
--- a/chrome/browser/sync/test/integration/sessions_helper.cc
+++ b/chrome/browser/sync/test/integration/sessions_helper.cc
@@ -23,11 +23,19 @@
 #include "chrome/browser/sync/sessions/notification_service_sessions_router.h"
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
 #include "chrome/browser/sync/test/integration/sync_datatype_helper.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_tabstrip.h"
 #include "chrome/browser/ui/singleton_tabs.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/test/base/ui_test_utils.h"
 #include "components/browser_sync/profile_sync_service.h"
 #include "components/sync/driver/sync_client.h"
+#include "components/sync/test/fake_server/fake_server.h"
+#include "components/sync/test/fake_server/sessions_hierarchy.h"
 #include "components/sync_sessions/open_tabs_ui_delegate.h"
+#include "content/public/browser/navigation_entry.h"
+#include "content/public/browser/web_contents.h"
 #include "content/public/test/test_utils.h"
 #include "url/gurl.h"
 
@@ -35,10 +43,12 @@ using sync_datatype_helper::test;
 
 namespace sessions_helper {
 
-bool GetLocalSession(int index, const sync_sessions::SyncedSession** session) {
-  return ProfileSyncServiceFactory::GetInstance()->GetForProfile(
-      test()->GetProfile(index))->GetOpenTabsUIDelegate()->
-          GetLocalSession(session);
+bool GetLocalSession(int profile_index,
+                     const sync_sessions::SyncedSession** session) {
+  return ProfileSyncServiceFactory::GetInstance()
+      ->GetForProfile(test()->GetProfile(profile_index))
+      ->GetOpenTabsUIDelegate()
+      ->GetLocalSession(session);
 }
 
 bool ModelAssociatorHasTabWithUrl(int index, const GURL& url) {
@@ -84,22 +94,60 @@ bool ModelAssociatorHasTabWithUrl(int index, const GURL& url) {
   return false;
 }
 
-bool OpenTab(int index, const GURL& url) {
-  DVLOG(1) << "Opening tab: " << url.spec() << " using profile "
-           << index << ".";
-  chrome::ShowSingletonTab(test()->GetBrowser(index), url);
-  return WaitForTabsToLoad(index, std::vector<GURL>(1, url));
+bool OpenTab(int browser_index, const GURL& url) {
+  DVLOG(1) << "Opening tab: " << url.spec() << " using browser "
+           << browser_index << ".";
+  return OpenTabAtIndex(browser_index, 0, url);
+}
+
+bool OpenTabAtIndex(int browser_index, int tab_index, const GURL& url) {
+  chrome::AddTabAt(test()->GetBrowser(browser_index), url, tab_index, true);
+  return WaitForTabsToLoad(browser_index, {url});
 }
 
-bool OpenMultipleTabs(int index, const std::vector<GURL>& urls) {
-  Browser* browser = test()->GetBrowser(index);
+bool OpenMultipleTabs(int browser_index, const std::vector<GURL>& urls) {
+  Browser* browser = test()->GetBrowser(browser_index);
   for (std::vector<GURL>::const_iterator it = urls.begin();
        it != urls.end(); ++it) {
-    DVLOG(1) << "Opening tab: " << it->spec() << " using profile " << index
-             << ".";
+    DVLOG(1) << "Opening tab: " << it->spec() << " using browser "
+             << browser_index << ".";
     chrome::ShowSingletonTab(browser, *it);
   }
-  return WaitForTabsToLoad(index, urls);
+  return WaitForTabsToLoad(browser_index, urls);
+}
+
+void MoveTab(int from_browser_index, int to_browser_index, int tab_index) {
+  content::WebContents* detached_contents =
+      test()
+          ->GetBrowser(from_browser_index)
+          ->tab_strip_model()
+          ->DetachWebContentsAt(tab_index);
+
+  TabStripModel* target_strip =
+      test()->GetBrowser(to_browser_index)->tab_strip_model();
+  target_strip->InsertWebContentsAt(target_strip->count(), detached_contents,
+                                    TabStripModel::ADD_ACTIVE);
+}
+
+bool NavigateTab(int browser_index, const GURL& url) {
+  chrome::NavigateParams params(test()->GetBrowser(browser_index), url,
+                                ui::PAGE_TRANSITION_LINK);
+  params.disposition = WindowOpenDisposition::CURRENT_TAB;
+
+  ui_test_utils::NavigateToURL(&params);
+  return WaitForTabsToLoad(browser_index, {url});
+}
+
+void NavigateTabBack(int browser_index) {
+  content::WebContents* tab_contents =
+      test()->GetBrowser(browser_index)->tab_strip_model()->GetWebContentsAt(0);
+  tab_contents->GetController().GoBack();

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

->GetController().GoBack();
vs
content::WebContents* tab_contents = 

By assigning a local variable, you're making the line longer.

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

Done.

+}
+
+void NavigateTabForward(int browser_index) {
+  content::WebContents* tab_contents =
+      test()->GetBrowser(browser_index)->tab_strip_model()->GetWebContentsAt(0);
+  tab_contents->GetController().GoForward();
 }
 
 namespace {
@@ -137,7 +185,7 @@ class TabEventHandler : public sync_sessions::LocalSessionEventHandler {
 
 }  // namespace
 
-bool WaitForTabsToLoad(int index, const std::vector<GURL>& urls) {
+bool WaitForTabsToLoad(int profile_index, const std::vector<GURL>& urls) {
   DVLOG(1) << "Waiting for session to propagate to associator.";
   base::TimeTicks start_time = base::TimeTicks::Now();
   base::TimeTicks end_time = start_time + TestTimeouts::action_max_timeout();
@@ -146,7 +194,7 @@ bool WaitForTabsToLoad(int index, const std::vector<GURL>& urls) {
        it != urls.end(); ++it) {
     found = false;
     while (!found) {
-      found = ModelAssociatorHasTabWithUrl(index, *it);
+      found = ModelAssociatorHasTabWithUrl(profile_index, *it);
       if (base::TimeTicks::Now() >= end_time) {
         LOG(ERROR) << "Failed to find all tabs after "
                    << TestTimeouts::action_max_timeout().InSecondsF()
@@ -156,9 +204,9 @@ bool WaitForTabsToLoad(int index, const std::vector<GURL>& urls) {
       if (!found) {
         TabEventHandler handler;
         sync_sessions::NotificationServiceSessionsRouter router(
-            test()->GetProfile(index),
+            test()->GetProfile(profile_index),
             ProfileSyncServiceFactory::GetInstance()
-                ->GetForProfile(test()->GetProfile(index))
+                ->GetForProfile(test()->GetProfile(profile_index))
                 ->GetSyncClient()
                 ->GetSyncSessionsClient(),
             syncer::SyncableService::StartSyncFlare());
@@ -170,11 +218,11 @@ bool WaitForTabsToLoad(int index, const std::vector<GURL>& urls) {
   return true;
 }
 
-bool GetLocalWindows(int index, ScopedWindowMap* local_windows) {
+bool GetLocalWindows(int profile_index, ScopedWindowMap* local_windows) {
   // The local session provided by GetLocalSession is owned, and has lifetime
   // controlled, by the model associator, so we must make our own copy.
   const sync_sessions::SyncedSession* local_session;
-  if (!GetLocalSession(index, &local_session)) {
+  if (!GetLocalSession(profile_index, &local_session)) {
     return false;
   }
   for (auto w = local_session->windows.begin();
@@ -199,47 +247,47 @@ bool GetLocalWindows(int index, ScopedWindowMap* local_windows) {
   return true;
 }
 
-bool OpenTabAndGetLocalWindows(int index,
+bool OpenTabAndGetLocalWindows(int profile_index,
                                const GURL& url,
                                ScopedWindowMap* local_windows) {
-  if (!OpenTab(index, url)) {
+  if (!OpenTab(profile_index, url)) {

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

So you're passing a profile_index in as a browser_index here. If you don't want
them to be interchangeable, maybe some of these functions need to take both in
as arguments.

Or is one way conversions okay, you can go from profile_index -> browser_index,
but not necessarily the other way around? If this is the case, you need to
document it.

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

This is kind of ugly. SessionsHelper deeply assumes the exact correspondence
between browser index and profile index. That's why every function used to just
take a generic "index" parameter. Adding multiple browsers per profile creates
the potential to violate this assumption.

To mitigate this, when you add a new browser with AddBrowser, it copies the
corresponding profile pointer into the same position as the new browser. So if
you create a new browser using profile 0, there will be two distinct browsers:
one in spot 0 and spot 1. There will be two profile pointers pointing to a
single distinct profile, one in spot 0 and one in spot 1. So specifying 1 as a
profile index will work fine. This is confusing, since there's only one distinct
profile. 

I've tried to insulate callers from these confusing details a little bit by
having some functions take in a profile index and others take in a browser
index. Functions that take in a profile index operate on whichever browser
happens to live in the corresponding spot. Functions that take in a browser
index operate on the specified browser. In practice this is fine since the only
tests that create multiple browsers per profile operate using browser indexes. 

I will think about how to make all this more clear to future users of these
classes.

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

When you have some functions take a profile index and others take a browser
index, you're doing the exact opposite of insulating callers from this
complexity. You're making it their problem, and forcing them to understand this.

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

Discussed offline, I went back to just naming everything index.

     return false;
   }
-  return GetLocalWindows(index, local_windows);
+  return GetLocalWindows(profile_index, local_windows);
 }
 
-bool CheckInitialState(int index) {
-  if (0 != GetNumWindows(index))
+bool CheckInitialState(int profile_index) {
+  if (0 != GetNumWindows(profile_index))
     return false;
-  if (0 != GetNumForeignSessions(index))
+  if (0 != GetNumForeignSessions(profile_index))
     return false;
   return true;
 }
 
-int GetNumWindows(int index) {
+int GetNumWindows(int profile_index) {
   const sync_sessions::SyncedSession* local_session;
-  if (!GetLocalSession(index, &local_session)) {
+  if (!GetLocalSession(profile_index, &local_session)) {
     return 0;
   }
   return local_session->windows.size();
 }
 
-int GetNumForeignSessions(int index) {
+int GetNumForeignSessions(int profile_index) {
   SyncedSessionVector sessions;
-  if (!ProfileSyncServiceFactory::GetInstance()->GetForProfile(
-          test()->GetProfile(index))->
-          GetOpenTabsUIDelegate()->GetAllForeignSessions(
-              &sessions)) {
+  if (!ProfileSyncServiceFactory::GetInstance()
+           ->GetForProfile(test()->GetProfile(profile_index))
+           ->GetOpenTabsUIDelegate()
+           ->GetAllForeignSessions(&sessions)) {
     return 0;
   }
   return sessions.size();
 }
 
-bool GetSessionData(int index, SyncedSessionVector* sessions) {
-  if (!ProfileSyncServiceFactory::GetInstance()->GetForProfile(
-          test()->GetProfile(index))->
-          GetOpenTabsUIDelegate()->GetAllForeignSessions(
-              sessions)) {
+bool GetSessionData(int profile_index, SyncedSessionVector* sessions) {
+  if (!ProfileSyncServiceFactory::GetInstance()
+           ->GetForProfile(test()->GetProfile(profile_index))
+           ->GetOpenTabsUIDelegate()
+           ->GetAllForeignSessions(sessions)) {
     return false;
   }
   SortSyncedSessions(sessions);
@@ -345,20 +393,20 @@ bool WindowsMatch(const SessionWindowMap& win1, const ScopedWindowMap& win2) {
   return WindowsMatchImpl(win1, win2);
 }
 
-bool CheckForeignSessionsAgainst(
-    int index,
-    const std::vector<ScopedWindowMap>& windows) {
+bool CheckForeignSessionsAgainst(int profile_index,
+                                 const std::vector<ScopedWindowMap>& windows) {
   SyncedSessionVector sessions;
 
-  if (!GetSessionData(index, &sessions)) {
+  if (!GetSessionData(profile_index, &sessions)) {
     LOG(ERROR) << "Cannot get session data";
     return false;
   }
 
   for (size_t w_index = 0; w_index < windows.size(); ++w_index) {
     // Skip the client's local window
-    if (static_cast<int>(w_index) == index)
+    if (static_cast<int>(w_index) == profile_index) {
       continue;
+    }
 
     size_t s_index = 0;
 
@@ -376,9 +424,9 @@ bool CheckForeignSessionsAgainst(
   return true;
 }
 
-void DeleteForeignSession(int index, std::string session_tag) {
+void DeleteForeignSession(int profile_index, std::string session_tag) {
   ProfileSyncServiceFactory::GetInstance()
-      ->GetForProfile(test()->GetProfile(index))
+      ->GetForProfile(test()->GetProfile(profile_index))
       ->GetOpenTabsUIDelegate()
       ->DeleteForeignSession(session_tag);
 }
@@ -386,17 +434,34 @@ void DeleteForeignSession(int index, std::string session_tag) {
 }  // namespace sessions_helper
 
 ForeignSessionsMatchChecker::ForeignSessionsMatchChecker(
-    int index,
+    int profile_index,
     const std::vector<sessions_helper::ScopedWindowMap>& windows)
     : MultiClientStatusChangeChecker(
           sync_datatype_helper::test()->GetSyncServices()),
-      index_(index),
+      profile_index_(profile_index),
       windows_(windows) {}
 
 bool ForeignSessionsMatchChecker::IsExitConditionSatisfied() {
-  return sessions_helper::CheckForeignSessionsAgainst(index_, windows_);
+  return sessions_helper::CheckForeignSessionsAgainst(profile_index_, windows_);
 }
 
 std::string ForeignSessionsMatchChecker::GetDebugMessage() const {
   return "Waiting for matching foreign sessions";
 }
+
+SessionHierarchyMatchChecker::SessionHierarchyMatchChecker(
+    browser_sync::ProfileSyncService* service,
+    const fake_server::SessionsHierarchy& sessions_hierarchy,
+    fake_server::FakeServer* fake_server)
+    : SingleClientStatusChangeChecker(service),
+      sessions_hierarchy_(sessions_hierarchy),
+      verifier_(fake_server) {}
+
+bool SessionHierarchyMatchChecker::IsExitConditionSatisfied() {
+  return verifier_.VerifySessions(sessions_hierarchy_);
+}
+
+std::string SessionHierarchyMatchChecker::GetDebugMessage() const {
+  return "Waiting for matching sessions hierarchy to be reflected in fake "
+         "server";
+}