Cleanup/simplify UserEventEqualityChecker

Before this CL, UserEventEqualityChecker would modify its
|expected_specifics_| in IsExitConditionSatisfied(), which usually gets
called multiple times. In the specific situations where it did this, it
was actually safe, but this was quite non-obvious.
The new version just makes a copy and modifies that. It's slightly more
verbose, but much less confusing.

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

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

@@ -126,9 +126,13 @@ IN_PROC_BROWSER_TEST_F(SingleClientUserEventsSyncTest, MAYBE_RetryParallel) {
 
   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.
+  // First wait for these two events to arrive on the server - only after this
+  // has happened will |retry_specifics| actually be populated.
+  // Note: The entity that got the transient error is still considered
+  // "committed" by the fake server.
   EXPECT_TRUE(ExpectUserEvents({specifics1, specifics2}));
+  // Now that |retry_specifics| got populated by the lambda above, make sure it
+  // also arrives on the server.
   EXPECT_TRUE(ExpectUserEvents({specifics1, specifics2, retry_specifics}));
 }
 

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

@@ -4,9 +4,6 @@
 
 #include "chrome/browser/sync/test/integration/user_events_helper.h"
 
-#include <string>
-#include <vector>
-
 #include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
 #include "components/sync/test/fake_server/fake_server.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -32,11 +29,9 @@ UserEventEqualityChecker::UserEventEqualityChecker(
     syncer::ProfileSyncService* service,
     FakeServer* fake_server,
     std::vector<UserEventSpecifics> expected_specifics)
-    : SingleClientStatusChangeChecker(service), fake_server_(fake_server) {
-  for (const UserEventSpecifics& specifics : expected_specifics) {
-    expected_specifics_.emplace(specifics.event_time_usec(), specifics);
-  }
-}
+    : SingleClientStatusChangeChecker(service),
+      fake_server_(fake_server),
+      expected_specifics_(expected_specifics) {}
 
 UserEventEqualityChecker::~UserEventEqualityChecker() = default;
 
@@ -53,24 +48,35 @@ bool UserEventEqualityChecker::IsExitConditionSatisfied() {
   }
 
   // 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_|.
+  // Let's verify that content matches as well.
+
+  // Make a copy of |expected_specifics_| so that we can safely modify it.
+  std::vector<sync_pb::UserEventSpecifics> remaining_expected_specifics =
+      expected_specifics_;
   for (const SyncEntity& entity : entities) {
     UserEventSpecifics server_specifics = entity.specifics().user_event();
-    auto iter = expected_specifics_.find(server_specifics.event_time_usec());
+    // Find a matching event in our expectations. Same event time should mean
+    // identical events, though there can be duplicates in some cases.
+    auto iter = std::find_if(
+        remaining_expected_specifics.begin(),
+        remaining_expected_specifics.end(),
+        [&server_specifics](const sync_pb::UserEventSpecifics& specifics) {
+          return server_specifics.event_time_usec() ==
+                 specifics.event_time_usec();
+        });
     // We don't expect to encounter id matching events with different values,
     // this isn't going to recover so fail the test case now.
-    EXPECT_TRUE(expected_specifics_.end() != iter);
-    if (expected_specifics_.end() == iter) {
+    EXPECT_NE(iter, remaining_expected_specifics.end());
+    if (remaining_expected_specifics.end() == iter) {
       return false;
     }
     // TODO(skym): This may need to change if we start updating navigation_id
     // based on what sessions data is committed, and end up committing the
     // same event multiple times.
-    EXPECT_EQ(iter->second.navigation_id(), server_specifics.navigation_id());
-    EXPECT_EQ(iter->second.event_case(), server_specifics.event_case());
+    EXPECT_EQ(iter->navigation_id(), server_specifics.navigation_id());
+    EXPECT_EQ(iter->event_case(), server_specifics.event_case());
 
-    expected_specifics_.erase(iter);
+    remaining_expected_specifics.erase(iter);
   }
 
   return true;

chrome/browser/sync/test/integration/user_events_helper.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_SYNC_TEST_INTEGRATION_USER_EVENTS_HELPER_H_
 
 #include <string>
+#include <vector>
 
 #include "base/time/time.h"
 #include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
@@ -36,7 +37,7 @@ class UserEventEqualityChecker : public SingleClientStatusChangeChecker {
 
  private:
   fake_server::FakeServer* fake_server_;
-  std::multimap<int64_t, sync_pb::UserEventSpecifics> expected_specifics_;
+  const std::vector<sync_pb::UserEventSpecifics> expected_specifics_;
 
   DISALLOW_COPY_AND_ASSIGN(UserEventEqualityChecker);
 };