Skip to content

GitLab

T
tangled
Commit ed1c99d3 authored by Mikel Astiz's avatar Mikel Astiz Committed by Commit Bot
Browse files
Options

Add sync integration tests related to history sync 

FakeServer/LoopbackServer is extended with logic that mimics what real
servers do, in order to verify what contributes to synced history.

This functionality is leveraged to add proper integration test coverage
for various relevant scenarios to history sync, starting with the
simplest, to more advanced including closed tabs.

This approach is expected to fix some flakes we've observed in recent
attempts to land tests, because it doesn't suffer from server-side
session entities potentially being overwritten by newly open tabs.

Bug: 882489,910947
Change-Id: I4a69fa080e3deacc8d33c64de17f964c34436ea3
Reviewed-on: https://chromium-review.googlesource.com/c/1356811
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: default avatarMarc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613127}
parent 74edc055
Hide whitespace changes
Inline Side-by-side
Showing with 140 additions and 18 deletions
chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
View file @ ed1c99d3
......@@ -28,6 +28,7 @@
#include "components/sync_sessions/session_sync_service.h"
#include "components/sync_sessions/session_sync_test_helper.h"
#include "components/sync_sessions/synced_session_tracker.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/base/mojo/window_open_disposition.mojom.h"
......@@ -54,6 +55,8 @@ using sessions_helper::SyncedSessionVector;
using sessions_helper::WaitForTabsToLoad;
using sessions_helper::WindowsMatch;
using sync_sessions::SessionSyncTestHelper;
using testing::IsEmpty;
using testing::UnorderedElementsAre;
using typed_urls_helper::GetUrlFromClient;
static const char* kURL1 = "data:text/html,<html><title>Test</title></html>";
......@@ -76,28 +79,18 @@ void ExpectUniqueSampleGE(const HistogramTester& histogram_tester,
EXPECT_EQ(sample_count, samples->TotalCount());
}
class IsUrlSyncedChecker : public SingleClientStatusChangeChecker {
class IsHistoryURLSyncedChecker : public SingleClientStatusChangeChecker {
public:
IsUrlSyncedChecker(const std::string& url,
fake_server::FakeServer* fake_server,
browser_sync::ProfileSyncService* service)
IsHistoryURLSyncedChecker(const std::string& url,
fake_server::FakeServer* fake_server,
browser_sync::ProfileSyncService* service)
: SingleClientStatusChangeChecker(service),
url_(url),
fake_server_(fake_server) {}
// StatusChangeChecker implementation.
bool IsExitConditionSatisfied() override {
std::vector<sync_pb::SyncEntity> entities =
fake_server_->GetSyncEntitiesByModelType(syncer::SESSIONS);
for (const sync_pb::SyncEntity& entity : entities) {
for (const auto& navigation :
entity.specifics().session().tab().navigation()) {
if (navigation.virtual_url() == url_) {
return true;
}
}
}
return false;
return fake_server_->GetCommittedHistoryURLs().count(url_) != 0;
}
std::string GetDebugMessage() const override {
......@@ -204,6 +197,41 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, Sanity) {
ASSERT_TRUE(WindowsMatch(old_windows, new_windows));
WaitForURLOnServer(url);
EXPECT_THAT(GetFakeServer()->GetCommittedHistoryURLs(),
UnorderedElementsAre(kURL1));
}
IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, NavigateInTab) {
ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
ASSERT_TRUE(CheckInitialState(0));
ASSERT_TRUE(OpenTab(0, GURL(kURL1)));
WaitForHierarchyOnServer(SessionsHierarchy({{kURL1}}));
NavigateTab(0, GURL(kURL2));
WaitForHierarchyOnServer(SessionsHierarchy({{kURL2}}));
EXPECT_THAT(GetFakeServer()->GetCommittedHistoryURLs(),
UnorderedElementsAre(kURL1, kURL2));
}
IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest,
SessionsWithoutHistorySync) {
ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
// If the user disables history sync on settings, but still enables tab sync,
// then sessions should be synced but the server should be able to tell the
// difference based on active datatypes.
ASSERT_TRUE(GetClient(0)->DisableSyncForDatatype(syncer::TYPED_URLS));
ASSERT_TRUE(CheckInitialState(0));
ASSERT_TRUE(OpenTab(0, GURL(kURL1)));
WaitForHierarchyOnServer(SessionsHierarchy({{kURL1}}));
NavigateTab(0, GURL(kURL2));
WaitForHierarchyOnServer(SessionsHierarchy({{kURL2}}));
EXPECT_THAT(GetFakeServer()->GetCommittedHistoryURLs(), IsEmpty());
}
IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, NoSessions) {
......@@ -212,7 +240,7 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, NoSessions) {
WaitForHierarchyOnServer(SessionsHierarchy());
}
IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, ChromeHistory) {
IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, ChromeHistoryPage) {
ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
ASSERT_TRUE(CheckInitialState(0));
......@@ -238,12 +266,46 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, NavigateThenCloseTab) {
NavigateTab(0, GURL(kURL4));
ASSERT_TRUE(
IsUrlSyncedChecker(kURL4, GetFakeServer(), GetSyncService(0)).Wait());
IsHistoryURLSyncedChecker(kURL4, GetFakeServer(), GetSyncService(0))
.Wait());
// All URLs should be synced, for synced history to be complete. In
// particular, |kURL3| should be synced despite the tab being closed.
EXPECT_TRUE(
IsHistoryURLSyncedChecker(kURL3, GetFakeServer(), GetSyncService(0))
.Wait());
}
IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest,
NavigateThenCloseTabThenOpenTab) {
base::test::ScopedFeatureList override_features;
override_features.InitAndEnableFeature(
sync_sessions::kDeferRecyclingOfSyncTabNodesIfUnsynced);
ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
ASSERT_TRUE(CheckInitialState(0));
// Two tabs are opened initially.
ASSERT_TRUE(OpenTab(0, GURL(kURL1)));
ASSERT_TRUE(OpenTab(0, GURL(kURL2)));
WaitForHierarchyOnServer(SessionsHierarchy({{kURL1, kURL2}}));
// Close one of the two tabs immediately after issuing an navigation. In
// addition, a new tab is opened.
NavigateTab(0, GURL(kURL3));
ASSERT_TRUE(WaitForTabsToLoad(0, {GURL(kURL1), GURL(kURL3)}));
CloseTab(/*index=*/0, /*tab_index=*/1);
ASSERT_TRUE(OpenTab(0, GURL(kURL4)));
ASSERT_TRUE(
IsHistoryURLSyncedChecker(kURL4, GetFakeServer(), GetSyncService(0))
.Wait());
// All URLs should be synced, for synced history to be complete. In
// particular, |kURL3| should be synced despite the tab being closed.
EXPECT_TRUE(
IsUrlSyncedChecker(kURL3, GetFakeServer(), GetSyncService(0)).Wait());
IsHistoryURLSyncedChecker(kURL3, GetFakeServer(), GetSyncService(0))
.Wait());
}
IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, TimestampMatchesHistory) {
......
chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc
View file @ ed1c99d3
......@@ -14,6 +14,7 @@
#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"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace {
......@@ -30,6 +31,7 @@ using sessions_helper::ScopedWindowMap;
using sessions_helper::SessionWindowMap;
using sessions_helper::SyncedSessionVector;
using sessions_helper::WindowsMatch;
using testing::IsEmpty;
class TwoClientSessionsSyncTest : public SyncTest {
public:
......@@ -241,4 +243,23 @@ IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest, MultipleWindowsMultipleTabs) {
WaitForForeignSessionsToSync(0, 1);
}
IN_PROC_BROWSER_TEST_F(TwoClientSessionsSyncTest,
NoHistoryIfEncryptionEnabled) {
ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
ASSERT_TRUE(CheckInitialState(0));
ASSERT_TRUE(CheckInitialState(1));
ASSERT_TRUE(EnableEncryption(0));
ASSERT_TRUE(EnableEncryption(1));
ASSERT_TRUE(AwaitQuiescence());
ASSERT_TRUE(IsEncryptionComplete(0));
ASSERT_TRUE(IsEncryptionComplete(1));
EXPECT_TRUE(OpenTab(0, GURL(kURL1)));
WaitForForeignSessionsToSync(0, 1);
EXPECT_THAT(GetFakeServer()->GetCommittedHistoryURLs(), IsEmpty());
}
} // namespace
components/sync/engine_impl/loopback_server/loopback_server.cc
View file @ ed1c99d3
......@@ -474,6 +474,11 @@ bool LoopbackServer::HandleCommitRequest(
string guid = commit.cache_guid();
ModelTypeSet committed_model_types;
ModelTypeSet enabled_types;
for (int field_number : commit.config_params().enabled_type_ids()) {
enabled_types.Put(GetModelTypeFromSpecificsFieldNumber(field_number));
}
// TODO(pvalenzuela): Add validation of CommitMessage.entries.
for (const sync_pb::SyncEntity& client_entity : commit.entries()) {
sync_pb::CommitResponse_EntryResponse* entry_response =
......@@ -498,6 +503,20 @@ bool LoopbackServer::HandleCommitRequest(
EntityMap::const_iterator iter = entities_.find(entity_id);
DCHECK(iter != entities_.end());
committed_model_types.Put(iter->second->GetModelType());
// Notify observers about history having been synced. "History" sync is
// guarded by the user's selection in the settings page. This also excludes
// custom passphrase users who, in addition to HISTORY_DELETE_DIRECTIVES not
// being enabled, will commit encrypted specifics and hence cannot be
// iterated over.
if (observer_for_tests_ && iter->second->GetModelType() == SESSIONS &&
enabled_types.Has(HISTORY_DELETE_DIRECTIVES) &&
enabled_types.Has(TYPED_URLS)) {
for (const sync_pb::TabNavigation& navigation :
client_entity.specifics().session().tab().navigation()) {
observer_for_tests_->OnHistoryCommit(navigation.virtual_url());
}
}
}
if (observer_for_tests_)
......
components/sync/engine_impl/loopback_server/loopback_server.h
View file @ ed1c99d3
......@@ -39,6 +39,11 @@ class LoopbackServer {
// updated as part of the commit are passed in |committed_model_types|.
virtual void OnCommit(const std::string& committer_id,
syncer::ModelTypeSet committed_model_types) = 0;
// Called when a page URL is committed via SESSIONS and the user has
// enabled "history sync" in the settings UI (which is detected by verifying
// if TYPED_URLS is an enabled type, as part of the commit request).
virtual void OnHistoryCommit(const std::string& url) = 0;
};
explicit LoopbackServer(const base::FilePath& persistent_file);
......
components/sync/test/fake_server/fake_server.cc
View file @ ed1c99d3
......@@ -426,6 +426,10 @@ void FakeServer::OnCommit(const std::string& committer_id,
observer.OnCommit(committer_id, committed_model_types);
}
void FakeServer::OnHistoryCommit(const std::string& url) {
committed_history_urls_.insert(url);
}
void FakeServer::EnableStrongConsistencyWithConflictDetectionModel() {
DCHECK(thread_checker_.CalledOnValidThread());
loopback_server_->EnableStrongConsistencyWithConflictDetectionModel();
......@@ -436,6 +440,10 @@ void FakeServer::SetMaxGetUpdatesBatchSize(int batch_size) {
loopback_server_->SetMaxGetUpdatesBatchSize(batch_size);
}
const std::set<std::string>& FakeServer::GetCommittedHistoryURLs() const {
return committed_history_urls_;
}
base::WeakPtr<FakeServer> FakeServer::AsWeakPtr() {
DCHECK(thread_checker_.CalledOnValidThread());
return weak_ptr_factory_.GetWeakPtr();
......
components/sync/test/fake_server/fake_server.h
View file @ ed1c99d3
......@@ -9,6 +9,7 @@
#include <map>
#include <memory>
#include <set>
#include <string>
#include <vector>
......@@ -172,6 +173,9 @@ class FakeServer : public syncer::LoopbackServer::ObserverForTests {
// Implement LoopbackServer::ObserverForTests:
void OnCommit(const std::string& committer_id,
syncer::ModelTypeSet committed_model_types) override;
void OnHistoryCommit(const std::string& url) override;
const std::set<std::string>& GetCommittedHistoryURLs() const;
// Returns the current FakeServer as a WeakPtr.
base::WeakPtr<FakeServer> AsWeakPtr();
......@@ -202,6 +206,9 @@ class FakeServer : public syncer::LoopbackServer::ObserverForTests {
// All Keystore keys known to the server.
std::vector<std::string> keystore_keys_;
// All URLs received via history sync (powered by SESSIONS).
std::set<std::string> committed_history_urls_;
// Used as the error_code field of ClientToServerResponse on all responses
// except when |triggered_actionable_error_| is set.
sync_pb::SyncEnums::ErrorType error_type_;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment