Commit 1b9dce9a authored by David Bienvenu's avatar David Bienvenu Committed by Chromium LUCI CQ

Reland "Fix flakiness in ChromeLabsButtonTest.ShowAndHideChromeLabsBubbleOnPress"

This reverts commit 12c756d2.

Reason for revert: <fix compile failure caused by racy submit>

Original change's description:
> Revert "Fix flakiness in ChromeLabsButtonTest.ShowAndHideChromeLabsBubbleOnPress"
>
> This reverts commit 9cc64477.
>
> Reason for revert: Breaks ChromeOS builder. See https://crbug.com/1165349.
>
> Original change's description:
> > Fix flakiness in ChromeLabsButtonTest.ShowAndHideChromeLabsBubbleOnPress
> >
> > This test is very flaky, I believe because it relies on global state,
> > instead of explicitly setting up the LabInfo used to populate the labs
> > button. For example, if there were no current labs in SetUpLabs, the
> > test would fail. So this CL explicitly sets up the LabInfo for the labs
> > button. It's a bit messy that it requires a change to the production
> > code, but I didn't see any other way of plumbing that information
> > through to the model.
> >
> > Bug: 956719
> > Change-Id: I8078c211e37fc1eeff50a44043eca760cdc4850a
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2618656
> > Reviewed-by: Collin Baker <collinbaker@chromium.org>
> > Commit-Queue: David Bienvenu <davidbienvenu@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#842150}
>
> TBR=davidbienvenu@chromium.org,collinbaker@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com
>
> Change-Id: I5cd05c29bb299b97fe8b491d865a9925c642a5f5
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 956719
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2622519
> Reviewed-by: Sammie Quon <sammiequon@chromium.org>
> Commit-Queue: Sammie Quon <sammiequon@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#842174}

TBR=sammiequon@chromium.org,davidbienvenu@chromium.org,collinbaker@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com

# Not skipping CQ checks because this is a reland.

Bug: 956719
Change-Id: I0f96304d5fae80b203f69064f9f68f7765011d7a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2623160Reviewed-by: default avatarDavid Bienvenu <davidbienvenu@chromium.org>
Reviewed-by: default avatarCollin Baker <collinbaker@chromium.org>
Commit-Queue: David Bienvenu <davidbienvenu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842525}
parent f8638d7e
...@@ -23,6 +23,12 @@ ChromeLabsBubbleViewModel::ChromeLabsBubbleViewModel() { ...@@ -23,6 +23,12 @@ ChromeLabsBubbleViewModel::ChromeLabsBubbleViewModel() {
SetUpLabs(); SetUpLabs();
} }
ChromeLabsBubbleViewModel::ChromeLabsBubbleViewModel(
const std::vector<LabInfo>& lab_info)
: lab_info_(lab_info) {
SetUpLabs();
}
ChromeLabsBubbleViewModel::~ChromeLabsBubbleViewModel() = default; ChromeLabsBubbleViewModel::~ChromeLabsBubbleViewModel() = default;
const std::vector<LabInfo>& ChromeLabsBubbleViewModel::GetLabInfo() const { const std::vector<LabInfo>& ChromeLabsBubbleViewModel::GetLabInfo() const {
......
...@@ -32,6 +32,8 @@ struct LabInfo { ...@@ -32,6 +32,8 @@ struct LabInfo {
class ChromeLabsBubbleViewModel { class ChromeLabsBubbleViewModel {
public: public:
ChromeLabsBubbleViewModel(); ChromeLabsBubbleViewModel();
// |lab_info_| will have `lab_info`, and whatever SetUpLabs appends to it.
explicit ChromeLabsBubbleViewModel(const std::vector<LabInfo>& lab_info);
~ChromeLabsBubbleViewModel(); ~ChromeLabsBubbleViewModel();
const std::vector<LabInfo>& GetLabInfo() const; const std::vector<LabInfo>& GetLabInfo() const;
......
...@@ -15,6 +15,8 @@ ChromeLabsButton::ChromeLabsButton() ...@@ -15,6 +15,8 @@ ChromeLabsButton::ChromeLabsButton()
views::ButtonController::NotifyAction::kOnPress); views::ButtonController::NotifyAction::kOnPress);
} }
ChromeLabsButton::~ChromeLabsButton() = default;
void ChromeLabsButton::UpdateIcon() { void ChromeLabsButton::UpdateIcon() {
UpdateIconsWithStandardColors(kChromeLabsIcon); UpdateIconsWithStandardColors(kChromeLabsIcon);
} }
...@@ -23,11 +25,17 @@ const char* ChromeLabsButton::GetClassName() const { ...@@ -23,11 +25,17 @@ const char* ChromeLabsButton::GetClassName() const {
return "ChromeLabsButton"; return "ChromeLabsButton";
} }
void ChromeLabsButton::SetLabInfoForTesting(
const std::vector<LabInfo>& test_lab_info) {
test_lab_info_ = test_lab_info;
}
void ChromeLabsButton::ButtonPressed() { void ChromeLabsButton::ButtonPressed() {
if (ChromeLabsBubbleView::IsShowing()) { if (ChromeLabsBubbleView::IsShowing()) {
ChromeLabsBubbleView::Hide(); ChromeLabsBubbleView::Hide();
return; return;
} }
ChromeLabsBubbleView::Show(this, std::unique_ptr<ChromeLabsBubbleViewModel> model =
std::make_unique<ChromeLabsBubbleViewModel>()); std::make_unique<ChromeLabsBubbleViewModel>(test_lab_info_);
ChromeLabsBubbleView::Show(this, std::move(model));
} }
...@@ -5,11 +5,13 @@ ...@@ -5,11 +5,13 @@
#ifndef CHROME_BROWSER_UI_VIEWS_TOOLBAR_CHROME_LABS_BUTTON_H_ #ifndef CHROME_BROWSER_UI_VIEWS_TOOLBAR_CHROME_LABS_BUTTON_H_
#define CHROME_BROWSER_UI_VIEWS_TOOLBAR_CHROME_LABS_BUTTON_H_ #define CHROME_BROWSER_UI_VIEWS_TOOLBAR_CHROME_LABS_BUTTON_H_
#include "chrome/browser/ui/views/toolbar/chrome_labs_bubble_view_model.h"
#include "chrome/browser/ui/views/toolbar/toolbar_button.h" #include "chrome/browser/ui/views/toolbar/toolbar_button.h"
class ChromeLabsButton : public ToolbarButton { class ChromeLabsButton : public ToolbarButton {
public: public:
ChromeLabsButton(); ChromeLabsButton();
~ChromeLabsButton() override;
// ToolbarButton: // ToolbarButton:
void UpdateIcon() override; void UpdateIcon() override;
...@@ -17,8 +19,14 @@ class ChromeLabsButton : public ToolbarButton { ...@@ -17,8 +19,14 @@ class ChromeLabsButton : public ToolbarButton {
// views::View: // views::View:
const char* GetClassName() const override; const char* GetClassName() const override;
void SetLabInfoForTesting(const std::vector<LabInfo>& test_lab_info);
private: private:
void ButtonPressed(); void ButtonPressed();
// Used by tests to customize the LabInfo used to populate the button's menu.
// This will be empty in production code.
std::vector<LabInfo> test_lab_info_;
}; };
#endif // CHROME_BROWSER_UI_VIEWS_TOOLBAR_CHROME_LABS_BUTTON_H_ #endif // CHROME_BROWSER_UI_VIEWS_TOOLBAR_CHROME_LABS_BUTTON_H_
...@@ -4,15 +4,21 @@ ...@@ -4,15 +4,21 @@
#include "chrome/browser/ui/views/toolbar/chrome_labs_button.h" #include "chrome/browser/ui/views/toolbar/chrome_labs_button.h"
#include "base/test/scoped_feature_list.h" #include "base/test/scoped_feature_list.h"
#include "chrome/browser/about_flags.h"
#include "chrome/browser/ui/ui_features.h" #include "chrome/browser/ui/ui_features.h"
#include "chrome/browser/ui/views/frame/browser_view.h" #include "chrome/browser/ui/views/frame/browser_view.h"
#include "chrome/browser/ui/views/frame/test_with_browser_view.h" #include "chrome/browser/ui/views/frame/test_with_browser_view.h"
#include "chrome/browser/ui/views/toolbar/chrome_labs_bubble_view.h" #include "chrome/browser/ui/views/toolbar/chrome_labs_bubble_view.h"
#include "chrome/browser/ui/views/toolbar/toolbar_view.h" #include "chrome/browser/ui/views/toolbar/toolbar_view.h"
#include "components/flags_ui/feature_entry_macros.h"
#include "ui/events/event_utils.h" #include "ui/events/event_utils.h"
#include "ui/views/test/button_test_api.h" #include "ui/views/test/button_test_api.h"
#include "ui/views/test/widget_test.h" #include "ui/views/test/widget_test.h"
namespace {
const char kFirstTestFeatureId[] = "feature-1";
} // namespace
class ChromeLabsButtonTest : public TestWithBrowserView { class ChromeLabsButtonTest : public TestWithBrowserView {
public: public:
void SetUp() override { void SetUp() override {
...@@ -28,6 +34,23 @@ TEST_F(ChromeLabsButtonTest, ShowAndHideChromeLabsBubbleOnPress) { ...@@ -28,6 +34,23 @@ TEST_F(ChromeLabsButtonTest, ShowAndHideChromeLabsBubbleOnPress) {
ChromeLabsButton* labs_button = ChromeLabsButton* labs_button =
browser_view()->toolbar()->chrome_labs_button(); browser_view()->toolbar()->chrome_labs_button();
EXPECT_FALSE(ChromeLabsBubbleView::IsShowing()); EXPECT_FALSE(ChromeLabsBubbleView::IsShowing());
// Explicitly set up the feature flags and LabInfo for the button instead of
// relying on ChromeLabsBubbleViewModel::SetUpLabs().
const base::Feature kTestFeature1{"FeatureName1",
base::FEATURE_ENABLED_BY_DEFAULT};
std::vector<flags_ui::FeatureEntry> entries = {
{kFirstTestFeatureId, "", "", flags_ui::FlagsState::GetCurrentPlatform(),
FEATURE_VALUE_TYPE(kTestFeature1)}};
about_flags::testing::SetFeatureEntries(entries);
std::vector<LabInfo> test_feature_info = {
{kFirstTestFeatureId, base::ASCIIToUTF16(""), base::ASCIIToUTF16(""),
version_info::Channel::STABLE}};
labs_button->SetLabInfoForTesting(test_feature_info);
ui::MouseEvent e(ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(), ui::MouseEvent e(ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(),
ui::EventTimeForNow(), 0, 0); ui::EventTimeForNow(), 0, 0);
views::test::ButtonTestApi test_api(labs_button); views::test::ButtonTestApi test_api(labs_button);
......
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