Reland 2: sync_integration_tests: don't instantiate ScopedFeatureList in test body
This is a reland of https://crrev.com/c/1402799 and
https://crrev.com/c/1402890, which were reverted for supposedly
introducing flakiness, but in fact in only renamed already-flaky tests.
Hopefully all such tests are now disabled, so trying again.
Creating a ScopedFeatureList instance in the test body creates a data
race related to the global FeatureList instance: The ScopedFeatureList
overrides the global instance, and then it gets reset after the test
body; however, the feature list might still be queried from the Sync
thread after that.
This CL avoids the issue by instead creating test (sub)classes to hold
the ScopedFeatureLists.
Bug: 915219
Change-Id: I193a5174a74fcf89e9dc1593ee5cdb6eae570ad3
Reviewed-on: https://chromium-review.googlesource.com/c/1406999
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622397}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 76e36d8..90a8a3a 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
@@ -59,12 +59,20 @@
DISALLOW_COPY_AND_ASSIGN(SingleClientSecondaryAccountSyncTest);
};
-IN_PROC_BROWSER_TEST_F(SingleClientSecondaryAccountSyncTest,
- DoesNotStartSyncWithStandaloneTransportDisabled) {
- base::test::ScopedFeatureList disable_standalone_transport;
- disable_standalone_transport.InitAndDisableFeature(
- switches::kSyncStandaloneTransport);
+class SingleClientSecondaryAccountWithoutStandaloneTransportSyncTest
+ : public SingleClientSecondaryAccountSyncTest {
+ public:
+ SingleClientSecondaryAccountWithoutStandaloneTransportSyncTest() {
+ features_.InitAndDisableFeature(switches::kSyncStandaloneTransport);
+ }
+ private:
+ base::test::ScopedFeatureList features_;
+};
+
+IN_PROC_BROWSER_TEST_F(
+ SingleClientSecondaryAccountWithoutStandaloneTransportSyncTest,
+ DoesNotStartSyncWithStandaloneTransportDisabled) {
ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
// Since standalone transport is disabled, just signing in (without making the
@@ -74,12 +82,20 @@
GetSyncService(0)->GetTransportState());
}
-IN_PROC_BROWSER_TEST_F(SingleClientSecondaryAccountSyncTest,
- DoesNotStartSyncWithSecondaryAccountSupportDisabled) {
- base::test::ScopedFeatureList disable_secondary_account_support;
- disable_secondary_account_support.InitAndDisableFeature(
- switches::kSyncSupportSecondaryAccount);
+class SingleClientSecondaryAccountWithoutSecondaryAccountSupportSyncTest
+ : public SingleClientSecondaryAccountSyncTest {
+ public:
+ SingleClientSecondaryAccountWithoutSecondaryAccountSupportSyncTest() {
+ features_.InitAndDisableFeature(switches::kSyncSupportSecondaryAccount);
+ }
+ private:
+ base::test::ScopedFeatureList features_;
+};
+
+IN_PROC_BROWSER_TEST_F(
+ SingleClientSecondaryAccountWithoutSecondaryAccountSupportSyncTest,
+ DoesNotStartSyncWithSecondaryAccountSupportDisabled) {
ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
// Since secondary account support is disabled, just signing in (without
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 7497db2..b896e44 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
@@ -321,12 +321,20 @@
.Wait());
}
-IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest,
- NavigateThenCloseTabThenOpenTab) {
- base::test::ScopedFeatureList override_features;
- override_features.InitAndEnableFeature(
- sync_sessions::kDeferRecyclingOfSyncTabNodesIfUnsynced);
+class SingleClientSessionsWithDeferRecyclingSyncTest
+ : public SingleClientSessionsSyncTest {
+ public:
+ SingleClientSessionsWithDeferRecyclingSyncTest() {
+ features_.InitAndEnableFeature(
+ sync_sessions::kDeferRecyclingOfSyncTabNodesIfUnsynced);
+ }
+ private:
+ base::test::ScopedFeatureList features_;
+};
+
+IN_PROC_BROWSER_TEST_F(SingleClientSessionsWithDeferRecyclingSyncTest,
+ NavigateThenCloseTabThenOpenTab) {
ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
ASSERT_TRUE(CheckInitialState(0));
diff --git a/chrome/browser/sync/test/integration/single_client_standalone_transport_sync_test.cc b/chrome/browser/sync/test/integration/single_client_standalone_transport_sync_test.cc
index f3c92dc..ab796265 100644
--- a/chrome/browser/sync/test/integration/single_client_standalone_transport_sync_test.cc
+++ b/chrome/browser/sync/test/integration/single_client_standalone_transport_sync_test.cc
@@ -53,12 +53,19 @@
DISALLOW_COPY_AND_ASSIGN(SingleClientStandaloneTransportSyncTest);
};
-IN_PROC_BROWSER_TEST_F(SingleClientStandaloneTransportSyncTest,
- DoesNotStartSyncWithFeatureDisabled) {
- base::test::ScopedFeatureList disable_standalone_transport;
- disable_standalone_transport.InitAndDisableFeature(
- switches::kSyncStandaloneTransport);
+class SingleClientStandaloneTransportFeatureDisabledSyncTest : public SyncTest {
+ public:
+ SingleClientStandaloneTransportFeatureDisabledSyncTest()
+ : SyncTest(SINGLE_CLIENT) {
+ features_.InitAndDisableFeature(switches::kSyncStandaloneTransport);
+ }
+ private:
+ base::test::ScopedFeatureList features_;
+};
+
+IN_PROC_BROWSER_TEST_F(SingleClientStandaloneTransportFeatureDisabledSyncTest,
+ DoesNotStartSyncWithFeatureDisabled) {
ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
// Since standalone transport is disabled, signing in should *not* start the
diff --git a/chrome/browser/sync/test/integration/single_client_user_consents_sync_test.cc b/chrome/browser/sync/test/integration/single_client_user_consents_sync_test.cc
index accb0b6..45d86bd 100644
--- a/chrome/browser/sync/test/integration/single_client_user_consents_sync_test.cc
+++ b/chrome/browser/sync/test/integration/single_client_user_consents_sync_test.cc
@@ -186,14 +186,22 @@
EXPECT_TRUE(ExpectUserConsents({specifics1, specifics2}));
}
+class SingleClientUserConsentsWithStandaloneTransportSyncTest
+ : public SingleClientUserConsentsSyncTest {
+ public:
+ SingleClientUserConsentsWithStandaloneTransportSyncTest() {
+ features_.InitAndEnableFeature(switches::kSyncStandaloneTransport);
+ }
+
+ private:
+ base::test::ScopedFeatureList features_;
+};
+
// ChromeOS does not support late signin after profile creation, so the test
// below does not apply, at least in the current form.
#if !defined(OS_CHROMEOS)
-IN_PROC_BROWSER_TEST_F(SingleClientUserConsentsSyncTest,
+IN_PROC_BROWSER_TEST_F(SingleClientUserConsentsWithStandaloneTransportSyncTest,
ShouldSubmitIfSignedInAlthoughFullSyncNotEnabled) {
- base::test::ScopedFeatureList feature_list;
- feature_list.InitAndEnableFeature(switches::kSyncStandaloneTransport);
-
// We avoid calling SetupSync(), because we don't want to turn on full sync,
// only sign in such that the standalone transport starts.
ASSERT_TRUE(SetupClients());
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 4d2a1a3..965a843 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
@@ -307,6 +307,22 @@
ExpectNoHistogramsForAddressesDiff();
}
+class SingleClientWalletWithAccountStorageSyncTest
+ : public SingleClientWalletSyncTest {
+ public:
+ SingleClientWalletWithAccountStorageSyncTest() {
+ features_.InitWithFeatures(
+ /*enabled_features=*/{switches::kSyncStandaloneTransport,
+ switches::kSyncUSSAutofillWalletData,
+ autofill::features::
+ kAutofillEnableAccountWalletStorage},
+ /*disabled_features=*/{});
+ }
+
+ private:
+ base::test::ScopedFeatureList features_;
+};
+
// ChromeOS does not support late signin after profile creation, so the test
// below does not apply, at least in the current form.
#if !defined(OS_CHROMEOS)
@@ -318,16 +334,8 @@
#endif
// The account storage requires USS, so we only test the USS implementation
// here.
-IN_PROC_BROWSER_TEST_F(SingleClientWalletSyncTest,
+IN_PROC_BROWSER_TEST_F(SingleClientWalletWithAccountStorageSyncTest,
MAYBE_DownloadAccountStorage_Card) {
- base::test::ScopedFeatureList features;
- features.InitWithFeatures(
- /*enabled_features=*/{switches::kSyncStandaloneTransport,
- switches::kSyncUSSAutofillWalletData,
- autofill::features::
- kAutofillEnableAccountWalletStorage},
- /*disabled_features=*/{});
-
ASSERT_TRUE(SetupClients());
autofill::PersonalDataManager* pdm = GetPersonalDataManager(0);
pdm->OnSyncServiceInitialized(GetSyncService(0));
@@ -1276,16 +1284,8 @@
SwitchesBetweenAccountAndProfileStorageOnTogglingSync
#endif
IN_PROC_BROWSER_TEST_F(
- SingleClientWalletSyncTest,
+ SingleClientWalletWithAccountStorageSyncTest,
MAYBE_SwitchesBetweenAccountAndProfileStorageOnTogglingSync) {
- base::test::ScopedFeatureList features;
- features.InitWithFeatures(
- /*enabled_features=*/{switches::kSyncStandaloneTransport,
- switches::kSyncUSSAutofillWalletData,
- autofill::features::
- kAutofillEnableAccountWalletStorage},
- /*disabled_features=*/{});
-
ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
GetPersonalDataManager(0)->OnSyncServiceInitialized(GetSyncService(0));
