Deflake Mac views_unittest's MenuRunnerCocoaTest.RunMenuAndCancel

Deflake Mac views_unittest's MenuRunnerCocoaTest.RunMenuAndCancel

The test occasionally fails with

menu_runner_cocoa_unittest.mm:182: Failure
Expected: (runner_->GetClosingEventTime())
    <= (ui::EventTimeForNow() - base::TimeTicks())),
actual: 13647.4s vs 0.002s

It happens because menu_runner_impl_cocoa.mm uses
base::TimeTicks::Now(), but the test uses ui::EventTimeForNow() to set
expectations. Usually this isn't a problem, but if another unit test in
the same process has created a ui::EventGenerator, this sets a mock time
source with ui::SetEventTickClockForTesting(..) which isn't cleared when
the EventGenerator is destroyed.

Fix both problems. The EventGenerator destructor should reset the global
state it created, and menu_runner_impl_cocoa.mm should use
ui::EventTimeForNow() for better consistency.

BUG=623455, 624555
Committed: https://crrev.com/80f15fe17e9095bd4d6c11d94db8e1dd42556458
Cr-Commit-Position: refs/heads/master@{#403373}

[tapted, 2016-06-30 01:06:05]

Description was changed from

==========
Deflake Mac views_unittest's MenuRunnerCocoaTest.RunMenuAndCancel

BUG=623455
==========

to

==========
Deflake Mac views_unittest's MenuRunnerCocoaTest.RunMenuAndCancel

BUG=623455, 624555
==========

[tapted, 2016-06-30 06:58:56]

Description was changed from

==========
Deflake Mac views_unittest's MenuRunnerCocoaTest.RunMenuAndCancel

BUG=623455, 624555
==========

to

==========
Deflake Mac views_unittest's MenuRunnerCocoaTest.RunMenuAndCancel

The test occasionally fails with

menu_runner_cocoa_unittest.mm:182: Failure Expected:
(runner_->GetClosingEventTime()) <= ((ui::EventTimeForNow() -
base::TimeTicks())), actual: 13647.4s vs 0.002s

It happens because menu_runner_impl_cocoa.mm uses
base::TimeTicks::Now(), but the test uses ui::EventTimeForNow() to set
expectations. Usually this isn't a problem, but if another unit test in
the same process has created a ui::EventGenerator, this sets a mock time
source with ui::SetEventTickClockForTesting(..) which isn't cleared when
the EventGenerator is destroyed.

Fix both problems. The EventGenerator destructor should reset the global
state it created, and menu_runner_impl_cocoa.mm should use
ui::EventTimeForNow() for better consistency.

BUG=623455, 624555
==========

[tapted, 2016-06-30 06:58:56]

tapted@chromium.org changed reviewers:
+ sadrul@chromium.org

[tapted, 2016-06-30 06:59:28]

Description was changed from

==========
Deflake Mac views_unittest's MenuRunnerCocoaTest.RunMenuAndCancel

The test occasionally fails with

menu_runner_cocoa_unittest.mm:182: Failure Expected:
(runner_->GetClosingEventTime()) <= ((ui::EventTimeForNow() -
base::TimeTicks())), actual: 13647.4s vs 0.002s

It happens because menu_runner_impl_cocoa.mm uses
base::TimeTicks::Now(), but the test uses ui::EventTimeForNow() to set
expectations. Usually this isn't a problem, but if another unit test in
the same process has created a ui::EventGenerator, this sets a mock time
source with ui::SetEventTickClockForTesting(..) which isn't cleared when
the EventGenerator is destroyed.

Fix both problems. The EventGenerator destructor should reset the global
state it created, and menu_runner_impl_cocoa.mm should use
ui::EventTimeForNow() for better consistency.

BUG=623455, 624555
==========

to

==========
Deflake Mac views_unittest's MenuRunnerCocoaTest.RunMenuAndCancel

The test occasionally fails with

menu_runner_cocoa_unittest.mm:182: Failure
Expected: (runner_->GetClosingEventTime())
    <= (ui::EventTimeForNow() - base::TimeTicks())),
actual: 13647.4s vs 0.002s

It happens because menu_runner_impl_cocoa.mm uses
base::TimeTicks::Now(), but the test uses ui::EventTimeForNow() to set
expectations. Usually this isn't a problem, but if another unit test in
the same process has created a ui::EventGenerator, this sets a mock time
source with ui::SetEventTickClockForTesting(..) which isn't cleared when
the EventGenerator is destroyed.

Fix both problems. The EventGenerator destructor should reset the global
state it created, and menu_runner_impl_cocoa.mm should use
ui::EventTimeForNow() for better consistency.

BUG=623455, 624555
==========

[tapted, 2016-06-30 07:02:49]

Hi sadrul, please take a look

https://codereview.chromium.org/2094193002/diff/1/ui/events/base_event_utils.cc
File ui/events/base_event_utils.cc (right):

https://codereview.chromium.org/2094193002/diff/1/ui/events/base_event_utils....
ui/events/base_event_utils.cc:54: DCHECK(!g_tick_clock.Get());
So I got this to fail at [ RUN      ] MenuRunnerCocoaTest.RunMenuAndCancel
[9380:1287:0626/225619:5850751330859:FATAL:base_event_utils.cc(54)] Check
failed: !g_tick_clock.Get(). 
0   views_unittests                     0x000000010bf179f3
_ZN4base5debug10StackTraceC1Ev + 19
1   views_unittests                     0x000000010bf3b3d7
_ZN7logging10LogMessageD2Ev + 71
2   views_unittests                     0x000000010c64b7b6
_ZN2ui15EventTimeForNowEv + 166
3   views_unittests                     0x000000010bd70413
_ZN5views4test41MenuRunnerCocoaTest_RunMenuAndCancel_Test8TestBodyEv + 35
4   views_unittests                     0x000000010d80496a
_ZN7testing4Test3RunEv + 474
5   views_unittests                     0x000000010d80555a
_ZN7testing8TestInfo3RunEv + 410
6   views_unittests                     0x000000010d805be3
_ZN7testing8TestCase3RunEv + 451
7   views_unittests                     0x000000010d80cfa9
_ZN7testing8internal12UnitTestImpl11RunAllTestsEv + 825
8   views_unittests                     0x000000010d80cc3d
_ZN7testing8UnitTest3RunEv + 269
9   views_unittests                     0x000000010c016f1f
_ZN4base9TestSuite3RunEv + 127
10  views_unittests                     0x000000010c02dd39
_ZN4base15LaunchUnitTestsEiPPcRKNS_8CallbackIFivELNS_8internal8CopyModeE1EEE +
137
11  views_unittests                     0x000000010c692c0e
_ZN5views14ViewsTestSuite8RunTestsEv + 110
12  views_unittests                     0x000000010be4cbd9 main + 41
13  views_unittests                     0x000000010bcbf0b4 start + 52
14  ???                                 0x0000000000000007 0x0 + 7

before it passed on a retry. So that confirms the test is picking up global
state when it shouldn't. Then it also allowed me to verify whether having the
EventGenerator destructor undo the stuff it does is enough to avoid the problem
(it did! \o/)

[sadrul, 2016-06-30 14:11:21]

lgtm

[tapted, 2016-07-01 00:04:21]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2016-07-01 00:04:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-01 00:37:04]

Description was changed from

==========
Deflake Mac views_unittest's MenuRunnerCocoaTest.RunMenuAndCancel

The test occasionally fails with

menu_runner_cocoa_unittest.mm:182: Failure
Expected: (runner_->GetClosingEventTime())
    <= (ui::EventTimeForNow() - base::TimeTicks())),
actual: 13647.4s vs 0.002s

It happens because menu_runner_impl_cocoa.mm uses
base::TimeTicks::Now(), but the test uses ui::EventTimeForNow() to set
expectations. Usually this isn't a problem, but if another unit test in
the same process has created a ui::EventGenerator, this sets a mock time
source with ui::SetEventTickClockForTesting(..) which isn't cleared when
the EventGenerator is destroyed.

Fix both problems. The EventGenerator destructor should reset the global
state it created, and menu_runner_impl_cocoa.mm should use
ui::EventTimeForNow() for better consistency.

BUG=623455, 624555
==========

to

==========
Deflake Mac views_unittest's MenuRunnerCocoaTest.RunMenuAndCancel

The test occasionally fails with

menu_runner_cocoa_unittest.mm:182: Failure
Expected: (runner_->GetClosingEventTime())
    <= (ui::EventTimeForNow() - base::TimeTicks())),
actual: 13647.4s vs 0.002s

It happens because menu_runner_impl_cocoa.mm uses
base::TimeTicks::Now(), but the test uses ui::EventTimeForNow() to set
expectations. Usually this isn't a problem, but if another unit test in
the same process has created a ui::EventGenerator, this sets a mock time
source with ui::SetEventTickClockForTesting(..) which isn't cleared when
the EventGenerator is destroyed.

Fix both problems. The EventGenerator destructor should reset the global
state it created, and menu_runner_impl_cocoa.mm should use
ui::EventTimeForNow() for better consistency.

BUG=623455, 624555
==========

[commit-bot: I haz the power, 2016-07-01 00:37:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-01 00:37:08]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-01 00:39:18]

Description was changed from

==========
Deflake Mac views_unittest's MenuRunnerCocoaTest.RunMenuAndCancel

The test occasionally fails with

menu_runner_cocoa_unittest.mm:182: Failure
Expected: (runner_->GetClosingEventTime())
    <= (ui::EventTimeForNow() - base::TimeTicks())),
actual: 13647.4s vs 0.002s

It happens because menu_runner_impl_cocoa.mm uses
base::TimeTicks::Now(), but the test uses ui::EventTimeForNow() to set
expectations. Usually this isn't a problem, but if another unit test in
the same process has created a ui::EventGenerator, this sets a mock time
source with ui::SetEventTickClockForTesting(..) which isn't cleared when
the EventGenerator is destroyed.

Fix both problems. The EventGenerator destructor should reset the global
state it created, and menu_runner_impl_cocoa.mm should use
ui::EventTimeForNow() for better consistency.

BUG=623455, 624555
==========

to

==========
Deflake Mac views_unittest's MenuRunnerCocoaTest.RunMenuAndCancel

The test occasionally fails with

menu_runner_cocoa_unittest.mm:182: Failure
Expected: (runner_->GetClosingEventTime())
    <= (ui::EventTimeForNow() - base::TimeTicks())),
actual: 13647.4s vs 0.002s

It happens because menu_runner_impl_cocoa.mm uses
base::TimeTicks::Now(), but the test uses ui::EventTimeForNow() to set
expectations. Usually this isn't a problem, but if another unit test in
the same process has created a ui::EventGenerator, this sets a mock time
source with ui::SetEventTickClockForTesting(..) which isn't cleared when
the EventGenerator is destroyed.

Fix both problems. The EventGenerator destructor should reset the global
state it created, and menu_runner_impl_cocoa.mm should use
ui::EventTimeForNow() for better consistency.

BUG=623455, 624555

Committed: https://crrev.com/80f15fe17e9095bd4d6c11d94db8e1dd42556458
Cr-Commit-Position: refs/heads/master@{#403373}
==========

[commit-bot: I haz the power, 2016-07-01 00:39:19]

Patchset 5 (id:??) landed as
https://crrev.com/80f15fe17e9095bd4d6c11d94db8e1dd42556458
Cr-Commit-Position: refs/heads/master@{#403373}