BackForwardCache: Refactoring: Move GetEntries/GetMetrics to TestUkmRecorder

This CL is a pure refactoring. This moves the duplicated implementations
of GetEntries/GetMetrics from test files to TestUkmRecorder.

Bug: 1033410
Change-Id: I97c4686421fb26d7084414826e5dc99e778045bb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1986655
Commit-Queue: Hajime Hoshi <hajimehoshi@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#732343}

components/ukm/test_ukm_recorder.cc

@@ -164,4 +164,48 @@ TestAutoSetUkmRecorder::~TestAutoSetUkmRecorder() {
   DelegatingUkmRecorder::Get()->RemoveDelegate(this);
 }
 
+std::vector<TestUkmRecorder::HumanReadableUkmMetrics>
+TestUkmRecorder::GetMetrics(std::string entry_name,
+                            const std::vector<std::string>& metric_names) {
+  std::vector<TestUkmRecorder::HumanReadableUkmMetrics> result;
+  for (const auto& entry : GetEntries(entry_name, metric_names)) {
+    result.push_back(entry.metrics);
+  }
+  return result;
+}
+
+std::vector<TestUkmRecorder::HumanReadableUkmEntry> TestUkmRecorder::GetEntries(
+    std::string entry_name,
+    const std::vector<std::string>& metric_names) {
+  std::vector<TestUkmRecorder::HumanReadableUkmEntry> results;
+  for (const ukm::mojom::UkmEntry* entry : GetEntriesByName(entry_name)) {
+    HumanReadableUkmEntry result;
+    result.source_id = entry->source_id;
+    for (const std::string& metric_name : metric_names) {
+      const int64_t* metric_value =
+          ukm::TestUkmRecorder::GetEntryMetric(entry, metric_name);
+      if (metric_value)
+        result.metrics[metric_name] = *metric_value;
+    }
+    results.push_back(std::move(result));
+  }
+  return results;
+}
+
+TestUkmRecorder::HumanReadableUkmEntry::HumanReadableUkmEntry() = default;
+
+TestUkmRecorder::HumanReadableUkmEntry::HumanReadableUkmEntry(
+    ukm::SourceId source_id,
+    TestUkmRecorder::HumanReadableUkmMetrics ukm_metrics)
+    : source_id(source_id), metrics(std::move(ukm_metrics)) {}
+
+TestUkmRecorder::HumanReadableUkmEntry::HumanReadableUkmEntry(
+    const HumanReadableUkmEntry&) = default;
+TestUkmRecorder::HumanReadableUkmEntry::~HumanReadableUkmEntry() = default;
+
+bool TestUkmRecorder::HumanReadableUkmEntry::operator==(
+    const HumanReadableUkmEntry& other) const {
+  return source_id == other.source_id && metrics == other.metrics;
+}
+
 }  // namespace ukm

components/ukm/test_ukm_recorder.h

@@ -26,6 +26,21 @@ namespace ukm {
 // Wraps an UkmRecorder with additional accessors used for testing.
 class TestUkmRecorder : public UkmRecorderImpl {
  public:
+  using HumanReadableUkmMetrics = std::map<std::string, int64_t>;
+
+  struct HumanReadableUkmEntry {
+    HumanReadableUkmEntry();
+    HumanReadableUkmEntry(ukm::SourceId source_id,
+                          HumanReadableUkmMetrics ukm_metrics);
+    ~HumanReadableUkmEntry();
+    HumanReadableUkmEntry(const HumanReadableUkmEntry&);
+
+    bool operator==(const HumanReadableUkmEntry& other) const;
+
+    ukm::SourceId source_id = kInvalidSourceId;
+    HumanReadableUkmMetrics metrics;
+  };
+
   TestUkmRecorder();
   ~TestUkmRecorder() override;
 
@@ -85,6 +100,18 @@ class TestUkmRecorder : public UkmRecorderImpl {
   static const int64_t* GetEntryMetric(const mojom::UkmEntry* entry,
                                        base::StringPiece metric_name);
 
+  // A test helper returning all metrics for all entries with a given name in a
+  // human-readable form, allowing to write clearer test expectations.
+  std::vector<HumanReadableUkmMetrics> GetMetrics(
+      std::string entry_name,
+      const std::vector<std::string>& metric_names);
+
+  // A test helper returning all entries for a given name in a human-readable
+  // form, allowing to write clearer test expectations.
+  std::vector<HumanReadableUkmEntry> GetEntries(
+      std::string entry_name,
+      const std::vector<std::string>& metric_names);
+
  private:
   uint64_t entry_hash_to_wait_for_ = 0;
   base::OnceClosure on_add_entry_;

content/browser/frame_host/back_forward_cache_metrics_browsertest.cc

@@ -48,38 +48,8 @@ constexpr uint64_t kFeaturesToIgnoreMask =
     1 << static_cast<size_t>(blink::scheduler::WebSchedulerTrackedFeature::
                                  kOutstandingNetworkRequest);
 
-using UkmMetrics = std::map<std::string, int64_t>;
-using UkmEntry = std::pair<ukm::SourceId, UkmMetrics>;
-
-std::vector<UkmEntry> GetEntries(ukm::TestUkmRecorder* recorder,
-                                 std::string entry_name,
-                                 const std::vector<std::string>& metrics) {
-  std::vector<UkmEntry> results;
-  for (const ukm::mojom::UkmEntry* entry :
-       recorder->GetEntriesByName(entry_name)) {
-    UkmEntry result;
-    result.first = entry->source_id;
-    for (const std::string& metric_name : metrics) {
-      const int64_t* metric_value =
-          ukm::TestUkmRecorder::GetEntryMetric(entry, metric_name);
-      EXPECT_TRUE(metric_value) << "Metric " << metric_name
-                                << " is not found in entry " << entry_name;
-      result.second[metric_name] = *metric_value;
-    }
-    results.push_back(std::move(result));
-  }
-  return results;
-}
-
-std::vector<UkmMetrics> GetMetrics(ukm::TestUkmRecorder* recorder,
-                                   std::string entry_name,
-                                   const std::vector<std::string>& metrics) {
-  std::vector<UkmMetrics> result;
-  for (const auto& entry : GetEntries(recorder, entry_name, metrics)) {
-    result.push_back(entry.second);
-  }
-  return result;
-}
+using UkmMetrics = ukm::TestUkmRecorder::HumanReadableUkmMetrics;
+using UkmEntry = ukm::TestUkmRecorder::HumanReadableUkmEntry;
 
 }  // namespace
 
@@ -181,7 +151,7 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheMetricsBrowserTest, UKM) {
   // Second back-forward navigation (#8) navigates back to navigation #2,
   // but it is subframe navigation and not reflected here. Third back-forward
   // navigation (#9) navigates back to navigation #1.
-  EXPECT_THAT(GetEntries(&recorder, "HistoryNavigation", {last_navigation_id}),
+  EXPECT_THAT(recorder.GetEntries("HistoryNavigation", {last_navigation_id}),
               testing::ElementsAre(UkmEntry{id6, {{last_navigation_id, id2}}},
                                    UkmEntry{id9, {{last_navigation_id, id1}}}));
 }
@@ -229,7 +199,7 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheMetricsBrowserTest,
   // The second back/forward navigation is a subframe one and should be ignored.
   // The last one navigates to the actual entry.
   EXPECT_THAT(
-      GetMetrics(&recorder, "HistoryNavigation", {navigated_to_last_entry}),
+      recorder.GetMetrics("HistoryNavigation", {navigated_to_last_entry}),
       testing::ElementsAre(UkmMetrics{{navigated_to_last_entry, false}},
                            UkmMetrics{{navigated_to_last_entry, true}}));
 }
@@ -287,7 +257,7 @@ IN_PROC_BROWSER_TEST_F(BackForwardCacheMetricsBrowserTest, CloneAndGoBack) {
   // The fourth goes back, but the metrics are not recorded due to it being
   // cloned and the metrics objects missing.
   // The last two navigations, however, should have metrics.
-  EXPECT_THAT(GetEntries(&recorder, "HistoryNavigation", {last_navigation_id}),
+  EXPECT_THAT(recorder.GetEntries("HistoryNavigation", {last_navigation_id}),
               testing::ElementsAre(UkmEntry{id5, {{last_navigation_id, id3}}},
                                    UkmEntry{id6, {{last_navigation_id, id4}}}));
 }
@@ -322,17 +292,18 @@ std::vector<FeatureUsage> GetFeatureUsageMetrics(
     ukm::TestAutoSetUkmRecorder* recorder) {
   std::vector<FeatureUsage> result;
   for (const auto& entry :
-       GetEntries(recorder, "HistoryNavigation",
-                  {"MainFrameFeatures", "SameOriginSubframesFeatures",
-                   "CrossOriginSubframesFeatures"})) {
+       recorder->GetEntries("HistoryNavigation",
+                            {"MainFrameFeatures", "SameOriginSubframesFeatures",
+                             "CrossOriginSubframesFeatures"})) {
     FeatureUsage feature_usage;
-    feature_usage.source_id = entry.first;
+    feature_usage.source_id = entry.source_id;
     feature_usage.main_frame_features =
-        entry.second.at("MainFrameFeatures") & ~kFeaturesToIgnoreMask;
+        entry.metrics.at("MainFrameFeatures") & ~kFeaturesToIgnoreMask;
     feature_usage.same_origin_subframes_features =
-        entry.second.at("SameOriginSubframesFeatures") & ~kFeaturesToIgnoreMask;
+        entry.metrics.at("SameOriginSubframesFeatures") &
+        ~kFeaturesToIgnoreMask;
     feature_usage.cross_origin_subframes_features =
-        entry.second.at("CrossOriginSubframesFeatures") &
+        entry.metrics.at("CrossOriginSubframesFeatures") &
         ~kFeaturesToIgnoreMask;
     result.push_back(feature_usage);
   }

content/browser/frame_host/back_forward_cache_metrics_unittest.cc

@@ -47,27 +47,7 @@ class BackForwardCacheMetricsTest : public RenderViewHostImplTestHarness,
   std::vector<int64_t> navigation_ids_;
 };
 
-using UkmEntry = std::pair<ukm::SourceId, std::map<std::string, int64_t>>;
-
-std::vector<UkmEntry> GetMetrics(ukm::TestUkmRecorder* recorder,
-                                 std::string entry_name,
-                                 std::vector<std::string> metrics) {
-  std::vector<UkmEntry> results;
-  for (const ukm::mojom::UkmEntry* entry :
-       recorder->GetEntriesByName(entry_name)) {
-    UkmEntry result;
-    result.first = entry->source_id;
-    for (const std::string& metric_name : metrics) {
-      const int64_t* metric_value =
-          ukm::TestUkmRecorder::GetEntryMetric(entry, metric_name);
-      EXPECT_TRUE(metric_value) << "Metric " << metric_name
-                                << " is not found in entry " << entry_name;
-      result.second[metric_name] = *metric_value;
-    }
-    results.push_back(std::move(result));
-  }
-  return results;
-}
+using UkmEntry = ukm::TestUkmRecorder::HumanReadableUkmEntry;
 
 ukm::SourceId ToSourceId(int64_t navigation_id) {
   return ukm::ConvertToSourceId(navigation_id,
@@ -108,8 +88,8 @@ TEST_F(BackForwardCacheMetricsTest, HistoryNavigationUKM) {
   std::string time_away = "TimeSinceNavigatedAwayFromDocument";
 
   EXPECT_THAT(
-      GetMetrics(&recorder_, "HistoryNavigation",
-                 {last_navigation_id, time_away}),
+      recorder_.GetEntries("HistoryNavigation",
+                           {last_navigation_id, time_away}),
       testing::ElementsAre(
           UkmEntry{id4, {{last_navigation_id, id2}, {time_away, 0b100}}},
           UkmEntry{id5, {{last_navigation_id, id1}, {time_away, 0b1110}}},
@@ -133,7 +113,7 @@ TEST_F(BackForwardCacheMetricsTest, LongDurationsAreClamped) {
 
   // The original interval of 5h + 50ms is clamped to just 5h.
   EXPECT_THAT(
-      GetMetrics(&recorder_, "HistoryNavigation", {time_away}),
+      recorder_.GetEntries("HistoryNavigation", {time_away}),
       testing::ElementsAre(UkmEntry{
           id3, {{time_away, base::TimeDelta::FromHours(5).InMilliseconds()}}}));
 }
@@ -178,7 +158,7 @@ TEST_F(BackForwardCacheMetricsTest, TimeRecordedAtStart) {
 
   std::string time_away = "TimeSinceNavigatedAwayFromDocument";
 
-  EXPECT_THAT(GetMetrics(&recorder_, "HistoryNavigation", {time_away}),
+  EXPECT_THAT(recorder_.GetEntries("HistoryNavigation", {time_away}),
               testing::ElementsAre(UkmEntry{id3, {{time_away, 0b1000}}}));
 }