Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2107993002:
Add --auto-update flag which uploads committed CL and initiates the CQ (Closed)
|
Created:
4 years, 5 months ago by raikiri Modified:
4 years, 5 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. |
DescriptionAdd --auto-update flag which uploads committed CL and initiates the CQ
The --auto-update flag is meant to provide the functionality of uploading a CL and initiating CQ to a bot for the automatic importing of w3c tests.
BUG=621599, 625252
Patch Set 1 #Patch Set 2 : finished auto_update flag functionality #
Total comments: 10
Patch Set 3 : made corrections #Patch Set 4 : dummy patch #Patch Set 5 : added unittest #
Total comments: 4
Patch Set 6 : style changes #
Total comments: 8
Patch Set 7 : corrections #
Total comments: 4
Patch Set 8 : logic corrections #
Messages
Total messages: 40 (17 generated)
The CQ bit was checked by raikiri@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by raikiri@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
raikiri@google.com changed reviewers: + dpranke@chromium.org, qyearsley@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Added --auto-import flag which uploads commited cl and initiates the CQ The --auto-import flag is meant to provide the functionality of uploading a cl and initiating CQ to a bot for the automatic importing of w3c tests BUG=621599 ========== to ========== Add --auto-import flag which uploads committed CL and initiates the CQ The --auto-import flag is meant to provide the functionality of uploading a CL and initiating CQ to a bot for the automatic importing of w3c tests. BUG=621599 ==========
Could you add a test for parse_try_job_results? We could also consider adding a test method for running the script with the added flag, although this would require replacing the real time.sleep and self.run, and I think that such a test method may not be worth it. https://codereview.chromium.org/2107993002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py (left): https://codereview.chromium.org/2107993002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:11: # Import destination directories (under LayoutTests/imported/). Why is this deleted? https://codereview.chromium.org/2107993002/diff/20001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py (right): https://codereview.chromium.org/2107993002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:70: del ret_code If a function returns a pair, but you don't care about one of the items in the pair, you can show this by using underscore as the name: _, out = self.run(['git', 'cl', 'try-results']) https://codereview.chromium.org/2107993002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:73: time.sleep(300) In general whenever you see a literal number (other than 0 or 1) in code, it's a "magic number" (http://c2.com/cgi/wiki?MagicNumber), and you could consider making it a global constant and give it a name. In this case, it doesn't make a particularly big difference though. https://codereview.chromium.org/2107993002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:76: sys.exit(2) Does exit code 2 have any particular significance? Also, it looks like the convention we're using here is that the return value of this function is the exit code, so we could also `return 2` (or `return 1`). https://codereview.chromium.org/2107993002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:78: return 0 Some cases to think about: - If there are no started jobs and no failed jobs (no jobs yet at all), then this may exit immediately. What if git cl try-results doesn't register the existence of try jobs until a second after `git cl set-commit` or `git cl try` is run? - If the format of `git cl try-results` changes (or if there's a bug) so that there is always a "Started" section, then there will be an infinite loop. - If there are no started jobs but also no failures (e.g. the output format of the command changed), then this will always exit and return 0, even if Rietveld failed to commit. It might be a good idea to use a time-out? It might also be reasonable to abort if the format of git cl try-results isn't as expected, perhaps by raising an exception. https://codereview.chromium.org/2107993002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:91: parser.add_argument('--auto-update', action='store_true', The CL description says "--auto-import" and this says "--auto-update"; they should be made consistent. I think you could think of a flag name that's clearer and less ambiguous than either of these names. What does this flag mean? https://codereview.chromium.org/2107993002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:92: help='uploads cl and initiates commit queue.') cl -> CL https://codereview.chromium.org/2107993002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:244: def parse_try_job_results(self, results): You could consider adding a docstring for this function (see https://google.github.io/styleguide/pyguide.html?showone=Comments#Comments). https://codereview.chromium.org/2107993002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:249: temp = line[:len(line)] `temp` isn't a very meaningful name (http://c2.com/cgi/wiki?BadVariableNames). And isn't `line[:len(line)]` always equal to `line`? https://codereview.chromium.org/2107993002/diff/20001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:251: sets[temp].add(line.split()[1]) At the bottom of the output it also prints "Total: <n> tryjobs" When the current code, this might be added into the last bucket, but is not a try job.
Description was changed from ========== Add --auto-import flag which uploads committed CL and initiates the CQ The --auto-import flag is meant to provide the functionality of uploading a CL and initiating CQ to a bot for the automatic importing of w3c tests. BUG=621599 ========== to ========== Add --auto-update flag which uploads committed CL and initiates the CQ The --auto-update flag is meant to provide the functionality of uploading a CL and initiating CQ to a bot for the automatic importing of w3c tests. BUG=621599 ==========
The CQ bit was checked by raikiri@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
raikiri@google.com changed reviewers: - dpranke@chromium.org
qyearsley@chromium.org changed reviewers: + dcampb@google.com
Make sure you have looked all the comments from earlier too -- remember you can respond to specific inline comments from earlier patches. If you want you can respond with just "Done" or "Ack" to previous comments to note that it's now changed or a comment was read. https://codereview.chromium.org/2107993002/diff/80001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py (right): https://codereview.chromium.org/2107993002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:17: FIVE_MINUTES = 300 FIVE_MINUTES is a bad name because if you ever change the value, you have to change the name too. For example, imagine we later want to change it to 600 seconds but forget to change the name; then, further down in the code we'd have sleep(FIVE_MINUTES) but it would actually be sleeping for 10 minutes. Ideally we should use names that explain the purpose and don't have to be changed, and don't require a comment to explain them. What about POLL_DELAY_SECONDS or something similar? https://codereview.chromium.org/2107993002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:70: self.run(['git', 'cl', 'set-commit', '-d']) If you change '-d' to '--dry-run', then it may be clearer what it's doing. https://codereview.chromium.org/2107993002/diff/80001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater_unittest.py (right): https://codereview.chromium.org/2107993002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater_unittest.py:26: # SUCH DAMAGE. This copyright notice isn't required for new files; instead use: # Copyright 2016 The Chromium Authors. All rights reserved. # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. https://codereview.chromium.org/2107993002/diff/80001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater_unittest.py:69: 'Started': set(['chromeos_amd64-generic_chromium_compile_only_ng', 'chromeos_daisy_chromium_compile_only_ng'])}) It might be easier to read the test if the test example strings were shorter and simpler -- example strings in tests don't need to be really realistic and full; they should just be big enough to let the reader know roughly what the input and output should look. In this case I think you could make the lines shorter and use an example with only one or two lines in each bucket.
https://codereview.chromium.org/2107993002/diff/100001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py (right): https://codereview.chromium.org/2107993002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:16: # Used for time.sleep(). Nit: I think this comment is unnecessary now, since the constant name is now clearer. https://codereview.chromium.org/2107993002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:259: from failures The wording can be improved here. > "A dict mapping a specified set of results to the builders that obtained those results." It's a dict mapping result type (e.g. Success, Failure) to list of bots with that result type. > "...and any bots in bot failures and successes set are removed from failures" Any bots with both success and failure results are not included in failures. https://codereview.chromium.org/2107993002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:262: AttributeError: An unexpected result was found Nit: add a period here. https://codereview.chromium.org/2107993002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:266: line.strip() The strip method doesn't change the original string (strings are immutable in Python), so this line doesn't do anything. You probably want to do `line = line.strip()`. https://codereview.chromium.org/2107993002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:267: print line Remember to remove debugging print lines. https://codereview.chromium.org/2107993002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:269: result_type = line[:len(line) - 1] In Python index and slice notation, negative numbers are indexes from the end of the list counting backwards, so this can also be done with: if line[-1] == ':': result_type = line[:-1] ... ... https://codereview.chromium.org/2107993002/diff/100001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater_unittest.py (right): https://codereview.chromium.org/2107993002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater_unittest.py:26: """ This can be made narrower (and easier to read) if the bot names and parts of the URL are replaced with fake, shorter strings, e.g.: """Successes: linux_builder http://example.com/linux_builder/builds/222 mac_builder http://example.com/mac_builder/builds/222 win_builder http://example.com/win_builder/builds/222 Failures: linux_builder http://example.com/linux_builder/builds/111 mac_builder http://example.com/mac_builder/builds/111 win_builder http://example.com/win_builder/builds/111 Started: chromeos_generic http://example.com/chromeos_generic/builds/111 chromeos_daisy http://example.com/chromeos_daisy/builds/111 Total: 8 tryjobs """ https://codereview.chromium.org/2107993002/diff/100001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater_unittest.py:33: 'chromeos_daisy_chromium_compile_only_ng'])}) Formatting suggestion: This will be easier to read if it's flattened out a bit, e.g. self.assertEqual(updater.parse_try_job_results(output), { 'Successes': set([ 'mac_chromium_compile_dbg_ng', 'win_chromium_rel_ng', 'linux_chromium_asan_rel_ng', ]), 'Failures': set([ 'mac_chromium_10.10_rel_ng', 'linux_chromium_rel_ng' ]), 'Started': set([ 'chromeos_amd64-generic_chromium_compile_only_ng', 'chromeos_daisy_chromium_compile_only_ng', ]) })
LGTM, with a few more optional suggestions/comments. https://codereview.chromium.org/2107993002/diff/120001/third_party/WebKit/Too... File third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py (right): https://codereview.chromium.org/2107993002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:76: if results['Failures'] != set([]): Note, this would be the same as if you changed it to: if results['Started'] or results['Scheduled']: continue if results['Failures']: return 1 Both should behave the same. Do you prefer the current way comparing explicitly to empty set? https://codereview.chromium.org/2107993002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:79: self.run(['git', 'cl', 'land', '-f']) Possible other things to consider: if one of the commands fails at any point, we may also want to abort... But it will become a little more verbose if we check this every time, e.g. return_code, _ = self.run(['git', 'cl', 'upload', '-f']) if return_code: return return_code return_code, _ = self.run(['git', 'cl', 'set-commit', '--dry-run']) if return_code: return return_code if we want to check each time and abort, then we could add an alternate run function that raises an exception in the event of failure, e.g.: def check_run(self, command): return_code, out = self.run(command) if return_code: raise Exception('%s failed with exit code %d.' % ' '.join(command), return_code) return out But this is optional. https://codereview.chromium.org/2107993002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:255: It's a dict mapping result type (e.g. Success, Failure) No need to include "It's" -- this can just be a noun phrase, e.g. "A dict mapping x to y..." https://codereview.chromium.org/2107993002/diff/120001/third_party/WebKit/Too... third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py:260: Extra blank line can be removed.
The CQ bit was checked by raikiri@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from qyearsley@chromium.org Link to the patchset: https://codereview.chromium.org/2107993002/#ps140001 (title: "logic corrections")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by raikiri@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 #8 (id:140001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Add --auto-update flag which uploads committed CL and initiates the CQ The --auto-update flag is meant to provide the functionality of uploading a CL and initiating CQ to a bot for the automatic importing of w3c tests. BUG=621599 ========== to ========== Add --auto-update flag which uploads committed CL and initiates the CQ The --auto-update flag is meant to provide the functionality of uploading a CL and initiating CQ to a bot for the automatic importing of w3c tests. BUG=621599 Committed: https://crrev.com/83f3951c0bb76227abc80faeaae2b884c18d193a Cr-Commit-Position: refs/heads/master@{#404309} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/83f3951c0bb76227abc80faeaae2b884c18d193a Cr-Commit-Position: refs/heads/master@{#404309}
Message was sent while issue was closed.
Description was changed from ========== Add --auto-update flag which uploads committed CL and initiates the CQ The --auto-update flag is meant to provide the functionality of uploading a CL and initiating CQ to a bot for the automatic importing of w3c tests. BUG=621599 Committed: https://crrev.com/83f3951c0bb76227abc80faeaae2b884c18d193a Cr-Commit-Position: refs/heads/master@{#404309} ========== to ========== Add --auto-update flag which uploads committed CL and initiates the CQ The --auto-update flag is meant to provide the functionality of uploading a CL and initiating CQ to a bot for the automatic importing of w3c tests. BUG=621599, 625252 ==========
lgtm
On 2016/07/11 at 16:51:12, dcampb wrote: > lgtm This is already committed; not sure why it's not in a "closed" state. Now closing. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
