Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2380173002:
Change update_test_expectations to not require builders for all configurations. (Closed)
DescriptionChange update_test_expectations to not require builders for all configurations.
This makes the update-test-expectations script less conservative.
The rationale is that there are some configurations for which we
don't have any builders, which are checked. Specifically:
[icecreamsandwich, x86, debug]
[icecreamsandwich, x86, release]
[mac10.10, x86, debug]
[mac10.9, x86, debug]
[retina, x86, debug]
[trusty, x86_64, debug]
[win10, x86, debug]
But if a test that's marked as flaky appears to be non-flaky on all builders that actually do exist, this seems like it might be adequate reason to remove the line.
Also in this CL:
Add more logging and a --verbose option, so that:
(1) by default when running the script would log what it's deleting.
(2) if --verbose is passed it will also log more information about why.
BUG=595414
Committed: https://crrev.com/89ff824cd7a145611011394e0bf8a90957cee81e
Cr-Commit-Position: refs/heads/master@{#421983}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Change one log line to warning, update docstring for test method #
Messages
Total messages: 19 (10 generated)
Description was changed from ========== Change update_test_expecatations to not require builders for all configurations. This makes the update-test-expectations script less conservative. The rationale is that there are some configurations for which we don't have any builders and aren't planning to have builders: Also in this CL: Add more logging and a --verbose option, so that: (1) by default when running the script would log what it's deleting. (2) if --verbose is passed it will also log more information about why. BUG=595414 ========== to ========== Change update_test_expecatations to not require builders for all configurations. This makes the update-test-expectations script less conservative. The rationale is that there are some configurations for which we don't have any builders, which are checked. Specifically: [icecreamsandwich, x86, debug] [icecreamsandwich, x86, release] [mac10.10, x86, debug] [mac10.9, x86, debug] [retina, x86, debug] [trusty, x86_64, debug] [win10, x86, debug] But if a test that's marked as flaky appears to be non-flaky on all builders that actually do exist, this seems like it might be adequate reason to remove the line. Also in this CL: Add more logging and a --verbose option, so that: (1) by default when running the script would log what it's deleting. (2) if --verbose is passed it will also log more information about why. BUG=595414 ==========
qyearsley@chromium.org changed reviewers: + bokan@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...
https://codereview.chromium.org/2380173002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py (right): https://codereview.chromium.org/2380173002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:39: logging.basicConfig(level=logging.DEBUG if args.verbose else logging.INFO, format="%(levelname)s: %(message)s") This could also be changed to have the level WARNING by default and change it to INFO if --verbose is passed, which is how some other scripts are, I think. https://codereview.chromium.org/2380173002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:127: _log.debug('No matching builders for line, deleting line.') Potentially this could be changed to emit an error and not delete a line if there's a line that has no matching builders (which I wouldn't expect to happen).
On 2016/09/29 at 20:36:14, qyearsley wrote: > https://codereview.chromium.org/2380173002/diff/1/third_party/WebKit/Tools/Sc... > File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py (right): > > https://codereview.chromium.org/2380173002/diff/1/third_party/WebKit/Tools/Sc... > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:39: logging.basicConfig(level=logging.DEBUG if args.verbose else logging.INFO, format="%(levelname)s: %(message)s") > This could also be changed to have the level WARNING by default and change it to INFO if --verbose is passed, which is how some other scripts are, I think. > > https://codereview.chromium.org/2380173002/diff/1/third_party/WebKit/Tools/Sc... > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:127: _log.debug('No matching builders for line, deleting line.') > Potentially this could be changed to emit an error and not delete a line if there's a line that has no matching builders (which I wouldn't expect to happen). Just made a CL after running update-test-expectations after making this change to show what difference it would make in the real TestExpectations file: http://crrev.com/2384573002.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Minor comments but lgtm once they're addressed. Thanks for carrying this forward! https://codereview.chromium.org/2380173002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py (right): https://codereview.chromium.org/2380173002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:39: logging.basicConfig(level=logging.DEBUG if args.verbose else logging.INFO, format="%(levelname)s: %(message)s") On 2016/09/29 20:36:13, qyearsley wrote: > This could also be changed to have the level WARNING by default and change it to > INFO if --verbose is passed, which is how some other scripts are, I think. Either sound fine to me. I'm not super familiar with our script collection so you have better insight into this than me. https://codereview.chromium.org/2380173002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:127: _log.debug('No matching builders for line, deleting line.') On 2016/09/29 20:36:13, qyearsley wrote: > Potentially this could be changed to emit an error and not delete a line if > there's a line that has no matching builders (which I wouldn't expect to > happen). I think deleting the line is probably the right thing but I would output this as a warning so that the user knows there was something unusual there. https://codereview.chromium.org/2380173002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py (right): https://codereview.chromium.org/2380173002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:676: that have builders, then it can be removed, even if there are extra Looks like this comment cuts off early?
By the way, it looks like this is at least basically usable now, can I close the bug?
https://codereview.chromium.org/2380173002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py (right): https://codereview.chromium.org/2380173002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:39: logging.basicConfig(level=logging.DEBUG if args.verbose else logging.INFO, format="%(levelname)s: %(message)s") On 2016/09/29 at 21:55:21, bokan wrote: > On 2016/09/29 20:36:13, qyearsley wrote: > > This could also be changed to have the level WARNING by default and change it to > > INFO if --verbose is passed, which is how some other scripts are, I think. > > Either sound fine to me. I'm not super familiar with our script collection so you have better insight into this than me. Leaving as-is for now. https://codereview.chromium.org/2380173002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:127: _log.debug('No matching builders for line, deleting line.') On 2016/09/29 at 21:55:21, bokan wrote: > On 2016/09/29 20:36:13, qyearsley wrote: > > Potentially this could be changed to emit an error and not delete a line if > > there's a line that has no matching builders (which I wouldn't expect to > > happen). > > I think deleting the line is probably the right thing but I would output this as a warning so that the user knows there was something unusual there. Done https://codereview.chromium.org/2380173002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py (right): https://codereview.chromium.org/2380173002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:676: that have builders, then it can be removed, even if there are extra On 2016/09/29 at 21:55:21, bokan wrote: > Looks like this comment cuts off early? Ah, looks like I started to write and then got distracted :-P
On 2016/09/29 at 21:58:34, bokan wrote: > By the way, it looks like this is at least basically usable now, can I close the bug? Yep! Later when we revisit this, I think we can file new bugs for new tasks. Also, I added an OKR for team blink-infra that says "Automatically remove lines from TestExpectations that can be removed based on latest results." -- would you consider that to be done? Feel free to go and update that OKR :-)
The CQ bit was checked by qyearsley@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org Link to the patchset: https://codereview.chromium.org/2380173002/#ps20001 (title: "Change one log line to warning, update docstring for test method")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Change update_test_expecatations to not require builders for all configurations. This makes the update-test-expectations script less conservative. The rationale is that there are some configurations for which we don't have any builders, which are checked. Specifically: [icecreamsandwich, x86, debug] [icecreamsandwich, x86, release] [mac10.10, x86, debug] [mac10.9, x86, debug] [retina, x86, debug] [trusty, x86_64, debug] [win10, x86, debug] But if a test that's marked as flaky appears to be non-flaky on all builders that actually do exist, this seems like it might be adequate reason to remove the line. Also in this CL: Add more logging and a --verbose option, so that: (1) by default when running the script would log what it's deleting. (2) if --verbose is passed it will also log more information about why. BUG=595414 ========== to ========== Change update_test_expectations to not require builders for all configurations. This makes the update-test-expectations script less conservative. The rationale is that there are some configurations for which we don't have any builders, which are checked. Specifically: [icecreamsandwich, x86, debug] [icecreamsandwich, x86, release] [mac10.10, x86, debug] [mac10.9, x86, debug] [retina, x86, debug] [trusty, x86_64, debug] [win10, x86, debug] But if a test that's marked as flaky appears to be non-flaky on all builders that actually do exist, this seems like it might be adequate reason to remove the line. Also in this CL: Add more logging and a --verbose option, so that: (1) by default when running the script would log what it's deleting. (2) if --verbose is passed it will also log more information about why. BUG=595414 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Change update_test_expectations to not require builders for all configurations. This makes the update-test-expectations script less conservative. The rationale is that there are some configurations for which we don't have any builders, which are checked. Specifically: [icecreamsandwich, x86, debug] [icecreamsandwich, x86, release] [mac10.10, x86, debug] [mac10.9, x86, debug] [retina, x86, debug] [trusty, x86_64, debug] [win10, x86, debug] But if a test that's marked as flaky appears to be non-flaky on all builders that actually do exist, this seems like it might be adequate reason to remove the line. Also in this CL: Add more logging and a --verbose option, so that: (1) by default when running the script would log what it's deleting. (2) if --verbose is passed it will also log more information about why. BUG=595414 ========== to ========== Change update_test_expectations to not require builders for all configurations. This makes the update-test-expectations script less conservative. The rationale is that there are some configurations for which we don't have any builders, which are checked. Specifically: [icecreamsandwich, x86, debug] [icecreamsandwich, x86, release] [mac10.10, x86, debug] [mac10.9, x86, debug] [retina, x86, debug] [trusty, x86_64, debug] [win10, x86, debug] But if a test that's marked as flaky appears to be non-flaky on all builders that actually do exist, this seems like it might be adequate reason to remove the line. Also in this CL: Add more logging and a --verbose option, so that: (1) by default when running the script would log what it's deleting. (2) if --verbose is passed it will also log more information about why. BUG=595414 Committed: https://crrev.com/89ff824cd7a145611011394e0bf8a90957cee81e Cr-Commit-Position: refs/heads/master@{#421983} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/89ff824cd7a145611011394e0bf8a90957cee81e Cr-Commit-Position: refs/heads/master@{#421983} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
