Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2759263002:
Add image-first-tests flag to run-webkit-tests (Closed)
|
Created:
3 years, 9 months ago by Gleb Lanbin Modified:
3 years, 9 months ago CC:
blink-reviews, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd image-first-tests flag to run-webkit-tests
--image-first-tests specifies directories or test that will be only
checked against image expectations if available and fall back to text
expectations otherwise.
The reason for this change:
Layout team is working on the new layout engine LayoutNG
http://goo.gl/1hwhfX that may change the output of layout tree that
we use in LayoutTests. So we want to experiment with LayoutTest runner
and switch some LayoutTests folders to rely on image expectations only.
BUG=703314
Review-Url: https://codereview.chromium.org/2759263002
Cr-Commit-Position: refs/heads/master@{#458850}
Committed: https://chromium.googlesource.com/chromium/src/+/f77fa82e53b77897192ea45788e4b2f5e697190b
Patch Set 1 #
Total comments: 8
Patch Set 2 : fix comments #
Total comments: 2
Patch Set 3 : fix more comments #Patch Set 4 : Only run compare_txt_fn if !is_testharness_test #
Messages
Total messages: 26 (14 generated)
glebl@chromium.org changed reviewers: + dpranke@chromium.org, eae@chromium.org
I like this change and think it makes a lot of sense for many of the layout layout tests where we only care about the green box being the right size and at the right coordinate and not the text describing it or on exactly which pixel boundary it fell. Dirk, what is your take?
dpranke@chromium.org changed reviewers: + jeffcarp@chromium.org, qyearsley@chromium.org
+qyearsley, +jeffcarp who are the real owners of this code now. I discussed this approach w/ Gleb at some point and it seems fine. I will point out, of course, that if there's visible text in the test it'll still need to be in the right places, but this will obviate the need to match the layout tree, which I'm assuming is pointless in this case anyway. https://codereview.chromium.org/2759263002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py (right): https://codereview.chromium.org/2759263002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:30: from collections import namedtuple Nit: this line goes between lines 32 and 34. https://codereview.chromium.org/2759263002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:290: function = namedtuple('Function', 'call args') Nit: I would do this slightly different. I'd just use unnamed 3-tuples here. https://codereview.chromium.org/2759263002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:310: for compare_function in compare_functions: and change this for line to: for func, first_arg, second_arg in compare_functions: failures.extend(func(first_arg, second_arg)) https://codereview.chromium.org/2759263002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): https://codereview.chromium.org/2759263002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:246: 'multiple times)'), Is there a reason to make this a command line argument rather than a hard-coded list somewhere in the code or in a config file? Where and when would we expect these flags to be passed?
> I will point out, of course, that if there's visible text in the test it'll > still need to be in the right places, but this will obviate the need to > match > the layout tree, which I'm assuming is pointless in this case anyway. Right, didn't mean to suggest otherwise. Poor example on my part. Thanks -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
> I will point out, of course, that if there's visible text in the test it'll > still need to be in the right places, but this will obviate the need to > match > the layout tree, which I'm assuming is pointless in this case anyway. Right, didn't mean to suggest otherwise. Poor example on my part. Thanks -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2759263002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py (right): https://codereview.chromium.org/2759263002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:30: from collections import namedtuple On 2017/03/20 23:22:27, Dirk Pranke wrote: > Nit: this line goes between lines 32 and 34. Done. https://codereview.chromium.org/2759263002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:290: function = namedtuple('Function', 'call args') On 2017/03/20 23:22:27, Dirk Pranke wrote: > Nit: I would do this slightly different. I'd just use unnamed 3-tuples here. Done. https://codereview.chromium.org/2759263002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:310: for compare_function in compare_functions: On 2017/03/20 23:22:27, Dirk Pranke wrote: > and change this for line to: > > for func, first_arg, second_arg in compare_functions: > failures.extend(func(first_arg, second_arg)) partially done. I switched to an unnamed tuple but I use 2 items tuple here. That's because I don't want to limit the number of parameters that can be used with a compare_function. https://codereview.chromium.org/2759263002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): https://codereview.chromium.org/2759263002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:246: 'multiple times)'), On 2017/03/20 23:22:27, Dirk Pranke wrote: > Is there a reason to make this a command line argument rather than a hard-coded > list somewhere in the code or in a config file? > > Where and when would we expect these flags to be passed? The initial plan was to add a flag that can be used locally for testing and with trybots(we need to update trybot JSON recipe for this?). So we want to start with the limited set of folders, experiment with it a little bit and if there are no complaints then send an announcement to blink-dev and make this testing approach as default. After that we can remove this flag. If you say that the updating trybot JSON configuration would be problematic then we can use the "default" option at the line 241 to set all hardcoded values. I can add a TODO to the flag's description and attach it to the bug so we can delete this flag later. What do you think?
LGTM. On 2017/03/21 17:31:52, Gleb Lanbin wrote: > On 2017/03/20 23:22:27, Dirk Pranke wrote: > > Is there a reason to make this a command line argument rather than a > hard-coded > > list somewhere in the code or in a config file? > > > > Where and when would we expect these flags to be passed? > > The initial plan was to add a flag that can be used locally for testing and with > trybots(we need to update trybot JSON recipe for this?). > > So we want to start with the limited set of folders, experiment with it a little > bit and if there are no complaints then send an announcement to blink-dev and > make this testing approach as default. After that we can remove this flag. > > If you say that the updating trybot JSON configuration would be problematic then > we can use the "default" option at the line 241 to set all hardcoded values. I > can add a TODO to the flag's description and attach it to the bug so we can > delete this flag later. What do you think? We're close to switching the layout tests over to run on swarming, and so the way we configure how and where the steps will run is changing. It's easy enough to add flags one place or another as needed when we're ready, but let's hold off on doing so until we know what we want to run and on what bots (which I'd guess is still a week or two away)? At some point, if the list of directories starts to become longer than, say, half a dozen, trying to keep the command lines in sync will become annoying and we should probably switch to a checked-in file like LayoutTests/ImageOnlyDirectories instead.
LGTM :-) Should there be an issue filed for (possibly, later) switching over all pixel tests to operate in this mode (ignoring text render tree baselines if an image baseline is available, and using the text render tree baseline as a backup if no image baseline exists)? https://codereview.chromium.org/2759263002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py (right): https://codereview.chromium.org/2759263002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:295: expected_driver_output.audio, driver_output.audio)) Formatting note: Technically, the line length limit in Tools/Scripts/webkitpy is 132 so you could use longer lines if you want to. https://codereview.chromium.org/2759263002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): https://codereview.chromium.org/2759263002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:246: 'multiple times)'), This help text could probably be made a bit clearer. Does it sound correct to say: "A directory (or test) where the test result will only be compared with the image baseline if an image baseline is available, and fall back to comparison with the text baseline when image baselines are missing. Specify multiple times to add multiple directories." Since this argument is sort of parallel to --pixel-test-directory, you could also change the option name and dest etc. to be more like --pixel-test-directory. optparse.make_option( '--image-first-directory', action='append', default=[], dest='image_first_directories', help=('A directory (or test) where the test result will ' 'only be compared with the image baseline if an image ' 'baseline is available, and fall back to comparison ' 'with the text baseline when image baselines are missing. ' 'Specify multiple times to add multiple directories.')),
On 2017/03/21 22:44:10, qyearsley wrote: > LGTM :-) > > Should there be an issue filed for (possibly, later) switching over all pixel > tests to operate in this mode (ignoring text render tree baselines if an image > baseline is available, and using the text render tree baseline as a backup if no > image baseline exists)? done. thanks http://crbug.com/703899 > > https://codereview.chromium.org/2759263002/diff/20001/third_party/WebKit/Tool... > File > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py > (right): > > https://codereview.chromium.org/2759263002/diff/20001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:295: > expected_driver_output.audio, driver_output.audio)) > Formatting note: Technically, the line length limit in Tools/Scripts/webkitpy is > 132 so you could use longer lines if you want to. > > https://codereview.chromium.org/2759263002/diff/20001/third_party/WebKit/Tool... > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py > (right): > > https://codereview.chromium.org/2759263002/diff/20001/third_party/WebKit/Tool... > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:246: > 'multiple times)'), > This help text could probably be made a bit clearer. Does it sound correct to > say: > "A directory (or test) where the test result will only be compared with the > image baseline if an image baseline is available, and fall back to comparison > with the text baseline when image baselines are missing. Specify multiple times > to add multiple directories." > > Since this argument is sort of parallel to --pixel-test-directory, you could > also change the option name and dest etc. to be more like > --pixel-test-directory. > > optparse.make_option( > '--image-first-directory', > action='append', > default=[], > dest='image_first_directories', > help=('A directory (or test) where the test result will ' > 'only be compared with the image baseline if an image ' > 'baseline is available, and fall back to comparison ' > 'with the text baseline when image baselines are missing. > ' > 'Specify multiple times to add multiple directories.')), thanks for the review.
The CQ bit was checked by glebl@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by glebl@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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by glebl@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, qyearsley@chromium.org Link to the patchset: https://codereview.chromium.org/2759263002/#ps60001 (title: "Only run compare_txt_fn if !is_testharness_test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490211970973650,
"parent_rev": "4e6cb54cd744456051a6fb5aca33e96ae7f14f1f", "commit_rev":
"f77fa82e53b77897192ea45788e4b2f5e697190b"}
Message was sent while issue was closed.
Description was changed from ========== Add image-first-tests flag to run-webkit-tests --image-first-tests specifies directories or test that will be only checked against image expectations if available and fall back to text expectations otherwise. The reason for this change: Layout team is working on the new layout engine LayoutNG http://goo.gl/1hwhfX that may change the output of layout tree that we use in LayoutTests. So we want to experiment with LayoutTest runner and switch some LayoutTests folders to rely on image expectations only. BUG=703314 ========== to ========== Add image-first-tests flag to run-webkit-tests --image-first-tests specifies directories or test that will be only checked against image expectations if available and fall back to text expectations otherwise. The reason for this change: Layout team is working on the new layout engine LayoutNG http://goo.gl/1hwhfX that may change the output of layout tree that we use in LayoutTests. So we want to experiment with LayoutTest runner and switch some LayoutTests folders to rely on image expectations only. BUG=703314 Review-Url: https://codereview.chromium.org/2759263002 Cr-Commit-Position: refs/heads/master@{#458850} Committed: https://chromium.googlesource.com/chromium/src/+/f77fa82e53b77897192ea45788e4... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/f77fa82e53b77897192ea45788e4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
