New issue
Advanced search Search tips

Issue 702819 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

In the rebaseline code, replace `test_prefix_list` with a class.

Project Member Reported by qyears...@chromium.org, Mar 17 2017

Issue description

In webkitpy/tool/commands/rebaseline.py and related code, there are now many places where a variable called `test_prefix_list` is used, which is expected to be a dict with a specific structure. This would be easier to understand if there were a class to represent this structure and related operations.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 18 2017

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

commit 4494b892750fe71d43c2dc0cef3247a38d777b42
Author: qyearsley <qyearsley@chromium.org>
Date: Sat Mar 18 02:16:08 2017

Simplification: Remove support for specifying suffix lists when rebaselining.

Reason: This would simplify the test_prefix_list variables and related code.

It would also remove support for passing suffixes to `webkit-patch
rebaseline`, but in practice, I think that's not a very useful feature
anyway because when one wants to rebaseline a test, one generally wants
to download all relevant baselines (both text and image, for example).

BUG= 702819 

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

[modify] https://crrev.com/4494b892750fe71d43c2dc0cef3247a38d777b42/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
[modify] https://crrev.com/4494b892750fe71d43c2dc0cef3247a38d777b42/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py
[modify] https://crrev.com/4494b892750fe71d43c2dc0cef3247a38d777b42/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl_unittest.py
[modify] https://crrev.com/4494b892750fe71d43c2dc0cef3247a38d777b42/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py

Owner: qyears...@chromium.org
Status: Started (was: Available)
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 21 2017

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

commit 09067d4bab0caeb8e1409148c9d6beabf762fd39
Author: qyearsley <qyearsley@chromium.org>
Date: Tue Mar 21 22:53:45 2017

Refactoring: Replace test_prefix_list variables with TestBaselineSet objects.

Purpose: I think that this improves the readability of the code, since
it replaces dicts with a specific format that are being passed around
with instances of a class, and that class has documentation and tests.

Also, this should make it easier to fix  http://crbug.com/673966 ; I'm
thinking that TestBaselineSet could be expanded to specify port names
separately from build names, so that rebaseline_cl could specify for
example that it wants to get baselines for some test from some Build,
but that the baseline location should correspond to another port's
baseline location.

This is a refactoring CL, so behavior should not change. In this CL:
 - Adds a class called TestBaselineSet (I'm open to name suggestions...)
   which represents some combinations of tests and platforms to rebaseline.
 - Adds a test for that class.
 - Updates other tests.
 - Removes _log_test_prefix_list; this is replaced with __str__ on the
   new class.
 - Refactors iteration through the collection of tests/builds to rebaseline.
 - Removes redundant method test_execute_with_issue_number_from_branch
   in rebaseline_cl_unittest.

BUG= 702819 

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

[modify] https://crrev.com/09067d4bab0caeb8e1409148c9d6beabf762fd39/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/auto_rebaseline.py
[modify] https://crrev.com/09067d4bab0caeb8e1409148c9d6beabf762fd39/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
[modify] https://crrev.com/09067d4bab0caeb8e1409148c9d6beabf762fd39/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py
[modify] https://crrev.com/09067d4bab0caeb8e1409148c9d6beabf762fd39/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl_unittest.py
[modify] https://crrev.com/09067d4bab0caeb8e1409148c9d6beabf762fd39/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py

Status: Fixed (was: Started)
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 23 2017

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

commit a75871227d372a8b0ddd0638a075b56eff4f7380
Author: qyearsley <qyearsley@chromium.org>
Date: Thu Mar 23 20:07:06 2017

Improve performance of rebaselining after TestBaselineSet change.

After https://codereview.chromium.org/2765863003, performance of
webkit-patch rebaseline-cl when rebaselining many tests regressed,
and there were a couple places where it looks like the code
was repeating work unnecessarily.

BUG= 702819 

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

[modify] https://crrev.com/a75871227d372a8b0ddd0638a075b56eff4f7380/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py

Comment 6 by drott@chromium.org, Mar 24 2017

Yes, it did occur to me that rebaseline-cl was taking its time. I just thought it was because I was rebaselining a larger number of tests, but yes, I was a bit surprised why it took that long. Thanks for the fix!

 Issue 650774  has been merged into this issue.

Sign in to add a comment