Move testing of Loading Predictor optimization guide to unit tests

This also re-enables the browser tests since the flakes were mostly tied
to the UMA histogram reporting.

Bug: 1060966,1060397
Change-Id: I67a1dfafa5d220e3a2165981e4bd4a6812174aa8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2134645
Commit-Queue: Sophie Chang <sophiechang@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756093}

chrome/browser/predictors/loading_predictor_browsertest.cc

@@ -1541,6 +1541,14 @@ class LoadingPredictorBrowserTestWithOptimizationGuide
     }
   }
 
+  void SetUpOnMainThread() override {
+    // Wait until command line hints have been processed before proceeding with
+    // tests.
+    RetryForHistogramUntilCountReached(
+        histogram_tester_, "OptimizationGuide.UpdateComponentHints.Result", 1);
+    LoadingPredictorBrowserTest::SetUpOnMainThread();
+  }
+
   void SetUpCommandLine(base::CommandLine* command_line) override {
     // TODO(crbug/1035698): Make this simpler when Optimization Guide has better
     // test support.
@@ -1585,6 +1593,7 @@ class LoadingPredictorBrowserTestWithOptimizationGuide
   }
 
  private:
+  base::HistogramTester histogram_tester_;
   base::test::ScopedFeatureList feature_list_;
   base::test::ScopedFeatureList local_predictions_feature_list_;
 };
@@ -1600,9 +1609,9 @@ INSTANTIATE_TEST_SUITE_P(,
 #define DISABLE_ON_WIN_MAC_CHROMEOS(x) x
 #endif
 
-IN_PROC_BROWSER_TEST_P(
-    LoadingPredictorBrowserTestWithOptimizationGuide,
-    DISABLED_NavigationHasLocalPredictionNoOptimizationHint) {
+IN_PROC_BROWSER_TEST_P(LoadingPredictorBrowserTestWithOptimizationGuide,
+                       DISABLE_ON_WIN_MAC_CHROMEOS(
+                           NavigationHasLocalPredictionNoOptimizationHint)) {
   // Navigate the first time to fill the predictor's database and the HTTP
   // cache.
   GURL url = embedded_test_server()->GetURL(
@@ -1613,8 +1622,6 @@ IN_PROC_BROWSER_TEST_P(
   ui_test_utils::NavigateToURL(browser(), url);
   ResetNetworkState();
 
-  base::HistogramTester histogram_tester;
-
   auto observer = NavigateToURLAsync(url);
   EXPECT_TRUE(observer->WaitForRequestStart());
   for (auto* const host : kHtmlSubresourcesHosts) {
@@ -1641,20 +1648,12 @@ IN_PROC_BROWSER_TEST_P(
             connection_tracker()->GetAcceptedSocketCount());
   // No reads since all resources should be cached.
   EXPECT_EQ(0u, connection_tracker()->GetReadSocketCount());
-
-  if (IsLocalPredictionEnabled()) {
-    histogram_tester.ExpectTotalCount(
-        "LoadingPredictor.OptimizationHintsReceiveStatus", 0);
-  } else {
-    histogram_tester.ExpectTotalCount(
-        "LoadingPredictor.OptimizationHintsReceiveStatus", 1);
-  }
 }
 
-// TODO(crbug.com/1060966): Tests are flakey.
 IN_PROC_BROWSER_TEST_P(
     LoadingPredictorBrowserTestWithOptimizationGuide,
-    DISABLED_NavigationWithBothLocalPredictionAndOptimizationHint) {
+    DISABLE_ON_WIN_MAC_CHROMEOS(
+        NavigationWithBothLocalPredictionAndOptimizationHint)) {
   base::HistogramTester histogram_tester;
 
   GURL url = embedded_test_server()->GetURL(
@@ -1705,32 +1704,12 @@ IN_PROC_BROWSER_TEST_P(
     EXPECT_TRUE(preconnect_manager_observer()->HasOriginAttemptedToPreconnect(
         expected_origin));
   }
-
-  if (IsLocalPredictionEnabled()) {
-    histogram_tester.ExpectUniqueSample(
-        "LoadingPredictor.OptimizationHintsReceiveStatus",
-        OptimizationHintsReceiveStatus::kAfterNavigationFinish, 1);
-  } else {
-    // We expect one for the setup navigation and one for the navigation we care
-    // about.
-    histogram_tester.ExpectTotalCount(
-        "LoadingPredictor.OptimizationHintsReceiveStatus", 2);
-    // We expect the hints to arrive before navigation for the navigation we
-    // care about.
-    histogram_tester.ExpectBucketCount(
-        "LoadingPredictor.OptimizationHintsReceiveStatus",
-        OptimizationHintsReceiveStatus::kBeforeNavigationFinish, 1);
-    // We expect the decision to arrive at finish for the setup navigation.
-    histogram_tester.ExpectBucketCount(
-        "LoadingPredictor.OptimizationHintsReceiveStatus",
-        OptimizationHintsReceiveStatus::kAfterNavigationFinish, 1);
-  }
 }
 
-// crbug.com/1060966
 IN_PROC_BROWSER_TEST_P(
     LoadingPredictorBrowserTestWithOptimizationGuide,
-    DISABLED_NavigationWithNoLocalPredictionsButHasOptimizationHint) {
+    DISABLE_ON_WIN_MAC_CHROMEOS(
+        NavigationWithNoLocalPredictionsButHasOptimizationHint)) {
   {
     base::HistogramTester histogram_tester;
 
@@ -1798,30 +1777,13 @@ IN_PROC_BROWSER_TEST_P(
         "LoadingPredictor.PreconnectLearningPrecision.OptimizationGuide", 0, 1);
     histogram_tester.ExpectUniqueSample(
         "LoadingPredictor.PreconnectLearningCount.OptimizationGuide", 2, 1);
-
-    // We expect one for the final navigation and one for the navigation we care
-    // about.
-    histogram_tester.ExpectTotalCount(
-        "LoadingPredictor.OptimizationHintsReceiveStatus", 2);
-    // We expect the hints to arrive before navigation for the navigation we
-    // care about.
-    histogram_tester.ExpectBucketCount(
-        "LoadingPredictor.OptimizationHintsReceiveStatus",
-        OptimizationHintsReceiveStatus::kBeforeNavigationFinish, 1);
-    // We expect the decision to arrive at finish for the navigation we do not
-    // have hints for.
-    histogram_tester.ExpectBucketCount(
-        "LoadingPredictor.OptimizationHintsReceiveStatus",
-        OptimizationHintsReceiveStatus::kAfterNavigationFinish, 1);
   }
 }
 
-// TODO(crbug.com/1060966): Tests are flakey.
 IN_PROC_BROWSER_TEST_P(
     LoadingPredictorBrowserTestWithOptimizationGuide,
-    DISABLED_OptimizationGuidePredictionsNotAppliedForAlreadyCommittedNavigation) {
-  base::HistogramTester histogram_tester;
-
+    DISABLE_ON_WIN_MAC_CHROMEOS(
+        OptimizationGuidePredictionsNotAppliedForAlreadyCommittedNavigation)) {
   GURL url = embedded_test_server()->GetURL("hints.com", "/simple.html");
   url::Origin origin = url::Origin::Create(url);
   net::NetworkIsolationKey network_isolation_key(origin, origin);
@@ -1833,17 +1795,11 @@ IN_PROC_BROWSER_TEST_P(
       "subresource.com", network_isolation_key));
   EXPECT_FALSE(preconnect_manager_observer()->HasHostBeenLookedUp(
       "otheresource.com", network_isolation_key));
-
-  histogram_tester.ExpectUniqueSample(
-      "LoadingPredictor.OptimizationHintsReceiveStatus",
-      OptimizationHintsReceiveStatus::kAfterNavigationFinish, 1);
 }
 
 IN_PROC_BROWSER_TEST_P(LoadingPredictorBrowserTestWithOptimizationGuide,
                        DISABLE_ON_WIN_MAC_CHROMEOS(
                            OptimizationGuidePredictionsNotAppliedForRedirect)) {
-  base::HistogramTester histogram_tester;
-
   GURL destination_url =
       embedded_test_server()->GetURL("otherhost.com", "/cachetime");
   GURL redirecting_url = embedded_test_server()->GetURL(
@@ -1859,11 +1815,6 @@ IN_PROC_BROWSER_TEST_P(LoadingPredictorBrowserTestWithOptimizationGuide,
       "subresource.com", network_isolation_key));
   EXPECT_FALSE(preconnect_manager_observer()->HasHostBeenLookedUp(
       "otheresource.com", network_isolation_key));
-
-  // We cannot force when the hint comes back, so we make sure that we at least
-  // received something back from the optimization guide instead.
-  histogram_tester.ExpectTotalCount(
-      "LoadingPredictor.OptimizationHintsReceiveStatus", 1);
 }
 
 class LoadingPredictorBrowserTestWithNoLocalPredictions

chrome/browser/predictors/loading_predictor_tab_helper.cc

@@ -308,8 +308,12 @@ void LoadingPredictorTabHelper::OnOptimizationGuideDecision(
   if (decision != optimization_guide::OptimizationGuideDecision::kTrue)
     return;
 
-  if (!metadata.loading_predictor_metadata())
+  if (!metadata.loading_predictor_metadata()) {
+    // Metadata is not applicable, so just log an unknown decision.
+    last_optimization_guide_prediction_->decision =
+        optimization_guide::OptimizationGuideDecision::kUnknown;
     return;
+  }
 
   PreconnectPrediction prediction;
   url::Origin main_frame_origin =

chrome/browser/predictors/loading_predictor_tab_helper_unittest.cc

@@ -5,11 +5,13 @@
 #include "chrome/browser/predictors/loading_predictor_tab_helper.h"
 
 #include "base/test/gmock_callback_support.h"
+#include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "chrome/browser/optimization_guide/optimization_guide_keyed_service.h"
 #include "chrome/browser/optimization_guide/optimization_guide_keyed_service_factory.h"
 #include "chrome/browser/predictors/loading_predictor.h"
 #include "chrome/browser/predictors/loading_test_util.h"
+#include "chrome/browser/predictors/predictors_enums.h"
 #include "chrome/browser/predictors/predictors_features.h"
 #include "chrome/browser/sessions/session_tab_helper_factory.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
@@ -26,10 +28,12 @@ using ::testing::_;
 using ::testing::ByRef;
 using ::testing::DoAll;
 using ::testing::Eq;
+using ::testing::Invoke;
 using ::testing::Mock;
 using ::testing::NiceMock;
 using ::testing::SaveArg;
 using ::testing::StrictMock;
+using ::testing::WithArg;
 
 namespace predictors {
 
@@ -105,7 +109,6 @@ void LoadingPredictorTabHelperTest::SetUp() {
                     return std::make_unique<MockOptimizationGuideKeyedService>(
                         context);
                   })));
-
   LoadingPredictorTabHelper::CreateForWebContents(web_contents());
   tab_helper_ = LoadingPredictorTabHelper::FromWebContents(web_contents());
 
@@ -342,6 +345,8 @@ class LoadingPredictorTabHelperOptimizationGuideDeciderTest
 // id and optimization guide prediction.
 TEST_F(LoadingPredictorTabHelperOptimizationGuideDeciderTest,
        DocumentOnLoadCompletedOptimizationGuide) {
+  base::HistogramTester histogram_tester;
+
   optimization_guide::OptimizationMetadata optimization_metadata;
   optimization_guide::proto::LoadingPredictorMetadata lp_metadata;
   lp_metadata.add_subresources()->set_url("http://test.org/resource1");
@@ -383,12 +388,141 @@ TEST_F(LoadingPredictorTabHelperOptimizationGuideDeciderTest,
   EXPECT_CALL(*mock_collector_,
               RecordMainFrameLoadComplete(navigation_id, prediction));
   tab_helper_->DocumentOnLoadCompletedInMainFrame();
+
+  histogram_tester.ExpectUniqueSample(
+      "LoadingPredictor.OptimizationHintsReceiveStatus",
+      OptimizationHintsReceiveStatus::kBeforeNavigationFinish, 1);
+}
+
+// Tests that document on load completed is recorded with correct navigation
+// id and optimization guide prediction.
+TEST_F(LoadingPredictorTabHelperOptimizationGuideDeciderTest,
+       DocumentOnLoadCompletedOptimizationGuidePredictionComesAfterCommit) {
+  base::HistogramTester histogram_tester;
+
+  optimization_guide::OptimizationMetadata optimization_metadata;
+  optimization_guide::proto::LoadingPredictorMetadata lp_metadata;
+  lp_metadata.add_subresources()->set_url("http://test.org/resource1");
+  lp_metadata.add_subresources()->set_url("http://other.org/resource2");
+  lp_metadata.add_subresources()->set_url("http://other.org/resource3");
+  optimization_metadata.set_loading_predictor_metadata(lp_metadata);
+  optimization_guide::OptimizationGuideDecisionCallback callback;
+  EXPECT_CALL(
+      *mock_optimization_guide_keyed_service_,
+      CanApplyOptimizationAsync(_, optimization_guide::proto::LOADING_PREDICTOR,
+                                base::test::IsNotNullCallback()))
+      .WillOnce(WithArg<2>(
+          Invoke([&](optimization_guide::OptimizationGuideDecisionCallback
+                         got_callback) -> void {
+            callback = std::move(got_callback);
+          })));
+  NavigateAndCommitInMainFrameAndVerifyMetrics("http://test.org");
+  auto navigation_id =
+      CreateNavigationID(GetTabID(), "http://test.org",
+                         web_contents()->GetLastCommittedSourceId());
+
+  // Invoke callback after commit.
+  std::move(callback).Run(optimization_guide::OptimizationGuideDecision::kTrue,
+                          optimization_metadata);
+
+  // Adding subframe navigation to ensure that the committed main frame url will
+  // be used.
+  auto* subframe =
+      content::RenderFrameHostTester::For(main_rfh())->AppendChild("subframe");
+  NavigateAndCommitInFrame("http://sub.test.org", subframe);
+
+  base::Optional<OptimizationGuidePrediction> prediction =
+      OptimizationGuidePrediction();
+  prediction->decision = optimization_guide::OptimizationGuideDecision::kTrue;
+  url::Origin main_frame_origin = url::Origin::Create(GURL("http://test.org"));
+  PreconnectPrediction preconnect_prediction = CreatePreconnectPrediction(
+      "", false,
+      {{url::Origin::Create(GURL("http://test.org")), 1,
+        net::NetworkIsolationKey(main_frame_origin, main_frame_origin)},
+       {url::Origin::Create(GURL("http://other.org")), 1,
+        net::NetworkIsolationKey(main_frame_origin, main_frame_origin)}});
+  prediction->preconnect_prediction = preconnect_prediction;
+  prediction->predicted_subresources = {GURL("http://test.org/resource1"),
+                                        GURL("http://other.org/resource2"),
+                                        GURL("http://other.org/resource3")};
+  EXPECT_CALL(*mock_collector_,
+              RecordMainFrameLoadComplete(navigation_id, prediction));
+  tab_helper_->DocumentOnLoadCompletedInMainFrame();
+
+  // Optimization guide predictions came after commit.
+  histogram_tester.ExpectUniqueSample(
+      "LoadingPredictor.OptimizationHintsReceiveStatus",
+      OptimizationHintsReceiveStatus::kAfterNavigationFinish, 1);
+}
+
+// Tests that an old and new navigation ids are correctly set if a navigation
+// has redirects.
+TEST_F(LoadingPredictorTabHelperOptimizationGuideDeciderTest,
+       DocumentOnLoadCompletedOptimizationGuidePredictionArrivedAfterRedirect) {
+  base::HistogramTester histogram_tester;
+
+  auto navigation = content::NavigationSimulator::CreateRendererInitiated(
+      GURL("http://test.org"), main_rfh());
+  // The problem here is that mock_collector_ is a strict mock, which expects
+  // a particular set of loading events and fails when extra is present.
+  // TOOO(ahemery): Consider refactoring this to rely on loading events
+  // in NavigationSimulator.
+  navigation->SetKeepLoading(true);
+  NavigationID initial_navigation_id;
+  EXPECT_CALL(*mock_collector_, RecordStartNavigation(_))
+      .WillOnce(SaveArg<0>(&initial_navigation_id));
+  optimization_guide::OptimizationMetadata optimization_metadata;
+  optimization_guide::proto::LoadingPredictorMetadata lp_metadata;
+  lp_metadata.add_subresources()->set_url("http://test.org/resource1");
+  lp_metadata.add_subresources()->set_url("http://other.org/resource2");
+  lp_metadata.add_subresources()->set_url("http://other.org/resource3");
+  optimization_metadata.set_loading_predictor_metadata(lp_metadata);
+  optimization_guide::OptimizationGuideDecisionCallback callback;
+  EXPECT_CALL(
+      *mock_optimization_guide_keyed_service_,
+      CanApplyOptimizationAsync(_, optimization_guide::proto::LOADING_PREDICTOR,
+                                base::test::IsNotNullCallback()))
+      .WillOnce(WithArg<2>(
+          Invoke([&](optimization_guide::OptimizationGuideDecisionCallback
+                         got_callback) -> void {
+            callback = std::move(got_callback);
+          })));
+  navigation->Start();
+  navigation->Redirect(GURL("http://test2.org"));
+  navigation->Redirect(GURL("http://test3.org"));
+
+  std::move(callback).Run(optimization_guide::OptimizationGuideDecision::kTrue,
+                          optimization_metadata);
+
+  NavigationID new_navigation_id;
+  EXPECT_CALL(*mock_collector_,
+              RecordFinishNavigation(initial_navigation_id, _,
+                                     /* is_error_page */ false))
+      .WillOnce(SaveArg<1>(&new_navigation_id));
+  navigation->Commit();
+
+  // Prediction decision should be unknown since what came in was for the wrong
+  // navigation ID.
+  base::Optional<OptimizationGuidePrediction> optimization_guide_prediction =
+      OptimizationGuidePrediction();
+  optimization_guide_prediction->decision =
+      optimization_guide::OptimizationGuideDecision::kUnknown;
+  EXPECT_CALL(*mock_collector_,
+              RecordMainFrameLoadComplete(new_navigation_id,
+                                          optimization_guide_prediction));
+  tab_helper_->DocumentOnLoadCompletedInMainFrame();
+
+  histogram_tester.ExpectUniqueSample(
+      "LoadingPredictor.OptimizationHintsReceiveStatus",
+      OptimizationHintsReceiveStatus::kAfterRedirectOrNextNavigationStart, 1);
 }
 
 // Tests that document on load completed is recorded with correct navigation
 // id and optimization guide prediction when the prediction has not arrived.
 TEST_F(LoadingPredictorTabHelperOptimizationGuideDeciderTest,
        DocumentOnLoadCompletedOptimizationGuidePredictionHasNotArrived) {
+  base::HistogramTester histogram_tester;
+
   EXPECT_CALL(
       *mock_optimization_guide_keyed_service_,
       CanApplyOptimizationAsync(_, optimization_guide::proto::LOADING_PREDICTOR,
@@ -411,12 +545,18 @@ TEST_F(LoadingPredictorTabHelperOptimizationGuideDeciderTest,
   EXPECT_CALL(*mock_collector_,
               RecordMainFrameLoadComplete(navigation_id, prediction));
   tab_helper_->DocumentOnLoadCompletedInMainFrame();
+
+  // Histogram should not be recorded since prediction did not come back.
+  histogram_tester.ExpectTotalCount(
+      "LoadingPredictor.OptimizationHintsReceiveStatus", 0);
 }
 
 // Tests that document on load completed is recorded with correct navigation
 // id and optimization guide prediction with no prediction..
 TEST_F(LoadingPredictorTabHelperOptimizationGuideDeciderTest,
        DocumentOnLoadCompletedOptimizationGuidePredictionArrivedNoPrediction) {
+  base::HistogramTester histogram_tester;
+
   // The problem here is that mock_collector_ is a strict mock, which expects
   // a particular set of loading events and fails when extra is present.
   // TOOO(ahemery): Consider refactoring this to rely on loading events
@@ -446,6 +586,59 @@ TEST_F(LoadingPredictorTabHelperOptimizationGuideDeciderTest,
   EXPECT_CALL(*mock_collector_,
               RecordMainFrameLoadComplete(navigation_id, prediction));
   tab_helper_->DocumentOnLoadCompletedInMainFrame();
+
+  // Histogram should still be recorded even though no predictions were
+  // returned.
+  histogram_tester.ExpectUniqueSample(
+      "LoadingPredictor.OptimizationHintsReceiveStatus",
+      OptimizationHintsReceiveStatus::kBeforeNavigationFinish, 1);
+}
+
+// Tests that document on load completed is recorded with correct navigation
+// id and optimization guide prediction with no prediction..
+TEST_F(
+    LoadingPredictorTabHelperOptimizationGuideDeciderTest,
+    DocumentOnLoadCompletedOptimizationGuidePredictionArrivedNoLoadingPredictorMetadata) {
+  base::HistogramTester histogram_tester;
+
+  // The problem here is that mock_collector_ is a strict mock, which expects
+  // a particular set of loading events and fails when extra is present.
+  // TOOO(ahemery): Consider refactoring this to rely on loading events
+  // in NavigationSimulator.
+  optimization_guide::OptimizationMetadata optimization_metadata;
+  EXPECT_CALL(
+      *mock_optimization_guide_keyed_service_,
+      CanApplyOptimizationAsync(_, optimization_guide::proto::LOADING_PREDICTOR,
+                                base::test::IsNotNullCallback()))
+      .WillOnce(base::test::RunOnceCallback<2>(
+          optimization_guide::OptimizationGuideDecision::kTrue,
+          ByRef(optimization_metadata)));
+  NavigateAndCommitInMainFrameAndVerifyMetrics("http://test.org");
+  auto navigation_id =
+      CreateNavigationID(GetTabID(), "http://test.org",
+                         web_contents()->GetLastCommittedSourceId());
+
+  // Adding subframe navigation to ensure that the committed main frame url will
+  // be used.
+  auto* subframe =
+      content::RenderFrameHostTester::For(main_rfh())->AppendChild("subframe");
+  NavigateAndCommitInFrame("http://sub.test.org", subframe);
+
+  // Decision should be unknown since we got invalid data.
+  base::Optional<OptimizationGuidePrediction> optimization_guide_prediction =
+      OptimizationGuidePrediction();
+  optimization_guide_prediction->decision =
+      optimization_guide::OptimizationGuideDecision::kUnknown;
+  EXPECT_CALL(*mock_collector_,
+              RecordMainFrameLoadComplete(navigation_id,
+                                          optimization_guide_prediction));
+  tab_helper_->DocumentOnLoadCompletedInMainFrame();
+
+  // Histogram should still be recorded even though no predictions were
+  // returned.
+  histogram_tester.ExpectUniqueSample(
+      "LoadingPredictor.OptimizationHintsReceiveStatus",
+      OptimizationHintsReceiveStatus::kBeforeNavigationFinish, 1);
 }
 
 class TestLoadingDataCollector : public LoadingDataCollector {