Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2119303003:
Create the harness and shell of update_test_expectations (Closed)
DescriptionCreate the harness and shell of update_test_expectations
This patch creates the main classes and stubs out the main methods. The meat of the
process is left for the next patch. update_test_expectations is a script that looks
at the TestExpectations file and checks each line against results from the build
waterfall to determine if a line is still flaky. Non-flaky entries in TestExpectations
are then removed.
BUG=595414
Committed: https://crrev.com/d70d9e2e8f1c9efb80d1dd1146816d5f265f367a
Cr-Commit-Position: refs/heads/master@{#404402}
Patch Set 1 #Patch Set 2 : Create the harness and shell of update_test_expectations #
Total comments: 28
Patch Set 3 : Addressed Quinten's comments #
Depends on Patchset: Messages
Total messages: 18 (8 generated)
Description was changed from ========== [update-test-expectations.py 2/3] Create the harness and shell of update_test_expectations BUG=595414 ========== to ========== [update-test-expectations.py 2/3] Create the harness and shell of update_test_expectations BUG=595414 ==========
bokan@chromium.org changed reviewers: + dpranke@chromium.org, qyearsley@chromium.org
Description was changed from ========== [update-test-expectations.py 2/3] Create the harness and shell of update_test_expectations BUG=595414 ========== to ========== [update-test-expectations.py 2/3] Create the harness and shell of update_test_expectations This patch creates the main classes and stubs out the main methods. The meat of the process is left for the next patch but the surroundings are all here. BUG=595414 ==========
https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/update-test-expectations (right): https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/update-test-expectations:14: update_test_expectations.main(_host, BotTestExpectationsFactory(_host.builders), sys.argv[1:]) Lines 13-14 should be wrapped in an if __name__ == '__main__': just to be consistent w/ other code. https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py (right): https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:68: Reads the current TestExpectatoins file and, using results from the Typo: "TestExpectations". https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:82: test_expectatoins: The TestExpectations object to write. ditto. https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py (right): https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:72: self._configuration_specifier_macros) as suggested in the earlier CL, update these names to refer to public instance variables, e.g., port.all_build_systems = ...
In this CL currently, is the unit test is testing functionality which is not yet in update_test_expectations.py? https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py (right): https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:24: Nit: This blank line could be removed. https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:37: parser.parse_args(argv) What args will this take? Later will you want to change this to `args = parser.parse_args(argv)` and then use `args` below? https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:46: return None Nit, also could just `return` instead of `return None`, to indicate that the purpose of this is to exit the function, not to return the value None. There are also other ways this could be done if you want to indicate an error with a non-zero exit code: 1. this function could return an exit code and in update-test-expectations you could call sys.exit(update_test_expectations.main(sys.argv[1:])) 2. Or, this function could call sys.exit() itself https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:72: Returns: A TestExpectations object with the passing lines filtered out. Nit: this could be formatted as: Returns: A TestExpectations object with the passing lines filtered out. https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py (right): https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:116: x.to_string(None) for x in expectations) Note: I wonder if it would be nice to change TestExpectationLine.to_string so that the argument `test_configuration_converter` is optional and has a default value of None. Then, this mysterious None wouldn't have to be passed in here. But this is separate from this CL. https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:223: """ Nit: end quote can go on the line above.
https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/update-test-expectations (right): https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/update-test-expectations:14: update_test_expectations.main(_host, BotTestExpectationsFactory(_host.builders), sys.argv[1:]) On 2016/07/06 21:51:44, Dirk Pranke wrote: > Lines 13-14 should be wrapped in an > > if __name__ == '__main__': > > just to be consistent w/ other code. Done. https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py (right): https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:24: On 2016/07/06 23:18:04, qyearsley wrote: > Nit: This blank line could be removed. Done. https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:37: parser.parse_args(argv) On 2016/07/06 23:18:04, qyearsley wrote: > What args will this take? Later will you want to change this to `args = > parser.parse_args(argv)` and then use `args` below? None at the moment, I added this to get --help, but there'll definitely be things in the future I could imagine. e.g. --run-on-slow-tests, --open-in-flakiness-dashboard, etc. https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:46: return None On 2016/07/06 23:18:04, qyearsley wrote: > Nit, also could just `return` instead of `return None`, to indicate that the > purpose of this is to exit the function, not to return the value None. > > There are also other ways this could be done if you want to indicate an error > with a non-zero exit code: > > 1. this function could return an exit code and in update-test-expectations you > could call sys.exit(update_test_expectations.main(sys.argv[1:])) > 2. Or, this function could call sys.exit() itself Done, used #1 https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:68: Reads the current TestExpectatoins file and, using results from the On 2016/07/06 21:51:44, Dirk Pranke wrote: > Typo: "TestExpectations". Done. https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:72: Returns: A TestExpectations object with the passing lines filtered out. On 2016/07/06 23:18:04, qyearsley wrote: > Nit: this could be formatted as: > > Returns: > A TestExpectations object with the passing lines filtered out. Done. https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:82: test_expectatoins: The TestExpectations object to write. On 2016/07/06 21:51:44, Dirk Pranke wrote: > ditto. Done. https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py (right): https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:72: self._configuration_specifier_macros) On 2016/07/06 21:51:44, Dirk Pranke wrote: > as suggested in the earlier CL, update these names to refer to public instance > variables, e.g., port.all_build_systems = ... Done. https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:116: x.to_string(None) for x in expectations) On 2016/07/06 23:18:04, qyearsley wrote: > Note: I wonder if it would be nice to change TestExpectationLine.to_string so > that the argument `test_configuration_converter` is optional and has a default > value of None. Then, this mysterious None wouldn't have to be passed in here. > But this is separate from this CL. It turns out this is the only use of TestExpectationLine.to_string that uses all defaults so I went ahead and made None the default for the converter in this patch. https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:223: """ On 2016/07/06 23:18:04, qyearsley wrote: > Nit: end quote can go on the line above. Done. Does this go for multiline comments too or just the single lines? Is there a python style guide we use? Is it just the Google Python SG?
On 2016/07/06 23:18:05, qyearsley wrote: > In this CL currently, is the unit test is testing functionality which is not yet > in update_test_expectations.py? No, all the tests are testing what's currently there and all tests pass.
LGTM I like the unit tests, especially the fact that the unit test methods have clear names and explanations of what they're supposed to test. I added a couple more comments but I didn't see anything that really needs to be changed. https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py (right): https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:8: from collections import OrderedDict Nit: In most of the files, there's a blank line between the standard library imports and the webkitpy imports. https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:67: fake object will always return the 'test' port factory.""" I personally think that docstrings with a one-line summary sentence, then a blank line, then details are the easiest to scan, e.g. """Returns an object implementing the Port interface. This fake object will always return the 'test' port factory. """ https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:223: """ On 2016/07/07 at 22:08:32, bokan wrote: > On 2016/07/06 23:18:04, qyearsley wrote: > > Nit: end quote can go on the line above. > > Done. Does this go for multiline comments too or just the single lines? > > Is there a python style guide we use? Is it just the Google Python SG? The Python style guide for Blink just say to use PEP8 (see https://www.chromium.org/blink/coding-style#TOC-Python, https://www.python.org/dev/peps/pep-0008/). The relevant section in PEP8 is: https://www.python.org/dev/peps/pep-0008/#documentation-strings https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:225: # Even though the results show all passing, none of the I assume it's not a problem that these lines start with whitespace? If you wanted to make a multi-line string where the lines don't start with space and the formatting is still nice like this, you can use the fact that adjacent strings are concatenated in Python, e.g.: test_expectations_before = ( '# Even though the results show all passing, none of the\n' '# expectations are flaky so we shouldn't remove any.\n' 'Bug(test) test/a.html [ Failure NeedsRebaseline Pass ]\n')
https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py (right): https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:8: from collections import OrderedDict On 2016/07/08 00:06:21, qyearsley wrote: > Nit: In most of the files, there's a blank line between the standard library > imports and the webkitpy imports. Done. https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:67: fake object will always return the 'test' port factory.""" On 2016/07/08 00:06:21, qyearsley wrote: > I personally think that docstrings with a one-line summary sentence, then a > blank line, then details are the easiest to scan, e.g. > > """Returns an object implementing the Port interface. > > This fake object will always return the 'test' port factory. > """ Done. https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:223: """ On 2016/07/08 00:06:21, qyearsley wrote: > On 2016/07/07 at 22:08:32, bokan wrote: > > On 2016/07/06 23:18:04, qyearsley wrote: > > > Nit: end quote can go on the line above. > > > > Done. Does this go for multiline comments too or just the single lines? > > > > Is there a python style guide we use? Is it just the Google Python SG? > > The Python style guide for Blink just say to use PEP8 (see > https://www.chromium.org/blink/coding-style#TOC-Python, > https://www.python.org/dev/peps/pep-0008/). The relevant section in PEP8 is: > https://www.python.org/dev/peps/pep-0008/#documentation-strings Thanks! https://codereview.chromium.org/2119303003/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:225: # Even though the results show all passing, none of the On 2016/07/08 00:06:21, qyearsley wrote: > I assume it's not a problem that these lines start with whitespace? > > If you wanted to make a multi-line string where the lines don't start with space > and the formatting is still nice like this, you can use the fact that adjacent > strings are concatenated in Python, e.g.: > > test_expectations_before = ( > '# Even though the results show all passing, none of the\n' > '# expectations are flaky so we shouldn't remove any.\n' > 'Bug(test) test/a.html [ Failure NeedsRebaseline Pass ]\n') I think this is easier to read and the assert_expectations_match method ignores surrounding white space so it doesn't matter in these cases. Also, TestExpectations can start with a blank so it's a valid file.
The CQ bit was checked by bokan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from qyearsley@chromium.org Link to the patchset: https://codereview.chromium.org/2119303003/#ps40001 (title: "Addressed Quinten's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [update-test-expectations.py 2/3] Create the harness and shell of update_test_expectations This patch creates the main classes and stubs out the main methods. The meat of the process is left for the next patch but the surroundings are all here. BUG=595414 ========== to ========== Create the harness and shell of update_test_expectations This patch creates the main classes and stubs out the main methods. The meat of the process is left for the next patch. update_test_expectations is a script that looks at the TestExpectations file and checks each line against results from the build waterfall to determine if a line is still flaky. Non-flaky entries in TestExpectations are then removed. BUG=595414 ==========
Message was sent while issue was closed.
Description was changed from ========== Create the harness and shell of update_test_expectations This patch creates the main classes and stubs out the main methods. The meat of the process is left for the next patch. update_test_expectations is a script that looks at the TestExpectations file and checks each line against results from the build waterfall to determine if a line is still flaky. Non-flaky entries in TestExpectations are then removed. BUG=595414 ========== to ========== Create the harness and shell of update_test_expectations This patch creates the main classes and stubs out the main methods. The meat of the process is left for the next patch. update_test_expectations is a script that looks at the TestExpectations file and checks each line against results from the build waterfall to determine if a line is still flaky. Non-flaky entries in TestExpectations are then removed. BUG=595414 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Create the harness and shell of update_test_expectations This patch creates the main classes and stubs out the main methods. The meat of the process is left for the next patch. update_test_expectations is a script that looks at the TestExpectations file and checks each line against results from the build waterfall to determine if a line is still flaky. Non-flaky entries in TestExpectations are then removed. BUG=595414 ========== to ========== Create the harness and shell of update_test_expectations This patch creates the main classes and stubs out the main methods. The meat of the process is left for the next patch. update_test_expectations is a script that looks at the TestExpectations file and checks each line against results from the build waterfall to determine if a line is still flaky. Non-flaky entries in TestExpectations are then removed. BUG=595414 Committed: https://crrev.com/d70d9e2e8f1c9efb80d1dd1146816d5f265f367a Cr-Commit-Position: refs/heads/master@{#404402} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d70d9e2e8f1c9efb80d1dd1146816d5f265f367a Cr-Commit-Position: refs/heads/master@{#404402} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
