Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2183913002:
Removes build and builder object refrences along with minor bug fixes (Closed)
|
Created:
4 years, 4 months ago by dcampb Modified:
4 years, 4 months ago CC:
blink-reviews, blink-reviews-w3ctests_chromium.org, chromium-reviews, Dirk Pranke Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemoves build and builder object references along with minor bug fixes
This CL:
1. Removes the uses of build and builder objects in main and get_failing_results_dict.
2. Returns a new dictionary with merged_keys in merge_same_valued_keys.
3. Adds more test failure types to get_expectations to return more accurate expectations.
4. Modifies create_line_list to only create a line if the test name starts with 'imported'.
5. Changes variable names in main.
6. Adds unit tests to cover multiple actual and expected results in get_expectations.
BUG=625254
Committed: https://crrev.com/201b0fae1eb4cbd645603683631fa3959faf74b9
Cr-Commit-Position: refs/heads/master@{#408217}
Patch Set 1 : Removes build and builder object refrences along with minor bug fixes #
Total comments: 16
Patch Set 2 : Modified create_line_list to only add imported/ tests #Patch Set 3 : Added unit tests and changed variable names in main #
Total comments: 24
Patch Set 4 : Adds more failure types and test expectation types to get_expectations #
Messages
Total messages: 31 (16 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
dcampb@google.com changed reviewers: + dpranke@chromium.org, qyearsley@chromium.org
dcampb@google.com changed reviewers: + raikiri@google.com
Description was changed from ========== Removes build and builder object refrences along with minor bug fixes BUG=625254 ========== to ========== Removes build and builder object references along with minor bug fixes BUG=625254 ==========
Description was changed from ========== Removes build and builder object references along with minor bug fixes BUG=625254 ========== to ========== Removes build and builder object references along with minor bug fixes This contains updates that: 1. Removes the uses of build and builder objects in main and get_failing_results_dict. 2. BUG=625254 ==========
Patchset #2 (id:60001) has been deleted
Description was changed from ========== Removes build and builder object references along with minor bug fixes This contains updates that: 1. Removes the uses of build and builder objects in main and get_failing_results_dict. 2. BUG=625254 ========== to ========== Removes build and builder object references along with minor bug fixes This contains updates that: 1. Removes the uses of build and builder objects in main and get_failing_results_dict. 2. Returns a new dictionary with merged_keys in merge_same_valued_keys. 3. Adds more test failure types to get_expectations to return more accurate expectations. 4. Modifies create_line_list to only create a line if the test name starts with 'imported'. 5. Variable name changes in main. BUG=625254 ==========
Description was changed from ========== Removes build and builder object references along with minor bug fixes This contains updates that: 1. Removes the uses of build and builder objects in main and get_failing_results_dict. 2. Returns a new dictionary with merged_keys in merge_same_valued_keys. 3. Adds more test failure types to get_expectations to return more accurate expectations. 4. Modifies create_line_list to only create a line if the test name starts with 'imported'. 5. Variable name changes in main. BUG=625254 ========== to ========== Removes build and builder object references along with minor bug fixes This contains updates that: 1. Removes the uses of build and builder objects in main and get_failing_results_dict. 2. Returns a new dictionary with merged_keys in merge_same_valued_keys. 3. Adds more test failure types to get_expectations to return more accurate expectations. 4. Modifies create_line_list to only create a line if the test name starts with 'imported'. 5. Variable name changes in main. 6. Updates unit test to reflect changes made. BUG=625254 ==========
Description was changed from ========== Removes build and builder object references along with minor bug fixes This contains updates that: 1. Removes the uses of build and builder objects in main and get_failing_results_dict. 2. Returns a new dictionary with merged_keys in merge_same_valued_keys. 3. Adds more test failure types to get_expectations to return more accurate expectations. 4. Modifies create_line_list to only create a line if the test name starts with 'imported'. 5. Variable name changes in main. 6. Updates unit test to reflect changes made. BUG=625254 ========== to ========== Removes build and builder object references along with minor bug fixes This cl contains updates that: 1. Removes the uses of build and builder objects in main and get_failing_results_dict. 2. Returns a new dictionary with merged_keys in merge_same_valued_keys. 3. Adds more test failure types to get_expectations to return more accurate expectations. 4. Modifies create_line_list to only create a line if the test name starts with 'imported'. 5. Changes variable names in main. 6. Updates unit test to reflect changes made. BUG=625254 ==========
A few nits and some potential improvements (all optional) listed below. Also, this could have been made into a couple smaller CLs with more specific titles, to be more specific, but for this CL I think it's fine to keep it as-is. https://codereview.chromium.org/2183913002/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2183913002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:34: platform_results_dict = expectations_line_adder.get_failing_results_dict(buildbot, builder_name, build_number) Somewhat unrelated to this CL: An alternative way of writing the above several lines would be: for job in try_jobs: platform_results = expectations_line_adder.get_failing_results_dict(BuildBot(), job.builder_name, job.build_number) ... Several points here: - Adding the type to the variable name (e.g. _dict, _list) isn't always clearer, and it makes the variable name longer. - Fewer variables and lines is often simpler. - For "named tuple" classes in Python (like TryJob), you can access attributes by name. https://codereview.chromium.org/2183913002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:88: layout_test_results = buildbot.fetch_layout_test_results(results_url) Side note: In a subsequent CL (after this CL is landed), I think it might be nice to add a method to BuildBot that is just "fetch_results", which does a combination of getting the URL and then calling fetch_layout_test_results. When I do this, I should update this code as well. https://codereview.chromium.org/2183913002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:136: 'three': {'foo': bar'} Unrelated to this CL, but there's a quote missing before the last "bar". https://codereview.chromium.org/2183913002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:143: keys = sorted(dictionary.keys()) Same as `keys = sorted(dictionary)` (both are OK). https://codereview.chromium.org/2183913002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:152: next_item = keys[i] If `i` isn't used below, then maybe you could also do `for next_item in keys[1:]: ...` https://codereview.chromium.org/2183913002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:160: keys.remove(key) For many short for loops, you can sometimes write it on one line using list comprehensions, e.g.: keys = [k for k in keys if k not in matching_value_keys] https://codereview.chromium.org/2183913002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:190: for actual in results['actual'].split(): For this change, I think it will be good to have unit test methods that cover the cases when we have multiple expectations in "expected" and/or multiple results in "actual". https://codereview.chromium.org/2183913002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:197: expectations.update([expected.capitalize()]) For sets, if you're adding only one item, you can use the `add` method. So, instead of `expectations.update([expected.capitalize()])`, you can write `expectations.add(expected.capitalize())`.
https://codereview.chromium.org/2183913002/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2183913002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:34: platform_results_dict = expectations_line_adder.get_failing_results_dict(buildbot, builder_name, build_number) On 2016/07/26 at 21:40:35, qyearsley wrote: > Somewhat unrelated to this CL: An alternative way of writing the above several lines would be: > > for job in try_jobs: > platform_results = expectations_line_adder.get_failing_results_dict(BuildBot(), job.builder_name, job.build_number) > ... > > Several points here: > - Adding the type to the variable name (e.g. _dict, _list) isn't always clearer, and it makes the variable name longer. > - Fewer variables and lines is often simpler. > - For "named tuple" classes in Python (like TryJob), you can access attributes by name. suggestions noted and changed made. https://codereview.chromium.org/2183913002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:88: layout_test_results = buildbot.fetch_layout_test_results(results_url) On 2016/07/26 at 21:40:35, qyearsley wrote: > Side note: In a subsequent CL (after this CL is landed), I think it might be nice to add a method to BuildBot that is just "fetch_results", which does a combination of getting the URL and then calling fetch_layout_test_results. When I do this, I should update this code as well. sounds good. https://codereview.chromium.org/2183913002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:136: 'three': {'foo': bar'} On 2016/07/26 at 21:40:35, qyearsley wrote: > Unrelated to this CL, but there's a quote missing before the last "bar". ahh, nice eye. done https://codereview.chromium.org/2183913002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:143: keys = sorted(dictionary.keys()) On 2016/07/26 at 21:40:35, qyearsley wrote: > Same as `keys = sorted(dictionary)` (both are OK). noted https://codereview.chromium.org/2183913002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:152: next_item = keys[i] On 2016/07/26 at 21:40:36, qyearsley wrote: > If `i` isn't used below, then maybe you could also do `for next_item in keys[1:]: ...` nicee, noted. https://codereview.chromium.org/2183913002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:160: keys.remove(key) On 2016/07/26 at 21:40:35, qyearsley wrote: > For many short for loops, you can sometimes write it on one line using list comprehensions, e.g.: > > keys = [k for k in keys if k not in matching_value_keys] nice tip, thanks. done https://codereview.chromium.org/2183913002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:190: for actual in results['actual'].split(): On 2016/07/26 at 21:40:35, qyearsley wrote: > For this change, I think it will be good to have unit test methods that cover the cases when we have multiple expectations in "expected" and/or multiple results in "actual". working on this now. https://codereview.chromium.org/2183913002/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:197: expectations.update([expected.capitalize()]) On 2016/07/26 at 21:40:35, qyearsley wrote: > For sets, if you're adding only one item, you can use the `add` method. So, instead of `expectations.update([expected.capitalize()])`, you can write `expectations.add(expected.capitalize())`. noted and done.
Please look over if you have time.
Description was changed from ========== Removes build and builder object references along with minor bug fixes This cl contains updates that: 1. Removes the uses of build and builder objects in main and get_failing_results_dict. 2. Returns a new dictionary with merged_keys in merge_same_valued_keys. 3. Adds more test failure types to get_expectations to return more accurate expectations. 4. Modifies create_line_list to only create a line if the test name starts with 'imported'. 5. Changes variable names in main. 6. Updates unit test to reflect changes made. BUG=625254 ========== to ========== Removes build and builder object references along with minor bug fixes This cl contains updates that: 1. Removes the uses of build and builder objects in main and get_failing_results_dict. 2. Returns a new dictionary with merged_keys in merge_same_valued_keys. 3. Adds more test failure types to get_expectations to return more accurate expectations. 4. Modifies create_line_list to only create a line if the test name starts with 'imported'. 5. Changes variable names in main. 6. Adds unit tests to cover multiple actual and expected results in get_expectations. BUG=625254 ==========
lgtm w/ qyearsley's approval.
Thanks for adding details to the CL description, that is helpful. I'm still not quite sure about the get_expectations method, which is a bit tricky. https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:162: """Returns a list of test expectations for a given test dict. In particular, it's a list of a expectations which should be added as a new line in TestExpectations, right? https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:178: ['Failure', 'Timeout'] This comment can be updated now that this function returns a set. https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:181: failure_expectations = ['SLOW', 'TEXT', 'FAIL', 'IMAGE+TEXT', 'IMAGE'] I'm not sure if SLOW counts as a failure, and also I don't think SLOW is ever in results["actual"] -- if SLOW is in results["expected"] for a test, I think that also means it's expected to pass. So, I think maybe SLOW can be removed from here. Also, in theory there's one more "baseline mismatch" failure type: AUDIO. I'm pretty sure there are no tests with audio baselines in web-platform-tests, but you could add it here for completeness I think. https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:184: for actual in results['actual'].split(): This morning I was saying that if there are multiple words in results["actual"] then it means the test was retried. If there are multiple words in results["expected"], though, I believe that this means that any of those expected results are OK. For example, if expected is "PASS CRASH" and actual is "CRASH" OR "PASS", then that's OK, and no new expectation line needs to be added. Would it also make sense to rewrite this function to take a LayoutTestResult object (from https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...) https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:190: expectations.add(actual.capitalize()) What if expected is "PASS" and actual is "PASS" -- then we don't want to add any new expectation line, right? https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py (right): https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:54: self.assertEqual(self.get_expectations({'expected': 'PASS', 'actual': 'TIMEOUT CRASH FAIL'}), set(['Crash', 'Failure', 'Timeout'])) Note: I'm not entirely sure if actual can ever contain "FAIL" for layout tests - instead I think it contains a specific failure mode like "TEXT" to indicate the specific type of baseline mismatch. https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:56: What about cases where the results are "as expected" and there should be no lines added to test expectations, e.g. self.get_expectations({'expected': 'PASS', 'actual': 'PASS'}) self.get_expectations({'expected': 'FAIL', 'actual': 'TEXT'}) self.get_expectations({'expected': 'TIMEOUT PASS', 'actual': 'PASS'}) self.get_expectations({'expected': 'TIMEOUT', 'actual': 'TIMEOUT'}) These should all be empty set, right? https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:57: def test_create_line_list_with_non_new_test(self): non_new_test -> no_new_tests
https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:162: """Returns a list of test expectations for a given test dict. On 2016/07/26 at 23:59:43, qyearsley wrote: > In particular, it's a list of a expectations which should be added as a new line in TestExpectations, right? This function returns a list of actual test expectations for a given test. for example, 'Timeout', 'Failure', 'Pass'. This list is used in the function that creates the test-expectations line list. Example, ['crbug.com/626777 [ Win7 ] fast/new/foo/bar.html [ Failure ]'] https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:181: failure_expectations = ['SLOW', 'TEXT', 'FAIL', 'IMAGE+TEXT', 'IMAGE'] On 2016/07/26 at 23:59:43, qyearsley wrote: > I'm not sure if SLOW counts as a failure, and also I don't think SLOW is ever in results["actual"] -- if SLOW is in results["expected"] for a test, I think that also means it's expected to pass. So, I think maybe SLOW can be removed from here. > > Also, in theory there's one more "baseline mismatch" failure type: AUDIO. I'm pretty sure there are no tests with audio baselines in web-platform-tests, but you could add it here for completeness I think. I do remember seeing an example of an expected that was SLOW TIMEOUT CRASH FAIL, however in TestExpectations the only expectations are 'Failure, Pass, Timeout'. Which is why I added SLOW to the list. I will look around for all the possible failure types and expected types and see which ones results in which expectation. https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:184: for actual in results['actual'].split(): On 2016/07/26 at 23:59:43, qyearsley wrote: > This morning I was saying that if there are multiple words in results["actual"] then it means the test was retried. If there are multiple words in results["expected"], though, I believe that this means that any of those expected results are OK. For example, if expected is "PASS CRASH" and actual is "CRASH" OR "PASS", then that's OK, and no new expectation line needs to be added. > > Would it also make sense to rewrite this function to take a LayoutTestResult object (from https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...) Would that result in a mismatch result? or wouldn't that test pass? If so, it would not be ran through this script. This script only deals with results with mismatch results. https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:190: expectations.add(actual.capitalize()) On 2016/07/26 at 23:59:43, qyearsley wrote: > What if expected is "PASS" and actual is "PASS" -- then we don't want to add any new expectation line, right? That wouldn't happen as this script only parses test that have mismatch results.
https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:184: for actual in results['actual'].split(): On 2016/07/27 at 16:26:36, dcampb wrote: > On 2016/07/26 at 23:59:43, qyearsley wrote: > > This morning I was saying that if there are multiple words in results["actual"] then it means the test was retried. If there are multiple words in results["expected"], though, I believe that this means that any of those expected results are OK. For example, if expected is "PASS CRASH" and actual is "CRASH" OR "PASS", then that's OK, and no new expectation line needs to be added. > > > > Would it also make sense to rewrite this function to take a LayoutTestResult object (from https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...) > > Would that result in a mismatch result? or wouldn't that test pass? > > If so, it would not be ran through this script. This script only deals with results with mismatch results. Correction: This script only works with tests that did not run as expected. However, I don't think that made a difference, as if it was expected to 'Crash' or 'Pass' and Passes, then it would not have been reported as a failure. https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:190: expectations.add(actual.capitalize()) On 2016/07/27 at 16:26:36, dcampb wrote: > On 2016/07/26 at 23:59:43, qyearsley wrote: > > What if expected is "PASS" and actual is "PASS" -- then we don't want to add any new expectation line, right? > > That wouldn't happen as this script only parses test that have mismatch results. Correction: This script parses tests that did not run as expected. therefore, the expected and pass would never be the same. Note: There is a function in LayoutTestResults called unexpected_mismatch_results().. not exactly sure how different that is from didnt_run_as_expected().
https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:162: """Returns a list of test expectations for a given test dict. On 2016/07/27 at 16:26:36, dcampb wrote: > On 2016/07/26 at 23:59:43, qyearsley wrote: > > In particular, it's a list of a expectations which should be added as a new line in TestExpectations, right? > > This function returns a list of actual test expectations for a given test. for example, 'Timeout', 'Failure', 'Pass'. > This list is used in the function that creates the test-expectations line list. Example, ['crbug.com/626777 [ Win7 ] fast/new/foo/bar.html [ Failure ]'] Right, so the output of this function determines the expectation type(s) in the new test expectations line. This could optionally be added to the docstring (although this is unnecessary since how the function is used should usually be understood by looking at the code). https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:165: and actual results of a given test name. This function has a precondition or assumption that only result dicts for results where the test didn't run as expected are passed in -- it's helpful to list preconditions and assumptions with the function documentation. https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:181: failure_expectations = ['SLOW', 'TEXT', 'FAIL', 'IMAGE+TEXT', 'IMAGE'] On 2016/07/27 at 16:26:36, dcampb wrote: > On 2016/07/26 at 23:59:43, qyearsley wrote: > > I'm not sure if SLOW counts as a failure, and also I don't think SLOW is ever in results["actual"] -- if SLOW is in results["expected"] for a test, I think that also means it's expected to pass. So, I think maybe SLOW can be removed from here. > > > > Also, in theory there's one more "baseline mismatch" failure type: AUDIO. I'm pretty sure there are no tests with audio baselines in web-platform-tests, but you could add it here for completeness I think. > > I do remember seeing an example of an expected that was SLOW TIMEOUT CRASH FAIL, however in TestExpectations the only expectations are 'Failure, Pass, Timeout'. Which is why I added SLOW to the list. > > I will look around for all the possible failure types and expected types and see which ones results in which expectation. Alright; here shouldn't be any more "baseline mismatch" types besides those four. Note, the "official" documentation for the results JSON format is https://www.chromium.org/developers/the-json-test-results-format. https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:184: for actual in results['actual'].split(): On 2016/07/27 at 16:26:36, dcampb wrote: > On 2016/07/26 at 23:59:43, qyearsley wrote: > > This morning I was saying that if there are multiple words in results["actual"] then it means the test was retried. If there are multiple words in results["expected"], though, I believe that this means that any of those expected results are OK. For example, if expected is "PASS CRASH" and actual is "CRASH" OR "PASS", then that's OK, and no new expectation line needs to be added. > > > > Would it also make sense to rewrite this function to take a LayoutTestResult object (from https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...) > > Would that result in a mismatch result? or wouldn't that test pass? > > If so, it would not be ran through this script. This script only deals with results with mismatch results. What I meant was, since the dict passed in contains basically the same information as a LayoutTestResult object, I was wondering whether it would make sense later to refactor the rest of the code to use LayoutTestResult objects -- but now I see that this isn't a simple refactoring if it were done, and shouldn't be done in this CL. https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:190: expectations.add(actual.capitalize()) On 2016/07/27 at 16:44:24, dcampb wrote: > On 2016/07/27 at 16:26:36, dcampb wrote: > > On 2016/07/26 at 23:59:43, qyearsley wrote: > > > What if expected is "PASS" and actual is "PASS" -- then we don't want to add any new expectation line, right? > > > > That wouldn't happen as this script only parses test that have mismatch results. > > Correction: This script parses tests that did not run as expected. therefore, the expected and pass would never be the same. > > Note: There is a function in LayoutTestResults called unexpected_mismatch_results().. not exactly sure how different that is from didnt_run_as_expected(). Yeah, those names aren't super clear. "Mismatch" was supposed to mean "mismatch with existing baselines", i.e. TEXT, IMAGE, etc. https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py (right): https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations_unittest.py:56: On 2016/07/26 at 23:59:43, qyearsley wrote: > What about cases where the results are "as expected" and there should be no lines added to test expectations, e.g. > self.get_expectations({'expected': 'PASS', 'actual': 'PASS'}) > self.get_expectations({'expected': 'FAIL', 'actual': 'TEXT'}) > self.get_expectations({'expected': 'TIMEOUT PASS', 'actual': 'PASS'}) > self.get_expectations({'expected': 'TIMEOUT', 'actual': 'TIMEOUT'}) > > These should all be empty set, right? Based on your answer above, I think that it's assumed that inputs like these should never be passed in. Still, maybe it's worth considering what would happen if such inputs are passed in.
Patchset #4 (id:120001) has been deleted
https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:162: """Returns a list of test expectations for a given test dict. On 2016/07/27 at 17:25:39, qyearsley wrote: > On 2016/07/27 at 16:26:36, dcampb wrote: > > On 2016/07/26 at 23:59:43, qyearsley wrote: > > > In particular, it's a list of a expectations which should be added as a new line in TestExpectations, right? > > > > This function returns a list of actual test expectations for a given test. for example, 'Timeout', 'Failure', 'Pass'. > > This list is used in the function that creates the test-expectations line list. Example, ['crbug.com/626777 [ Win7 ] fast/new/foo/bar.html [ Failure ]'] > > Right, so the output of this function determines the expectation type(s) in the new test expectations line. This could optionally be added to the docstring (although this is unnecessary since how the function is used should usually be understood by looking at the code). agreed. https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:178: ['Failure', 'Timeout'] On 2016/07/26 at 23:59:43, qyearsley wrote: > This comment can be updated now that this function returns a set. done https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:184: for actual in results['actual'].split(): On 2016/07/27 at 17:25:39, qyearsley wrote: > On 2016/07/27 at 16:26:36, dcampb wrote: > > On 2016/07/26 at 23:59:43, qyearsley wrote: > > > This morning I was saying that if there are multiple words in results["actual"] then it means the test was retried. If there are multiple words in results["expected"], though, I believe that this means that any of those expected results are OK. For example, if expected is "PASS CRASH" and actual is "CRASH" OR "PASS", then that's OK, and no new expectation line needs to be added. > > > > > > Would it also make sense to rewrite this function to take a LayoutTestResult object (from https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...) > > > > Would that result in a mismatch result? or wouldn't that test pass? > > > > If so, it would not be ran through this script. This script only deals with results with mismatch results. > > What I meant was, since the dict passed in contains basically the same information as a LayoutTestResult object, I was wondering whether it would make sense later to refactor the rest of the code to use LayoutTestResult objects -- but now I see that this isn't a simple refactoring if it were done, and shouldn't be done in this CL. I see what your saying. I agree, it could be refactored in a later cl. https://codereview.chromium.org/2183913002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:190: expectations.add(actual.capitalize()) On 2016/07/27 at 17:25:39, qyearsley wrote: > On 2016/07/27 at 16:44:24, dcampb wrote: > > On 2016/07/27 at 16:26:36, dcampb wrote: > > > On 2016/07/26 at 23:59:43, qyearsley wrote: > > > > What if expected is "PASS" and actual is "PASS" -- then we don't want to add any new expectation line, right? > > > > > > That wouldn't happen as this script only parses test that have mismatch results. > > > > Correction: This script parses tests that did not run as expected. therefore, the expected and pass would never be the same. > > > > Note: There is a function in LayoutTestResults called unexpected_mismatch_results().. not exactly sure how different that is from didnt_run_as_expected(). > > Yeah, those names aren't super clear. "Mismatch" was supposed to mean "mismatch with existing baselines", i.e. TEXT, IMAGE, etc. understood. I think it would be wise to get this cl up first, then take into account tests that need -expected.txt files added later.
Description was changed from ========== Removes build and builder object references along with minor bug fixes This cl contains updates that: 1. Removes the uses of build and builder objects in main and get_failing_results_dict. 2. Returns a new dictionary with merged_keys in merge_same_valued_keys. 3. Adds more test failure types to get_expectations to return more accurate expectations. 4. Modifies create_line_list to only create a line if the test name starts with 'imported'. 5. Changes variable names in main. 6. Adds unit tests to cover multiple actual and expected results in get_expectations. BUG=625254 ========== to ========== Removes build and builder object references along with minor bug fixes This CL: 1. Removes the uses of build and builder objects in main and get_failing_results_dict. 2. Returns a new dictionary with merged_keys in merge_same_valued_keys. 3. Adds more test failure types to get_expectations to return more accurate expectations. 4. Modifies create_line_list to only create a line if the test name starts with 'imported'. 5. Changes variable names in main. 6. Adds unit tests to cover multiple actual and expected results in get_expectations. BUG=625254 ==========
lgtm
The CQ bit was checked by dcampb@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2183913002/#ps140001 (title: "Adds more failure types and test expectation types to get_expectations")
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 #4 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Removes build and builder object references along with minor bug fixes This CL: 1. Removes the uses of build and builder objects in main and get_failing_results_dict. 2. Returns a new dictionary with merged_keys in merge_same_valued_keys. 3. Adds more test failure types to get_expectations to return more accurate expectations. 4. Modifies create_line_list to only create a line if the test name starts with 'imported'. 5. Changes variable names in main. 6. Adds unit tests to cover multiple actual and expected results in get_expectations. BUG=625254 ========== to ========== Removes build and builder object references along with minor bug fixes This CL: 1. Removes the uses of build and builder objects in main and get_failing_results_dict. 2. Returns a new dictionary with merged_keys in merge_same_valued_keys. 3. Adds more test failure types to get_expectations to return more accurate expectations. 4. Modifies create_line_list to only create a line if the test name starts with 'imported'. 5. Changes variable names in main. 6. Adds unit tests to cover multiple actual and expected results in get_expectations. BUG=625254 Committed: https://crrev.com/201b0fae1eb4cbd645603683631fa3959faf74b9 Cr-Commit-Position: refs/heads/master@{#408217} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/201b0fae1eb4cbd645603683631fa3959faf74b9 Cr-Commit-Position: refs/heads/master@{#408217} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
