New issue
Advanced search Search tips

Issue 827607 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug



Sign in to add a comment

VariationsServiceTest.RequestsInitiallyNotAllowed is failing in official builds

Project Member Reported by thakis@chromium.org, Mar 30 2018

Issue description

e.g. here:

https://logs.chromium.org/v/?s=chromium%2Fbb%2Fchromium.clang%2FToTWin64%2F1194%2F%2B%2Frecipes%2Fsteps%2Fcomponents_unittests%2F0%2Flogs%2FVariationsServiceTest.RequestsInitiallyNotAllowed%2F0

[ RUN      ] VariationsServiceTest.RequestsInitiallyNotAllowed
../../components/variations/service/variations_service_unittest.cc(398): error: Value of: test_service.fetch_attempted()
  Actual: true
Expected: false

Probably repros in is_official_build=true builds. sky, you touched this test yesterday. Not sure if this is a regression, but might be.
 

Comment 1 by thakis@chromium.org, Mar 30 2018

Started in https://ci.chromium.org/buildbot/chromium.clang/ToTWin64/1191 so very likely a regression due to your change.

Comment 2 by thakis@chromium.org, Mar 30 2018

Cc: r...@chromium.org

Comment 3 by sky@chromium.org, Mar 30 2018

Status: Started (was: Untriaged)
Non-main waterfall builders make me sad.

Comment 4 by sky@chromium.org, Mar 30 2018

Oy! Windows only and official only failure. Ugh!

Comment 5 by sky@chromium.org, Mar 30 2018

Even worse is when the test passes locally. *SIGH*
Project Member

Comment 6 by bugdroid1@chromium.org, Mar 30 2018

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

commit 6335baf97518a395308bfdff475fe2a78ca32549
Author: Scott Violet <sky@chromium.org>
Date: Fri Mar 30 23:31:59 2018

Fix VariationsServiceTest.RequestsInitiallyNotAllowed for branded builds

In https://chromium-review.googlesource.com/c/chromium/src/+/985769 I
changed VariationsServiceTest.RequestsInitiallyNotAllowed to call
PerformPreMainMessageLoopStartup(). That turns out problematic for
branded builds, because PerformPreMainMessageLoopStartup() calls to
IsFetchingEnabled(), which returns true and results in
doing more processing the test isn't expecting. This works in
non-branded builds, because IsFetchingEnabled() returns false.

The fix is to move Init()'ing the resource_request_allowed_notifier_
to a separate function that this test and
PerformPreMainMessageLoopStartup() call.

BUG=827607,826930
TEST=covered by test

Change-Id: I89e31b3856cbf67c5ac74ebf85c1694256d8a740
Reviewed-on: https://chromium-review.googlesource.com/988952
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547317}
[modify] https://crrev.com/6335baf97518a395308bfdff475fe2a78ca32549/components/variations/service/variations_service.cc
[modify] https://crrev.com/6335baf97518a395308bfdff475fe2a78ca32549/components/variations/service/variations_service.h
[modify] https://crrev.com/6335baf97518a395308bfdff475fe2a78ca32549/components/variations/service/variations_service_unittest.cc

Project Member

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

Labels: merge-merged-3359
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5cd09234158bdef6dd88449fb2c4f8ecf074b6d9

commit 5cd09234158bdef6dd88449fb2c4f8ecf074b6d9
Author: Scott Violet <sky@chromium.org>
Date: Tue Apr 03 21:06:17 2018

MERGE 66: Fix VariationsServiceTest.RequestsInitiallyNotAllowed for branded builds

In https://chromium-review.googlesource.com/c/chromium/src/+/985769 I
changed VariationsServiceTest.RequestsInitiallyNotAllowed to call
PerformPreMainMessageLoopStartup(). That turns out problematic for
branded builds, because PerformPreMainMessageLoopStartup() calls to
IsFetchingEnabled(), which returns true and results in
doing more processing the test isn't expecting. This works in
non-branded builds, because IsFetchingEnabled() returns false.

The fix is to move Init()'ing the resource_request_allowed_notifier_
to a separate function that this test and
PerformPreMainMessageLoopStartup() call.

BUG=827607,826930
TEST=covered by test

Change-Id: I89e31b3856cbf67c5ac74ebf85c1694256d8a740
Reviewed-on: https://chromium-review.googlesource.com/988952
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Ilya Sherman <isherman@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#547317}(cherry picked from commit 6335baf97518a395308bfdff475fe2a78ca32549)
Reviewed-on: https://chromium-review.googlesource.com/993913
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#565}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/5cd09234158bdef6dd88449fb2c4f8ecf074b6d9/components/variations/service/variations_service.cc
[modify] https://crrev.com/5cd09234158bdef6dd88449fb2c4f8ecf074b6d9/components/variations/service/variations_service.h
[modify] https://crrev.com/5cd09234158bdef6dd88449fb2c4f8ecf074b6d9/components/variations/service/variations_service_unittest.cc

Sign in to add a comment