Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1766583002:
Added update_test_expectations script to remove non-flaky test expectations. (Closed)
DescriptionAdded update_test_expectations script to remove non-flaky test expectations.
BUG=595414
Patch Set 1 #
Total comments: 30
Patch Set 2 : Addressed Quinten's feedback #Patch Set 3 : Added tests and other fixes #Patch Set 4 : Another test and help description #Patch Set 5 : tests++ #
Total comments: 32
Patch Set 6 : Review from Quinten #Patch Set 7 : Mock out builders and specifiers, dont remove lines if bot results arent available #
Total comments: 42
Patch Set 8 : Rebase #Patch Set 9 : More feedback addressed from dpranke@ and qyearsley@ #Patch Set 10 : Added tests for the main() function #Patch Set 11 : Rebase after long break #Messages
Total messages: 24 (7 generated)
bokan@chromium.org changed reviewers: + ojan@chromium.org
How does this look? If it's on the right track I'll go ahead and add tests. https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py (right): https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py:96: def builder_name_for_specifiers(version, build_type): It seems like this would be needed elsewhere, does this function exist already? https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py (right): https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:50: # TODO(bokan): Matching configurations often give us bots that don't have a There seems to be more matching_configurations that builders in builders.py, am I using the right list or do the others not matter? https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:77: # TODO(bokan): Does this not exist in a more central place? Ditto here, is there a function that turns "failure types" into "FAIL"? https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:123: expectations_file = port.path_to_generic_test_expectations_file() Presumably we also want to update ASAN and other bots of that sort?
qyearsley@chromium.org changed reviewers: + qyearsley@chromium.org
Hi bokan@, I'm interested in reading this CL as well -- although I'm unfamiliar with the code, so my comments are mostly questions. First question: Where and when will update-test-expectations be run? Must it be run after running all of the layout tests? Does it update the TestExpectations file for one specific builder/platform at a time? https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/update-test-expectations (right): https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/update-test-expectations:3: # Copyright $YEAR The Chromium Authors. All rights reserved. $YEAR -> 2016 https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py (right): https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py:143: class BotTestExpectations(object): Does a BotTestExpectations object represent the set of layout test expectations for a given single bot and test combination? https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py:234: result_strings = map(self.results_json.expectation_for_type, result_types) What are the possible result strings that are returned here? What would it mean for the result strings to be encoded? https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py (right): https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:21: options, args = parser.parse_args(argv) This could be switched to argparse, the successor to optparse: parser = argparse.ArgumentParser(epilog='...') parser.add_argument('-f', '--forward', type=int help='group times by first N directories of test') parser.add_argument('-b', '--backward', type=int, help='group times by last N directories of test') parser.add_argument('--fastest', type=float, help='print a list of tests that will take N % of the time') args = parser.parse_args(argv) https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:24: remove_flakes_o_matic.execute(host) Possible refactoring: Maybe RemoveFlakesOMatic.execute could be changed to a top-level function called remove_flakes(expectations)? port = host.port_factory.get() test_expectations = TestExpectations(, include_overrides=False)._expectations updated_test_expectations = remove_flakes(test_expectations) expectations_file = port.path_to_generic_test_expectations_file() host.filesystem.write_text_file( expectations_file, TestExpectations.list_to_string(updated_test_expectations, reconstitute_only_these=[])) Is there an advantage to using a RemoveFlakesOMatic class? https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:27: class RemoveFlakesOMatic: What do instances of this class represent? Could the methods here could be changed to top-level functions or static methods? https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:31: def _can_delete_line(self, test_expectation_line): test_expectation_line is an instance of test_expectations.TestExpectationLine, right? (Could potentially add a docstring to explicitly state this.) What is the class of the objects in test_expectations? Are they strings? https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:34: return False What does an expectation list of length 0 or 1 indicate? Why does it mean we can't delete the line? https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:84: test_results = set(results_for_test) This line could be removed without changing the output; From the variable names, results_for_test and test_results sound like two names for the same thing. https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:86: actual_results = set(map(replace_failing_with_fail, test_results)) Possible rephrasing: actual_results = {replace_failing_with_fail(r) for r in results_for_test} https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:90: def _has_unstripable_expectations(self, expectations): unstripable -> unstrippable https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:94: or 'SLOW' in expectations) Possible alternate phrasing: passing_statuses = ('PASS', 'SLOW', 'NEEDSREBASELINE', 'NEEDSMANUALREBASELINE') return any(s in expectations for s in passing_statuses) https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:98: test_expectations = TestExpectations(port, include_overrides=False)._expectations 1. What is the class of the objects in test_expectations? 2. This is accessing a private property, maybe TestExpectations should have a get_expectations method, or that property should be public?
>> Hi bokan@, I'm interested in reading this CL as well -- although I'm unfamiliar >> with the code, so my comments are mostly questions. Thanks for the feedback. I'm also new to this code so I'll do my best. >> First question: Where and when will update-test-expectations be run? Must it be >> run after running all of the layout tests? Does it update the TestExpectations >> file for one specific builder/platform at a time? This script pulls the results for all the bots/platforms; for each line in TestExpectations, checks whether the flaky expectation is actually passing on all the configurations specified on the line and removes it if so; and writes a new TestExpectations without the removed lines. We haven't decided where/when this should run. You could run it manually and commit the results but I think ideally we would place it on a bot somewhere and run it at some interval (once a day?) so that we automatically keep TestExpectations accurate. I've also done the simple thing here but I think an obvious followup that would be valuable would be to update lines in addition to just removing them. So, for example, if you have a line like: [Linux Win] mytest.html [ PASS FAIL TIMEOUT ] But all the recent runs on the bots pass completely on Linux and only timeout on Windows we could update this to: [Win] mytest.html [ PASS TIMEOUT ] We could also add new lines as we see flaky tests automatically as well. These are just my thoughts after working on this for a week though. I'm not sure if there's issues with more automation here or if this has been tried or discussed before. https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/update-test-expectations (right): https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/update-test-expectations:3: # Copyright $YEAR The Chromium Authors. All rights reserved. On 2016/03/07 19:09:53, qyearsley wrote: > $YEAR -> 2016 Doh! Thanks https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py (right): https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py:143: class BotTestExpectations(object): On 2016/03/07 19:09:53, qyearsley wrote: > Does a BotTestExpectations object represent the set of layout test expectations > for a given single bot and test combination? No, it's the results of running the layout tests on the bots. I admit the name is a bit misleading. https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py:234: result_strings = map(self.results_json.expectation_for_type, result_types) On 2016/03/07 19:09:53, qyearsley wrote: > What are the possible result strings that are returned here? What would it mean > for the result strings to be encoded? The result codes come from the bot so I'm not sure these are exhaustive and if they vary by bot, this came off the Nexus 4 bot: u'A': u'AUDIO', u'C': u'CRASH', u'F': u'TEXT', u'I': u'IMAGE', u'K': u'LEAK', u'L': u'FLAKY', u'N': u'NO DATA', u'O': u'MISSING', u'P': u'PASS', u'Q': u'FAIL', u'S': u'SLOW', u'T': u'TIMEOUT', u'U': u'UNKNOWN', u'V': u'VERYFLAKY', u'X': u'SKIP', u'Y': u'NOTRUN', u'Z': u'IMAGE+TEXT'} The encoded version is the one letter version which is what's stored in the JSON. the expectation_for_type method converts to the human readable string. https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py (right): https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:21: options, args = parser.parse_args(argv) On 2016/03/07 19:09:53, qyearsley wrote: > This could be switched to argparse, the successor to optparse: > > parser = argparse.ArgumentParser(epilog='...') > parser.add_argument('-f', '--forward', type=int > help='group times by first N directories of test') > parser.add_argument('-b', '--backward', type=int, > help='group times by last N directories of test') > parser.add_argument('--fastest', type=float, > help='print a list of tests that will take N % of the > time') > args = parser.parse_args(argv) Thanks, done. https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:24: remove_flakes_o_matic.execute(host) On 2016/03/07 19:09:53, qyearsley wrote: > Possible refactoring: Maybe RemoveFlakesOMatic.execute could be changed to a > top-level function called remove_flakes(expectations)? > > port = host.port_factory.get() > test_expectations = TestExpectations(, > include_overrides=False)._expectations > > updated_test_expectations = remove_flakes(test_expectations) > > expectations_file = port.path_to_generic_test_expectations_file() > host.filesystem.write_text_file( > expectations_file, > TestExpectations.list_to_string(updated_test_expectations, > reconstitute_only_these=[])) > > Is there an advantage to using a RemoveFlakesOMatic class? My patch is based on Ojan's CL which originally used webkit-patch so I moved it over as a class for convenience. I suspect it might be easier to unittest by keeping it as a class (allows feeding it mock objects, faking its state, etc). I'm about to start writing tests so I'll find out how testing in these scripts actually works. I'll keep in mind whether I can convert it to a standalone function. https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:27: class RemoveFlakesOMatic: On 2016/03/07 19:09:53, qyearsley wrote: > What do instances of this class represent? Could the methods here could be > changed to top-level functions or static methods? Addressed above. https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:31: def _can_delete_line(self, test_expectation_line): On 2016/03/07 19:09:53, qyearsley wrote: > test_expectation_line is an instance of test_expectations.TestExpectationLine, > right? (Could potentially add a docstring to explicitly state this.) Done. > > What is the class of the objects in test_expectations? Are they strings? test_expectations is a list of TestExpectationLine instances. https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:34: return False On 2016/03/07 19:09:53, qyearsley wrote: > What does an expectation list of length 0 or 1 indicate? Why does it mean we > can't delete the line? It means the test isn't flaky and this script (right now) only removes flaky lines. e.g. test/mytest.html [ Failure ] doesn't indicate a flake. I can't think of a reason why we shouldn't update these kinds of lines too. I'm not sure if a 0 length expectation is possible. https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:84: test_results = set(results_for_test) On 2016/03/07 19:09:53, qyearsley wrote: > This line could be removed without changing the output; > > From the variable names, results_for_test and test_results sound like two names > for the same thing. Yes, you're right, removed. https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:86: actual_results = set(map(replace_failing_with_fail, test_results)) On 2016/03/07 19:09:54, qyearsley wrote: > Possible rephrasing: > > actual_results = {replace_failing_with_fail(r) for r in results_for_test} Done. https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:90: def _has_unstripable_expectations(self, expectations): On 2016/03/07 19:09:53, qyearsley wrote: > unstripable -> unstrippable Done. https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:94: or 'SLOW' in expectations) On 2016/03/07 19:09:53, qyearsley wrote: > Possible alternate phrasing: > > passing_statuses = ('PASS', 'SLOW', 'NEEDSREBASELINE', > 'NEEDSMANUALREBASELINE') > return any(s in expectations for s in passing_statuses) Done. https://codereview.chromium.org/1766583002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:98: test_expectations = TestExpectations(port, include_overrides=False)._expectations On 2016/03/07 19:09:53, qyearsley wrote: > 1. What is the class of the objects in test_expectations? test_expectations is a list of TestExpectationLine. > 2. This is accessing a private property, maybe TestExpectations should have a > get_expectations method, or that property should be public? Done.
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1766583002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1766583002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== <WIP> Added update_test_expectations script to remove non-flaky test expectations. BUG= ========== to ========== Added update_test_expectations script to remove non-flaky test expectations. BUG= ==========
Ok, I've added tests too, I think this is ready for a real review. Right now, the script looks only at flaky (PASS + <other expectations>) lines and removes them locally if the bot results don't have <other expectations>
The unit test is good and helps with understanding the behavior of this script :-) Is there currently an issue filed that can be associated with this CL? https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (left): https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:24: This CL shouldn't contain changes this file, right? https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py (right): https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:24: expectation (e.g. Crash) this line will be remoed. Excellent, this is helpful. I'm not sure, but maybe the last paragraph might potentially be clarified by adding more detail to the example: Additionally, the runs don't all have to be Passing to remove the line; as long as the non-Passing results are of a type not specified in the expectation; this line will be removed. For example, if the expectation is Crash and the actual failure is Timeout, then the line will be removed. (Also, there's a minor typo on the last line: remoed -> removed) https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:46: self._port = port Host is the host on which the expectations file will be updated (and not the buidlers), right? Might this ever be a different machine than the one on which the script is running? Or is this host object just an abstraction that makes it easier to find and update the TestExpectations file? https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:56: Optional docstring recommendations: - It's faster to skim through the docstring if the first line is really short, e.g.: "Checks whether a given line can be deleted." - Then more explanation can be given in a paragraph below, e.g. "This function checks the actual results from the bots, and if those results show that the test is not flaky or is not failing as expected, then the expectation line can be removed." - If the function returns something, it might be helpful to add a Returns section, even if it's fairly obvious from the function name what it returns, e.g. Returns: True if the line can be removed, False otherwise. https://google.github.io/styleguide/pyguide.html?showone=Comments#Comments https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:85: # a bot. Sounds reasonable -- question is, should it be considered a fatal error? It should be safe to continue and update the expectations for other bots, so I guess it could just print a warning? https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:104: def _met_expectations(self, test_expectation_line, results_for_single_test): The name of this function currently suggests it might return a boolean -- to clarify this, it might be helpful to add a docstring describing in English the set that's returned; e.g. the set of expectations that were met. results_for_single_test is a list of test result/expectation strings (e.g. PASS, TEXT, IMAGE), and test_expectation_line is TestExpectationLine object presumably? Potentially, a couple unit test methods could also clarify how this function behaves. For example, if the expectation is FAIL and result is TEXT, this returns set(['FAIL']). If the expectation was ['PASS', 'CRASH'] and the results were the same, then this would return set(['PASS', 'CRASH']). https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:116: def _has_unstrippable_expectations(self, expectations): expectations here is a list of strings, right? https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:136: ) Does getting the builder results require making network requests to various different builders, or are the results all stored in some central place currently? https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py (right): https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:12: from webkitpy.layout_tests.models.test_expectations import * If you make a separate line for each of the names imported, then it makes it a little clearer where things come from when reading the code: from webkitpy.layout_tests.models.test_expectations import TestExpectations Is anything else used from test_configuration and test_expectations below? https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:15: This blank line could be removed. https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:20: def __init__(self, results_by_path): One blank line is added above the first method in the classes below; optionally it could also be added here. https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:34: Sets the results that will be returned by expectationts_for_builder. Method summary can go on the first line: """Sets the results... https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:37: results_by_path_by_builder(Dict): The distinct results seen in results_by_path_by_builder(Dict) -> results_by_path_by_builder (dict) https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:47: Blank lines at the end of docs could be removed. https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:87: def _parse_exp(self, expectations): Could rename _exp and _parse_exp to _expectations and _parse_expectations, to lessen ambiguity (exp can also stand for expression sometimes). This function could also use a docstring. https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:298: """ Test correct operation of Debug/Release specifiers - Space after """ is unnecessary - In other function summaries, third-person singular form is used (i.e. "Tests" rather than "Test"). - Docstring one-liner generally ends with a period. - Since this is one line, the closing """ can go on the same line.
Created an issue and CC'd you on it. https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (left): https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:24: On 2016/03/10 19:13:58, qyearsley wrote: > This CL shouldn't contain changes this file, right? Right, sorry, this snuck in by accident. Removed. https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py (right): https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:24: expectation (e.g. Crash) this line will be remoed. On 2016/03/10 19:13:58, qyearsley wrote: > Excellent, this is helpful. > > I'm not sure, but maybe the last paragraph might potentially be clarified by > adding more detail to the example: > > Additionally, the runs don't all have to be Passing to remove the line; > as long as the non-Passing results are of a type not specified in the > expectation; this line will be removed. For example, if the expectation > is > Crash and the actual failure is Timeout, then the line will be removed. > > (Also, there's a minor typo on the last line: remoed -> removed) Done. https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:46: self._port = port On 2016/03/10 19:13:58, qyearsley wrote: > Host is the host on which the expectations file will be updated (and not the > buidlers), right? Might this ever be a different machine than the one on which > the script is running? Or is this host object just an abstraction that makes it > easier to find and update the TestExpectations file? I'm not 100% clear on what Host and Port are but I think host is just an abstraction layer between the script and the environment its running in. It provides filesystem functions and other things of that nature. https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:56: On 2016/03/10 19:13:58, qyearsley wrote: > Optional docstring recommendations: > > - It's faster to skim through the docstring if the first line is really short, > e.g.: > "Checks whether a given line can be deleted." > > - Then more explanation can be given in a paragraph below, e.g. > "This function checks the actual results from the bots, and if those results > show that the test is not flaky or is not failing as expected, then the > expectation line can be removed." > > - If the function returns something, it might be helpful to add a Returns > section, even if it's fairly obvious from the function name what it returns, > e.g. > Returns: > True if the line can be removed, False otherwise. > > https://google.github.io/styleguide/pyguide.html?showone=Comments#Comments Thanks, done. https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:85: # a bot. On 2016/03/10 19:13:58, qyearsley wrote: > Sounds reasonable -- question is, should it be considered a fatal error? It > should be safe to continue and update the expectations for other bots, so I > guess it could just print a warning? I think not because, for example, if we have an expectation that specifies no platforms (i.e. all configurations), we only want to remove it if all bots show it passing. If, for whatever reason, we fail to get the builder results for one platform we may choose to remove the expectation even though there's failing runs on that platform. I mainly left this like this so I could write the tests without having to write a results for each configuration specified in builders._exact_matches but I think the proper thing to do is to stub out the functions in builders (e.g. builders.all_builder_names) so we can mock those for tests. I'll go ahead and do this now, and change this to cause failure. Do we have a canonical way to report an error and exit? https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:104: def _met_expectations(self, test_expectation_line, results_for_single_test): On 2016/03/10 19:13:58, qyearsley wrote: > The name of this function currently suggests it might return a boolean -- to > clarify this, it might be helpful to add a docstring describing in English the > set that's returned; e.g. the set of expectations that were met. > > results_for_single_test is a list of test result/expectation strings (e.g. PASS, > TEXT, IMAGE), and test_expectation_line is TestExpectationLine object > presumably? > > Potentially, a couple unit test methods could also clarify how this function > behaves. For example, if the expectation is FAIL and result is TEXT, this > returns set(['FAIL']). If the expectation was ['PASS', 'CRASH'] and the results > were the same, then this would return set(['PASS', 'CRASH']). I don't really like testing private internals but I've renamed the method and added a descriptive docstring. https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:116: def _has_unstrippable_expectations(self, expectations): On 2016/03/10 19:13:58, qyearsley wrote: > expectations here is a list of strings, right? Right, docstring added. https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:136: ) On 2016/03/10 19:13:58, qyearsley wrote: > Does getting the builder results require making network requests to various > different builders, or are the results all stored in some central place > currently? Not sure since that all happens elsewhere but my belief is that they're aggregated and fetched from the test result dashboard (i.e. the flakiness dash). https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py (right): https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:12: from webkitpy.layout_tests.models.test_expectations import * On 2016/03/10 19:13:59, qyearsley wrote: > If you make a separate line for each of the names imported, then it makes it a > little clearer where things come from when reading the code: > > from webkitpy.layout_tests.models.test_expectations import TestExpectations > > Is anything else used from test_configuration and test_expectations below? They were cargo-culted over here. Removed test_configuration, TestExpectations was all that was needed in the other. https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:15: On 2016/03/10 19:13:59, qyearsley wrote: > This blank line could be removed. Removed all the blanks in import statements. https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:20: def __init__(self, results_by_path): On 2016/03/10 19:13:58, qyearsley wrote: > One blank line is added above the first method in the classes below; optionally > it could also be added here. Done. https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:34: Sets the results that will be returned by expectationts_for_builder. On 2016/03/10 19:13:59, qyearsley wrote: > Method summary can go on the first line: """Sets the results... Done. https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:37: results_by_path_by_builder(Dict): The distinct results seen in On 2016/03/10 19:13:59, qyearsley wrote: > results_by_path_by_builder(Dict) -> results_by_path_by_builder (dict) Done. https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:47: On 2016/03/10 19:13:59, qyearsley wrote: > Blank lines at the end of docs could be removed. Done. https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:87: def _parse_exp(self, expectations): On 2016/03/10 19:13:59, qyearsley wrote: > Could rename _exp and _parse_exp to _expectations and _parse_expectations, to > lessen ambiguity (exp can also stand for expression sometimes). This function > could also use a docstring Seems self._exp was unneeded. Renamed and docstring'd the rest. https://codereview.chromium.org/1766583002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:298: """ Test correct operation of Debug/Release specifiers On 2016/03/10 19:13:59, qyearsley wrote: > - Space after """ is unnecessary > - In other function summaries, third-person singular form is used (i.e. "Tests" > rather than "Test"). > - Docstring one-liner generally ends with a period. > - Since this is one line, the closing """ can go on the same line. Done.
Description was changed from ========== Added update_test_expectations script to remove non-flaky test expectations. BUG= ========== to ========== Added update_test_expectations script to remove non-flaky test expectations. BUG=595414 ==========
Made some changes to allow better testing. Also, the script is now more conservative: if it can't find the results for a bot specified as a matching_configuration on the expectation it won't remove the line. How's this look? Only thing I think remaining is better error/warning logging. What's the standard way for us to report those?
It looks like logging in webkitpy is done by adding a line at the top level at the top of the module that says: _log = logging.getLogger(__name__) And then calling _log.error, _log.warning, _log.debug etc. Of course this requires having `import logging` in the import section of the module. There's a module called logutils that has a slightly different get_logger function which is supposed to be used when your module may be run directly, but not many modules in webkitpy use that. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... If your test case class subclasses webkitpy.common.system.logtesting.LoggingTestCase, then you can put `self._log = LogTesting.setUp(self)` in setUp, and use `self._log.assertMessages(expected_log_messages)` in test methods. Example: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py (right): https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py:361: class TestPort(Port): Question, unrelated to this CL: Do you know what a TestPort represents? Does it represent one layout test on one platform? (or multiple platforms, or multiple tests)? https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py (right): https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:51: expectations_file) Note, the unit test doesn't cover main(), or write_test_expectations(), but I think that's fine assuming that it's manually tested by running the script. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:101: return False For here and above, I agree that logging an error would be good. It may also be a good idea to test the logging of these different kinds of error in the unit test. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:105: # No results means the test was all skipped or all results are passing. test was all skipped -> tests were all skipped https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:107: continue This line is not covered by the unit test right now; maybe we could add one test method where all tests are pass/skip (and the test expectations are not modified)? https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:177: return None Logging an error here sounds good to me. Also, this line isn't currently covered by the unit tests. When will self._expectations_factory.expectations_for_builder(builder_name) be None or empty? https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:195: # whitespace. Would it be correct to rephrase "if it's not separated by the tests by whitespace" as "if it's not separated by the test expectation lines by a blank line"? https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:201: removed_whitespace = True I think there may be no test case that covers this while clause currently -- this should be covered as long as we have a test method where there's an example test expectations with multiple blank lines in a row, right? https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py (right): https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:54: def __init__(self, testFunc): What is testFunc here? Also, would it make sense to use setUpClass and tearDownClass to do this? https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:68: self._old_builders = builders._exact_matches Maybe this attribute would be clearer if it were called `self._old_builders_exact_matches`, or `self._original_builders_exact_matches`? https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:215: Bug(test) test/c.html [ Crash Pass ]""") Formatting side note: If it's OK to have a newline at the the string, you could remove the parentheses; it's also valid to write: test_expectations_before = """ # Remove this since it's passing all runs. ... Bug(test) test/c.html [ Crash Pass ]""" But the current way with parentheses is good too.
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
Unfortunately, this patch is awfully large to digest in one chunk. I've left some comments, but we might want to explore splitting this up in to multiple patches so that it can be more easily reviewed ... https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py (right): https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py:361: class TestPort(Port): On 2016/03/21 21:45:25, qyearsley wrote: > Question, unrelated to this CL: > > Do you know what a TestPort represents? Does it represent one layout test on one > platform? (or multiple platforms, or multiple tests)? A TestPort is a fake implementation of the Port class, used for unit testing the webkitpy implementations. FakePort would probably be a better name. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py:403: } Nit: I would leave out the "_mock" part of these names. These aren't actually mocks (they're fakes), and even calling them "_fake" doesn't add much, since the whole class is a fake. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py:560: return self._mock_all_systems Why not just make these things public instance variables? It's not very pythonic to use setters and getters. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py (right): https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:32: """), formatter_class=argparse.RawTextHelpFormatter) I would actually move this string up to the top-level docstring (i.e., move it after line 4), and then use epilog=__doc__). https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:51: expectations_file) On 2016/03/21 21:45:25, qyearsley wrote: > Note, the unit test doesn't cover main(), or write_test_expectations(), but I > think that's fine assuming that it's manually tested by running the script. By passing a MockHost() to main, you can (and should) stub out everything without actually incurring any file i/o, so you should be able to write tests for that as well. You should probably add a method to MockHost() to return a faked-out BotTestExpectationsFactory() so you can control the output of that as well. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:54: class RemoveFlakesOMatic: derive this class from object, i.e. RemoveFlakesOMatic(object). https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:95: # and assume we need these expectations to make a decision? Are you wondering about cases like Debug bots where we don't run the tests on every o/s in debug? https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py (right): https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:26: self._all_results_by_builder = {} Just make these public instance variables and get rid of the set() method. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:54: def __init__(self, testFunc): On 2016/03/21 21:45:25, qyearsley wrote: > What is testFunc here? > > Also, would it make sense to use setUpClass and tearDownClass to do this? Don't override __init__ or use setUpClass or tearDownClass(), just use setUp() and tearDown(). There's no reason to try and optimize this and using things that aren't reset per method can lead to weird results. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:76: builders._exact_matches = self._old_builders It is really ugly to be mutating module-level statics here, and it's lame that builders.py is just a bunch of free functions in the first place. Any idea how hard it would be to refactor that a bit to make builders.py contain a class instead? https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:94: """Parses a TestExpectatoin file given as string. nit: s/TestExpectatoin/TestExpectation/.
https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py (right): https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py:361: class TestPort(Port): On 2016/03/21 21:45:25, qyearsley wrote: > Question, unrelated to this CL: > > Do you know what a TestPort represents? Does it represent one layout test on one > platform? (or multiple platforms, or multiple tests)? I'm not sure. TestPort is a testing version of the Port class. My impression is that it might have related to the WebKit notion of a port. e.g Chromium was a port, Safari was a port, etc. In this case, I think different ports would have their own builders and infrastructure. That's my best guess though. Ojan or Dirk might know more. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py:403: } On 2016/03/29 22:39:01, Dirk Pranke wrote: > Nit: I would leave out the "_mock" part of these names. These aren't actually > mocks (they're fakes), and even calling them "_fake" doesn't add much, since the > whole class is a fake. Done. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py:560: return self._mock_all_systems On 2016/03/29 22:39:01, Dirk Pranke wrote: > Why not just make these things public instance variables? It's not very pythonic > to use setters and getters. Done. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py (right): https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:32: """), formatter_class=argparse.RawTextHelpFormatter) On 2016/03/29 22:39:01, Dirk Pranke wrote: > I would actually move this string up to the top-level docstring (i.e., move it > after line 4), and then use epilog=__doc__). Done. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:51: expectations_file) On 2016/03/29 22:39:01, Dirk Pranke wrote: > On 2016/03/21 21:45:25, qyearsley wrote: > > Note, the unit test doesn't cover main(), or write_test_expectations(), but I > > think that's fine assuming that it's manually tested by running the script. > > By passing a MockHost() to main, you can (and should) stub out everything > without actually incurring any file i/o, > so you should be able to write tests for that as well. > > You should probably add a method to MockHost() to return a faked-out > BotTestExpectationsFactory() so you > can control the output of that as well. Added a TODO, will add shortly. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:54: class RemoveFlakesOMatic: On 2016/03/29 22:39:01, Dirk Pranke wrote: > derive this class from object, i.e. RemoveFlakesOMatic(object). Done. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:95: # and assume we need these expectations to make a decision? On 2016/03/29 22:39:01, Dirk Pranke wrote: > Are you wondering about cases like Debug bots where we don't run the tests on > every o/s in debug? Yah, I think it's mostly debug, but not all. The matching_configurations that aren't in builders.py include: [icecreamsandwich, x86, release] The Mac10.11 bot has this configuration: [mac10.11, x86, debug] But builders.py's specifiers use "10.11" instead of "Mac10.11". Is it right to be matching up the specifiers in builders.py to the ones in matching_configurations? https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:101: return False On 2016/03/21 21:45:25, qyearsley (OOO til April 22) wrote: > For here and above, I agree that logging an error would be good. > > It may also be a good idea to test the logging of these different kinds of error > in the unit test. Added logging messages. Will add tests shortly. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:105: # No results means the test was all skipped or all results are passing. On 2016/03/21 21:45:25, qyearsley wrote: > test was all skipped -> tests were all skipped Done. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:107: continue On 2016/03/21 21:45:25, qyearsley (OOO til April 22) wrote: > This line is not covered by the unit test right now; maybe we could add one test > method where all tests are pass/skip (and the test expectations are not > modified)? Ack. Added a TODO, will do shortly. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:177: return None On 2016/03/21 21:45:25, qyearsley (OOO til April 22) wrote: > Logging an error here sounds good to me. Also, this line isn't currently covered > by the unit tests. Added log message. TODO and working on test. > > When will self._expectations_factory.expectations_for_builder(builder_name) be > None or empty? It shouldn't happen normally, I think. If the results JSON doesn't include a builder from builders.py. I'm not sure how the JSON is generated so I don't know what happens if a bot goes down or we decomission one. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:195: # whitespace. On 2016/03/21 21:45:25, qyearsley wrote: > Would it be correct to rephrase "if it's not separated by the tests by > whitespace" as "if it's not separated by the test expectation lines by a blank > line"? Done. (replaced with "if it's not separated _from_ the test expectation line by whitespace) https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py:201: removed_whitespace = True On 2016/03/21 21:45:25, qyearsley (OOO til April 22) wrote: > I think there may be no test case that covers this while clause currently -- > this should be covered as long as we have a test method where there's an example > test expectations with multiple blank lines in a row, right? It should be covered by test_preserve_comments_and_whitespace https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py (right): https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:26: self._all_results_by_builder = {} On 2016/03/29 22:39:01, Dirk Pranke wrote: > Just make these public instance variables and get rid of the set() method. Done. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:54: def __init__(self, testFunc): On 2016/03/21 21:45:25, qyearsley wrote: > What is testFunc here? > > Also, would it make sense to use setUpClass and tearDownClass to do this? It's part of the Python unittest framework. testFunc is a string, the name of the method to run for the test. I cargo-culted this from some other test. The test harness will pass the different test method names in here when running tests. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:54: def __init__(self, testFunc): On 2016/03/29 22:39:01, Dirk Pranke wrote: > On 2016/03/21 21:45:25, qyearsley wrote: > > What is testFunc here? > > > > Also, would it make sense to use setUpClass and tearDownClass to do this? > > Don't override __init__ or use setUpClass or tearDownClass(), just use setUp() > and tearDown(). There's no reason to try and optimize this and using things that > aren't reset per method can lead to weird results. Done. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:68: self._old_builders = builders._exact_matches On 2016/03/21 21:45:26, qyearsley wrote: > Maybe this attribute would be clearer if it were called > `self._old_builders_exact_matches`, or `self._original_builders_exact_matches`? Done. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:76: builders._exact_matches = self._old_builders On 2016/03/29 22:39:01, Dirk Pranke wrote: > It is really ugly to be mutating module-level statics here, and it's lame that > builders.py is just a bunch of free functions in the first place. > > Any idea how hard it would be to refactor that a bit to make builders.py contain > a class instead? Yah, I agree. I'll look into doing this as a separate patch before landing this. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:94: """Parses a TestExpectatoin file given as string. On 2016/03/29 22:39:01, Dirk Pranke wrote: > nit: s/TestExpectatoin/TestExpectation/. Done. https://codereview.chromium.org/1766583002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py:215: Bug(test) test/c.html [ Crash Pass ]""") On 2016/03/21 21:45:25, qyearsley wrote: > Formatting side note: If it's OK to have a newline at the the string, you could > remove the parentheses; it's also valid to write: > > test_expectations_before = """ > # Remove this since it's passing all runs. > ... > Bug(test) test/c.html [ Crash Pass ]""" > > But the current way with parentheses is good too. Yah, but that includes the newline in the string at the beginning (I had to deal with lots of \n headaches here :).
Sorry for the delay, this is a 20% for me so I work on it when I can find the time. On 2016/03/29 22:39:01, Dirk Pranke wrote: > Unfortunately, this patch is awfully large to digest in one chunk. > > I've left some comments, but we might want to explore splitting this up in to > multiple patches so that it can be more easily reviewed ... > Yah, it is basically one piece of functionality so I'm not sure how best to do that. I'll take a look and see what I can split off. Otherwise, if you have suggestions on how to do that it'd also be appreciated. Thanks.
On 2016/04/05 at 12:32:32, bokan wrote: > Sorry for the delay, this is a 20% for me so I work on it when I can find the time. > > On 2016/03/29 22:39:01, Dirk Pranke wrote: > > Unfortunately, this patch is awfully large to digest in one chunk. > > > > I've left some comments, but we might want to explore splitting this up in to > > multiple patches so that it can be more easily reviewed ... > > > > Yah, it is basically one piece of functionality so I'm not sure how best to do that. I'll take a look and see what I can split off. Otherwise, if you have suggestions on how to do that it'd also be appreciated. Thanks. Hi bokan! Do you think it seems reasonable to first submit the changes in: third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py With unit tests? (Since update_test_expectations.py is dependent on those changes but those changes don't depend on anything else)
On 2016/04/28 17:59:38, qyearsley wrote: > On 2016/04/05 at 12:32:32, bokan wrote: > > Sorry for the delay, this is a 20% for me so I work on it when I can find the > time. > > > > On 2016/03/29 22:39:01, Dirk Pranke wrote: > > > Unfortunately, this patch is awfully large to digest in one chunk. > > > > > > I've left some comments, but we might want to explore splitting this up in > to > > > multiple patches so that it can be more easily reviewed ... > > > > > > > Yah, it is basically one piece of functionality so I'm not sure how best to do > that. I'll take a look and see what I can split off. Otherwise, if you have > suggestions on how to do that it'd also be appreciated. Thanks. > > Hi bokan! > > Do you think it seems reasonable to first submit the changes in: > > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py > > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py > third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/test.py > > With unit tests? (Since update_test_expectations.py is dependent on those > changes but those changes don't depend on anything else) Yup, that's along the lines of what I was thinking. If I can I'll try to split up the functionality in update-test-expectations but I'll have to take a look if that feasible. Right now, I'm trying to update builders.py to be a mockable object as suggested by dpranke@ and I'll land that first. Hope to get some work on this done tomorrow or early next week.
Sorry about the break, I've been 20%'ing this and I got busy but I've got some time to push this to the finish now :) As per Dirk's earlier recommendation, I've done my best to split this patch up. I broke it into 3 parts: http://crrev.com/2121823003 [1/3] Refactor test.py to be easier to fake. http://crrev.com/2119303003 [2/3] Create the harness and shell of update_test_expectations http://crrev.com/2125633002 [3/3] Fill out implementation of UpdateTestExpectations I've tried to keep each CL a coherent as possible so they each stand on their own. We can land them separately or, to keep everything together, you can review those separately and we can land as one from this CL when they're all l-g-t-m'd. Let me know which you'd prefer.
On 2016/07/05 at 12:53:26, bokan wrote: > Sorry about the break, I've been 20%'ing this and I got busy but I've got some time to push this to the finish now :) > > As per Dirk's earlier recommendation, I've done my best to split this patch up. I broke it into 3 parts: > > http://crrev.com/2121823003 [1/3] Refactor test.py to be easier to fake. > http://crrev.com/2119303003 [2/3] Create the harness and shell of update_test_expectations > http://crrev.com/2125633002 [3/3] Fill out implementation of UpdateTestExpectations > > I've tried to keep each CL a coherent as possible so they each stand on their own. We can land them separately or, to keep everything together, you can review those separately and we can land as one from this CL when they're all l-g-t-m'd. Let me know which you'd prefer. Reviewing and landing them split up SGTM :-) I think that one disadvantage is that it requires you to rebase your changes, but on the positive side I think it makes it easier to think of the changes as individual independent changes. |
