Make secondary account sync tests share a single TestURLLoaderFactory.
Before this change, these tests had multiple TestURLLoaderFactory
instances. One, which is declared in SyncTest, plus an additional
instance each time BuildFakeGaiaCookieManagerServiceWithOptions was
invoked.
Eventually, FakeGCMS will be eliminated entirely in these tests. This
change separates out one of the logical side effects of eliminating
FakeGCMS, which that there will be a single TestURLLoaderFactory
instance.
We are splitting out this change to be able to verify whether the switch
to a single shared factory was the cause of the flake in
https://chromium-review.googlesource.com/c/chromium/src/+/1396148. Our
suspicion is that the flake was rather caused by the fact that in that
CL, the TestURLLoaderFactory was *not* used by the main profile, whereas
prior to that CL FakeGCMS instances had been installed for all Profiles.
Assuming that this change sticks, we will follow it up by hiding the
usage of |test_url_loader_factory_| inside a new SecondarySyncTest
class, thus eliminating the passing of the raw pointer.
This is part of step 1a of the plan to eliminate FakeGCMS entirely:
TBR=rkaplow@chromium.org
https: //docs.google.com/document/d/1t0ZtuV7h-znzdItFgBW0aKPscAwWXIBuNZnNlEGgi7g/edit
Change-Id: I86837090af84e94bdd9887ac9ee5e9c96bbef8da
Bug: 907782
Reviewed-on: https://chromium-review.googlesource.com/c/1411877
Commit-Queue: Lowell Manners <lowell@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623188}diff --git a/chrome/browser/metrics/ukm_browsertest.cc b/chrome/browser/metrics/ukm_browsertest.cc
index 1648a6dee..6f81883 100644
--- a/chrome/browser/metrics/ukm_browsertest.cc
+++ b/chrome/browser/metrics/ukm_browsertest.cc
@@ -341,7 +341,8 @@
// cookies) in tests. Without this, the real GaiaCookieManagerService would
// try talking to Google servers which of course wouldn't work in tests.
fake_gaia_cookie_manager_factory_ =
- secondary_account_helper::SetUpFakeGaiaCookieManagerService();
+ secondary_account_helper::SetUpFakeGaiaCookieManagerService(
+ &test_url_loader_factory_);
UkmBrowserTest::SetUpInProcessBrowserTestFixture();
}
diff --git a/chrome/browser/sync/test/integration/secondary_account_helper.cc b/chrome/browser/sync/test/integration/secondary_account_helper.cc
index dc50be6..f1e8782 100644
--- a/chrome/browser/sync/test/integration/secondary_account_helper.cc
+++ b/chrome/browser/sync/test/integration/secondary_account_helper.cc
@@ -27,20 +27,23 @@
namespace {
-void OnWillCreateBrowserContextServices(content::BrowserContext* context) {
+void OnWillCreateBrowserContextServices(
+ network::TestURLLoaderFactory* test_url_loader_factory,
+ content::BrowserContext* context) {
GaiaCookieManagerServiceFactory::GetInstance()->SetTestingFactory(
context,
- base::BindRepeating(
- &BuildFakeGaiaCookieManagerServiceWithOptions,
- /*create_fake_url_loader_factory_for_cookie_requests=*/true));
+ base::BindRepeating(&BuildFakeGaiaCookieManagerServiceWithURLLoader,
+ test_url_loader_factory));
}
} // namespace
-ScopedFakeGaiaCookieManagerServiceFactory SetUpFakeGaiaCookieManagerService() {
+ScopedFakeGaiaCookieManagerServiceFactory SetUpFakeGaiaCookieManagerService(
+ network::TestURLLoaderFactory* test_url_loader_factory) {
return BrowserContextDependencyManager::GetInstance()
->RegisterWillCreateBrowserContextServicesCallbackForTesting(
- base::BindRepeating(&OnWillCreateBrowserContextServices));
+ base::BindRepeating(&OnWillCreateBrowserContextServices,
+ test_url_loader_factory));
}
#if defined(OS_CHROMEOS)
diff --git a/chrome/browser/sync/test/integration/secondary_account_helper.h b/chrome/browser/sync/test/integration/secondary_account_helper.h
index b0bdc98..ad21a70 100644
--- a/chrome/browser/sync/test/integration/secondary_account_helper.h
+++ b/chrome/browser/sync/test/integration/secondary_account_helper.h
@@ -17,6 +17,10 @@
class BrowserContext;
} // namespace content
+namespace network {
+class TestURLLoaderFactory;
+}
+
namespace secondary_account_helper {
using ScopedFakeGaiaCookieManagerServiceFactory = std::unique_ptr<
@@ -26,7 +30,8 @@
// called from SetUpInProcessBrowserTestFixture. The caller should hold on to
// the returned object for the duration of the test, e.g. store it in a member
// of the test fixture class.
-ScopedFakeGaiaCookieManagerServiceFactory SetUpFakeGaiaCookieManagerService();
+ScopedFakeGaiaCookieManagerServiceFactory SetUpFakeGaiaCookieManagerService(
+ network::TestURLLoaderFactory* test_url_loader_factory);
#if defined(OS_CHROMEOS)
// Sets up necessary fakes for fake network responses to work. Meant to be
diff --git a/chrome/browser/sync/test/integration/single_client_secondary_account_sync_test.cc b/chrome/browser/sync/test/integration/single_client_secondary_account_sync_test.cc
index 90a8a3a..0ed3a9f 100644
--- a/chrome/browser/sync/test/integration/single_client_secondary_account_sync_test.cc
+++ b/chrome/browser/sync/test/integration/single_client_secondary_account_sync_test.cc
@@ -39,7 +39,8 @@
void SetUpInProcessBrowserTestFixture() override {
fake_gaia_cookie_manager_factory_ =
- secondary_account_helper::SetUpFakeGaiaCookieManagerService();
+ secondary_account_helper::SetUpFakeGaiaCookieManagerService(
+ &test_url_loader_factory_);
}
void SetUpOnMainThread() override {
diff --git a/chrome/browser/sync/test/integration/single_client_wallet_sync_test.cc b/chrome/browser/sync/test/integration/single_client_wallet_sync_test.cc
index 4b920ec..5bbb1a0 100644
--- a/chrome/browser/sync/test/integration/single_client_wallet_sync_test.cc
+++ b/chrome/browser/sync/test/integration/single_client_wallet_sync_test.cc
@@ -1079,7 +1079,8 @@
void SetUpInProcessBrowserTestFixture() override {
fake_gaia_cookie_manager_factory_ =
- secondary_account_helper::SetUpFakeGaiaCookieManagerService();
+ secondary_account_helper::SetUpFakeGaiaCookieManagerService(
+ &test_url_loader_factory_);
}
void SetUpOnMainThread() override {
diff --git a/chrome/browser/sync/test/integration/sync_test.h b/chrome/browser/sync/test/integration/sync_test.h
index eb4aa17..266371fe 100644
--- a/chrome/browser/sync/test/integration/sync_test.h
+++ b/chrome/browser/sync/test/integration/sync_test.h
@@ -43,7 +43,7 @@
// To disable a test from running on Chromium waterfalls, you would still use
// the default DISABLED_test_name macro. To disable it from running as an E2E
// test outside Chromium waterfalls you would need to remove the E2E* macro.
-#define MACRO_CONCAT(prefix, test_name) prefix ## _ ## test_name
+#define MACRO_CONCAT(prefix, test_name) prefix##_##test_name
#define E2E_ONLY(test_name) MACRO_CONCAT(DISABLED_E2ETest, test_name)
#define E2E_ENABLED(test_name) MACRO_CONCAT(test_name, E2ETest)
@@ -328,6 +328,9 @@
// The FakeServer used in tests with server type IN_PROCESS_FAKE_SERVER.
std::unique_ptr<fake_server::FakeServer> fake_server_;
+ // The factory used to mock out GAIA signin.
+ network::TestURLLoaderFactory test_url_loader_factory_;
+
protected:
virtual void BeforeSetupClient(int index);
@@ -501,9 +504,6 @@
// Used to start and stop the local test server.
base::Process test_server_;
- // The factory used to mock out GAIA signin.
- network::TestURLLoaderFactory test_url_loader_factory_;
-
// The shared URLLoaderFactory backed by |test_url_loader_factory_|.
scoped_refptr<network::WeakWrapperSharedURLLoaderFactory>
test_shared_url_loader_factory_;
