From debf1f306a4120027415deaf1dbb64f2d2eeb869 Mon Sep 17 00:00:00 2001 From: Ralph Giles Date: Wed, 29 Sep 2021 17:20:00 -0700 Subject: [PATCH 1/3] SearchEngineTracker: Re-enable DefaultSearchEngineP3A test. This was disabled in 27bf9e3b0b97 for intermittent failures. Add a call to wait for TemplateURLServiceFactory to finish loading, and re-enable to see if that stabilizes the browser test. See https://github.com/brave/brave-browser/issues/13057 for tracking. This duplicates VerifyTemplateURLServiceLoad() from the search_engine_provider browser test, but it's a small piece of code and having two file-local implementations is simple enough. --- .../search_engine_tracker_browsertest.cc | 22 ++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/browser/search_engines/search_engine_tracker_browsertest.cc b/browser/search_engines/search_engine_tracker_browsertest.cc index 0f182197f734..2e26fa866c05 100644 --- a/browser/search_engines/search_engine_tracker_browsertest.cc +++ b/browser/search_engines/search_engine_tracker_browsertest.cc @@ -13,6 +13,7 @@ #include "chrome/browser/search_engines/template_url_service_factory.h" #include "chrome/browser/ui/browser.h" #include "chrome/test/base/in_process_browser_test.h" +#include "chrome/test/base/search_test_utils.h" #include "components/search_engines/template_url_prepopulate_data.h" #include "content/public/test/browser_test.h" @@ -26,14 +27,29 @@ class SearchEngineProviderP3ATest : public InProcessBrowserTest { std::unique_ptr histogram_tester_; }; +testing::AssertionResult VerifyTemplateURLServiceLoad( + TemplateURLService* service) { + if (service->loaded()) { + return testing::AssertionSuccess(); + } + search_test_utils::WaitForTemplateURLServiceToLoad(service); + if (service->loaded()) { + return testing::AssertionSuccess(); + } + return testing::AssertionFailure() << "TemplateURLService isn't loaded"; +} + IN_PROC_BROWSER_TEST_F(SearchEngineProviderP3ATest, - DISABLED_DefaultSearchEngineP3A) { + DefaultSearchEngineP3A) { // Check that the metric is reported on startup. histogram_tester_->ExpectUniqueSample(kDefaultSearchEngineMetric, SearchEngineP3A::kGoogle, 1); - auto* service = TemplateURLServiceFactory::GetForProfile( - browser()->profile()); + auto* service = + TemplateURLServiceFactory::GetForProfile(browser()->profile()); + + // Make sure the service is initialized. + EXPECT_TRUE(VerifyTemplateURLServiceLoad(service)); // Check that changing the default engine triggers emitting of a new value. auto ddg_data = TemplateURLPrepopulateData::GetPrepopulatedEngine( From 7ec491d398aa6846f52ec92bb4ac97f83989e040 Mon Sep 17 00:00:00 2001 From: Ralph Giles Date: Thu, 30 Sep 2021 11:54:14 -0700 Subject: [PATCH 2/3] SearchEngineTracker: Run clang-format. Address a style lint. --- browser/search_engines/search_engine_tracker_browsertest.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/browser/search_engines/search_engine_tracker_browsertest.cc b/browser/search_engines/search_engine_tracker_browsertest.cc index 2e26fa866c05..a23978ae974d 100644 --- a/browser/search_engines/search_engine_tracker_browsertest.cc +++ b/browser/search_engines/search_engine_tracker_browsertest.cc @@ -39,8 +39,7 @@ testing::AssertionResult VerifyTemplateURLServiceLoad( return testing::AssertionFailure() << "TemplateURLService isn't loaded"; } -IN_PROC_BROWSER_TEST_F(SearchEngineProviderP3ATest, - DefaultSearchEngineP3A) { +IN_PROC_BROWSER_TEST_F(SearchEngineProviderP3ATest, DefaultSearchEngineP3A) { // Check that the metric is reported on startup. histogram_tester_->ExpectUniqueSample(kDefaultSearchEngineMetric, SearchEngineP3A::kGoogle, 1); From 91feae6c8fe525b4d6a6524978fde8ca81dbb6f6 Mon Sep 17 00:00:00 2001 From: Ralph Giles Date: Fri, 1 Oct 2021 09:56:43 -0700 Subject: [PATCH 3/3] SearchEngineTracker: Simplify service wait in test. WaitForTemplateURLServiceLoad() already checks service->loaded() and asserts if the notification callback completes without changing the state, so the wrapper is redundant except for the better test failure message. Addresses a review comment from Ivan Efremov. --- .../search_engine_tracker_browsertest.cc | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/browser/search_engines/search_engine_tracker_browsertest.cc b/browser/search_engines/search_engine_tracker_browsertest.cc index a23978ae974d..8c62f17ccec4 100644 --- a/browser/search_engines/search_engine_tracker_browsertest.cc +++ b/browser/search_engines/search_engine_tracker_browsertest.cc @@ -27,18 +27,6 @@ class SearchEngineProviderP3ATest : public InProcessBrowserTest { std::unique_ptr histogram_tester_; }; -testing::AssertionResult VerifyTemplateURLServiceLoad( - TemplateURLService* service) { - if (service->loaded()) { - return testing::AssertionSuccess(); - } - search_test_utils::WaitForTemplateURLServiceToLoad(service); - if (service->loaded()) { - return testing::AssertionSuccess(); - } - return testing::AssertionFailure() << "TemplateURLService isn't loaded"; -} - IN_PROC_BROWSER_TEST_F(SearchEngineProviderP3ATest, DefaultSearchEngineP3A) { // Check that the metric is reported on startup. histogram_tester_->ExpectUniqueSample(kDefaultSearchEngineMetric, @@ -46,9 +34,7 @@ IN_PROC_BROWSER_TEST_F(SearchEngineProviderP3ATest, DefaultSearchEngineP3A) { auto* service = TemplateURLServiceFactory::GetForProfile(browser()->profile()); - - // Make sure the service is initialized. - EXPECT_TRUE(VerifyTemplateURLServiceLoad(service)); + search_test_utils::WaitForTemplateURLServiceToLoad(service); // Check that changing the default engine triggers emitting of a new value. auto ddg_data = TemplateURLPrepopulateData::GetPrepopulatedEngine(