Discharge7443/brave-core
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix test: SpeedReaderBrowserTest.EnableDisableSpeedreaderB (#33551)

Browse Source 
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
This commit is contained in:
Netzenbot
2026-02-02 21:58:12 -05:00
committed by GitHub
parent d8899e430a
commit fde87ff06e
1 changed files with 91 additions and 67 deletions
Download Patch File Download Diff File Expand all files Collapse all files
browser/speedreader/speedreader_browsertest.cc
+91 -67
View File
@@ -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());
}