New issue
Advanced search Search tips

Issue 655196 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

rebaseline-cl added unnecessary android-specific baseline for test that is skipped.

Project Member Reported by qyears...@chromium.org, Oct 12 2016

Issue description

In http://crrev.com/2419473003, the test imported/wpt/custom-elements/htmlconstructor/newtarget.html was updated, and this resulted in needing to make a new baseline; this baseline was not platform-specific, but still a copy of the old baseline was moved over to the android baseline directory.

My initial thought is that the copying was done by `webkit-patch copy-existing-baselines-internal` and maybe there should be logic that prevents copying baselines to platform-specific directories for platforms where the test is skipped.
 
Note, when running webkit-patch rebaseline-cl -v, it may print:

"No test result for test imported/wpt/custom-elements/reactions/NamedNodeMap.html in build Build(builder_name=u'android_blink_rel', build_number=1076)"

It may be the case that:
 - When a test is skipped for some platform, there's no test result for the builder for that platform, and
 - In this case, any baselines for that platform should be removed?
After checking, I realized that copy-existing-baselines-internal already basically has the behavior that I want:

https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py?l=195 (added in http://crrev.com/23135008).

Haven't yet verified whether expectations.get_expectations(test_name) returns SKIP if the test is skipped because of not being in SmokeTests.
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 27 2016

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

commit f437383bbfd2cd52b8cacf0adff4e4fb17293127
Author: qyearsley <qyearsley@chromium.org>
Date: Thu Oct 27 17:25:20 2016

Refactoring: Move _port_skips_test to Port class.

As part of rebaselining, when copying existing baselines, we don't want to
make new platform-specific baseline files for platforms where tests are
skipped. In particular, we don't want to make new baselines for android
for non-smoke-tests now since non-smoke-tests are skipped on android.

There's already logic in the copy-existing-baselines to do this; it uses
the TestExpectations.get_expectations method, which, as far as I understand,
returns expectations based on test expectations files, but not based on
whether the port only runs smoke tests.

Meanwhile, elsewhere in rebaseline.py, there's already logic to check
whether a test is skipped on a particular port including because it's not
a smoke test.

The purpose of this CL is to extract that method out so it could
be re-used. An alternative to doing this would be to change
TestExpectations.get_expectations to also include "SKIP" if
port.default_smoke_test_only() and the test is in the smoke tests file.

(Either way, there should be a centralized way of checking whether
a test runs on a port, and this should be used in both places in rebaseline.py.)

BUG= 655196 

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

[modify] https://crrev.com/f437383bbfd2cd52b8cacf0adff4e4fb17293127/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
[modify] https://crrev.com/f437383bbfd2cd52b8cacf0adff4e4fb17293127/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 28 2016

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

commit f2543789d3b0638e3afed6efaf0a9e8e244a3c42
Author: qyearsley <qyearsley@chromium.org>
Date: Fri Oct 28 23:22:59 2016

Use Port.skips_test in copy-existing-baselines command.

After this CL, I expect that when rebaselining (with rebaseline-cl or other rebaseline commands), new baselines shouldn't be added for non-smoke-tests on android.

BUG= 655196 

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

[modify] https://crrev.com/f2543789d3b0638e3afed6efaf0a9e8e244a3c42/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
[modify] https://crrev.com/f2543789d3b0638e3afed6efaf0a9e8e244a3c42/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py

Status: Fixed (was: Assigned)
I think that this is considered fixed now; just tried it out and no extra baselines were added for android (test CL: https://codereview.chromium.org/2463043003).


Sign in to add a comment