[Sync] Run password browser test exclusively for USS or non-USS clients

Before this Patch:
TwoClientPasswordsSyncTest were exercised against the 4 combinations
of USS and non-USS clients. This caused some tests to be flaky.
(check the linked bugs).

After this patch:
Since the plan is to roll out Sync USS Passwords for M77 (where all the
4 combinations have been tested already) we are now restricting the
testing to only USS or non-USS to remove the flakiness.

In case the rollout doesn't make it to M77, we should rethink about
reinstating the tests again.

Change-Id: I402f481723541a1a77314a8173a1144a1560cf77
Bug: 984944,980557,915219
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1768530Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#690326}

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

@@ -13,6 +13,7 @@
 #include "base/rand_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
+#include "chrome/browser/sync/test/integration/feature_toggler.h"
 #include "chrome/browser/sync/test/integration/passwords_helper.h"
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
 #include "chrome/browser/sync/test/integration/sync_integration_test_util.h"
@@ -39,60 +40,18 @@ using autofill::PasswordForm;
 
 static const char* kValidPassphrase = "passphrase!";
 
-class TwoClientPasswordsSyncTest
-    : public testing::WithParamInterface<std::tuple<bool, bool>>,
-      public SyncTest {
+class TwoClientPasswordsSyncTest : public FeatureToggler, public SyncTest {
  public:
-  TwoClientPasswordsSyncTest() : SyncTest(TWO_CLIENT) {}
+  TwoClientPasswordsSyncTest()
+      : FeatureToggler(switches::kSyncUSSPasswords), SyncTest(TWO_CLIENT) {}
 
   ~TwoClientPasswordsSyncTest() override {}
 
- protected:
-  // TODO(crbug.com/915219): This leads to a data race and thus all tests here
-  // are disabled on TSan. It is hard to avoid as overriding g_feature_list
-  // after it has been used is needed for this test (by setting up each client
-  // with a different ScopedFeatureList).
-  void BeforeSetupClient(int index) override {
-    const bool should_enable_uss =
-        index == 0 ? std::get<0>(GetParam()) : std::get<1>(GetParam());
-
-    // In order to avoid test flakiness, for any client other than the first, we
-    // need to make sure the feature toggle has been fully read by PasswordStore
-    // before overriding it again. The way to achieve that, for the current
-    // implementation of PasswordStore, is to make a round trip to the backend
-    // sequence, which guarantees that initialization has completed.
-    if (index != 0) {
-      // We ignore the returned value since all we want is to wait for the
-      // round trip to be completed.
-      GetPasswordCount(index - 1);
-    }
-
-    // The value of the feature kSyncUSSPasswords only matters during the
-    // setup of each client, when the profile is created, ProfileSyncService
-    // instantiated as well as the datatype controllers. By overriding the
-    // feature, we can influence whether client |index| is running with the new
-    // codepath or the legacy one.
-    override_features_ = std::make_unique<base::test::ScopedFeatureList>();
-    if (should_enable_uss) {
-      override_features_->InitAndEnableFeature(switches::kSyncUSSPasswords);
-    } else {
-      override_features_->InitAndDisableFeature(switches::kSyncUSSPasswords);
-    }
-  }
-
  private:
-  std::unique_ptr<base::test::ScopedFeatureList> override_features_;
-
   DISALLOW_COPY_AND_ASSIGN(TwoClientPasswordsSyncTest);
 };
 
-// Flaky on TSAN: crbug.com/915219
-#if defined(THREAD_SANITIZER)
-#define MAYBE_Add DISABLED_Add
-#else
-#define MAYBE_Add Add
-#endif
-IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, E2E_ENABLED(MAYBE_Add)) {
+IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, E2E_ENABLED(Add)) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(SamePasswordFormsChecker().Wait());
 
@@ -104,13 +63,7 @@ IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, E2E_ENABLED(MAYBE_Add)) {
   ASSERT_EQ(1, GetPasswordCount(1));
 }
 
-// Flaky on TSAN: crbug.com/915219
-#if defined(THREAD_SANITIZER)
-#define MAYBE_Race DISABLED_Race
-#else
-#define MAYBE_Race Race
-#endif
-IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, E2E_ENABLED(MAYBE_Race)) {
+IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, E2E_ENABLED(Race)) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(AllProfilesContainSamePasswordForms());
 
@@ -124,14 +77,7 @@ IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, E2E_ENABLED(MAYBE_Race)) {
   ASSERT_TRUE(SamePasswordFormsChecker().Wait());
 }
 
-// Flaky on TSAN: crbug.com/915219
-#if defined(THREAD_SANITIZER)
-#define MAYBE_MergeWithTheMostRecent DISABLED_MergeWithTheMostRecent
-#else
-#define MAYBE_MergeWithTheMostRecent MergeWithTheMostRecent
-#endif
-IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest,
-                       MAYBE_MergeWithTheMostRecent) {
+IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, MergeWithTheMostRecent) {
   // Setup the test to have Form 0 and Form 1 added on both clients. Form 0 is
   // more recent on Client 0, and Form 1 is more recent on Client 1. They should
   // be merged such that recent passwords are chosen.
@@ -175,14 +121,8 @@ IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest,
   }
 }
 
-// Flaky on TSAN: crbug.com/915219
-#if defined(THREAD_SANITIZER)
-#define MAYBE_SetPassphraseAndAddPassword DISABLED_SetPassphraseAndAddPassword
-#else
-#define MAYBE_SetPassphraseAndAddPassword SetPassphraseAndAddPassword
-#endif
 IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest,
-                       E2E_ENABLED(MAYBE_SetPassphraseAndAddPassword)) {
+                       E2E_ENABLED(SetPassphraseAndAddPassword)) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
   GetSyncService(0)->GetUserSettings()->SetEncryptionPassphrase(
@@ -201,13 +141,7 @@ IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest,
   ASSERT_TRUE(SamePasswordFormsChecker().Wait());
 }
 
-// Flaky on TSAN: crbug.com/915219
-#if defined(THREAD_SANITIZER)
-#define MAYBE_Update DISABLED_Update
-#else
-#define MAYBE_Update Update
-#endif
-IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, MAYBE_Update) {
+IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, Update) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(AllProfilesContainSamePasswordFormsAsVerifier());
 
@@ -228,13 +162,7 @@ IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, MAYBE_Update) {
   ASSERT_TRUE(AllProfilesContainSamePasswordFormsAsVerifier());
 }
 
-// Flaky on TSAN: crbug.com/915219
-#if defined(THREAD_SANITIZER)
-#define MAYBE_AddTwice DISABLED_AddTwice
-#else
-#define MAYBE_AddTwice AddTwice
-#endif
-IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, MAYBE_AddTwice) {
+IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, AddTwice) {
   // Password store supports adding the same form twice, so this is testing this
   // behaviour.
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
@@ -258,13 +186,7 @@ IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, MAYBE_AddTwice) {
   ASSERT_EQ(1, GetPasswordCount(1));
 }
 
-// Flaky on TSAN: crbug.com/915219
-#if defined(THREAD_SANITIZER)
-#define MAYBE_Delete DISABLED_Delete
-#else
-#define MAYBE_Delete Delete
-#endif
-IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, MAYBE_Delete) {
+IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, Delete) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(AllProfilesContainSamePasswordFormsAsVerifier());
 
@@ -287,15 +209,8 @@ IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, MAYBE_Delete) {
   ASSERT_TRUE(AllProfilesContainSamePasswordFormsAsVerifier());
 }
 
-#if defined(THREAD_SANITIZER)
-// Flaky on TSAN: crbug.com/915219
-#define MAYBE_SetPassphraseAndThenSetupSync \
-  DISABLED_SetPassphraseAndThenSetupSync
-#else
-#define MAYBE_SetPassphraseAndThenSetupSync SetPassphraseAndThenSetupSync
-#endif
 IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest,
-                       MAYBE_SetPassphraseAndThenSetupSync) {
+                       SetPassphraseAndThenSetupSync) {
   ASSERT_TRUE(SetupClients());
 
   ASSERT_TRUE(GetClient(0)->SetupSync());
@@ -327,13 +242,7 @@ IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest,
   ASSERT_TRUE(SamePasswordFormsChecker().Wait());
 }
 
-// Flaky on TSAN: crbug.com/915219
-#if defined(THREAD_SANITIZER)
-#define MAYBE_DeleteTwo DISABLED_DeleteTwo
-#else
-#define MAYBE_DeleteTwo DeleteTwo
-#endif
-IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, E2E_ONLY(MAYBE_DeleteTwo)) {
+IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, E2E_ONLY(DeleteTwo)) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(AllProfilesContainSamePasswordForms());
 
@@ -361,13 +270,7 @@ IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, E2E_ONLY(MAYBE_DeleteTwo)) {
   ASSERT_EQ(init_password_count - 2, GetPasswordCount(0));
 }
 
-// Flaky on TSAN: crbug.com/915219
-#if defined(THREAD_SANITIZER)
-#define MAYBE_DeleteAll DISABLED_DeleteAll
-#else
-#define MAYBE_DeleteAll DeleteAll
-#endif
-IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, MAYBE_DeleteAll) {
+IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, DeleteAll) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(AllProfilesContainSamePasswordFormsAsVerifier());
 
@@ -387,13 +290,7 @@ IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, MAYBE_DeleteAll) {
   ASSERT_EQ(0, GetVerifierPasswordCount());
 }
 
-// Flaky on TSAN: crbug.com/915219
-#if defined(THREAD_SANITIZER)
-#define MAYBE_Merge DISABLED_Merge
-#else
-#define MAYBE_Merge Merge
-#endif
-IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, E2E_ENABLED(MAYBE_Merge)) {
+IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, E2E_ENABLED(Merge)) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(AllProfilesContainSamePasswordForms());
 
@@ -408,14 +305,7 @@ IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, E2E_ENABLED(MAYBE_Merge)) {
   ASSERT_EQ(3, GetPasswordCount(0));
 }
 
-// Flaky on TSAN: crbug.com/915219
-#if defined(THREAD_SANITIZER)
-#define MAYBE_TwoClientAddPass DISABLED_TwoClientAddPass
-#else
-#define MAYBE_TwoClientAddPass TwoClientAddPass
-#endif
-IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest,
-                       E2E_ONLY(MAYBE_TwoClientAddPass)) {
+IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest, E2E_ONLY(TwoClientAddPass)) {
   ASSERT_TRUE(SetupSync()) <<  "SetupSync() failed.";
   // All profiles should sync same passwords.
   ASSERT_TRUE(SamePasswordFormsChecker().Wait())
@@ -439,10 +329,6 @@ IN_PROC_BROWSER_TEST_P(TwoClientPasswordsSyncTest,
   }
 }
 
-// We instantiate every test 4 times, for every combination of USS being enabled
-// in individual clients. This verifies backward-compatibility between the two
-// implementations.
 INSTANTIATE_TEST_SUITE_P(USS,
                          TwoClientPasswordsSyncTest,
-                         ::testing::Combine(::testing::Values(false, true),
-                                            ::testing::Values(false, true)));
+                         ::testing::Values(false, true));