Add --auto-update flag which uploads committed CL and initiates the CQ

third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py

 # Copyright 2014 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.
 
 """Pull latest revisions of a W3C test repo and make a local commit."""
 
 import argparse
+import time
+import sys
 
 from webkitpy.common.webkit_finder import WebKitFinder
 
-# Import destination directories (under LayoutTests/imported/).

[qyearsley, 2016/07/01 17:40:40]

Why is this deleted?

 WPT_DEST_NAME = 'wpt'
 CSS_DEST_NAME = 'csswg-test'
 
 
 class DepsUpdater(object):
 
     def __init__(self, host):
         self.host = host
         self.executive = host.executive
         self.fs = host.filesystem
         self.finder = WebKitFinder(self.fs)
         self.verbose = False
         self.allow_local_commits = False
         self.keep_w3c_repos_around = False
         self.target = None
+        self.auto_update = False
 
     def main(self, argv=None):
         self.parse_args(argv)
 
         if not self.checkout_is_okay():
             return 1
 
         self.print_('## Noting the current Chromium commit.')
         _, show_ref_output = self.run(['git', 'show-ref', 'HEAD'])
         chromium_commitish = show_ref_output.split()[0]
@@ skipping 16 matching lines @@
 
         elif self.target == 'css':
             import_commitish = self.update(
                 CSS_DEST_NAME,
                 'https://chromium.googlesource.com/external/w3c/csswg-test.git')
         else:
             raise AssertionError("Unsupported target %s" % self.target)
 
         self.commit_changes_if_needed(chromium_commitish, import_commitish)
 
+        if self.auto_update:
+            self.run(['git', 'cl', 'upload', '-f'])
+            self.run(['git', 'cl', 'set-commit'])
+            while True:
+                ret_code, out = self.run(['git', 'cl', 'try-results'])
+                del ret_code

[qyearsley, 2016/07/01 17:40:39]

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'])

+                results = self.parse_try_job_results(out)
+                if results['Started'] != set([]):
+                    time.sleep(300)

[qyearsley, 2016/07/01 17:40:39]

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.

+                    continue
+                if results['Failures'] != set([]):
+                    sys.exit(2)

[qyearsley, 2016/07/01 17:40:40]

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`).

+                break
         return 0

[qyearsley, 2016/07/01 17:40:40]

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.

 
     def parse_args(self, argv):
         parser = argparse.ArgumentParser()
         parser.description = __doc__
         parser.add_argument('-v', '--verbose', action='store_true',
                             help='log what we are doing')
         parser.add_argument('--allow-local-commits', action='store_true',
                             help='allow script to run even if we have local commits')
         parser.add_argument('--keep-w3c-repos-around', action='store_true',
                             help='leave the w3c repos around that were imported previously.')
         parser.add_argument('target', choices=['css', 'wpt'],
                             help='Target repository.  "css" for csswg-test, "wpt" for web-platform-tests.')
+        parser.add_argument('--auto-update', action='store_true',

[qyearsley, 2016/07/01 17:40:40]

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?

+                            help='uploads cl and initiates commit queue.')

[qyearsley, 2016/07/01 17:40:40]

cl -> CL

 
         args = parser.parse_args(argv)
         self.allow_local_commits = args.allow_local_commits
         self.keep_w3c_repos_around = args.keep_w3c_repos_around
         self.verbose = args.verbose
         self.target = args.target
+        self.auto_update = args.auto_update
 
     def checkout_is_okay(self):
         git_diff_retcode, _ = self.run(['git', 'diff', '--quiet', 'HEAD'], exit_on_failure=False)
         if git_diff_retcode:
             self.print_('## Checkout is dirty; aborting.')
             return False
 
         local_commits = self.run(['git', 'log', '--oneline', 'origin/master..HEAD'])[1]
         if local_commits and not self.allow_local_commits:
             self.print_('## Checkout has local commits; aborting. Use --allow-local-commits to allow this.')
@@ skipping 123 matching lines @@
         dest = self.path_from_webkit_base(*comps)
         if self.verbose:
             self.print_('rm -fr %s' % dest)
         self.fs.rmtree(dest)
 
     def path_from_webkit_base(self, *comps):
         return self.finder.path_from_webkit_base(*comps)
 
     def print_(self, msg):
         self.host.print_(msg)
+
+    def parse_try_job_results(self, results):

[qyearsley, 2016/07/01 17:40:40]

You could consider adding a docstring for this function (see
https://google.github.io/styleguide/pyguide.html?showone=Comments#Comments).

+        fields = ['Successes:', 'Started:', 'Scheduled:', 'Failures:']
+        sets = {'Successes': set(), 'Started': set(), 'Scheduled': set(), 'Failures': set()}
+        for line in results.splitlines():
+            if line in fields:
+                temp = line[:len(line)]

[qyearsley, 2016/07/01 17:40:40]

`temp` isn't a very meaningful name (http://c2.com/cgi/wiki?BadVariableNames).

And isn't `line[:len(line)]` always equal to `line`?

+            else:
+                sets[temp].add(line.split()[1])

[qyearsley, 2016/07/01 17:40:40]

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.

+        sets['Failures'] -= sets['Successes']
+        return sets