Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2136723002:
Create test expectations line automatically from failing test results (Closed)
|
Created:
4 years, 5 months ago by dcampb Modified:
4 years, 4 months ago CC:
blink-reviews, blink-reviews-w3ctests_chromium.org, cbentzel+watch_chromium.org, chromium-reviews, jsbell Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate Test Expectation lines automatically from failing test results.
BUG=625254
Committed: https://crrev.com/22c2cad13b358a133ae399532ce910c1ebb96073
Cr-Commit-Position: refs/heads/master@{#406868}
Patch Set 1 #Patch Set 2 : Remove Layouttestresults.py from patch #
Total comments: 37
Patch Set 3 : Create list of test expectations lines from failing test tree #
Total comments: 13
Patch Set 4 : adds unittests and implements a class in update_w3c_test_expectations #Patch Set 5 : Add function to write to TestExpectations with unittest #
Total comments: 14
Patch Set 6 : Refactored script to pass in Host object to main use its attributes #
Total comments: 5
Patch Set 7 : Modified doc strings for functions. #
Total comments: 21
Patch Set 8 : Formatting changes and unittest updates #
Total comments: 23
Patch Set 9 : Refactored unittests #
Total comments: 12
Messages
Total messages: 48 (16 generated)
dcampb@google.com changed reviewers: + jsbell@chromium.org, qyearsley@chromium.org, raikiri@google.com, tkent@chromium.org
These files are a work in progress. Looking for feedback and code optimizations. Thanks!
https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:19: webo = Web() maybe rename webo to something a little more clear? https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:75: path = [] can also be written: path = path or [] https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:79: merge_dicts(final[key], temp[key], path + [str(key)]) iiuc this line seems to do nothing as you don't keep the value that's returned
Apart from the initial comments below, there are two other things to work on: 1. Docstrings: ideally there should be a module-level docstrings before the imports, which explains the purpose and usage of the module, and docstrings for functions -- these are especially helpful if the arguments or return value are not very obvious. 2. Unit tests: ideally there should be one test method per "behavior" -- which usually means one or two unit test methods per function in the code under test. https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/update-w3c-test-expectations (right): https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/update-w3c-test-expectations:28: # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. This old-style header can be removed, and at the top of the file you should add: # Copyright 2016 The Chromium Authors. All rights reserved. # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/update-w3c-test-expectations:33: multiprocessing_bootstrap.run('webkitpy', 'w3c', 'update_w3c_test_expectations.py') In this case you probably don't want to use multiprocessing_bootstrap. many of the scripts in Tools/Scripts just call a function and exit with some status code, e.g. import sys from webkitpy.w3c import main sys.exit(main()) Example: https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/format-... Later if you want to get arguments/flags in your script, you could get them from sys.argv here and pass them to main(). https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:1: """TODO(dcampb): DO NOT SUBMIT without one-line documentation for print-layout-test-paths.""" This line can be removed, and at the top of the file you should add: # Copyright 2016 The Chromium Authors. All rights reserved. # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:3: from webkitpy.layout_tests.models.test_expectations import * It's best to avoid "wildcard imports" like these, because they may "pollute the global namespace"; that is, it defines some names at the global level, which may potentially conflict with other names. The other disadvantage of wildcard imports It's not clear exactly what's being used from the source module. See http://legacy.python.org/dev/peps/pep-0008/#imports https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:7: from webkitpy.common.checkout.scm.git import Git Imports should be sorted, for consistency. https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:16: def Main(): For consistency with most of the other scripts here, I think that the main function is usually lower-cased and put at the bottom of the file (although I personally think the top of the file is good). https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:17: json_obj = {} This name can be made more descriptive; how would you describe what it contains? A good name should be 1-3 words, descriptive, unambiguous, and not too verbose. Words like "obj", "data", and "info" should generally be avoided. https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:19: webo = Web() On 2016/07/11 at 18:46:55, raikiri wrote: > maybe rename webo to something a little more clear? I'd suggest just calling it web. https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:28: def get_tryjobs_information(web): tryjobs -> try_jobs I prefer to treat this as two words, since that's how it is in the official documentation (https://www.chromium.org/developers/testing/try-server-usage). https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:29: git_obj = Git('.') You could rename git_obj -> git; the _obj suffix doesn't add much information, and since it's only used in this short function, it's clear that it's an instance of Git. https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:39: platform = builder_name[:builder_name.find("_")] We can't assume that builder names are always for the form <platform>_something; I suspect that what we actually want to use is "port name", or possibly another name which we can get from the port name; see https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp... https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:40: for obj in layout_test_list: The name `obj` can be improved. In this case, it's a LayoutTestResult object, so you can call it `result`. If it were in scope outside of this short function, I'd suggest calling it layout_test_result. https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:51: def get_tryjob_results(try_jobs_data, web, index): tryjob -> try_job https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:56: results_url = '/'.join(url_components) The results URL can be constructed by methods on the Builder class in https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp..., I believe.
https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/update-w3c-test-expectations (right): https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/update-w3c-test-expectations:28: # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. On 2016/07/11 at 23:17:32, qyearsley wrote: > This old-style header can be removed, and at the top of the file you should add: > > # Copyright 2016 The Chromium Authors. All rights reserved. > # Use of this source code is governed by a BSD-style license that can be > # found in the LICENSE file. For some reason this patch didn't have the updated headers in them. I must have changed it after you noted it in the office. https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/update-w3c-test-expectations:33: multiprocessing_bootstrap.run('webkitpy', 'w3c', 'update_w3c_test_expectations.py') On 2016/07/11 at 23:17:32, qyearsley wrote: > In this case you probably don't want to use multiprocessing_bootstrap. many of the scripts in Tools/Scripts just call a function and exit with some status code, e.g. > > import sys > > from webkitpy.w3c import main > > > sys.exit(main()) > > Example: https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/format-... > > Later if you want to get arguments/flags in your script, you could get them from sys.argv here and pass them to main(). Made the changes to the calling process. I will be looking into adding arguments in later patches. https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:1: """TODO(dcampb): DO NOT SUBMIT without one-line documentation for print-layout-test-paths.""" On 2016/07/11 at 23:17:33, qyearsley wrote: > This line can be removed, and at the top of the file you should add: > > # Copyright 2016 The Chromium Authors. All rights reserved. > # Use of this source code is governed by a BSD-style license that can be > # found in the LICENSE file. Corrections made. https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:3: from webkitpy.layout_tests.models.test_expectations import * On 2016/07/11 at 23:17:33, qyearsley wrote: > It's best to avoid "wildcard imports" like these, because they may "pollute the global namespace"; that is, it defines some names at the global level, which may potentially conflict with other names. The other disadvantage of wildcard imports It's not clear exactly what's being used from the source module. > > See http://legacy.python.org/dev/peps/pep-0008/#imports Ahh, never knew that. Changes made. https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:7: from webkitpy.common.checkout.scm.git import Git On 2016/07/11 at 23:17:32, qyearsley wrote: > Imports should be sorted, for consistency. sorted by directory, file name or module name? https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:16: def Main(): On 2016/07/11 at 23:17:32, qyearsley wrote: > For consistency with most of the other scripts here, I think that the main function is usually lower-cased and put at the bottom of the file (although I personally think the top of the file is good). done https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:17: json_obj = {} On 2016/07/11 at 23:17:33, qyearsley wrote: > This name can be made more descriptive; how would you describe what it contains? A good name should be 1-3 words, descriptive, unambiguous, and not too verbose. Words like "obj", "data", and "info" should generally be avoided. Is that the same for local variable names in sub functions? https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:17: json_obj = {} On 2016/07/11 at 23:17:33, qyearsley wrote: > This name can be made more descriptive; how would you describe what it contains? A good name should be 1-3 words, descriptive, unambiguous, and not too verbose. Words like "obj", "data", and "info" should generally be avoided. Is that the same for local variable names in sub functions? https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:19: webo = Web() On 2016/07/11 at 18:46:55, raikiri wrote: > maybe rename webo to something a little more clear? done, web_object? https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:28: def get_tryjobs_information(web): On 2016/07/11 at 23:17:33, qyearsley wrote: > tryjobs -> try_jobs > > I prefer to treat this as two words, since that's how it is in the official documentation (https://www.chromium.org/developers/testing/try-server-usage). done https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:29: git_obj = Git('.') On 2016/07/11 at 23:17:33, qyearsley wrote: > You could rename git_obj -> git; the _obj suffix doesn't add much information, and since it's only used in this short function, it's clear that it's an instance of Git. makes sense, did the same for the web object. https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:40: for obj in layout_test_list: On 2016/07/11 at 23:17:33, qyearsley wrote: > The name `obj` can be improved. In this case, it's a LayoutTestResult object, so you can call it `result`. If it were in scope outside of this short function, I'd suggest calling it layout_test_result. done https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:51: def get_tryjob_results(try_jobs_data, web, index): On 2016/07/11 at 23:17:32, qyearsley wrote: > tryjob -> try_job done https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:56: results_url = '/'.join(url_components) On 2016/07/11 at 23:17:32, qyearsley wrote: > The results URL can be constructed by methods on the Builder class in https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp..., I believe. I believe I tried using it and it was giving me an error. I will try it again just to confirm. https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:79: merge_dicts(final[key], temp[key], path + [str(key)]) On 2016/07/11 at 18:46:55, raikiri wrote: > iiuc this line seems to do nothing as you don't keep the value that's returned this line recursively calls the function with a sub dictionary all the way down until final[key] is no longer a dictionary. The str(key) is there for debugging purposes, if there is a merging conflict.
https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:7: from webkitpy.common.checkout.scm.git import Git On 2016/07/12 at 03:55:55, dcampb wrote: > On 2016/07/11 at 23:17:32, qyearsley wrote: > > Imports should be sorted, for consistency. > > sorted by directory, file name or module name? Sorted lexicographically (which ends up to be by directory, generally) -- usually in vim I use `:'<,'>!sort`. https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:8: import logging One more thing about import section format: Standard library headers are usually listed above imports from the project itself. https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:17: json_obj = {} On 2016/07/12 at 03:55:55, dcampb wrote: > On 2016/07/11 at 23:17:33, qyearsley wrote: > > This name can be made more descriptive; how would you describe what it contains? A good name should be 1-3 words, descriptive, unambiguous, and not too verbose. Words like "obj", "data", and "info" should generally be avoided. > > Is that the same for local variable names in sub functions? Yep -- there may be some cases where `obj, `data` and `info` are OK, but should be avoided if possible, since they often don't make it clearer what the variable does.
Description was changed from ========== [WIP] Create w3c failing tests tree BUG=625254 ========== to ========== [WIP] Create Test Expectation lines automatically from failing test results. BUG=625254 ==========
https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:19: webo = Web() On 2016/07/12 at 03:55:55, dcampb wrote: > On 2016/07/11 at 18:46:55, raikiri wrote: > > maybe rename webo to something a little more clear? > > done, web_object? just made it web, per qyearsley@ comment on line 29.
https://codereview.chromium.org/2136723002/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2136723002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:1: # Coppyright 2016 The Chromium Authors. All rights reserved. Coppyright -> Copyright https://codereview.chromium.org/2136723002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:18: # {Test Name: {Platform: {'actual': 'foo', 'expected': 'foo', 'bug':'crbug.com/#####'}}} Good - module-level descriptions with examples of input/output are very helpful. In this case, it looks like the format used internally is an implementation detail though, so it may belong somewhere else rather than in the module-level docstring. The usual convention is to put comments like this in a docstring between the file header and the imports. As with other docstrings, the ideal is to have one short summary followed by details. For example: # Copyright 2016 The Chromium Authors. All rights reserved. # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. """A script to modify TestExpectations lines based layout test failures in try jobs. This script outputs a list of test expectation lines to add to a "TestExpectations" file by retrieving the try job results for the current CL. """ import logging from webkitpy.common.checkout.scm.git import Git from webkitpy.common.net.buildbot import BuildBot, Builder ... https://codereview.chromium.org/2136723002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:130: return final On first look, I'm having difficulty understanding what this function does -- I think we'll want to find a way to re-factor this function and extract helper functions to make this easier to understand. As a general rule, easy-to-understand functions have the following properties: - function name, argument names and variable names are descriptive and clear - less than 15 lines - less than 3 levels of indentation https://codereview.chromium.org/2136723002/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py (right): https://codereview.chromium.org/2136723002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:1: from webkitpy.w3c.update_w3c_test_expectations import * Good start to the unit test :-) Remember to add that file header at the top. https://codereview.chromium.org/2136723002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:2: import unittest Regarding the imports: 1. Wildcard imports are generally discouraged; we may also want to consider importing the module with `from webkitpy.w3c import update_w3c_test_expectations`. 2. Standard library imports (e.g. unittest) generally go first. https://codereview.chromium.org/2136723002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:5: class UpdateW3cTestExpectationsTest(unittest.TestCase): W3c -> W3C https://codereview.chromium.org/2136723002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:8: example_one = {'foo/bar/man.html': {'one': {'expected': u'FAIL', 'actual': u'PASS', 'bug': 'crbug.com/626703'}, 'two': {'expected': "FAIL", 'actual': 'PASS', 'bug': 'crbug.com/626703'}}} These are much easier to read when formatted more like this: example_one = { 'foo/bar/man.html': { 'one': {'expected': 'FAIL', 'actual': 'PASS', 'bug': 'crbug.com/626703'}, 'two': {'expected': "FAIL", 'actual': 'PASS', 'bug': 'crbug.com/626703'}, } } https://codereview.chromium.org/2136723002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:13: def test__get_expectations(self): test__ -> test_ https://codereview.chromium.org/2136723002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:21: self.assertEqual(create_line_list(example_one), ['crbug.com/626703 [ two ] foo/bar/man.html [ Pass ]', u'crbug.com/626703 [ one ] foo/bar/man.html [ Pass ]']) Lines like this can also be made more readable by reformatting, e.g. self.assertEqual( create_line_list(example_one), [ 'crbug.com/626703 [ two ] foo/bar/man.html [ Pass ]', 'crbug.com/626703 [ one ] foo/bar/man.html [ Pass ]' ])
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2136723002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:39: platform = builder_name[:builder_name.find("_")] On 2016/07/11 at 23:17:33, qyearsley wrote: > We can't assume that builder names are always for the form <platform>_something; I suspect that what we actually want to use is "port name", or possibly another name which we can get from the port name; see https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp... taken care of. https://codereview.chromium.org/2136723002/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py (right): https://codereview.chromium.org/2136723002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:1: from webkitpy.w3c.update_w3c_test_expectations import * On 2016/07/12 at 23:00:15, qyearsley wrote: > Good start to the unit test :-) Remember to add that file header at the top. I know you were saying to refrain from using the * when importing, however my script is not object oriented. Should I continue to use the star or just import the functions. Or, somehow modify my script ? https://codereview.chromium.org/2136723002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:13: def test__get_expectations(self): On 2016/07/12 at 23:00:15, qyearsley wrote: > test__ -> test_ Should I be testing private functions? Or should I just test the function that call them and mock their functionality?
https://codereview.chromium.org/2136723002/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py (right): https://codereview.chromium.org/2136723002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:1: from webkitpy.w3c.update_w3c_test_expectations import * On 2016/07/15 at 00:24:13, dcampb wrote: > On 2016/07/12 at 23:00:15, qyearsley wrote: > > Good start to the unit test :-) Remember to add that file header at the top. > > I know you were saying to refrain from using the * when importing, however my script is not object oriented. Should I continue to use the star or just import the functions. Or, somehow modify my script ? If you don't use * when importing, then your choices are: 1. Import the module itself, e.g. `from webkitpy.w3c import update_w3c_test_expectations` Then specific functions inside your class are referenced as `update_w3c_test_expectations.update_platforms`. If you feel that update_w3c_test_expectations is a bit too long, you can give it an alias after importing, e.g. `from webkitpy.w3c import update_w3c_test_expectations as update_module`. Even though this is a little more verbose, it has the advantage that when reading the tests, it's clear where all of the functions are defined. 2. If you're testing a relatively small number of names from a module, it might be nicer to just import the names you're interested in, e.g. `from webkitpy.w3c.update_w3c_test_expectations import create_line_list`. https://codereview.chromium.org/2136723002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:13: def test__get_expectations(self): On 2016/07/15 at 00:24:13, dcampb wrote: > On 2016/07/12 at 23:00:15, qyearsley wrote: > > test__ -> test_ > > Should I be testing private functions? Or should I just test the function that call them and mock their functionality? You can test private functions :-) If it's possible to test them by testing the function that calls them, then it's better to do that, and you can skip testing the private functions directly. When testing a private function named _foo, you don't need to name the test method test__foo, you can just call it test_foo. In general, test method names only serves as a description of what's being tested, it doesn't necessarily need to be of the "form test_<function>".
dcampb@google.com changed reviewers: + dpranke@chromium.org
A few quick formatting comments: https://codereview.chromium.org/2136723002/diff/100001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2136723002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:23: from webkitpy.common.config.builders import BUILDERS as Builders_list_dict You can keep the name BUILDERS, since that's simpler than renaming it as Builders_list_dict here. https://codereview.chromium.org/2136723002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:37: builderlist = BuilderList(Builders_list_dict) builderlist -> builder_list https://codereview.chromium.org/2136723002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:41: issue_number = expectations_line_adder._git.get_issue_number() Here a private attribute of another object is accessed -- you can either move some logic from main into a method in W3CExpectationsLineAdder, or if the logic is kept in main, then git should be marked as public. https://codereview.chromium.org/2136723002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:55: print "no tryjob info was collected" You could add capitalization and punctuation to this message, and put a space in "try job". https://codereview.chromium.org/2136723002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:63: class W3CExpectationsLineAdder(object): Formatting note: Usually there's a blank line before the first method of a class. https://codereview.chromium.org/2136723002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:64: def __init__(self, git, web, builderlist): builderlist -> builder_list https://codereview.chromium.org/2136723002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:71: # has two try jobs, it will return both. This comment can go inside a docstring, and updated when that CL that changes latest_try_jobs is landed. https://codereview.chromium.org/2136723002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:74: return info The variable info doesn't provide any extra information here; you can just write: return rietveld.latest_try_jobs(issue_number, try_bots, self._web) https://codereview.chromium.org/2136723002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:79: if dash != -1: To find if a character exists in a string you can also use the keyword `in`: if '-' in platform: ... https://codereview.chromium.org/2136723002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:82: test_dict[result.test_name()] = {platform: {'expected': result.expected_results(), 'actual': result.actual_results(), 'bug': 'crbug.com/626703'}} This line is a bit long; reformatting it may make it more readable.
Patchset #6 (id:120001) has been deleted
Description was changed from ========== [WIP] Create Test Expectation lines automatically from failing test results. BUG=625254 ========== to ========== Create Test Expectation lines automatically from failing test results. BUG=625254 ==========
https://codereview.chromium.org/2136723002/diff/100001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2136723002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:23: from webkitpy.common.config.builders import BUILDERS as Builders_list_dict On 2016/07/18 at 18:31:42, qyearsley wrote: > You can keep the name BUILDERS, since that's simpler than renaming it as Builders_list_dict here. I have removed this import as well as builder_list. Refactored the script to use the builder_list on the host object. https://codereview.chromium.org/2136723002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:41: issue_number = expectations_line_adder._git.get_issue_number() On 2016/07/18 at 18:31:42, qyearsley wrote: > Here a private attribute of another object is accessed -- you can either move some logic from main into a method in W3CExpectationsLineAdder, or if the logic is kept in main, then git should be marked as public. I moved this line and the line 42 into their own functions that return the issue number and the try_bots names. https://codereview.chromium.org/2136723002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:79: if dash != -1: On 2016/07/18 at 18:31:42, qyearsley wrote: > To find if a character exists in a string you can also use the keyword `in`: > > if '-' in platform: > ... done https://codereview.chromium.org/2136723002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:82: test_dict[result.test_name()] = {platform: {'expected': result.expected_results(), 'actual': result.actual_results(), 'bug': 'crbug.com/626703'}} On 2016/07/18 at 18:31:42, qyearsley wrote: > This line is a bit long; reformatting it may make it more readable. done
Patchset #6 (id:140001) has been deleted
Patchset #6 (id:140001) has been deleted
Last patch had a modified file that shouldn't have been there. I deleted it and this is the correct one
Note about docstring style: Blink officially uses the PEP8 style, which says to follow PEP257 for docstrings: https://www.python.org/dev/peps/pep-0257/ Unofficially, for consistency, it's also helpful to look at the recommendations in the Google Python style guide: https://google.github.io/styleguide/pyguide.html?showone=Comments#Comments https://codereview.chromium.org/2136723002/diff/160001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/update-w3c-test-expections (right): https://codereview.chromium.org/2136723002/diff/160001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/update-w3c-test-expections:14: _host._scm = Git('.') It might also work if you call host.initialize_scm() here -- in practice, I believe this should always set up a Git object as long as the current working directory is in a git repo. https://codereview.chromium.org/2136723002/diff/160001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/update-w3c-test-expections:15: _port = _host.port_factory.get() No need to use underscores here, since these are local variables, not attributes of a class. https://codereview.chromium.org/2136723002/diff/160001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2136723002/diff/160001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:5: '''A script to modify TestExpectations lines based layout test failures in try jobs. Usually triple-double-quotes (""") are used for docstrings (same below). https://codereview.chromium.org/2136723002/diff/160001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:99: '''This function takes a dictionary of dictionaries and creates a new tuple key You could omit "This function", and just start with "Takes a dictionary of dictionaries ... https://codereview.chromium.org/2136723002/diff/160001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:100: if two or more values match. Example: { You should add a blank line after the first sentence of the docstring (same below).
hopefully this is the last patch =D
dcampb@google.com changed reviewers: + bokan@chromium.org - jsbell@chromium.org
Still mostly formatting nits. I'm fairly nit-picky about formatting :-P https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:33: builder = buildbot.Builder(builder_name, expectations_line_adder.get_build_bot) Note: As-is, get_build_bot is a function, not a BuildBot object. https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:49: return self._host.buildbot This method could be removed, and line 33 above could be changed to: builder = buildbot.Builder(builder_name, host.buildbot) https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:51: def get_try_jobs_information(self, issue_number, try_bots): This method could also be called latest_try_jobs -- or it could be removed, and line 25 above could be changed to: try_jobs = rietveld.latest_try_jobs(issue_number, try_bots, host.web) https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:74: """ Returns a nested dict of failing test results. Extra space before "Returns" https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:76: Retrieves a full list oflayout test results from a builder result url. Collects "oflayout" -> "of layout" "url" -> "URL" https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:87: } Could be reformatted as: A dictionary with the structure: { 'key': { 'expected': 'TIMEOUT', 'actual': 'CRASH', 'bug': 'crbug.com/11111', } } https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:126: } is converted to I'd dedent this line so that the closing curly brace is lined up with the start of the line that has the opening curly brace. https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:131: isLastItem = False Variable names should be lower_with_underscores. https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:145: def get_expectations(self, results): This method could also use a docstring -- Is results a dict with results for one test? If so, it's worth noting that results['actual'] can be a space-delimited string with results for different runs, e.g. "FAIL PASS". It may also be worth mentioning that the return value is a list which should have one or more strings with the first letter capitalized, e.g. ["Failure"] https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:165: dictionary: A dictionary with the format { We should think of a name for the argument that describes the contents or meaning of the input dictionary. Here, this dictionary contains results for all platforms for all failed tests. Obviously, you can't describe everything about it in the argument name, and just "results" is also very vague. Maybe merged_results? https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:173: } Indentation for closing braces usually lines up with the start of the line where the opening brace is. For example: dictionary: A dictionary with the format { 'test_name': { 'platform': { 'expected: 'PASS', 'actual': 'FAIL', 'bug': 'crbug.com/11111' } } } OR dictionary: A dictionary with the format: { 'test_name': { 'platform': { 'expected: 'PASS', 'actual': 'FAIL', 'bug': 'crbug.com/11111' } } } https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:183: test_name = key We can also think of more descriptive names than key and value here, e.g. `for test_name, platform_results in ...` https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:184: for key2 in value: Any time you find yourself adding digits after a variable name, that's a sign that there's probably a clearer and more descriptive name that you could use. The keys in the inner dicts are either single platforms or tuples of platforms, right? Instead of key2, it could be called platform (and the code below should make it clear that it can be either a string or tuple). https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:203: '#Tests added from W3C auto import bot' and writes expectation "#Tests" -> "# Tests" https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py (right): https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:43: 'two': {'expected': "FAIL", 'actual': 'PASS', 'bug': 'crbug.com/626703'} The u can be taken off of the front of the string literals, and the quote type (double or single) could be made consistent. https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:90: W3C = W3CExpectationsLineAdder(MockHost()) For a short name for this object, you could consider calling it `adder` or `line_adder`. In general, variable names should be lower case. https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:98: self.assertEqual(W3C._generate_results_dict(platform, layout_test_list), {'test/name.html': {'dummy_platform': {'expected': 'bar', 'actual': 'foo', 'bug': 'crbug.com/626703'}}}) Nit: long lines like this can be made more readable by breaking up across multiple lines. https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:100: def test_write_to_testexpectations(self): "testexpectations" -> "test_expectations"
https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:165: dictionary: A dictionary with the format { On 2016/07/19 at 17:59:48, qyearsley wrote: > We should think of a name for the argument that describes the contents or meaning of the input dictionary. > > Here, this dictionary contains results for all platforms for all failed tests. Obviously, you can't describe everything about it in the argument name, and just "results" is also very vague. > > Maybe merged_results? agreed, I used merged_results. https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:183: test_name = key On 2016/07/19 at 17:59:47, qyearsley wrote: > We can also think of more descriptive names than key and value here, e.g. `for test_name, platform_results in ...` done https://codereview.chromium.org/2136723002/diff/180001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:184: for key2 in value: On 2016/07/19 at 17:59:48, qyearsley wrote: > Any time you find yourself adding digits after a variable name, that's a sign that there's probably a clearer and more descriptive name that you could use. > > The keys in the inner dicts are either single platforms or tuples of platforms, right? Instead of key2, it could be called platform (and the code below should make it clear that it can be either a string or tuple). agreed
dcampb@google.com changed reviewers: - tkent@chromium.org
https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:49: return self._host._scm.get_issue_number() This line is accessing a private attribute of another object directly (_scm attribute of Host object). In this case it's not really dangerous, but as a general principle accessing private attributes should always be avoided. Host has a method scm() which could be used here: return self._host.scm().get_issue_number() https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:93: def merge_dicts(self, final, temp, path=None): A docstring could also be useful here. saying something like: """Recursively merges two dictionaries, modifying the first.""" Other notes: The name `temp` doesn't provide much information. Maybe call the arguments `target` and `source` since the first one is being modified? Alternatively, this this function could also be changed to return a new dict and not modify either of its arguments; in that case the arguments could be called `first` and `second`. https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:102: raise Exception('conflict at %s' % '.'.join(path)) Usually it's better to raise a more specific exception, so that callers can catch a more specific exception. One possible exception to use here would be ValueError, which usually indicates "something about the value of the passed-in argument was incorrect". https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:123: } is converted to Indentation is off here -- maybe due to tab characters? https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:143: '''Returns a list of test expectations for a given test dict. Use double-quotes (""") for docstrings for consistency https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:230: w3c_comment_line_index = file_contents.find('# Tests added from W3C auto import bot') The string '# Tests added from W3C auto import bot' could be a global constant. It could also be re-used below on line 236. https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py (right): https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:38: }) Looks like this isn't used currently -- do you think we could add a test for the main() function? https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:59: } Looks like this isn't used anywhere currently https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:86: self.assertTrue('four' in self.merge_dicts(self.mock_dict_one, self.mock_dict_three)['fake/test/path.html']) This test method could make an assertion about what the whole output of merge_dicts looks like. It could also assert that the output dict is the same as the first argument, and that the first argument was changed, e.g.: output = self.merge_dicts(self.mock_dict_one, self.mock_dict_three) self.assertEqual(output, self.mock_dict_one) self.assertEqual(output, ...) Also, one alternative to `self.assertTrue(x in y)` is `self.assertIn(x, y)`, which is a bit more concise. Finally, I want to note that ideally, each test method should test one "behavior", and you could consider "raising an exception when the inputs conflict" and "successfully merging recursively when there is no conflict" as two different behaviors. So you can split this up into test methods: test_merge_dicts_with_conflict_raises_exception test_merge_dicts_merges_second_dict_into_first https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:111: host = MockHost() It's probably simpler to use the same host object for the W3CExpectationsLineAdder and for the filesystem -- and this is what's done in the real main() function. host = MockHost() line_adder = W3CExpectationsLineAdder(host) https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:121: self.assertEqual(value, 'not empty\n\n# Tests added from W3C auto import bot\nfake crbug [foo] fake/file/path.html [Pass]') This test method looks like it could be split into two -- testing with the comment marker already existing, and without.
https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:49: return self._host._scm.get_issue_number() On 2016/07/20 at 16:55:28, qyearsley wrote: > This line is accessing a private attribute of another object directly (_scm attribute of Host object). In this case it's not really dangerous, but as a general principle accessing private attributes should always be avoided. > > Host has a method scm() which could be used here: > > return self._host.scm().get_issue_number() done https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:93: def merge_dicts(self, final, temp, path=None): On 2016/07/20 at 16:55:27, qyearsley wrote: > A docstring could also be useful here. saying something like: > > """Recursively merges two dictionaries, modifying the first.""" > > Other notes: > > The name `temp` doesn't provide much information. Maybe call the arguments `target` and `source` since the first one is being modified? > > > Alternatively, this this function could also be changed to return a new dict and not modify either of its arguments; in that case the arguments could be called `first` and `second`. I chose the first option, I don't really see a benefit in returning a new dictionary. It's a preference of mine to just update the first parameter. https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:102: raise Exception('conflict at %s' % '.'.join(path)) On 2016/07/20 at 16:55:28, qyearsley wrote: > Usually it's better to raise a more specific exception, so that callers can catch a more specific exception. > > One possible exception to use here would be ValueError, which usually indicates "something about the value of the passed-in argument was incorrect". Changed it to a value error, with the conflict line still there for debugging. https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:123: } is converted to On 2016/07/20 at 16:55:28, qyearsley wrote: > Indentation is off here -- maybe due to tab characters? fixed. https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:143: '''Returns a list of test expectations for a given test dict. On 2016/07/20 at 16:55:27, qyearsley wrote: > Use double-quotes (""") for docstrings for consistency fixed. https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:143: '''Returns a list of test expectations for a given test dict. On 2016/07/20 at 16:55:27, qyearsley wrote: > Use double-quotes (""") for docstrings for consistency fixed. https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:143: '''Returns a list of test expectations for a given test dict. On 2016/07/20 at 16:55:27, qyearsley wrote: > Use double-quotes (""") for docstrings for consistency fixed. https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:230: w3c_comment_line_index = file_contents.find('# Tests added from W3C auto import bot') On 2016/07/20 at 16:55:28, qyearsley wrote: > The string '# Tests added from W3C auto import bot' could be a global constant. It could also be re-used below on line 236. made it into a variable in the function. Don't use it outside this function so no need for a global variable. https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py (right): https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:111: host = MockHost() On 2016/07/20 at 16:55:28, qyearsley wrote: > It's probably simpler to use the same host object for the W3CExpectationsLineAdder and for the filesystem -- and this is what's done in the real main() function. > > host = MockHost() > line_adder = W3CExpectationsLineAdder(host) done, had to make a public filesystem attribute to the W3CExpectationsAdder class. https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:121: self.assertEqual(value, 'not empty\n\n# Tests added from W3C auto import bot\nfake crbug [foo] fake/file/path.html [Pass]') On 2016/07/20 at 16:55:28, qyearsley wrote: > This test method looks like it could be split into two -- testing with the comment marker already existing, and without. done
LGTM - Joshua or Dirk, if you have time to review this CL, could you read over it as well? https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:93: def merge_dicts(self, final, temp, path=None): On 2016/07/20 at 21:11:59, dcampb wrote: > I chose the first option, I don't really see a benefit in returning a new dictionary. It's a preference of mine to just update the first parameter. Sounds good -- and with the docstring and argument names changed, it should be pretty clear to future readers that that's the behavior. https://codereview.chromium.org/2136723002/diff/200001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:230: w3c_comment_line_index = file_contents.find('# Tests added from W3C auto import bot') On 2016/07/20 at 21:11:59, dcampb wrote: > On 2016/07/20 at 16:55:28, qyearsley wrote: > > The string '# Tests added from W3C auto import bot' could be a global constant. It could also be re-used below on line 236. > > made it into a variable in the function. Don't use it outside this function so no need for a global variable. Sounds fine to me. Note, sometimes people extract literal strings and numbers up to the top of the file even if they're only used in one place -- the main advantage of this is that "magical values" are then grouped at the top of the file, and when one wants to tweak a "magical value", then any change is at the top of the file. But I think that this is a matter of personal preference, and having as few global values as possible seems like a good idea to me.
The CQ bit was checked by dcampb@google.com
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
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_...)
The CQ bit was checked by dcampb@google.com
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
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_...)
The CQ bit was checked by dcampb@google.com
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 #9 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Create Test Expectation lines automatically from failing test results. BUG=625254 ========== to ========== Create Test Expectation lines automatically from failing test results. BUG=625254 Committed: https://crrev.com/22c2cad13b358a133ae399532ce910c1ebb96073 Cr-Commit-Position: refs/heads/master@{#406868} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/22c2cad13b358a133ae399532ce910c1ebb96073 Cr-Commit-Position: refs/heads/master@{#406868}
Message was sent while issue was closed.
A few very-belated comments, but this basically looks fine. https://codereview.chromium.org/2136723002/diff/220001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/update-w3c-test-expections (right): https://codereview.chromium.org/2136723002/diff/220001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/update-w3c-test-expections:18: sys.exit(return_code) this line probably should've been combined with line 16: sys.exit(main(host, port)) as it is, if this file was somehow eval'ed directly, it wouldn't work right and would always call sys.exit(), but with an uninitialized variable. https://codereview.chromium.org/2136723002/diff/220001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2136723002/diff/220001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:40: expectations_line_adder.write_to_test_expectations(host, expectations_file, line_list) Nit: it's usually a good idea to break up longer functions like this into "paragraphs" by inserting blank lines between logically-related groups of statements; it makes things easier to read. In this case, I'd add blank lines after lines 26, 29, 36, and 38. https://codereview.chromium.org/2136723002/diff/220001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:208: ['BUG_URL [PLATFORM(S)] TEST_MAME [EXPECTATION(S)]'] Nit: typo ("TEST_MAME"). https://codereview.chromium.org/2136723002/diff/220001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:223: line_list.append(str(line)) similar comment about blank lines. https://codereview.chromium.org/2136723002/diff/220001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:241: Writes to a file on the filesystem called LayoutTests/TestExpectations. This doc string is a bit weird. You take the path as an input arg, but say that it should be called LayoutTests/TestExpectations. If this routine doesn't actually care what the path is -- and it doesn't -- then I wouldn't explicitly talk about a particular expected value. https://codereview.chromium.org/2136723002/diff/220001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:257: host.filesystem.write_text_file(path, file_contents) same.
Message was sent while issue was closed.
https://codereview.chromium.org/2136723002/diff/220001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/update-w3c-test-expections (right): https://codereview.chromium.org/2136723002/diff/220001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/update-w3c-test-expections:18: sys.exit(return_code) On 2016/08/09 at 03:59:39, Dirk Pranke wrote: > this line probably should've been combined with line 16: > > sys.exit(main(host, port)) > > as it is, if this file was somehow eval'ed directly, it wouldn't work right and would always call sys.exit(), but with an uninitialized variable. What do you mean by eval'ed directly?? I'm a little confused. https://codereview.chromium.org/2136723002/diff/220001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2136723002/diff/220001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:40: expectations_line_adder.write_to_test_expectations(host, expectations_file, line_list) On 2016/08/09 at 03:59:39, Dirk Pranke wrote: > Nit: it's usually a good idea to break up longer functions like this into "paragraphs" by inserting blank lines between logically-related groups of statements; it makes things easier to read. > > In this case, I'd add blank lines after lines 26, 29, 36, and 38. Interesting, noted. https://codereview.chromium.org/2136723002/diff/220001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:208: ['BUG_URL [PLATFORM(S)] TEST_MAME [EXPECTATION(S)]'] On 2016/08/09 at 03:59:39, Dirk Pranke wrote: > Nit: typo ("TEST_MAME"). done https://codereview.chromium.org/2136723002/diff/220001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:241: Writes to a file on the filesystem called LayoutTests/TestExpectations. On 2016/08/09 at 03:59:39, Dirk Pranke wrote: > This doc string is a bit weird. You take the path as an input arg, but say that it should be called LayoutTests/TestExpectations. > > If this routine doesn't actually care what the path is -- and it doesn't -- then I wouldn't explicitly talk about a particular expected > value. noted
Message was sent while issue was closed.
https://codereview.chromium.org/2136723002/diff/220001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/update-w3c-test-expections (right): https://codereview.chromium.org/2136723002/diff/220001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/update-w3c-test-expections:18: sys.exit(return_code) On 2016/08/09 18:10:44, dcampb wrote: > On 2016/08/09 at 03:59:39, Dirk Pranke wrote: > > this line probably should've been combined with line 16: > > > > sys.exit(main(host, port)) > > > > as it is, if this file was somehow eval'ed directly, it wouldn't work right > and would always call sys.exit(), but with an uninitialized variable. > > What do you mean by eval'ed directly?? I'm a little confused. Good question, I wasn't super clear here ... I'm talking about what Python does when a file is loaded. If, for example, this file was a python module that got imported, and not a top-level script, then if you were to run `python update_w3c_test_expectations.py`, __name__ would be set to "__main__" and the block on lines 12-16 would execute, as would line 18, so things would be fine. However, if you were to run another python file that had `import update_w3_test_expectations`, __name__ would *not* be set to "__main__", so that block wouldn't execute, but line 18 would still execute. Which would be even worse, since that would cause python to quit (both because of the uninitialized variable and because you're calling sys.exit()). Of course, this isn't a module, it's a top-level script. So, you can't import it, and don't have that problem, but you could still call execfile("update-w3-test-expectations") and the same thing would happen. Though it's not real likely that anyone's going to do this, either. So this isn't a bug that's likely to actually show up. This is a long winded-way of saying be careful what you have as unindented python code in scripts, because it'll always execute, and be careful about what you have in `if __name__ == "__main__"` blocks :).
Message was sent while issue was closed.
https://codereview.chromium.org/2136723002/diff/220001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/update-w3c-test-expections (right): https://codereview.chromium.org/2136723002/diff/220001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/update-w3c-test-expections:18: sys.exit(return_code) On 2016/08/09 at 18:35:04, Dirk Pranke wrote: > On 2016/08/09 18:10:44, dcampb wrote: > > On 2016/08/09 at 03:59:39, Dirk Pranke wrote: > > > this line probably should've been combined with line 16: > > > > > > sys.exit(main(host, port)) > > > > > > as it is, if this file was somehow eval'ed directly, it wouldn't work right > > and would always call sys.exit(), but with an uninitialized variable. > > > > What do you mean by eval'ed directly?? I'm a little confused. > > Good question, I wasn't super clear here ... I'm talking about what Python does when a file is loaded. > > If, for example, this file was a python module that got imported, and not a top-level script, then > > if you were to run `python update_w3c_test_expectations.py`, __name__ would be set to "__main__" > and the block on lines 12-16 would execute, as would line 18, so things would be fine. > > However, if you were to run another python file that had `import update_w3_test_expectations`, > __name__ would *not* be set to "__main__", so that block wouldn't execute, but line 18 would > still execute. Which would be even worse, since that would cause python to quit (both because > of the uninitialized variable and because you're calling sys.exit()). > > Of course, this isn't a module, it's a top-level script. So, you can't import it, and don't have that > problem, but you could still call execfile("update-w3-test-expectations") and the same thing would > happen. Though it's not real likely that anyone's going to do this, either. So this isn't a bug that's > likely to actually show up. > > This is a long winded-way of saying be careful what you have as unindented python code in > scripts, because it'll always execute, and be careful about what you have in > `if __name__ == "__main__"` blocks :). So should I unindent the block and remove the 'if __name__ == "__main__"' line?? and add sys.exit(main(host, port))? I reckon this would run no matter what calls it?
Message was sent while issue was closed.
On 2016/08/09 18:49:51, dcampb wrote: > https://codereview.chromium.org/2136723002/diff/220001/third_party/WebKit/Too... > File third_party/WebKit/Tools/Scripts/update-w3c-test-expections (right): > > https://codereview.chromium.org/2136723002/diff/220001/third_party/WebKit/Too... > third_party/WebKit/Tools/Scripts/update-w3c-test-expections:18: > sys.exit(return_code) > On 2016/08/09 at 18:35:04, Dirk Pranke wrote: > > On 2016/08/09 18:10:44, dcampb wrote: > > > On 2016/08/09 at 03:59:39, Dirk Pranke wrote: > > > > this line probably should've been combined with line 16: > > > > > > > > sys.exit(main(host, port)) > > > > > > > > as it is, if this file was somehow eval'ed directly, it wouldn't work > right > > > and would always call sys.exit(), but with an uninitialized variable. > > > > > > What do you mean by eval'ed directly?? I'm a little confused. > > > > Good question, I wasn't super clear here ... I'm talking about what Python > does when a file is loaded. > > > > If, for example, this file was a python module that got imported, and not a > top-level script, then > > > > if you were to run `python update_w3c_test_expectations.py`, __name__ would be > set to "__main__" > > and the block on lines 12-16 would execute, as would line 18, so things would > be fine. > > > > However, if you were to run another python file that had `import > update_w3_test_expectations`, > > __name__ would *not* be set to "__main__", so that block wouldn't execute, but > line 18 would > > still execute. Which would be even worse, since that would cause python to > quit (both because > > of the uninitialized variable and because you're calling sys.exit()). > > > > Of course, this isn't a module, it's a top-level script. So, you can't import > it, and don't have that > > problem, but you could still call execfile("update-w3-test-expectations") and > the same thing would > > happen. Though it's not real likely that anyone's going to do this, either. So > this isn't a bug that's > > likely to actually show up. > > > > This is a long winded-way of saying be careful what you have as unindented > python code in > > scripts, because it'll always execute, and be careful about what you have in > > `if __name__ == "__main__"` blocks :). > > So should I unindent the block and remove the 'if __name__ == "__main__"' line?? > and add sys.exit(main(host, port))? > I reckon this would run no matter what calls it? Either that or remove line 18 and change line 16. I'd probably do the latter, as I tend to avoid having code that executes at the top level, even in scripts that are meant to be invoked directly, because too often you end up needing to move such things into modules so you can test them or reuse them, and then you have to re-indent everything :). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
