Convert OneClickSigninBubbleViewTest to browser_tests.

Convert OneClickSigninBubbleViewTest to browser_tests.

When a blur event occurs during the execution of OneClickSigninBubbleViewTest, the bubble is dismissed and some assertions fail. For that reason, this CL adds a set_close_on_deactivate_for_testing() method to OneClickSigninBubbleView to prevent it from being affected by focus during tests. OneClickSigninBubbleViewTest has also been converted to browser_tests, which is more appropriate for a test that handles focus events.

There is no need to move the test to interactive_ui_tests, because it doesn't fail even if focus events occur during the execution.

TEST=Run browser_tests:OneClickSigninBubbleViewTest.*
BUG=266972
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221130

[fdoray, 2013-08-09 18:22:42]

Could you review this CL?
Thanks.

https://codereview.chromium.org/22743002/diff/1/chrome/browser/ui/views/sync/...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.h (right):

https://codereview.chromium.org/22743002/diff/1/chrome/browser/ui/views/sync/...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:60: static void
set_close_on_deactivate_for_testing(bool close) {
The method is not called set_close_on_deactivate() because I don't want to
override BubbleDelegateView::set_close_on_deactivate.

This is an issue in AvatarMenuBubbleView and ProfileChooserView
crbug.com/270888.

[Roger Tawa OOO till Jul 10th, 2013-08-09 18:55:10]

lgtm, with comment below.

https://codereview.chromium.org/22743002/diff/1/chrome/browser/ui/views/sync/...
File chrome/browser/ui/views/sync/one_click_signin_bubble_view.h (right):

https://codereview.chromium.org/22743002/diff/1/chrome/browser/ui/views/sync/...
chrome/browser/ui/views/sync/one_click_signin_bubble_view.h:159: static bool
close_on_deactivate_;
Better to rename this variable so its not the same as the base class member. 
Maybe |close_on_deactivate_for_testing_| ?

[commit-bot: I haz the power, 2013-08-09 19:22:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/22743002/7001

[commit-bot: I haz the power, 2013-08-09 20:31:46]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[fdoray, 2013-08-09 20:44:43]

+pkasting@chromium.org
Owner.

Can you review this CL?
Thanks.

[fdoray, 2013-08-15 22:30:47]

Peter: Can you review this CL?

[Peter Kasting, 2013-08-15 22:33:56]

Is this an interactive_uitest?  If not, it needs to be made one.  That's the
correct way to fix tests that cannot cope with things like focus loss.  Such
tests should not be unittests.

[fdoray, 2013-08-19 16:01:31]

On 2013/08/15 22:33:56, Peter Kasting wrote:
> Is this an interactive_uitest?  If not, it needs to be made one.  That's the
> correct way to fix tests that cannot cope with things like focus loss.  Such
> tests should not be unittests.

Peter:

Initially, the test was in browser_tests. First thing I tried to fix the
flakiness is to move it to interactive_uitest, but it didn't work
(https://codereview.chromium.org/18558009/#ps1). There was still some events
that changed the focus.

Changing the test so that it can be executed in unit_tests solved the flakiness
for all bots, except for win_drmemory. It also made the test faster. With this
CL, the test is stable on win_drmemory.

Do you have any idea about what caused the flakiness in interactive_uitests?

[Peter Kasting, 2013-08-19 18:41:30]

On 2013/08/19 16:01:31, fdoray wrote:
> Do you have any idea about what caused the flakiness in interactive_uitests?

No, but any test that is affected at all by things like focus isn't actually a
"unit" test.

I'd like sky to comment here about why he signed off on the change to make this
a unit test, since he probably knows more than me about ViewsTestBase.

[sky, 2013-08-19 22:43:09]

If this test relies on native focus, then Peter is right, it should be
an interactive ui test.

On Mon, Aug 19, 2013 at 11:41 AM,  <pkasting@chromium.org> wrote:
> On 2013/08/19 16:01:31, fdoray wrote:
>>
>> Do you have any idea about what caused the flakiness in
>> interactive_uitests?
>
>
> No, but any test that is affected at all by things like focus isn't actually
> a
> "unit" test.
>
> I'd like sky to comment here about why he signed off on the change to make
> this
> a unit test, since he probably knows more than me about ViewsTestBase.
>
> https://codereview.chromium.org/22743002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[fdoray, 2013-09-03 00:10:31]

I moved the test to interactive_ui_tests and it seems stable. Could you review
the CL?
Thanks.

[sky, 2013-09-03 16:18:01]

LGTM

[commit-bot: I haz the power, 2013-09-03 23:36:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fdoray@chromium.org/22743002/42006

[commit-bot: I haz the power, 2013-09-04 03:11:20]

Change committed as 221130

[fdoray, 2013-09-08 20:31:42]

My change was reverted because it made the test flaky
(http://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20%28...).
It seems that some events affect focus during the execution of
interactive_ui_tests.

I reused a trick from https://chromiumcodereview.appspot.com/19537006
(set_close_on_deactivate_for_testing in
one_click_signin_bubble_view_browsertest.cc) to make the test focus-independant.

Scott: Does it look good to you?

[sky, 2013-09-09 15:35:35]

Interactive ui tests should run serially with no other tests running. So, I'm
not sure why you would see flakiness. Do you understand what was causing the
flake?

[Peter Kasting, 2013-10-08 00:24:45]

On 2013/09/09 15:35:35, sky wrote:
> Interactive ui tests should run serially with no other tests running. So, I'm
> not sure why you would see flakiness. Do you understand what was causing the
> flake?

Ping fdoray (this change seems to have gone silent)