New issue
Advanced search Search tips

Issue 832057 link

Starred by 2 users
Status: Fixed
Owner:
grt@chromium.org
Closed: Apr 2018
Cc:
pbomm...@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug



Sign in to add a comment

Flaky failures in UpgradeDetectorImplTest.TestPeriodChanges on win_trunk

Project Member Reported by grt@chromium.org, Apr 12 2018 
[ RUN      ] UpgradeDetectorImplTest.TestPeriodChanges
unknown file: error: Uninteresting mock function call - returning directly.
    Function call: OnUpgradeRecommended()
[  FAILED  ] UpgradeDetectorImplTest.TestPeriodChanges (2 ms)

The failure started with r548924. The actual unexpected call takes place when the clock is moved forward on line 261 of the test. This line was added in the aforementioned CL. I suspect that the test is flaky rather than the implementation. I am investigating now.

Aside: I have https://chromium-review.googlesource.com/c/chromium/src/+/1009947 out for review which turns the failure above into the following much more useful failure:

[ RUN      ] UpgradeDetectorImplTest.TestPeriodChanges
unknown file: error: Uninteresting mock function call - returning directly.
    Function call: OnUpgradeRecommended()
Stack trace:
Backtrace:
        testing::internal::UnitTestImpl::CurrentOsStackTraceExceptTop [0x02069D89+69] (C:\src\chromium\src\third_party\googletest\src\googletest\src\gtest.cc:810)
        testing::internal::AssertHelper::operator= [0x0206990C+48] (C:\src\chromium\src\third_party\googletest\src\googletest\src\gtest.cc:382)
        testing::internal::GoogleTestFailureReporter::ReportFailure [0x02056F84+76] (C:\src\chromium\src\third_party\googletest\src\googlemock\src\gmock-internal-utils.cc:99)
        testing::internal::ReportUninterestingCall [0x02059724+194] (C:\src\chromium\src\third_party\googletest\src\googlemock\src\gmock-spec-builders.cc:267)
        testing::internal::UntypedFunctionMockerBase::UntypedInvokeWith [0x02059BCA+452] (C:\src\chromium\src\third_party\googletest\src\googlemock\src\gmock-spec-builders.cc:382)
        testing::internal::FunctionMockerBase<void ()>::InvokeWith [0x00B43216+12] (C:\src\chromium\src\third_party\googletest\src\googlemock\include\gmock\gmock-spec-builders.h:1587)
        `anonymous namespace'::MockUpgradeObserver::OnUpgradeRecommended [0x01EA5B7B+47] (C:\src\chromium\src\chrome\browser\upgrade_detector_impl_unittest.cc:88)
        UpgradeDetector::NotifyUpgradeRecommended [0x032D6201+81] (C:\src\chromium\src\chrome\browser\upgrade_detector.cc:137)
        UpgradeDetector::NotifyUpgrade [0x032D6167+25] (C:\src\chromium\src\chrome\browser\upgrade_detector.cc:126)
        UpgradeDetectorImpl::NotifyOnUpgradeWithTimePassed [0x034B8242+542] (C:\src\chromium\src\chrome\browser\upgrade_detector_impl.cc:546)
        UpgradeDetectorImpl::NotifyOnUpgrade [0x034B761E+148] (C:\src\chromium\src\chrome\browser\upgrade_detector_impl.cc:594)
        base::Timer::RunScheduledTask [0x67487475+245] (C:\src\chromium\src\base\timer\timer.cc:263)
        base::OnceCallback<void ()>::Run [0x00AB9F32+44] (C:\src\chromium\src\base\callback.h:95)
        base::TestMockTimeTaskRunner::ProcessAllTasksNoLaterThan [0x02CB36BF+347] (C:\src\chromium\src\base\test\test_mock_time_task_runner.cc:337)
        base::TestMockTimeTaskRunner::FastForwardBy [0x02CB3401+189] (C:\src\chromium\src\base\test\test_mock_time_task_runner.cc:195)
        base::test::ScopedTaskEnvironment::FastForwardBy [0x02CAFB74+114] (C:\src\chromium\src\base\test\scoped_task_environment.cc:239)
        UpgradeDetectorImplTest::FastForwardBy [0x01EA3645+35] (C:\src\chromium\src\chrome\browser\upgrade_detector_impl_unittest.cc:129)
        UpgradeDetectorImplTest_TestPeriodChanges_Test::TestBody [0x01EA3089+1829] (C:\src\chromium\src\chrome\browser\upgrade_detector_impl_unittest.cc:261)

[  FAILED  ] UpgradeDetectorImplTest.TestPeriodChanges (288 ms)

Comment 1 by grt@chromium.org, Apr 13 2018 

Confirmed: the test was putting an instance into an invalid state, leading to a race condition. This only affected Google Chrome builds on Windows. A fix for the test is out for review here: https://chromium-review.googlesource.com/c/chromium/src/+/1012041.

Comment 2 by grt@chromium.org, Apr 13 2018 

Issue 832059 has been merged into this issue.
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 13 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8d5362a07dace977713167a10bbe561975a30b09

commit 8d5362a07dace977713167a10bbe561975a30b09
Author: Greg Thompson <grt@chromium.org>
Date: Fri Apr 13 20:00:49 2018

Fix flaky UpgradeDetectorImplTest.TestPeriodChanges failures.

The test harness drives an UpgradeDetectorImpl by calling its
UpgradeDetected method. In a normal instance, this method is called by
tasks scheduled on detect_upgrade_timer_, which is only started after
the instance has determined whether or not updates are enabled. Since
the test harness was not waiting for this determination to complete, it
was racing with this task. The fix is to disable the update enablement
check in the test so that there's no race.

BUG= 832057 

Change-Id: Ie8b69a59e7de29aa7da37fe1edba5d324dbed450
Reviewed-on: https://chromium-review.googlesource.com/1012041
Commit-Queue: Greg Thompson <grt@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550734}
[modify] https://crrev.com/8d5362a07dace977713167a10bbe561975a30b09/chrome/browser/upgrade_detector_impl_unittest.cc

Comment 4 by grt@chromium.org, Apr 16 2018

Labels: Merge-Request-67
Status: Fixed (was: Started)
No longer failing on win_trunk. Requesting merge to M67, as the (test-only) fix missed the branch by a smidge. Thanks.
Project Member

Comment 5 by sheriffbot@chromium.org, Apr 17 2018

Labels: -Merge-Request-67 Merge-Approved-67 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M67. Please go ahead and merge the CL to branch 3396 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 6 by gov...@chromium.org, Apr 17 2018 

Pls merge your change to M67 branch 3396 by 1:00 PM PT today, Tuesday (04/17/18) so we can pick it up for tomorrow's M67 dev release. Thank you.
Project Member

Comment 7 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8d5362a07dace977713167a10bbe561975a30b09

commit 8d5362a07dace977713167a10bbe561975a30b09
Author: Greg Thompson <grt@chromium.org>
Date: Fri Apr 13 20:00:49 2018

Fix flaky UpgradeDetectorImplTest.TestPeriodChanges failures.

The test harness drives an UpgradeDetectorImpl by calling its
UpgradeDetected method. In a normal instance, this method is called by
tasks scheduled on detect_upgrade_timer_, which is only started after
the instance has determined whether or not updates are enabled. Since
the test harness was not waiting for this determination to complete, it
was racing with this task. The fix is to disable the update enablement
check in the test so that there's no race.

BUG= 832057 

Change-Id: Ie8b69a59e7de29aa7da37fe1edba5d324dbed450
Reviewed-on: https://chromium-review.googlesource.com/1012041
Commit-Queue: Greg Thompson <grt@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550734}
[modify] https://crrev.com/8d5362a07dace977713167a10bbe561975a30b09/chrome/browser/upgrade_detector_impl_unittest.cc
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 18 2018

Labels: -merge-approved-67 merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1dff54753129be94838e5141ae17bab60ec63daf

commit 1dff54753129be94838e5141ae17bab60ec63daf
Author: Greg Thompson <grt@chromium.org>
Date: Wed Apr 18 07:48:08 2018

Fix flaky UpgradeDetectorImplTest.TestPeriodChanges failures.

The test harness drives an UpgradeDetectorImpl by calling its
UpgradeDetected method. In a normal instance, this method is called by
tasks scheduled on detect_upgrade_timer_, which is only started after
the instance has determined whether or not updates are enabled. Since
the test harness was not waiting for this determination to complete, it
was racing with this task. The fix is to disable the update enablement
check in the test so that there's no race.

BUG= 832057 

Change-Id: Ie8b69a59e7de29aa7da37fe1edba5d324dbed450
Reviewed-on: https://chromium-review.googlesource.com/1012041
Commit-Queue: Greg Thompson <grt@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#550734}(cherry picked from commit 8d5362a07dace977713167a10bbe561975a30b09)
Reviewed-on: https://chromium-review.googlesource.com/1015362
Reviewed-by: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#76}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/1dff54753129be94838e5141ae17bab60ec63daf/chrome/browser/upgrade_detector_impl_unittest.cc
►

Sign in to add a comment