Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(254)

Issue 2456073002: Rename test port baseline directories in unit tests. (Closed)

Created:
4 years, 1 month ago by qyearsley
Modified:
4 years, 1 month ago
Reviewers:
Dirk Pranke
CC:
blink-reviews, chromium-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Rename test port baseline directories in unit tests. Reason: This CL changes the example platform-specific baseline directory strings in order to highlight the fact that the directories don't have to be port names, and they can be general-sounding directories like "win" or "mac".

Patch Set 1 #

Patch Set 2 : Whitespace / comments #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+37 lines, -35 lines) Patch
M third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py View 1 1 chunk +8 lines, -6 lines 1 comment Download
M third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py View 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/views/printing_unittest.py View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py View 1 11 chunks +25 lines, -25 lines 0 comments Download

Messages

Total messages: 7 (2 generated)
qyearsley
4 years, 1 month ago (2016-10-27 17:03:18 UTC) #3
Dirk Pranke
https://codereview.chromium.org/2456073002/diff/20001/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py (right): https://codereview.chromium.org/2456073002/diff/20001/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py#newcode369 third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py:369: # these directory names don't necessarily have to be ...
4 years, 1 month ago (2016-10-27 21:01:44 UTC) #4
qyearsley
On 2016/10/27 at 21:01:44, dpranke wrote: > https://codereview.chromium.org/2456073002/diff/20001/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py (right): > > https://codereview.chromium.org/2456073002/diff/20001/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py#newcode369 ...
4 years, 1 month ago (2016-10-27 21:34:54 UTC) #5
Dirk Pranke
On 2016/10/27 21:34:54, qyearsley wrote: > On 2016/10/27 at 21:01:44, dpranke wrote: > > > ...
4 years, 1 month ago (2016-10-27 22:35:05 UTC) #6
qyearsley
4 years, 1 month ago (2016-10-28 00:11:25 UTC) #7
On 2016/10/27 at 22:35:05, dpranke wrote:
> On 2016/10/27 21:34:54, qyearsley wrote:
> > On 2016/10/27 at 21:01:44, dpranke wrote:
> > >
> >
https://codereview.chromium.org/2456073002/diff/20001/third_party/WebKit/Tool...
> > > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py
> > (right):
> > > 
> > >
> >
https://codereview.chromium.org/2456073002/diff/20001/third_party/WebKit/Tool...
> > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py:369: #
> > these directory names don't necessarily have to be port names.
> > > They don't have to be, but it's probably a good idea for them to always
be.
> > So, you might want to change things to enforce that they are instead.
> > 
> > Alright -- right now the directory names are always either port names
("linux",
> > "mac", ...) or "full port names" (port name + "-" + version).
> > So maybe it would make sense to assert that somewhere?
> > 
> > Relatedly, this morning I also noticed something that I wanted to ask about:
> > 
> > There's a method Port.baseline_platform_dir(), which always returns the
> > directory with just a port name (no version name); I think that this is used
> > when deciding where to put new baselines when the flag --new-test-results is
> > used, depending on the type of baseline: audio baselines and render tree
dump
> > baselines are put in the baseline platform dir:
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...
> > 
> > But I think that this is different from how baselines are usually treated
now --
> > when deciding where to put baselines when rebaselining, we now usually don't
> > care what type of baseline it is; it's just put into the first directory in
the
> > baseline search path, and then de-duped. Is that right?
> 
> You can run run-webkit-tests in a way such that it ignores what the existing
baselines
> are and just generates new baselines. In that situation, there are three
different options 
> for controlling where new baselines are put: --new-test-results which does
what you say, 
> --new-baseline which writes to baseline_platform_dir(), and --reset-results,
which
> overwrites the existing baseline used on the port, whereever it is found.
> 
> The theory is that you might know whether the test results should produce one
kind of
> result or another and can then put the new baselines in the right place.
> 
> All of this long predates rebaseline-o-matic and rebaseline-cl. It's unclear
to me which
> of these options are still being used. --new-baseline is probably the least
commonly
> used, and it might be that only --reset-results is really useful. The reason
that's useful
> is because lots of tests only have one generic baseline, and you can know you
can safely
> just update it in your initial patch and not have to gather any
platform-specific versions.

Ah, that's good to know -- definitely sounds like something that could
potentially
be simplified.

Closing this CL, and filed http://crbug.com/660234 for the suggestion of
enforcing that baseline directories are port names.

Powered by Google App Engine
This is Rietveld 408576698