New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 631014 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 590036



Sign in to add a comment

Blink try bots on tryserver.blink don't run layout tests when there are only layout test file changes.

Project Member Reported by xidac...@chromium.org, Jul 25 2016

Issue description

Here is the CL:
https://codereview.chromium.org/2178513002/#ps20001

In PS#2, I added a layout test, linux_blink_rel and mac_blink_rel doesn't run the layout tests.
 
Cc: phajdan.jr@chromium.org
Owner: qyears...@chromium.org
Status: Assigned (was: Untriaged)
Layout tests are run for some builds (e.g. https://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/88762), but not others (e.g. https://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/88741 or https://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/88764)

This is probably related to the output for the "analyze" step. For the builds where layout tests are not run, the analyze step output is:
{
  "compile_targets": [], 
  "status": "No dependency", 
  "test_targets": []
}

Pawel, if I'm understanding right, it seems like the layout tests should always be added when api.chromium_tests.get_tests is called (at https://cs.chromium.org/chromium/build/scripts/slave/recipes/chromium_trybot.py?l=125) if it's configured in chromium_tests that these bots should run layout tests. Is that correct? Should the analyze step affect whether layout tests are run?
another instance:
https://codereview.chromium.org/2188893003/

please see PS1.
Cc: raikiri@google.com dcampb@google.com
Status: Started (was: Assigned)
Summary: Blink try bots on tryserver.blink don't run layout tests when there are only layout test file changes. (was: linux_blink_rel and mac_blink_rel doesn't run layout tests)
My hypothesis for the cause of this bug: 
During the execution of the chromium_trybots recipe:
 1. test_targets and compile_targets are emptied out by the analyze filter on this line: https://cs.chromium.org/chromium/build/scripts/slave/recipes/chromium_trybot.py?l=172
 2. an else branch is taken and the list of tests is emptied out because the list of compile targets is empty: https://cs.chromium.org/chromium/build/scripts/slave/recipes/chromium_trybot.py?l=217.

I think the easy fix to make this work for the bots on tryserver.blink would be to add all of the bot names to CHROMIUM_BLINK_TESTS_BUILDERS at the top of the recipe module. This wouldn't hurt, in any case, but may be unnecessary. CL for this potential change: http://crrev.com/2194553002.

There's a related issue that should also be resolved though: A new bot on tryserver.chromium.linux (see bug 622865) needs to run a special BlinkTest (with special args), and that's also getting filtered out and never run.

Maybe the actual best solution would be to change the analyze filter... or find some other way to change the chromium_trybot recipe so it doesn't filter out
the blink_tests target and the BlinkTest test if they're present.
Another change to consider that may have the desired effect may be to modify src/testing/buildbot/trybot_analyze_config.json (proposed change: https://codereview.chromium.org/2190163003).
Cc: dpranke@chromium.org
Blocking: 590036
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build.git/+/f0cf894af9bbfc9eec5e1237088ea790d25d3008

commit f0cf894af9bbfc9eec5e1237088ea790d25d3008
Author: qyearsley <qyearsley@chromium.org>
Date: Sat Jul 30 01:11:06 2016

Make chromium_trybots always run layout tests on tryserver.blink bots.

Note how this makes it so that use_{skia,v8}_patch_on_blink_trybot
simulation expectation files include more tests. The webkit_tests
was in the simulation expectation files for the other blink try bots
because "suppress_analyze" was in the test, but that wasn't in the
step for use_{skia,v8}_patch_on_blink_trybot.

Pros:
 - This makes tryserver.blink bots behave similarly to the *_chromium_rel_ng builders

Cons
 - This would not make the new
linux_layout_tests_slimming_paint_v2 bot work as it should.
 - This results in running some blink-related tests that may not be necessary depending on the CL

This is one alternative to http://crrev.com/2190163003.

BUG= 631014 

Review-Url: https://codereview.chromium.org/2194553002

[modify] https://crrev.com/f0cf894af9bbfc9eec5e1237088ea790d25d3008/scripts/slave/recipes/chromium_trybot.expected/use_skia_patch_on_blink_trybot.json
[modify] https://crrev.com/f0cf894af9bbfc9eec5e1237088ea790d25d3008/scripts/slave/recipes/chromium_trybot.expected/use_v8_patch_on_blink_trybot.json
[modify] https://crrev.com/f0cf894af9bbfc9eec5e1237088ea790d25d3008/scripts/slave/recipes/chromium_trybot.py

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 30 2016

Project Member

Comment 9 by bugdroid1@chromium.org, Jul 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra.git/+/97ae77495b9393dc69022b44a34403371eff1db8

commit 97ae77495b9393dc69022b44a34403371eff1db8
Author: recipe-roller <recipe-roller@chromium.org>
Date: Sat Jul 30 01:26:22 2016

Roll recipe dependencies (trivial).

This is an automated CL created by the recipe roller. This CL rolls recipe
changes from upstream projects (e.g. depot_tools) into downstream projects
(e.g. tools/build).

More info is at https://goo.gl/zkKdpD. Use https://goo.gl/noib3a to file a bug
(or complain)

build:
  https://crrev.com/f0cf894af9bbfc9eec5e1237088ea790d25d3008 Make chromium_trybots always run layout tests on tryserver.blink bots. (qyearsley@chromium.org)

R=qyearsley@chromium.org
BUG= 631014 

TBR=martiniss@chromium.org,phajdan.jr@chromium.org

Review-Url: https://codereview.chromium.org/2192343002

[modify] https://crrev.com/97ae77495b9393dc69022b44a34403371eff1db8/infra/config/recipes.cfg

Project Member

Comment 10 by bugdroid1@chromium.org, Aug 1 2016

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

commit 165a17a8f5c39b2e0ab1df3bdaa381222605b0ce
Author: qyearsley <qyearsley@chromium.org>
Date: Mon Aug 01 17:58:50 2016

Add data dependency directories for blink_tests in BUILD.gn.

Suggested in http://crrev.com/2190163003. This should make it so that the analyze step identifies blink_tests as a dependency for changes in layout tests and WebKit/Tools/Scripts.

BUG= 631014 

Review-Url: https://codereview.chromium.org/2195223002
Cr-Commit-Position: refs/heads/master@{#408996}

[modify] https://crrev.com/165a17a8f5c39b2e0ab1df3bdaa381222605b0ce/BUILD.gn

Project Member

Comment 11 by bugdroid1@chromium.org, Aug 3 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build.git/+/051348e7c052a488bc021506501dd1da231b12d5

commit 051348e7c052a488bc021506501dd1da231b12d5
Author: qyearsley <qyearsley@chromium.org>
Date: Wed Aug 03 21:46:34 2016

Revert of Make chromium_trybots always run layout tests on tryserver.blink bots. (patchset #1 id:1 of https://codereview.chromium.org/2194553002/ )

Reason for revert:
After http://crbug.com/2195223002, layout tests are not filtered out of the test list for Blink try jobs, so there's no need to add Blink tests again.

See  http://crbug.com/634104 .

Original issue's description:
> Make chromium_trybots always run layout tests on tryserver.blink bots.
>
> Note how this makes it so that use_{skia,v8}_patch_on_blink_trybot
> simulation expectation files include more tests. The webkit_tests
> was in the simulation expectation files for the other blink try bots
> because "suppress_analyze" was in the test, but that wasn't in the
> step for use_{skia,v8}_patch_on_blink_trybot.
>
> Pros:
>  - This makes tryserver.blink bots behave similarly to the *_chromium_rel_ng builders
>
> Cons
>  - This would not make the new
> linux_layout_tests_slimming_paint_v2 bot work as it should.
>  - This results in running some blink-related tests that may not be necessary depending on the CL
>
> This is one alternative to http://crrev.com/2190163003.
>
> BUG= 631014 
>
> Committed: https://chromium.googlesource.com/chromium/tools/build/+/f0cf894af9bbfc9eec5e1237088ea790d25d3008

TBR=dpranke@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 631014 

Review-Url: https://codereview.chromium.org/2206763003

[modify] https://crrev.com/051348e7c052a488bc021506501dd1da231b12d5/scripts/slave/recipes/chromium_trybot.expected/use_skia_patch_on_blink_trybot.json
[modify] https://crrev.com/051348e7c052a488bc021506501dd1da231b12d5/scripts/slave/recipes/chromium_trybot.expected/use_v8_patch_on_blink_trybot.json
[modify] https://crrev.com/051348e7c052a488bc021506501dd1da231b12d5/scripts/slave/recipes/chromium_trybot.py

Project Member

Comment 12 by bugdroid1@chromium.org, Aug 3 2016

Project Member

Comment 13 by bugdroid1@chromium.org, Aug 3 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra.git/+/15664522da59153a52441289a4f9e06f8148a656

commit 15664522da59153a52441289a4f9e06f8148a656
Author: recipe-roller <recipe-roller@chromium.org>
Date: Wed Aug 03 22:11:31 2016

Roll recipe dependencies (trivial).

This is an automated CL created by the recipe roller. This CL rolls recipe
changes from upstream projects (e.g. depot_tools) into downstream projects
(e.g. tools/build).

More info is at https://goo.gl/zkKdpD. Use https://goo.gl/noib3a to file a bug
(or complain)

build:
  https://crrev.com/051348e7c052a488bc021506501dd1da231b12d5 Revert of Make chromium_trybots always run layout tests on tryserver.blink bots. (patchset #1 id:1 of https://codereview.chromium.org/2194553002/ ) (qyearsley@chromium.org)

R=dpranke@chromium.org,qyearsley@chromium.org
BUG= 631014 

TBR=martiniss@chromium.org,phajdan.jr@chromium.org

Review-Url: https://codereview.chromium.org/2214433002

[modify] https://crrev.com/15664522da59153a52441289a4f9e06f8148a656/infra/config/recipes.cfg

Status: Fixed (was: Started)
I believe this is fixed by r408996.

Sign in to add a comment