Fix flakiness on VariationsServiceStartsRequestOnNetworkChange

Change VariationsServiceStartsRequestOnNetworkChange to unit test
instead of browser test. Using the mock variations service to perform
the same test.

Bug: 905714
Change-Id: I66c4b6c91ec94933c19218398af9057f736ad736
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1550189Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Auto-Submit: Weilun Shi <sweilun@chromium.org>
Cr-Commit-Position: refs/heads/master@{#648705}

chrome/browser/chrome_browser_main_browsertest.cc

-// Copyright 2018 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "chrome/browser/chrome_browser_main.h"
-
-#include <stddef.h>
-
-#include "base/command_line.h"
-#include "base/macros.h"
-#include "base/run_loop.h"
-#include "base/test/scoped_feature_list.h"
-#include "chrome/browser/browser_process.h"
-#include "chrome/browser/chrome_browser_main_extra_parts.h"
-#include "chrome/test/base/in_process_browser_test.h"
-#include "components/variations/service/variations_service.h"
-#include "components/variations/variations_switches.h"
-#include "content/public/test/network_connection_change_simulator.h"
-
-// Friend of ChromeBrowserMainPartsTestApi to poke at internal state.
-class ChromeBrowserMainPartsTestApi {
- public:
-  explicit ChromeBrowserMainPartsTestApi(ChromeBrowserMainParts* main_parts)
-      : main_parts_(main_parts) {}
-  ~ChromeBrowserMainPartsTestApi() = default;
-
-  void EnableVariationsServiceInit() {
-    main_parts_
-        ->should_call_pre_main_loop_start_startup_on_variations_service_ = true;
-  }
-
- private:
-  ChromeBrowserMainParts* main_parts_;
-  DISALLOW_COPY_AND_ASSIGN(ChromeBrowserMainPartsTestApi);
-};
-
-namespace {
-
-class ChromeBrowserMainBrowserTest : public InProcessBrowserTest {
- public:
-  ChromeBrowserMainBrowserTest() {
-    net::NetworkChangeNotifier::SetTestNotificationsOnly(true);
-    // Since the test currently performs an actual request to localhost (which
-    // is expected to fail since no variations server is running), retries are
-    // disabled to prevent race conditions from causing flakiness in tests.
-    scoped_feature_list_.InitAndDisableFeature(variations::kHttpRetryFeature);
-  }
-
- protected:
-  // InProcessBrowserTest:
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    // Without this (and EnableFetchForTesting() below) VariationsService won't
-    // do requests in non-branded builds.
-    command_line->AppendSwitchASCII(variations::switches::kVariationsServerURL,
-                                    "http://localhost");
-  }
-
-  void CreatedBrowserMainParts(
-      content::BrowserMainParts* browser_main_parts) override {
-    variations::VariationsService::EnableFetchForTesting();
-    ChromeBrowserMainParts* chrome_browser_main_parts =
-        static_cast<ChromeBrowserMainParts*>(browser_main_parts);
-    ChromeBrowserMainPartsTestApi(chrome_browser_main_parts)
-        .EnableVariationsServiceInit();
-  }
-
- private:
-  base::test::ScopedFeatureList scoped_feature_list_;
-
-  DISALLOW_COPY_AND_ASSIGN(ChromeBrowserMainBrowserTest);
-};
-
-// Verifies VariationsService does a request when network status changes from
-// none to connected. This is a regression test for https://crbug.com/826930.
-// TODO(crbug.com/905714): This test should use a mock variations server
-// instead of performing an actual request.
-#if defined(OS_CHROMEOS)
-#define MAYBE_VariationsServiceStartsRequestOnNetworkChange \
-  DISABLED_VariationsServiceStartsRequestOnNetworkChange
-#else
-#define MAYBE_VariationsServiceStartsRequestOnNetworkChange \
-  VariationsServiceStartsRequestOnNetworkChange
-#endif
-IN_PROC_BROWSER_TEST_F(ChromeBrowserMainBrowserTest,
-                       MAYBE_VariationsServiceStartsRequestOnNetworkChange) {
-  variations::VariationsService* variations_service =
-      g_browser_process->variations_service();
-  variations_service->CancelCurrentRequestForTesting();
-
-  content::NetworkConnectionChangeSimulator network_change_simulator;
-  network_change_simulator.SetConnectionType(
-      network::mojom::ConnectionType::CONNECTION_NONE);
-  const int initial_request_count = variations_service->request_count();
-
-  // The variations service will only send a request the first time the
-  // connection goes online, or after the 30min delay. Tell it that it hasn't
-  // sent a request yet to make sure the next time we go online a request will
-  // be sent.
-  variations_service->GetResourceRequestAllowedNotifierForTesting()
-      ->SetObserverRequestedForTesting(true);
-
-  network_change_simulator.SetConnectionType(
-      network::mojom::ConnectionType::CONNECTION_WIFI);
-  // NotifyObserversOfNetworkChangeForTests uses PostTask, so run the loop until
-  // idle to ensure VariationsService processes the network change.
-  base::RunLoop().RunUntilIdle();
-  const int final_request_count = variations_service->request_count();
-  EXPECT_EQ(initial_request_count + 1, final_request_count);
-}
-
-}  // namespace

chrome/test/BUILD.gn

@@ -629,7 +629,6 @@ test("browser_tests") {
       "../browser/browsing_data/counters/passwords_counter_browsertest.cc",
       "../browser/browsing_data/counters/sync_aware_counter_browsertest.cc",
       "../browser/browsing_data/navigation_entry_remover_browsertest.cc",
-      "../browser/chrome_browser_main_browsertest.cc",
       "../browser/chrome_content_browser_client_browsertest.cc",
       "../browser/chrome_content_browser_client_browsertest_chromeos.cc",
       "../browser/chrome_do_not_track_browsertest.cc",

components/variations/service/variations_service.cc

@@ -924,6 +924,11 @@ void VariationsService::CancelCurrentRequestForTesting() {
   pending_seed_request_.reset();
 }
 
+void VariationsService::StartRepeatedVariationsSeedFetchForTesting() {
+  InitResourceRequestedAllowedNotifier();
+  return StartRepeatedVariationsSeedFetch();
+}
+
 std::string VariationsService::GetStoredPermanentCountry() {
   const base::ListValue* list_value =
       local_state_->GetList(prefs::kVariationsPermanentConsistencyCountry);

components/variations/service/variations_service.h

@@ -198,6 +198,9 @@ class VariationsService
   // Cancels the currently pending fetch request.
   void CancelCurrentRequestForTesting();
 
+  // Exposes StartRepeatedVariationsSeedFetch for testing.
+  void StartRepeatedVariationsSeedFetchForTesting();
+
  protected:
   // Starts the fetching process once, where |OnURLFetchComplete| is called with
   // the response. This calls DoFetchToURL with the set url.

components/variations/service/variations_service_unittest.cc

@@ -1040,6 +1040,38 @@ TEST_F(VariationsServiceTest, NullResponseReceivedWithHTTPOk) {
                                       net::ERR_FAILED, 1);
 }
 
+TEST_F(VariationsServiceTest,
+       VariationsServiceStartsRequestOnNetworkChange) {
+  // Verifies VariationsService does a request when network status changes from
+  // none to connected. This is a regression test for https://crbug.com/826930.
+  VariationsService::EnableFetchForTesting();
+  network_tracker_->SetConnectionType(
+      network::mojom::ConnectionType::CONNECTION_NONE);
+  TestVariationsService service(
+      std::make_unique<web_resource::TestRequestAllowedNotifier>(
+          &prefs_, network_tracker_),
+      &prefs_, GetMetricsStateManager(), true);
+  service.set_intercepts_fetch(false);
+  service.CancelCurrentRequestForTesting();
+  base::RunLoop().RunUntilIdle();
+  // Simulate starting Chrome browser.
+  service.StartRepeatedVariationsSeedFetchForTesting();
+  const int initial_request_count = service.request_count();
+  // The variations seed can not be fetched if disconnected. So even we start
+  // repeated variations seed fetch (on Chrome start), no requests will be made.
+  EXPECT_EQ(0, initial_request_count);
+
+  service.GetResourceRequestAllowedNotifierForTesting()
+      ->SetObserverRequestedForTesting(true);
+  network_tracker_->SetConnectionType(
+      network::mojom::ConnectionType::CONNECTION_WIFI);
+  base::RunLoop().RunUntilIdle();
+
+  const int final_request_count = service.request_count();
+  // The request will be made once Chrome gets online.
+  EXPECT_EQ(initial_request_count + 1, final_request_count);
+}
+
 // TODO(isherman): Add an integration test for saving and loading a safe seed,
 // once the loading functionality is implemented on the seed store.