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

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

Index: third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py
diff --git a/third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py
index d56f1682c31ce8048e74391adff42f7825775c3c..16301cddfb632689c2daf8cdfcc32fee3986deb6 100644
--- a/third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py
+++ b/third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py
@@ -5,10 +5,11 @@
 """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'
 
@@ -24,6 +25,7 @@ class DepsUpdater(object):
         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)
@@ -60,6 +62,19 @@ class DepsUpdater(object):
 
         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):
@@ -73,12 +88,15 @@ class DepsUpdater(object):
                             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)
@@ -222,3 +240,14 @@ class DepsUpdater(object):
 
     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