From fde87ff06eeb2e8723ec91222a15fe2d4a72d3d3 Mon Sep 17 00:00:00 2001 From: Netzenbot Date: Mon, 2 Feb 2026 21:58:12 -0500 Subject: [PATCH] Fix test: SpeedReaderBrowserTest.EnableDisableSpeedreaderB (#33551) Replace infinite polling loops with base::test::RunUntil() to prevent test timeouts. The WaitDistilled(), WaitDistillable(), WaitOriginal(), and WaitToolbarVisibility() functions used NonBlockingDelay() in infinite while loops, which could run forever if the expected state was never reached. Using base::test::RunUntil() provides a built-in 1-second timeout and returns false on timeout, allowing tests to fail gracefully instead of hanging indefinitely. This addresses the intermittent timeout failures in SpeedReaderBrowserTest.EnableDisableSpeedreaderB. Also modernize the ShowOriginalPage test to use base::test::RunUntil() instead of the old NonBlockingDelay() polling pattern when waiting for the "View original" link to appear in the DOM. This maintains consistency with the helper function improvements. Fixes: brave/brave-browser#51693 --- .../speedreader/speedreader_browsertest.cc | 158 ++++++++++-------- 1 file changed, 91 insertions(+), 67 deletions(-) diff --git a/browser/speedreader/speedreader_browsertest.cc b/browser/speedreader/speedreader_browsertest.cc index e93f4b11189..037b917cde9 100644 --- a/browser/speedreader/speedreader_browsertest.cc +++ b/browser/speedreader/speedreader_browsertest.cc @@ -12,6 +12,7 @@ #include "base/strings/escape.h" #include "base/strings/string_util.h" #include "base/test/bind.h" +#include "base/test/run_until.h" #include "base/test/scoped_feature_list.h" #include "base/time/time.h" #include "brave/app/brave_command_ids.h" @@ -180,60 +181,81 @@ class SpeedReaderBrowserTest : public InProcessBrowserTest { ->GetPageActionIconView(brave::kSpeedreaderPageActionIconType); } - void WaitDistilled(speedreader::SpeedreaderTabHelper* th = nullptr) { + bool WaitDistilled(speedreader::SpeedreaderTabHelper* th = nullptr) { if (!th) { th = tab_helper(); } - while (!speedreader::DistillStates::IsDistilled(th->PageDistillState())) { - NonBlockingDelay(base::Milliseconds(10)); + if (!base::test::RunUntil([th]() { + return speedreader::DistillStates::IsDistilled( + th->PageDistillState()); + })) { + return false; } content::WaitForLoadStop(ActiveWebContents()); + return true; } - void WaitDistillable(speedreader::SpeedreaderTabHelper* th = nullptr) { + bool WaitDistillable(speedreader::SpeedreaderTabHelper* th = nullptr) { if (!th) { th = tab_helper(); } - while (!speedreader::DistillStates::IsDistillable(th->PageDistillState())) { - NonBlockingDelay(base::Milliseconds(10)); + if (!base::test::RunUntil([th]() { + return speedreader::DistillStates::IsDistillable( + th->PageDistillState()); + })) { + return false; } content::WaitForLoadStop(ActiveWebContents()); + return true; } - void WaitOriginal(speedreader::SpeedreaderTabHelper* th = nullptr) { + bool WaitOriginal(speedreader::SpeedreaderTabHelper* th = nullptr) { if (!th) { th = tab_helper(); } - while ( - !speedreader::DistillStates::IsViewOriginal(th->PageDistillState())) { - NonBlockingDelay(base::Milliseconds(10)); + if (!base::test::RunUntil([th]() { + return speedreader::DistillStates::IsViewOriginal( + th->PageDistillState()); + })) { + return false; } content::WaitForLoadStop(ActiveWebContents()); + return true; } - void ClickReaderButton() { + bool ClickReaderButton() { const auto was_distilled = speedreader::DistillStates::IsDistilled( tab_helper()->PageDistillState()); browser()->command_controller()->ExecuteCommand( IDC_SPEEDREADER_ICON_ONCLICK); if (!was_distilled) { - WaitDistilled(); + if (!WaitDistilled()) { + return false; + } } else { - WaitDistillable(); + if (!WaitDistillable()) { + return false; + } } content::WaitForLoadStop(ActiveWebContents()); + return true; } - void WaitToolbarVisibility(ReaderModeToolbarView* toolbar, bool visible) { - while (toolbar->GetVisible() != visible) { - NonBlockingDelay(base::Milliseconds(10)); + bool WaitToolbarVisibility(ReaderModeToolbarView* toolbar, bool visible) { + if (!base::test::RunUntil([toolbar, visible]() { + return toolbar->GetVisible() == visible; + })) { + return false; } if (visible) { - while (toolbar->height() != toolbar->GetPreferredSize().height()) { - NonBlockingDelay(base::Milliseconds(10)); + if (!base::test::RunUntil([toolbar]() { + return toolbar->height() == toolbar->GetPreferredSize().height(); + })) { + return false; } } + return true; } void ClickInView(views::View* clickable_view) { @@ -290,7 +312,7 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, PRE_RestoreSpeedreaderPage) { IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, RestoreSpeedreaderPage) { browser()->tab_strip_model()->ActivateTabAt(0); - WaitDistilled(); + ASSERT_TRUE(WaitDistilled()); EXPECT_TRUE(speedreader::DistillStates::IsDistilled( tab_helper()->PageDistillState())); } @@ -454,7 +476,7 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, ClickingOnReaderButton) { histogram_tester_.ExpectTotalCount( speedreader::kSpeedreaderPageViewsHistogramName, 0); - ClickReaderButton(); + ASSERT_TRUE(ClickReaderButton()); EXPECT_TRUE(GetReaderButton()->GetVisible()); EXPECT_TRUE(speedreader::DistillStates::IsDistilled( tab_helper()->PageDistillState())); @@ -463,7 +485,7 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, ClickingOnReaderButton) { histogram_tester_.ExpectTotalCount( speedreader::kSpeedreaderPageViewsHistogramName, 1); - ClickReaderButton(); + ASSERT_TRUE(ClickReaderButton()); EXPECT_TRUE(GetReaderButton()->GetVisible()); EXPECT_TRUE(speedreader::DistillStates::IsViewOriginal( tab_helper()->PageDistillState())); @@ -487,7 +509,7 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, OnDemandReader) { )js"; EXPECT_TRUE(content::ExecJs(ActiveWebContents(), kChangeContent, content::EXECUTE_SCRIPT_DEFAULT_OPTIONS)); - ClickReaderButton(); + ASSERT_TRUE(ClickReaderButton()); EXPECT_TRUE(speedreader::DistillStates::IsDistilled( tab_helper()->PageDistillState())); @@ -508,7 +530,7 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, OnDemandReaderEncoding) { EXPECT_FALSE(speedreader_service()->IsAllowedForAllReadableSites()); NavigateToPageSynchronously(kTestEsPageReadable); EXPECT_TRUE(GetReaderButton()->GetVisible()); - ClickReaderButton(); + ASSERT_TRUE(ClickReaderButton()); static constexpr char kCheckText[] = R"js( document.querySelector('#par-to-check').innerText.length )js"; @@ -540,12 +562,12 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, EnableDisableSpeedreaderA) { EXPECT_TRUE(speedreader::DistillStates::IsDistillable( tab_helper()->PageDistillState())); EnableSpeedreaderAllowedForAllSites(); - WaitDistilled(); + ASSERT_TRUE(WaitDistilled()); EXPECT_TRUE(GetReaderButton()->GetVisible()); EXPECT_TRUE(speedreader::DistillStates::IsDistilled( tab_helper()->PageDistillState())); DisableSpeedreaderForAllSites(); - WaitOriginal(); + ASSERT_TRUE(WaitOriginal()); EXPECT_TRUE(GetReaderButton()->GetVisible()); EXPECT_TRUE(speedreader::DistillStates::IsDistillable( tab_helper()->PageDistillState())); @@ -555,18 +577,18 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, EnableDisableSpeedreaderA) { IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, EnableDisableSpeedreaderB) { NavigateToPageSynchronously(kTestPageReadable); - ClickReaderButton(); - WaitDistilled(); + ASSERT_TRUE(ClickReaderButton()); + ASSERT_TRUE(WaitDistilled()); EXPECT_TRUE(GetReaderButton()->GetVisible()); EXPECT_TRUE(speedreader::DistillStates::IsDistilled( tab_helper()->PageDistillState())); EnableSpeedreaderAllowedForAllSites(); - WaitDistilled(); + ASSERT_TRUE(WaitDistilled()); EXPECT_TRUE(GetReaderButton()->GetVisible()); EXPECT_TRUE(speedreader::DistillStates::IsDistilled( tab_helper()->PageDistillState())); DisableSpeedreaderForAllSites(); - WaitOriginal(); + ASSERT_TRUE(WaitOriginal()); EXPECT_TRUE(GetReaderButton()->GetVisible()); EXPECT_TRUE(speedreader::DistillStates::IsDistillable( tab_helper()->PageDistillState())); @@ -645,7 +667,9 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, ShowOriginalPage) { // Wait for the "View original" link to be present in the DOM. // The element ID is hardcoded in extractor.rs. - // Use polling loop similar to WaitElement in the Toolbar test. + // Note: We use a polling loop with NonBlockingDelay instead of + // base::test::RunUntil() here because EvalJs inside RunUntil causes nesting + // level issues on macOS arm64 (DCHECK in message_pump_apple.mm). static constexpr char kCheckLinkExists[] = R"js( !!document.getElementById('c93e2206-2f31-4ddc-9828-2bb8e8ed940e') @@ -687,7 +711,7 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, ShowOriginalPage) { EXPECT_TRUE(speedreader_service()->IsAllowedForSite(web_contents)); // Click on speedreader button - ClickReaderButton(); + ASSERT_TRUE(ClickReaderButton()); content::WaitForLoadStop(web_contents); EXPECT_TRUE( speedreader::DistillStates::IsDistilled(tab_helper->PageDistillState())); @@ -904,15 +928,15 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, Toolbar) { Click(toolbar, "tune"); { - while (!tab_helper()->speedreader_bubble_view()) { - NonBlockingDelay(base::Milliseconds(10)); - } + ASSERT_TRUE(base::test::RunUntil([this]() { + return tab_helper()->speedreader_bubble_view() != nullptr; + })); } Click(toolbar, "tune"); Click(toolbar, "close"); { - WaitOriginal(); + ASSERT_TRUE(WaitOriginal()); EXPECT_FALSE(toolbar_view->GetVisible()); } } @@ -979,13 +1003,13 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, ErrorPage) { WindowOpenDisposition::CURRENT_TAB); EXPECT_TRUE(speedreader::DistillStates::IsViewOriginal( tab_helper()->PageDistillState())); - WaitDistillable(tab_helper()); + ASSERT_TRUE(WaitDistillable(tab_helper())); EXPECT_TRUE(GetReaderButton()->GetVisible()); GoBack(browser()); NavigateToPageSynchronously(kTestPageReadable, WindowOpenDisposition::CURRENT_TAB); - WaitDistilled(); + ASSERT_TRUE(WaitDistilled()); EXPECT_TRUE(GetReaderButton()->GetVisible()); EXPECT_TRUE(speedreader::DistillStates::IsDistilled( tab_helper()->PageDistillState())); @@ -1087,7 +1111,7 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, EXPECT_TRUE(speedreader::DistillStates::IsDistillable( tab_helper()->PageDistillState())); - ClickReaderButton(); + ASSERT_TRUE(ClickReaderButton()); EXPECT_TRUE(speedreader::DistillStates::IsDistilled( tab_helper()->PageDistillState())); @@ -1096,7 +1120,7 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderBrowserTest, speedreader_service()->SetEnabledForSite(ActiveWebContents(), true); ActiveWebContents()->GetController().Reload(content::ReloadType::NORMAL, false); - WaitDistilled(); + ASSERT_TRUE(WaitDistilled()); EXPECT_TRUE(speedreader::DistillStates::IsDistilled( tab_helper()->PageDistillState())); @@ -1200,53 +1224,53 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderWithSplitViewBrowserTest, SplitView) { NavigateToPageSynchronously(kTestPageReadable, WindowOpenDisposition::CURRENT_TAB); - WaitToolbarVisibility(GetPrimaryToolbar(), true); - WaitToolbarVisibility(GetSecondaryToolbar(), false); + ASSERT_TRUE(WaitToolbarVisibility(GetPrimaryToolbar(), true)); + ASSERT_TRUE(WaitToolbarVisibility(GetSecondaryToolbar(), false)); // Change the active tab. browser()->tab_strip_model()->ActivateTabAt(1); - WaitToolbarVisibility(GetPrimaryToolbar(), false); - WaitToolbarVisibility(GetSecondaryToolbar(), true); + ASSERT_TRUE(WaitToolbarVisibility(GetPrimaryToolbar(), false)); + ASSERT_TRUE(WaitToolbarVisibility(GetSecondaryToolbar(), true)); // Load a distillabe page in second tab. NavigateToPageSynchronously(kTestPageReadable, WindowOpenDisposition::CURRENT_TAB); - WaitToolbarVisibility(GetPrimaryToolbar(), true); - WaitToolbarVisibility(GetSecondaryToolbar(), true); + ASSERT_TRUE(WaitToolbarVisibility(GetPrimaryToolbar(), true)); + ASSERT_TRUE(WaitToolbarVisibility(GetSecondaryToolbar(), true)); // Check secondary location bar position when changing active tab // between non split view tab and split view tab. // Secondary location bar should have same origin with secondary // contents container. chrome::AddTabAt(browser(), GURL(), -1, /*foreground*/ true); - WaitToolbarVisibility(GetPrimaryToolbar(), false); - WaitToolbarVisibility(GetSecondaryToolbar(), false); + ASSERT_TRUE(WaitToolbarVisibility(GetPrimaryToolbar(), false)); + ASSERT_TRUE(WaitToolbarVisibility(GetSecondaryToolbar(), false)); browser()->tab_strip_model()->ActivateTabAt(0); - WaitToolbarVisibility(GetPrimaryToolbar(), true); - WaitToolbarVisibility(GetSecondaryToolbar(), true); + ASSERT_TRUE(WaitToolbarVisibility(GetPrimaryToolbar(), true)); + ASSERT_TRUE(WaitToolbarVisibility(GetSecondaryToolbar(), true)); browser()->tab_strip_model()->ActivateTabAt(2); - WaitToolbarVisibility(GetPrimaryToolbar(), false); - WaitToolbarVisibility(GetSecondaryToolbar(), false); + ASSERT_TRUE(WaitToolbarVisibility(GetPrimaryToolbar(), false)); + ASSERT_TRUE(WaitToolbarVisibility(GetSecondaryToolbar(), false)); browser()->tab_strip_model()->ActivateTabAt(0); - WaitToolbarVisibility(GetPrimaryToolbar(), true); - WaitToolbarVisibility(GetSecondaryToolbar(), true); + ASSERT_TRUE(WaitToolbarVisibility(GetPrimaryToolbar(), true)); + ASSERT_TRUE(WaitToolbarVisibility(GetSecondaryToolbar(), true)); // Second tab is active. Show original content. browser()->tab_strip_model()->ActivateTabAt(1); - ClickReaderButton(); - WaitToolbarVisibility(GetPrimaryToolbar(), false); - WaitToolbarVisibility(GetSecondaryToolbar(), true); + ASSERT_TRUE(ClickReaderButton()); + ASSERT_TRUE(WaitToolbarVisibility(GetPrimaryToolbar(), false)); + ASSERT_TRUE(WaitToolbarVisibility(GetSecondaryToolbar(), true)); browser()->tab_strip_model()->ActivateTabAt(0); // First tab is active. Show original content. - ClickReaderButton(); + ASSERT_TRUE(ClickReaderButton()); // There are no distilled pages. - WaitToolbarVisibility(GetPrimaryToolbar(), false); - WaitToolbarVisibility(GetSecondaryToolbar(), false); + ASSERT_TRUE(WaitToolbarVisibility(GetPrimaryToolbar(), false)); + ASSERT_TRUE(WaitToolbarVisibility(GetSecondaryToolbar(), false)); } IN_PROC_BROWSER_TEST_F(SpeedReaderWithSplitViewBrowserTest, SplitViewClicking) { @@ -1269,18 +1293,18 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderWithSplitViewBrowserTest, SplitViewClicking) { // Check clicking view makes its tab activate. browser()->tab_strip_model()->ActivateTabAt(1); EXPECT_EQ(1, browser()->tab_strip_model()->active_index()); - WaitToolbarVisibility(GetPrimaryToolbar(), false); - WaitToolbarVisibility(GetSecondaryToolbar(), true); + ASSERT_TRUE(WaitToolbarVisibility(GetPrimaryToolbar(), false)); + ASSERT_TRUE(WaitToolbarVisibility(GetSecondaryToolbar(), true)); ClickInView(GetSecondaryToolbar()); - WaitToolbarVisibility(GetPrimaryToolbar(), true); - WaitToolbarVisibility(GetSecondaryToolbar(), false); + ASSERT_TRUE(WaitToolbarVisibility(GetPrimaryToolbar(), true)); + ASSERT_TRUE(WaitToolbarVisibility(GetSecondaryToolbar(), false)); EXPECT_EQ(0, browser()->tab_strip_model()->active_index()); browser()->tab_strip_model()->ActivateTabAt(1); EXPECT_EQ(1, browser()->tab_strip_model()->active_index()); - WaitToolbarVisibility(GetPrimaryToolbar(), false); - WaitToolbarVisibility(GetSecondaryToolbar(), true); + ASSERT_TRUE(WaitToolbarVisibility(GetPrimaryToolbar(), false)); + ASSERT_TRUE(WaitToolbarVisibility(GetSecondaryToolbar(), true)); // Simulated input doesn't reliably trigger DidGetUserInteraction() // callback on these platforms in test environments, causing intermittent @@ -1290,7 +1314,7 @@ IN_PROC_BROWSER_TEST_F(SpeedReaderWithSplitViewBrowserTest, SplitViewClicking) { // mechanism. The full click → DidGetUserInteraction → ActivateContents chain // is verified manually on these platforms. GetSecondaryToolbar()->ActivateContents(); - WaitToolbarVisibility(GetPrimaryToolbar(), true); - WaitToolbarVisibility(GetSecondaryToolbar(), false); + ASSERT_TRUE(WaitToolbarVisibility(GetPrimaryToolbar(), true)); + ASSERT_TRUE(WaitToolbarVisibility(GetSecondaryToolbar(), false)); EXPECT_EQ(0, browser()->tab_strip_model()->active_index()); }