Commit af871dc7 authored by Guido Urdaneta's avatar Guido Urdaneta Committed by Commit Bot

Revert "device chooser: de-flake tests"

This reverts commit e4effcd8.

Reason for revert: Suspect of causing consistent failure 

First failure: https://ci.chromium.org/p/chromium/builders/ci/Mac10.11%20Tests/49293

browser_tests on (none) GPU on Mac failed because of:
WebBluetoothTest.NavigateWithChooserCrossOrigin
SerialTest.NavigateWithChooserCrossOrigin
WebUsbTest.NavigateWithChooserCrossOrigin


[ RUN      ] WebBluetoothTest.NavigateWithChooserCrossOrigin
[2504:3843:0415/131012.437247:WARNING:notification_platform_bridge_mac.mm(563)] AlertNotificationService: XPC connection invalidated.
../../chrome/browser/bluetooth/web_bluetooth_browsertest.cc:492: Failure
Value of: waiter->has_shown()
  Actual: false
Expected: true
Stack trace:
0   browser_tests                       0x0000000101095a49 (anonymous namespace)::WebBluetoothTest_NavigateWithChooserCrossOrigin_Test::RunTestOnMainThread() + 617
1   browser_tests                       0x0000000106fff6ee content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() + 718
2   browser_tests                       0x0000000106a5b62a ChromeBrowserMainParts::PreMainMessageLoopRunImpl() + 3994
...

Original change's description:
> device chooser: de-flake tests
> 
> Right now, the count of extant bubbles is used to keep track of
> how many bubbles exist. This leads to flaky tests though: the bubble's
> view is only deallocated after closing the bubble finishes, and since
> closing the bubble is async, whether the view has been deallocated or
> not at the end of the test is variable.
> 
> This change has the tests instead ensure that the close process has
> *started* at the end of the test, which is always true.
> 
> Bug: 1069695
> Change-Id: I29535149fd3f318c22b44d8f93d22c5b248816da
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2149685
> Reviewed-by: Reilly Grant <reillyg@chromium.org>
> Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#759330}

TBR=ellyjones@chromium.org,reillyg@chromium.org

Change-Id: I380bb34779c4623775826d02ce6bd10a7d1c6870
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1069695
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2151507Reviewed-by: default avatarGuido Urdaneta <guidou@chromium.org>
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759554}
parent 9c8b766c
...@@ -13,7 +13,6 @@ ...@@ -13,7 +13,6 @@
#include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_commands.h" #include "chrome/browser/ui/browser_commands.h"
#include "chrome/browser/ui/browser_dialogs.h" #include "chrome/browser/ui/browser_dialogs.h"
#include "chrome/browser/ui/chooser_bubble_testapi.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h" #include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/test/base/in_process_browser_test.h" #include "chrome/test/base/in_process_browser_test.h"
#include "chrome/test/base/ui_test_utils.h" #include "chrome/test/base/ui_test_utils.h"
...@@ -481,17 +480,13 @@ IN_PROC_BROWSER_TEST_F(WebBluetoothTest, NavigateWithChooserCrossOrigin) { ...@@ -481,17 +480,13 @@ IN_PROC_BROWSER_TEST_F(WebBluetoothTest, NavigateWithChooserCrossOrigin) {
web_contents_, 1 /* number_of_navigations */, web_contents_, 1 /* number_of_navigations */,
content::MessageLoopRunner::QuitMode::DEFERRED); content::MessageLoopRunner::QuitMode::DEFERRED);
auto waiter = test::ChooserBubbleUiWaiter::Create();
EXPECT_TRUE(content::ExecuteScript( EXPECT_TRUE(content::ExecuteScript(
web_contents_, web_contents_,
"navigator.bluetooth.requestDevice({filters: [{name: 'Hello'}]});" "navigator.bluetooth.requestDevice({filters: [{name: 'Hello'}]});"
"document.location.href = \"https://google.com\";")); "document.location.href = \"https://google.com\";"));
observer.Wait(); observer.Wait();
EXPECT_TRUE(waiter->has_shown()); EXPECT_FALSE(chrome::IsDeviceChooserShowingForTesting(browser()));
waiter->WaitForClose();
EXPECT_TRUE(waiter->has_closed());
EXPECT_EQ(GURL("https://google.com"), web_contents_->GetLastCommittedURL()); EXPECT_EQ(GURL("https://google.com"), web_contents_->GetLastCommittedURL());
} }
......
...@@ -7,7 +7,6 @@ ...@@ -7,7 +7,6 @@
#include "chrome/browser/serial/serial_chooser_context_factory.h" #include "chrome/browser/serial/serial_chooser_context_factory.h"
#include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_dialogs.h" #include "chrome/browser/ui/browser_dialogs.h"
#include "chrome/browser/ui/chooser_bubble_testapi.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h" #include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/test/base/in_process_browser_test.h" #include "chrome/test/base/in_process_browser_test.h"
#include "chrome/test/base/ui_test_utils.h" #include "chrome/test/base/ui_test_utils.h"
...@@ -59,16 +58,12 @@ IN_PROC_BROWSER_TEST_F(SerialTest, NavigateWithChooserCrossOrigin) { ...@@ -59,16 +58,12 @@ IN_PROC_BROWSER_TEST_F(SerialTest, NavigateWithChooserCrossOrigin) {
web_contents, 1 /* number_of_navigations */, web_contents, 1 /* number_of_navigations */,
content::MessageLoopRunner::QuitMode::DEFERRED); content::MessageLoopRunner::QuitMode::DEFERRED);
auto waiter = test::ChooserBubbleUiWaiter::Create();
EXPECT_TRUE(content::ExecJs(web_contents, EXPECT_TRUE(content::ExecJs(web_contents,
R"(navigator.serial.requestPort({}); R"(navigator.serial.requestPort({});
document.location.href = "https://google.com";)")); document.location.href = "https://google.com";)"));
observer.Wait(); observer.Wait();
EXPECT_TRUE(waiter->has_shown()); EXPECT_FALSE(chrome::IsDeviceChooserShowingForTesting(browser()));
waiter->WaitForClose();
EXPECT_TRUE(waiter->has_closed());
} }
IN_PROC_BROWSER_TEST_F(SerialTest, RemovePort) { IN_PROC_BROWSER_TEST_F(SerialTest, RemovePort) {
......
...@@ -21,6 +21,10 @@ base::OnceClosure ShowDeviceChooserDialog( ...@@ -21,6 +21,10 @@ base::OnceClosure ShowDeviceChooserDialog(
NOTIMPLEMENTED(); NOTIMPLEMENTED();
return base::DoNothing(); return base::DoNothing();
} }
bool IsDeviceChooserShowingForTesting() {
NOTIMPLEMENTED();
return false;
}
#endif #endif
} // namespace chrome } // namespace chrome
......
...@@ -325,6 +325,7 @@ void ShowExtensionInstallBlockedDialog( ...@@ -325,6 +325,7 @@ void ShowExtensionInstallBlockedDialog(
base::OnceClosure ShowDeviceChooserDialog( base::OnceClosure ShowDeviceChooserDialog(
content::RenderFrameHost* owner, content::RenderFrameHost* owner,
std::unique_ptr<ChooserController> controller); std::unique_ptr<ChooserController> controller);
bool IsDeviceChooserShowingForTesting(Browser* browser);
} // namespace chrome } // namespace chrome
......
// Copyright 2020 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.
#ifndef CHROME_BROWSER_UI_CHOOSER_BUBBLE_TESTAPI_H_
#define CHROME_BROWSER_UI_CHOOSER_BUBBLE_TESTAPI_H_
#include <memory>
namespace test {
// A ChooserBubbleUiWaiter allows waiting for a device chooser bubble to be
// closed, and keeps track of whether such a bubble has been shown or closed
// yet.
class ChooserBubbleUiWaiter {
public:
virtual ~ChooserBubbleUiWaiter() {}
// Creates an instance of this class appropriate for the current platform.
static std::unique_ptr<ChooserBubbleUiWaiter> Create();
bool has_shown() { return has_shown_; }
bool has_closed() { return has_closed_; }
virtual void WaitForClose() = 0;
protected:
bool has_shown_ = false;
bool has_closed_ = false;
};
} // namespace test
#endif // CHROME_BROWSER_UI_CHOOSER_BUBBLE_TESTAPI_H_
// Copyright 2020 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 "base/test/bind_test_util.h"
#include "chrome/browser/ui/chooser_bubble_testapi.h"
#include "ui/views/widget/any_widget_observer.h"
#include "ui/views/widget/widget.h"
namespace test {
namespace {
const char* kViewClassName = "ChooserBubbleUiViewDelegate";
class ChooserBubbleUiWaiterViews : public ChooserBubbleUiWaiter {
public:
ChooserBubbleUiWaiterViews()
: observer_(views::test::AnyWidgetTestPasskey{}) {
observer_.set_shown_callback(
base::BindLambdaForTesting([&](views::Widget* widget) {
if (widget->GetName() == kViewClassName)
has_shown_ = true;
}));
observer_.set_closing_callback(
base::BindLambdaForTesting([&](views::Widget* widget) {
if (widget->GetName() == kViewClassName) {
has_closed_ = true;
run_loop_.Quit();
}
}));
}
void WaitForClose() override { run_loop_.Run(); }
private:
base::RunLoop run_loop_;
views::AnyWidgetObserver observer_;
};
} // namespace
// static
std::unique_ptr<ChooserBubbleUiWaiter> ChooserBubbleUiWaiter::Create() {
return std::make_unique<ChooserBubbleUiWaiterViews>();
}
} // namespace test
...@@ -39,8 +39,6 @@ gfx::Rect GetChooserAnchorRect(Browser* browser) { ...@@ -39,8 +39,6 @@ gfx::Rect GetChooserAnchorRect(Browser* browser) {
class ChooserBubbleUiViewDelegate : public LocationBarBubbleDelegateView, class ChooserBubbleUiViewDelegate : public LocationBarBubbleDelegateView,
public views::TableViewObserver { public views::TableViewObserver {
public: public:
METADATA_HEADER(ChooserBubbleUiViewDelegate);
ChooserBubbleUiViewDelegate( ChooserBubbleUiViewDelegate(
Browser* browser, Browser* browser,
content::WebContents* web_contents, content::WebContents* web_contents,
...@@ -69,6 +67,8 @@ class ChooserBubbleUiViewDelegate : public LocationBarBubbleDelegateView, ...@@ -69,6 +67,8 @@ class ChooserBubbleUiViewDelegate : public LocationBarBubbleDelegateView,
base::OnceClosure MakeCloseClosure(); base::OnceClosure MakeCloseClosure();
void Close(); void Close();
static int g_num_instances_for_testing_;
private: private:
DeviceChooserContentView* device_chooser_content_view_ = nullptr; DeviceChooserContentView* device_chooser_content_view_ = nullptr;
...@@ -77,6 +77,8 @@ class ChooserBubbleUiViewDelegate : public LocationBarBubbleDelegateView, ...@@ -77,6 +77,8 @@ class ChooserBubbleUiViewDelegate : public LocationBarBubbleDelegateView,
DISALLOW_COPY_AND_ASSIGN(ChooserBubbleUiViewDelegate); DISALLOW_COPY_AND_ASSIGN(ChooserBubbleUiViewDelegate);
}; };
int ChooserBubbleUiViewDelegate::g_num_instances_for_testing_ = 0;
ChooserBubbleUiViewDelegate::ChooserBubbleUiViewDelegate( ChooserBubbleUiViewDelegate::ChooserBubbleUiViewDelegate(
Browser* browser, Browser* browser,
content::WebContents* contents, content::WebContents* contents,
...@@ -84,6 +86,7 @@ ChooserBubbleUiViewDelegate::ChooserBubbleUiViewDelegate( ...@@ -84,6 +86,7 @@ ChooserBubbleUiViewDelegate::ChooserBubbleUiViewDelegate(
: LocationBarBubbleDelegateView( : LocationBarBubbleDelegateView(
GetChooserAnchorConfiguration(browser).anchor_view, GetChooserAnchorConfiguration(browser).anchor_view,
contents) { contents) {
g_num_instances_for_testing_++;
// ------------------------------------ // ------------------------------------
// | Chooser bubble title | // | Chooser bubble title |
// | -------------------------------- | // | -------------------------------- |
...@@ -124,7 +127,9 @@ ChooserBubbleUiViewDelegate::ChooserBubbleUiViewDelegate( ...@@ -124,7 +127,9 @@ ChooserBubbleUiViewDelegate::ChooserBubbleUiViewDelegate(
chrome::RecordDialogCreation(chrome::DialogIdentifier::CHOOSER_UI); chrome::RecordDialogCreation(chrome::DialogIdentifier::CHOOSER_UI);
} }
ChooserBubbleUiViewDelegate::~ChooserBubbleUiViewDelegate() = default; ChooserBubbleUiViewDelegate::~ChooserBubbleUiViewDelegate() {
g_num_instances_for_testing_--;
}
void ChooserBubbleUiViewDelegate::AddedToWidget() { void ChooserBubbleUiViewDelegate::AddedToWidget() {
GetBubbleFrameView()->SetTitleView(CreateTitleOriginLabel(GetWindowTitle())); GetBubbleFrameView()->SetTitleView(CreateTitleOriginLabel(GetWindowTitle()));
...@@ -170,10 +175,6 @@ void ChooserBubbleUiViewDelegate::Close() { ...@@ -170,10 +175,6 @@ void ChooserBubbleUiViewDelegate::Close() {
GetWidget()->CloseWithReason(views::Widget::ClosedReason::kUnspecified); GetWidget()->CloseWithReason(views::Widget::ClosedReason::kUnspecified);
} }
BEGIN_METADATA(ChooserBubbleUiViewDelegate)
METADATA_PARENT_CLASS(LocationBarBubbleDelegateView)
END_METADATA()
namespace chrome { namespace chrome {
base::OnceClosure ShowDeviceChooserDialog( base::OnceClosure ShowDeviceChooserDialog(
...@@ -205,4 +206,8 @@ base::OnceClosure ShowDeviceChooserDialog( ...@@ -205,4 +206,8 @@ base::OnceClosure ShowDeviceChooserDialog(
return close_closure; return close_closure;
} }
bool IsDeviceChooserShowingForTesting(Browser* browser) {
return ChooserBubbleUiViewDelegate::g_num_instances_for_testing_ > 0;
}
} // namespace chrome } // namespace chrome
...@@ -13,7 +13,6 @@ ...@@ -13,7 +13,6 @@
#include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_dialogs.h" #include "chrome/browser/ui/browser_dialogs.h"
#include "chrome/browser/ui/chooser_bubble_testapi.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h" #include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/browser/usb/usb_chooser_context_factory.h" #include "chrome/browser/usb/usb_chooser_context_factory.h"
#include "chrome/browser/usb/usb_chooser_controller.h" #include "chrome/browser/usb/usb_chooser_controller.h"
...@@ -297,7 +296,8 @@ IN_PROC_BROWSER_TEST_F(WebUsbTest, AddRemoveDeviceEphemeral) { ...@@ -297,7 +296,8 @@ IN_PROC_BROWSER_TEST_F(WebUsbTest, AddRemoveDeviceEphemeral) {
EXPECT_EQ("", content::EvalJs(web_contents, "removedPromise")); EXPECT_EQ("", content::EvalJs(web_contents, "removedPromise"));
} }
IN_PROC_BROWSER_TEST_F(WebUsbTest, NavigateWithChooserCrossOrigin) { // TODO(https://crbug.com/1069695): This is flaky on Linux, Mac, and Win.
IN_PROC_BROWSER_TEST_F(WebUsbTest, DISABLED_NavigateWithChooserCrossOrigin) {
UseRealChooser(); UseRealChooser();
content::WebContents* web_contents = content::WebContents* web_contents =
browser()->tab_strip_model()->GetActiveWebContents(); browser()->tab_strip_model()->GetActiveWebContents();
...@@ -306,8 +306,6 @@ IN_PROC_BROWSER_TEST_F(WebUsbTest, NavigateWithChooserCrossOrigin) { ...@@ -306,8 +306,6 @@ IN_PROC_BROWSER_TEST_F(WebUsbTest, NavigateWithChooserCrossOrigin) {
web_contents, 1 /* number_of_navigations */, web_contents, 1 /* number_of_navigations */,
content::MessageLoopRunner::QuitMode::DEFERRED); content::MessageLoopRunner::QuitMode::DEFERRED);
auto waiter = test::ChooserBubbleUiWaiter::Create();
EXPECT_TRUE(content::ExecJs(web_contents, EXPECT_TRUE(content::ExecJs(web_contents,
R"( R"(
navigator.usb.requestDevice({ filters: [] }); navigator.usb.requestDevice({ filters: [] });
...@@ -315,9 +313,7 @@ IN_PROC_BROWSER_TEST_F(WebUsbTest, NavigateWithChooserCrossOrigin) { ...@@ -315,9 +313,7 @@ IN_PROC_BROWSER_TEST_F(WebUsbTest, NavigateWithChooserCrossOrigin) {
)")); )"));
observer.Wait(); observer.Wait();
EXPECT_TRUE(waiter->has_shown()); EXPECT_FALSE(chrome::IsDeviceChooserShowingForTesting(browser()));
waiter->WaitForClose();
EXPECT_TRUE(waiter->has_closed());
} }
} // namespace } // namespace
...@@ -337,8 +337,6 @@ static_library("test_support") { ...@@ -337,8 +337,6 @@ static_library("test_support") {
sources += [ sources += [
"../browser/autofill/autofill_uitest_util.cc", "../browser/autofill/autofill_uitest_util.cc",
"../browser/autofill/autofill_uitest_util.h", "../browser/autofill/autofill_uitest_util.h",
"../browser/ui/chooser_bubble_testapi.h",
"../browser/ui/views/chooser_bubble_testapi_views.cc",
"../browser/ui/views/desktop_capture/desktop_media_picker_views_test_api.cc", "../browser/ui/views/desktop_capture/desktop_media_picker_views_test_api.cc",
"../browser/ui/views/desktop_capture/desktop_media_picker_views_test_api.h", "../browser/ui/views/desktop_capture/desktop_media_picker_views_test_api.h",
"../browser/ui/views/media_router/app_menu_test_api.h", "../browser/ui/views/media_router/app_menu_test_api.h",
...@@ -1175,7 +1173,6 @@ if (!is_android) { ...@@ -1175,7 +1173,6 @@ if (!is_android) {
"../browser/ui/views/sharing/sharing_browsertest.cc", "../browser/ui/views/sharing/sharing_browsertest.cc",
"../browser/ui/views/sharing/sharing_browsertest.h", "../browser/ui/views/sharing/sharing_browsertest.h",
"../browser/ui/views/tabs/tab_strip_browsertest.cc", "../browser/ui/views/tabs/tab_strip_browsertest.cc",
"../browser/usb/usb_browsertest.cc",
"../browser/wake_lock/wake_lock_browsertest.cc", "../browser/wake_lock/wake_lock_browsertest.cc",
"../browser/web_components_browsertest.cc", "../browser/web_components_browsertest.cc",
...@@ -1377,6 +1374,7 @@ if (!is_android) { ...@@ -1377,6 +1374,7 @@ if (!is_android) {
"../browser/ui/webui/webui_webview_browsertest.cc", "../browser/ui/webui/webui_webview_browsertest.cc",
"../browser/ui/zoom/zoom_controller_browsertest.cc", "../browser/ui/zoom/zoom_controller_browsertest.cc",
"../browser/unload_browsertest.cc", "../browser/unload_browsertest.cc",
"../browser/usb/usb_browsertest.cc",
"../browser/webauthn/authenticator_extension_browsertest.cc", "../browser/webauthn/authenticator_extension_browsertest.cc",
"../common/mac/app_mode_chrome_locator_browsertest.mm", "../common/mac/app_mode_chrome_locator_browsertest.mm",
"../common/mac/mock_launchd.h", "../common/mac/mock_launchd.h",
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment