Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2200433002:
Invoke webkit-patch in update-w3c-test-expectations (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. |
DescriptionInvoke webkit-patch in update-w3c-test-expectations.
Download necessary baselines from failing JS tests that did not crash or timeout.
BUG=632780
Committed: https://crrev.com/73e778368d74a726f01ebbdb65a592b35fbf6a47
Cr-Commit-Position: refs/heads/master@{#409310}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Modifies get_expected_txt_files #
Total comments: 9
Patch Set 3 : Adds unit tests for new functions #
Messages
Total messages: 20 (7 generated)
Description was changed from ========== added function to update-w3c-test-expectations BUG=632780 ========== to ========== Invokes WebKit-Patch in update-w3c-test-expectations. Downloads necessary baselines from failing js tests that did not crash or timeout. BUG=632780 ==========
dcampb@google.com changed reviewers: + dpranke@chromium.org, qyearsley@chromium.org, raikiri@google.com
On 2016/07/29 at 19:07:58, dcampb wrote: > Not exactly sure about the approach I should take in order to test this function. Any suggestions?
Description was changed from ========== Invokes WebKit-Patch in update-w3c-test-expectations. Downloads necessary baselines from failing js tests that did not crash or timeout. BUG=632780 ========== to ========== Invoke webkit-patch in update-w3c-test-expectations. Download necessary baselines from failing JS tests that did not crash or timeout. BUG=632780 ==========
A few initial comments, will continue to review later. https://codereview.chromium.org/2200433002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2200433002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:15: from webkitpy.common.webkit_finder import WebKitFinder Remember to sort the new imports https://codereview.chromium.org/2200433002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:278: Invokes WebKit-Patch RebaselineFromTryJobs in order WebKit-Patch RebaselineFromTryJobs -> webkit-patch rebaseline-from-try-jobs https://codereview.chromium.org/2200433002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:280: tests that did not Crash or Timeout. Afterwords, the platform- Afterwards -> Afterwards or "Then, ..." https://codereview.chromium.org/2200433002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:290: WebKit-Patch RebaselineFromTryJobs. WebKit-Patch RebaselineFromTryJobs -> webkit-patch rebaseline-from-try-jobs https://codereview.chromium.org/2200433002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:296: crash_or_timeout = ['CRASH', 'TIMEOUT'] The fewer variables there are in a function, then easier the function is to understand. So, if some value is used in one place and it's not a very complicated expression, then the value can be used directly, e.g. webkit_patch = self._host.filesystem.join(root, finder.webkit_base(), ...) tests_results[test_path][platform]['actual'] not in ['CRASH', 'TIMEOUT']: Exceptions: If the variable is used in lots of places, or if it gives a name to a complex expression, then it can make the function easier to read. https://codereview.chromium.org/2200433002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:311: # Was used for testing I'm not quite sure what this comment means now https://codereview.chromium.org/2200433002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:318: ' '.join(tests_to_rebaseline)]) Each of the tests is a separate arg, so the input to run_command can be: ['python', webkit_patch, 'rebaseline-from-try-jobs', '-v'] + tests_to_rebaseline
https://codereview.chromium.org/2200433002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2200433002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:278: Invokes WebKit-Patch RebaselineFromTryJobs in order On 2016/07/29 at 20:59:45, qyearsley wrote: > WebKit-Patch RebaselineFromTryJobs -> webkit-patch rebaseline-from-try-jobs done https://codereview.chromium.org/2200433002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:280: tests that did not Crash or Timeout. Afterwords, the platform- On 2016/07/29 at 20:59:45, qyearsley wrote: > Afterwards -> Afterwards > or "Then, ..." done https://codereview.chromium.org/2200433002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:290: WebKit-Patch RebaselineFromTryJobs. On 2016/07/29 at 20:59:44, qyearsley wrote: > WebKit-Patch RebaselineFromTryJobs -> webkit-patch rebaseline-from-try-jobs done https://codereview.chromium.org/2200433002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:296: crash_or_timeout = ['CRASH', 'TIMEOUT'] On 2016/07/29 at 20:59:45, qyearsley wrote: > The fewer variables there are in a function, then easier the function is to understand. So, if some value is used in one place and it's not a very complicated expression, then the value can be used directly, e.g. > > webkit_patch = self._host.filesystem.join(root, finder.webkit_base(), ...) > > tests_results[test_path][platform]['actual'] not in ['CRASH', 'TIMEOUT']: > > Exceptions: If the variable is used in lots of places, or if it gives a name to a complex expression, then it can make the function easier to read. noted https://codereview.chromium.org/2200433002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:296: crash_or_timeout = ['CRASH', 'TIMEOUT'] On 2016/07/29 at 20:59:45, qyearsley wrote: > The fewer variables there are in a function, then easier the function is to understand. So, if some value is used in one place and it's not a very complicated expression, then the value can be used directly, e.g. > > webkit_patch = self._host.filesystem.join(root, finder.webkit_base(), ...) > > tests_results[test_path][platform]['actual'] not in ['CRASH', 'TIMEOUT']: > > Exceptions: If the variable is used in lots of places, or if it gives a name to a complex expression, then it can make the function easier to read. noted https://codereview.chromium.org/2200433002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:311: # Was used for testing On 2016/07/29 at 20:59:45, qyearsley wrote: > I'm not quite sure what this comment means now removed it now. It was because I had some newly imported tests on my local machine that weren't run on the bots so their keys weren't in the layouttestsresults. https://codereview.chromium.org/2200433002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:318: ' '.join(tests_to_rebaseline)]) On 2016/07/29 at 20:59:45, qyearsley wrote: > Each of the tests is a separate arg, so the input to run_command can be: > > ['python', webkit_patch, 'rebaseline-from-try-jobs', '-v'] + tests_to_rebaseline tests_to_rebaseline is a of test names. This way I can pass all of the tests as a string. Its the only way I thought of to call rebaseline-from-try-jobs without having a loop.
https://codereview.chromium.org/2200433002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2200433002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:318: ' '.join(tests_to_rebaseline)]) On 2016/08/01 at 17:26:02, dcampb wrote: > On 2016/07/29 at 20:59:45, qyearsley wrote: > > Each of the tests is a separate arg, so the input to run_command can be: > > > > ['python', webkit_patch, 'rebaseline-from-try-jobs', '-v'] + tests_to_rebaseline > > tests_to_rebaseline is a of test names. This way I can pass all of the tests as a string. Its the only way I thought of to call rebaseline-from-try-jobs without having a loop. Concatenating the start of the argument list with the list of test names with + should still work -- Remember, list concatenation with + also works, e.g. >>> ['foo', 'bar'] + ['a', 'b', 'c'] ['foo', 'bar', 'a', 'b', 'c'] And if that's being interpreted as a command to run, then `foo bar a b c` would be run. https://codereview.chromium.org/2200433002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2200433002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:296: js_test = self.is_js_test(finder, test_dir) `test_dir` is generally not a directory, right? It's a file which has been modified by importing new tests, right? https://codereview.chromium.org/2200433002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:297: if js_test: To simplify the above two lines and avoid adding another variable, you can call self.is_js_test directly in the if expression. https://codereview.chromium.org/2200433002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:302: tests_to_rebaseline.append(test_path) It seems like you may be able to extract a helper function here, which takes a list of changed files and a dict of test names to platforms to test results, and returns a list of tests to rebaseline. This helper function may be easier to test since it wouldn't depend on executive -- it would require setting up a MockFileSystem with some JS files, and using an example results dictionary and example list of changed files. https://codereview.chromium.org/2200433002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:309: return tests_results If we were to write a unit test for this function, I think it would: - Set up canned results for running the various commands using `MockExecutive2`. - Assert that certain entries were removed from the dictionary (or not removed, if there was nothing to remove). If this would be helpful, you could consider adding a unit test for this whole function. It might be easier to extract help functions and test those, which may also be helpful. https://codereview.chromium.org/2200433002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:311: def is_js_test(self, webkit_finder, test_path): 1. Is test_path relative to the LayoutTests directory, or relative to the chromium/src repo root? Normally for layout tests, "test path" and "test name" both refer to path relative to LayoutTests.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2200433002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2200433002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:296: js_test = self.is_js_test(finder, test_dir) On 2016/08/01 at 18:33:19, qyearsley wrote: > `test_dir` is generally not a directory, right? It's a file which has been modified by importing new tests, right? correct. The name 'test_dir' is a bit confusing. Changed to 'tests' instead. https://codereview.chromium.org/2200433002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:297: if js_test: On 2016/08/01 at 18:33:19, qyearsley wrote: > To simplify the above two lines and avoid adding another variable, you can call self.is_js_test directly in the if expression. done https://codereview.chromium.org/2200433002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:309: return tests_results On 2016/08/01 at 18:33:19, qyearsley wrote: > If we were to write a unit test for this function, I think it would: > > - Set up canned results for running the various commands using `MockExecutive2`. > - Assert that certain entries were removed from the dictionary (or not removed, if there was nothing to remove). > > If this would be helpful, you could consider adding a unit test for this whole function. It might be easier to extract help functions and test those, which may also be helpful. I have extracted helper functions and am currently writing unit test for them. https://codereview.chromium.org/2200433002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:311: def is_js_test(self, webkit_finder, test_path): On 2016/08/01 at 18:33:18, qyearsley wrote: > 1. Is test_path relative to the LayoutTests directory, or relative to the chromium/src repo root? > > Normally for layout tests, "test path" and "test name" both refer to path relative to LayoutTests. It is relative to layoutTests.
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/2200433002/diff/1/third_party/WebKit/Tools/Sc... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py (right): https://codereview.chromium.org/2200433002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:15: from webkitpy.common.webkit_finder import WebKitFinder On 2016/07/29 at 20:59:44, qyearsley wrote: > Remember to sort the new imports done https://codereview.chromium.org/2200433002/diff/1/third_party/WebKit/Tools/Sc... third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py:318: ' '.join(tests_to_rebaseline)]) On 2016/08/01 at 18:33:18, qyearsley wrote: > On 2016/08/01 at 17:26:02, dcampb wrote: > > On 2016/07/29 at 20:59:45, qyearsley wrote: > > > Each of the tests is a separate arg, so the input to run_command can be: > > > > > > ['python', webkit_patch, 'rebaseline-from-try-jobs', '-v'] + tests_to_rebaseline > > > > tests_to_rebaseline is a of test names. This way I can pass all of the tests as a string. Its the only way I thought of to call rebaseline-from-try-jobs without having a loop. > > Concatenating the start of the argument list with the list of test names with + should still work -- Remember, list concatenation with + also works, e.g. > >>> ['foo', 'bar'] + ['a', 'b', 'c'] > ['foo', 'bar', 'a', 'b', 'c'] > And if that's being interpreted as a command to run, then `foo bar a b c` would be run. okay, I see. Changes made
lgtm
The CQ bit was checked by dcampb@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Invoke webkit-patch in update-w3c-test-expectations. Download necessary baselines from failing JS tests that did not crash or timeout. BUG=632780 ========== to ========== Invoke webkit-patch in update-w3c-test-expectations. Download necessary baselines from failing JS tests that did not crash or timeout. BUG=632780 Committed: https://crrev.com/73e778368d74a726f01ebbdb65a592b35fbf6a47 Cr-Commit-Position: refs/heads/master@{#409310} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/73e778368d74a726f01ebbdb65a592b35fbf6a47 Cr-Commit-Position: refs/heads/master@{#409310}
Message was sent while issue was closed.
lgtm (belatedly). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
