New issue
Advanced search Search tips

Issue 663411 link

Starred by 1 user

Issue metadata

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

Blocking:
issue 474273



Sign in to add a comment

webkit-patch rebaseline-cl never rebaselines virtual tests

Project Member Reported by qyears...@chromium.org, Nov 8 2016

Issue description

Currently rebaseline-cl checks whether tests exist before trying to rebaseline, although for virtual tests, there is no file no the filesystem but nevertheless the test still exists and can have baselines.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 16 2016

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

commit daf1ddbd71f55b0765611a7273c74860dbd3d6b2
Author: qyearsley <qyearsley@chromium.org>
Date: Wed Nov 16 17:52:02 2016

In rebaseline-cl, don't check for local file existence.

I originally added filtering of tests to only include files that exist
locally in http://crrev.com/2371803003.

But now, it seems clearer that this was the wrong choice, since:

 1. Virtual tests don't exist on the local file system
 2. When --issue is passed (when downloading baselines for try jobs
    on another CL), the files don't exist locally.

In order to stop checking for file existence, another little issue
needed to be solved, which was that when getting the list of files
that were changed, the list included deleted files, which we don't
want to rebaseline; so this CL also fixes that.

BUG= 663411 , 656154 , 649691 

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

[modify] https://crrev.com/daf1ddbd71f55b0765611a7273c74860dbd3d6b2/third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py
[modify] https://crrev.com/daf1ddbd71f55b0765611a7273c74860dbd3d6b2/third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld_unittest.py
[modify] https://crrev.com/daf1ddbd71f55b0765611a7273c74860dbd3d6b2/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py
[modify] https://crrev.com/daf1ddbd71f55b0765611a7273c74860dbd3d6b2/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl_unittest.py

Status: Fixed (was: Assigned)
I just ran webkit-patch rebaseline-cl and it added both of the following two identical files:

- third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/japanese-rl-selection-repaint-expected.png
- third_party/WebKit/LayoutTests/platform/linux/virtual/spinvalidation/paint/invalidation/japanese-rl-selection-repaint-expected.png

It seems to me that only the first file is needed? In which case, should rebaseline-cl ever rebaseline virtual tests?
I think you're right that only the first is needed in this case - if there's no baseline for a virtual test, then the corresponding non-virtual test baseline should be used.

I also think that in some cases, virtual tests may need to have different baselines than their corresponding non-virtual tests, so in general, I still think it's correct to rebaseline virtual tests.

Looking at the related historical  bug 237701 , it looks like the "baseline optimizer" which is run after downloading baselines should de-dupe these, but apparently it is not doing so. Filed new bug 674202.

Note: I think that currently there may be many duplicate baselines for virtual tests checked in:

  $ md5sum $CHROME_SRC/third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/caret-contenteditable-content-after-expected.png 
6439f10a23d61fe2124de593864cc6b2  /usr/local/google/home/qyearsley/chromium/src/third_party/WebKit/LayoutTests/platform/linux/paint/invalidation/caret-contenteditable-content-after-expected.png
  $ md5sum $CHROME_SRC/third_party/WebKit/LayoutTests/platform/linux/virtual/spinvalidation/paint/invalidation/caret-contenteditable-content-after-expected.png
6439f10a23d61fe2124de593864cc6b2  /usr/local/google/home/qyearsley/chromium/src/third_party/WebKit/LayoutTests/platform/linux/virtual/spinvalidation/paint/invalidation/caret-contenteditable-content-after-expected.png

Sign in to add a comment