Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2445963002:
Remove all all-PASS testharness baselines after rebaselining. (Closed)
DescriptionRemove all all-PASS testharness baselines after rebaselining.
In http://crrev.com/2409903002 I added logic to webkit-patch
rebaseline-test-internal so that when rebaselining particular tests,
all-PASS testharness.js test baselines would be removed.
Later, I discovered that if a generic non-all-PASS baseline already
exists, it won't be removed when rebaselining, so that approach
doesn't quite solve the problem.
In general, we don't want any all-PASS baselines for testharness.js,
so this CL takes a different approach: After rebaselining, enumerate
all relevant baselines, and remove any all-PASS testharness.js
baselines. Removing such a baseline should never be incorrect.
BUG=654030
Committed: https://crrev.com/8a567b64e5bbed2d0f2c0cfdbce6e291b5d669ad
Cr-Commit-Position: refs/heads/master@{#427470}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Clarify returns section of docstring, sort baseline paths #
Messages
Total messages: 17 (8 generated)
qyearsley@chromium.org changed reviewers: + dpranke@chromium.org, wkorman@chromium.org
The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2445963002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py (right): https://codereview.chromium.org/2445963002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:529: new all-PASS baselines may be downloaded, but they should not be kept. Is removing restoring the repo to the baselines-don't-exist state, or are we relying on a subsequent 'git checkout' or otherwise to restore them? https://codereview.chromium.org/2445963002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:537: contents = filesystem.read_text_file(path) How many of these do we expect there to be typically? I assume on the order of tens that would show up in a change that actually affected some of these; and that in a degenerate case there are maybe hundreds (thousands?) in LayoutTests directory. In any case it's probably fast enough even in that degenerate case but figured worth a checkpoint.
https://codereview.chromium.org/2445963002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py (right): https://codereview.chromium.org/2445963002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:529: new all-PASS baselines may be downloaded, but they should not be kept. On 2016/10/25 at 00:14:20, wkorman wrote: > Is removing restoring the repo to the baselines-don't-exist state, or are we relying on a subsequent 'git checkout' or otherwise to restore them? I think it could be either removing an existing file or removing a newly-added file. If there were existing baselines with FAIL lines but now they're passing, then rebaselining should remove the file, so in general we don't want to restore the file (although the user could use git checkout to get the file back if they want to). https://codereview.chromium.org/2445963002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:537: contents = filesystem.read_text_file(path) On 2016/10/25 at 00:14:20, wkorman wrote: > How many of these do we expect there to be typically? I assume on the order of tens that would show up in a change that actually affected some of these; and that in a degenerate case there are maybe hundreds (thousands?) in LayoutTests directory. In any case it's probably fast enough even in that degenerate case but figured worth a checkpoint. Right -- hypothetically there could be hundreds of tests tests that all have platform-specific baselines for 10 platforms, but reading up to 10000 small-ish text files should hopefully be alright... Note: I just tried `time cat $(find . -name '*.html' | head -n 10000) | wc -l`, which read 406854 lines from 10000 files in about 1-2 seconds on my workstation. But in general, there would more likely be less than 100 tests with 1-5 baselines each. https://codereview.chromium.org/2445963002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:552: A list of absolute paths to baseline files. Currently this actually returns potential places where there *could* be baselines, not places where baseline files currently actually exist. If it makes more sense, then maybe this file could be changed to only return paths for files that exist?
https://codereview.chromium.org/2445963002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py (right): https://codereview.chromium.org/2445963002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:552: A list of absolute paths to baseline files. On 2016/10/25 00:33:04, qyearsley wrote: > Currently this actually returns potential places where there *could* be > baselines, not places where baseline files currently actually exist. If it makes > more sense, then maybe this file could be changed to only return paths for files > that exist? Seems reasonable, this method is only used in this file, are you talking about changing the results returned from other methods? The only thing I could see as issue is introducing file I/O ops where there are none today.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2445963002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py (right): https://codereview.chromium.org/2445963002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py:552: A list of absolute paths to baseline files. On 2016/10/25 at 00:37:17, wkorman wrote: > On 2016/10/25 00:33:04, qyearsley wrote: > > Currently this actually returns potential places where there *could* be > > baselines, not places where baseline files currently actually exist. If it makes > > more sense, then maybe this file could be changed to only return paths for files > > that exist? > > Seems reasonable, this method is only used in this file, are you talking about changing the results returned from other methods? The only thing I could see as issue is introducing file I/O ops where there are none today. I was thinking of filtering out files that don't exist in this function rather than in _remove_all_pass_testharness_baselines, but it's basically the same one way or the other. I added a note in the docstring of this function to note that it may return file paths for files that don't exist.
The CQ bit was checked by qyearsley@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org Link to the patchset: https://codereview.chromium.org/2445963002/#ps20001 (title: "Clarify returns section of docstring, sort baseline paths")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Remove all all-PASS testharness baselines after rebaselining. In http://crrev.com/2409903002 I added logic to webkit-patch rebaseline-test-internal so that when rebaselining particular tests, all-PASS testharness.js test baselines would be removed. Later, I discovered that if a generic non-all-PASS baseline already exists, it won't be removed when rebaselining, so that approach doesn't quite solve the problem. In general, we don't want any all-PASS baselines for testharness.js, so this CL takes a different approach: After rebaselining, enumerate all relevant baselines, and remove any all-PASS testharness.js baselines. Removing such a baseline should never be incorrect. BUG=654030 ========== to ========== Remove all all-PASS testharness baselines after rebaselining. In http://crrev.com/2409903002 I added logic to webkit-patch rebaseline-test-internal so that when rebaselining particular tests, all-PASS testharness.js test baselines would be removed. Later, I discovered that if a generic non-all-PASS baseline already exists, it won't be removed when rebaselining, so that approach doesn't quite solve the problem. In general, we don't want any all-PASS baselines for testharness.js, so this CL takes a different approach: After rebaselining, enumerate all relevant baselines, and remove any all-PASS testharness.js baselines. Removing such a baseline should never be incorrect. BUG=654030 Committed: https://crrev.com/8a567b64e5bbed2d0f2c0cfdbce6e291b5d669ad Cr-Commit-Position: refs/heads/master@{#427470} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8a567b64e5bbed2d0f2c0cfdbce6e291b5d669ad Cr-Commit-Position: refs/heads/master@{#427470}
Message was sent while issue was closed.
lgtm also |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
