Clean up SingleClientUserEventsSyncTest.RetryParallel

This mostly adds some comments, and inlines a helper function which is
critical for understanding the test.
It also deletes UserEventCaseChecker (in the same file) which was unused.

Bug: none
Change-Id: I70b206240d9aa83c486d03115e6ec3aaf4b2be1a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1795446Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#695155}

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

@@ -2,11 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <stdint.h>
-
 #include "base/bind.h"
 #include "base/macros.h"
-#include "base/test/scoped_feature_list.h"
+#include "base/test/bind_test_util.h"
 #include "base/time/time.h"
 #include "chrome/browser/sync/test/integration/bookmarks_helper.h"
 #include "chrome/browser/sync/test/integration/encryption_helper.h"
@@ -18,13 +16,9 @@
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/browser/sync/test/integration/user_events_helper.h"
 #include "chrome/browser/sync/user_event_service_factory.h"
-#include "components/sync/driver/sync_driver_switches.h"
-#include "components/sync/model/model_type_sync_bridge.h"
 #include "components/sync/protocol/user_event_specifics.pb.h"
 #include "components/sync_user_events/user_event_service.h"
-#include "components/variations/variations_associated_data.h"
 
-using fake_server::FakeServer;
 using sync_pb::CommitResponse;
 using sync_pb::SyncEntity;
 using sync_pb::UserEventSpecifics;
@@ -38,74 +32,6 @@ CommitResponse::ResponseType BounceType(
   return type;
 }
 
-CommitResponse::ResponseType TransientErrorFirst(
-    bool* first,
-    UserEventSpecifics* retry_specifics,
-    const syncer::LoopbackServerEntity& entity) {
-  if (*first) {
-    *first = false;
-    SyncEntity sync_entity;
-    entity.SerializeAsProto(&sync_entity);
-    *retry_specifics = sync_entity.specifics().user_event();
-    return CommitResponse::TRANSIENT_ERROR;
-  } else {
-    return CommitResponse::SUCCESS;
-  }
-}
-
-// A more simplistic version of UserEventEqualityChecker that only checks the
-// case of the events. This is helpful if you do not know (or control) some of
-// the fields of the events that are created.
-class UserEventCaseChecker : public SingleClientStatusChangeChecker {
- public:
-  UserEventCaseChecker(
-      syncer::ProfileSyncService* service,
-      FakeServer* fake_server,
-      std::multiset<UserEventSpecifics::EventCase> expected_cases)
-      : SingleClientStatusChangeChecker(service),
-        fake_server_(fake_server),
-        expected_cases_(expected_cases) {}
-
-  bool IsExitConditionSatisfied() override {
-    std::vector<SyncEntity> entities =
-        fake_server_->GetSyncEntitiesByModelType(syncer::USER_EVENTS);
-
-    // |entities.size()| is only going to grow, if |entities.size()| ever
-    // becomes bigger then all hope is lost of passing, stop now.
-    EXPECT_GE(expected_cases_.size(), entities.size());
-
-    if (expected_cases_.size() > entities.size()) {
-      return false;
-    }
-
-    // Number of events on server matches expected, exit condition is satisfied.
-    // Let's verify that content matches as well. It is safe to modify
-    // |expected_specifics_|.
-    for (const SyncEntity& entity : entities) {
-      UserEventSpecifics::EventCase actual =
-          entity.specifics().user_event().event_case();
-      auto iter = expected_cases_.find(actual);
-      if (iter != expected_cases_.end()) {
-        expected_cases_.erase(iter);
-      } else {
-        ADD_FAILURE() << actual;
-      }
-    }
-
-    return true;
-  }
-
-  std::string GetDebugMessage() const override {
-    return "Waiting server side USER_EVENTS to match expected.";
-  }
-
- private:
-  FakeServer* fake_server_;
-  std::multiset<UserEventSpecifics::EventCase> expected_cases_;
-
-  DISALLOW_COPY_AND_ASSIGN(UserEventCaseChecker);
-};
-
 class SingleClientUserEventsSyncTest : public SyncTest {
  public:
   SingleClientUserEventsSyncTest() : SyncTest(SINGLE_CLIENT) {
@@ -170,24 +96,38 @@ IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, RetrySequential) {
 #endif
 IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, MAYBE_RetryParallel) {
   ASSERT_TRUE(SetupSync());
-  bool first = true;
+
   const UserEventSpecifics specifics1 =
       CreateTestEvent(base::Time() + base::TimeDelta::FromMicroseconds(1));
   const UserEventSpecifics specifics2 =
       CreateTestEvent(base::Time() + base::TimeDelta::FromMicroseconds(2));
-  UserEventSpecifics retry_specifics;
 
   syncer::UserEventService* event_service =
       browser_sync::UserEventServiceFactory::GetForProfile(GetProfile(0));
 
+  // Set up the server so that the first entity that arrives results in a
+  // transient error.
   // We're not really sure if |specifics1| or |specifics2| is going to see the
   // error, so record the one that does into |retry_specifics| and use it in
   // expectations.
-  GetFakeServer()->OverrideResponseType(
-      base::Bind(&TransientErrorFirst, &first, &retry_specifics));
+  bool first = true;
+  UserEventSpecifics retry_specifics;
+  GetFakeServer()->OverrideResponseType(base::BindLambdaForTesting(
+      [&](const syncer::LoopbackServerEntity& entity) {
+        if (first) {
+          first = false;
+          SyncEntity sync_entity;
+          entity.SerializeAsProto(&sync_entity);
+          retry_specifics = sync_entity.specifics().user_event();
+          return CommitResponse::TRANSIENT_ERROR;
+        }
+        return CommitResponse::SUCCESS;
+      }));
 
   event_service->RecordUserEvent(specifics2);
   event_service->RecordUserEvent(specifics1);
+  // The entity that got the transient error is still considered "committed" by
+  // the fake server, so we should see it *and* its retry.
   EXPECT_TRUE(ExpectUserEvents({specifics1, specifics2}));
   EXPECT_TRUE(ExpectUserEvents({specifics1, specifics2, retry_specifics}));
 }