rebaseline-cl: Don't trigger new try jobs for builders that already have jobs started.

In the preliminary CL http://crrev.com/2406153003, I changed Rietveld.latest_try_jobs to Rietveld.latest_try_job_results and made it return a dict of Builds (named tuple objects with builder_name and build_number) to result info dicts.

The purpose of this was to make it so that rebaseline-cl could look through the result info dicts to see whether each build is completed or just started, to avoid starting extra builds.

After looking at it some more, I see that in the Rietveld response, builds that are just started but not completed have no build number information.

To avoid having to pass around the result dict information from Rietveld, it's possible to just have a list of Build objects with build_number set to None when the build is just pending or started but not completed.

So, in summary, this CL makes it so that when one runs rebaseline-cl twice in a row, it now prints out:

$ ./webkit-patch rebaseline-cl
 Triggering try jobs for:
   android_blink_rel
   linux_precise_blink_rel
   linux_trusty_blink_rel
   mac10.10_blink_rel
   mac10.11_blink_rel
   mac10.11_retina_blink_rel
   mac10.9_blink_rel
   win10_blink_rel
   win7_blink_rel
 Please re-run webkit-patch rebaseline-cl once all pending try jobs have finished.
 $ ./webkit-patch rebaseline-cl
 There are existing pending builds for:
   android_blink_rel
   linux_precise_blink_rel
   linux_trusty_blink_rel
   mac10.10_blink_rel
   mac10.11_blink_rel
   mac10.11_retina_blink_rel
   mac10.9_blink_rel
   win10_blink_rel
   win7_blink_rel
 Please re-run webkit-patch rebaseline-cl once all pending try jobs have finished.

And this CL undoes some of the changes made in http://crrev.com/2406153003 in order to simplify the code, because now I think passing full result info dicts is unnecessary.

BUG=654919

Review-Url: https://chromiumcodereview.appspot.com/2439693003
Cr-Commit-Position: refs/heads/master@{#426837}

third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py

@@ -20,7 +20,7 @@ class Rietveld(object):
     def __init__(self, web):
         self.web = web
 
-    def latest_try_job_results(self, issue_number, builder_names=None, patchset_number=None):
+    def latest_try_jobs(self, issue_number, builder_names=None, patchset_number=None):
         """Returns a list of Build objects for builds on the latest patchset.
 
         Args:
@@ -30,8 +30,8 @@ class Rietveld(object):
             patchset_number: If given, a specific patchset will be used instead of the latest one.
 
         Returns:
-            A dict mapping Build objects to result dicts for the latest build
-            for each builder on the latest patchset.
+            A list of Build objects, where Build objects for completed jobs have a build number,
+            and Build objects for pending jobs have no build number.
         """
         try:
             if patchset_number:
@@ -40,18 +40,22 @@ class Rietveld(object):
                 url = self._latest_patchset_url(issue_number)
             patchset_data = self._get_json(url)
         except (urllib2.URLError, ValueError):
-            return {}
-
-        def build(job):
-            return Build(builder_name=job['builder'], build_number=job['buildnumber'])
-
-        results = {build(job): job for job in patchset_data['try_job_results']}
+            return []
+
+        builds = []
+        for result_dict in patchset_data['try_job_results']:
+            build = Build(result_dict['builder'], result_dict['buildnumber'])
+            # Normally, a value of -1 or 6 in the "result" field indicates the job is
+            # started or pending, and the "buildnumber" field is null.
+            if build.build_number and result_dict['result'] in (-1, 6):
+                _log.warning('Build %s has result %d, but unexpectedly has a build number.', build, result_dict['result'])
+                build.build_number = None
+            builds.append(build)
 
         if builder_names is not None:
-            results = {b: result for b, result in results.iteritems() if b.builder_name in builder_names}
+            builds = [b for b in builds if b.builder_name in builder_names]
 
-        latest_builds = self._filter_latest_builds(list(results))
-        return {b: result for b, result in results.iteritems() if b in latest_builds}
+        return self._filter_latest_builds(builds)
 
     def _filter_latest_builds(self, builds):
         """Filters out a collection of Build objects to include only the latest for each builder.
@@ -60,19 +64,17 @@ class Rietveld(object):
             jobs: A list of Build objects.
 
         Returns:
-            A list of Build objects that contains only the latest build for each builder.
+            A list of Build objects; only one Build object per builder name. If there are only
+            Builds with no build number, then one is kept; if there are Builds with build numbers,
+            then the one with the highest build number is kept.
         """
-        builder_to_highest_number = {}
+        builder_to_latest_build = {}
         for build in builds:
-            if build.build_number > builder_to_highest_number.get(build.builder_name, 0):
-                builder_to_highest_number[build.builder_name] = build.build_number
-
-        def is_latest_build(build):
-            if build.builder_name not in builder_to_highest_number:
-                return False
-            return builder_to_highest_number[build.builder_name] == build.build_number
-
-        return [b for b in builds if is_latest_build(b)]
+            if build.builder_name not in builder_to_latest_build:
+                builder_to_latest_build[build.builder_name] = build
+            elif build.build_number > builder_to_latest_build[build.builder_name].build_number:
+                builder_to_latest_build[build.builder_name] = build
+        return sorted(builder_to_latest_build.values())
 
     def changed_files(self, issue_number):
         """Lists the files included in a CL, or None if this can't be determined.

third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld_unittest.py

@@ -22,7 +22,7 @@ class RietveldTest(LoggingTestCase):
                 'try_job_results': [
                     {
                         'builder': 'foo-builder',
-                        'buildnumber': 10,
+                        'buildnumber': None,
                         'result': -1
                     },
                     {
@@ -56,8 +56,8 @@ class RietveldTest(LoggingTestCase):
     def test_latest_try_jobs(self):
         rietveld = Rietveld(self.mock_web())
         self.assertEqual(
-            rietveld.latest_try_job_results(11112222, ('bar-builder', 'other-builder')),
-            {Build('bar-builder', 60): {'builder': 'bar-builder', 'buildnumber': 60, 'result': 0}})
+            rietveld.latest_try_jobs(11112222, ('bar-builder', 'other-builder')),
+            [Build('bar-builder', 60)])
 
     def test_latest_try_jobs_http_error(self):
         def raise_error(_):
@@ -65,23 +65,23 @@ class RietveldTest(LoggingTestCase):
         web = self.mock_web()
         web.get_binary = raise_error
         rietveld = Rietveld(web)
-        self.assertEqual(rietveld.latest_try_job_results(11112222, ('bar-builder',)), {})
+        self.assertEqual(rietveld.latest_try_jobs(11112222, ('bar-builder',)), [])
         self.assertLog(['ERROR: Request failed to URL: https://codereview.chromium.org/api/11112222\n'])
 
     def test_latest_try_jobs_non_json_response(self):
         rietveld = Rietveld(self.mock_web())
-        self.assertEqual(rietveld.latest_try_job_results(11113333, ('bar-builder',)), {})
+        self.assertEqual(rietveld.latest_try_jobs(11113333, ('bar-builder',)), [])
         self.assertLog(['ERROR: Invalid JSON: my non-JSON contents\n'])
 
     def test_latest_try_jobs_with_patchset(self):
         rietveld = Rietveld(self.mock_web())
         self.assertEqual(
-            rietveld.latest_try_job_results(11112222, ('bar-builder', 'other-builder'), patchset_number=2),
-            {Build('bar-builder', 50): {'builder': 'bar-builder', 'buildnumber': 50, 'result': 0}})
+            rietveld.latest_try_jobs(11112222, ('bar-builder', 'other-builder'), patchset_number=2),
+            [Build('bar-builder', 50)])
 
     def test_latest_try_jobs_no_relevant_builders(self):
         rietveld = Rietveld(self.mock_web())
-        self.assertEqual(rietveld.latest_try_job_results(11112222, ('foo', 'bar')), {})
+        self.assertEqual(rietveld.latest_try_jobs(11112222, ('foo', 'bar')), [])
 
     def test_changed_files(self):
         rietveld = Rietveld(self.mock_web())
@@ -103,7 +103,7 @@ class RietveldTest(LoggingTestCase):
         rietveld = Rietveld(self.mock_web())
         self.assertEqual(
             rietveld._filter_latest_builds([Build('foo', 5), Build('foo', 3), Build('bar', 5)]),
-            [Build('foo', 5), Build('bar', 5)])
+            [Build('bar', 5), Build('foo', 5)])
 
     def test_filter_latest_jobs_higher_build_last(self):
         rietveld = Rietveld(self.mock_web())
@@ -114,5 +114,5 @@ class RietveldTest(LoggingTestCase):
     def test_filter_latest_jobs_no_build_number(self):
         rietveld = Rietveld(self.mock_web())
         self.assertEqual(
-            rietveld._filter_latest_builds([Build('foo', 3), Build('bar')]),
-            [Build('foo', 3)])
+            rietveld._filter_latest_builds([Build('foo', 3), Build('bar'), Build('bar')]),
+            [Build('bar'), Build('foo', 3)])

third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py

@@ -56,15 +56,12 @@ class RebaselineCL(AbstractParallelRebaselineCommand):
         if not issue_number:
             return
 
-        builds = self.rietveld.latest_try_job_results(issue_number, self._try_bots())
+        builds = self.rietveld.latest_try_jobs(issue_number, self._try_bots())
         if options.trigger_jobs:
             if self.trigger_jobs_for_missing_builds(builds):
                 _log.info('Please re-run webkit-patch rebaseline-cl once all pending try jobs have finished.')
                 return
         if not builds:
-            # TODO(qyearsley): Also check that there are *finished* builds.
-            # The current behavior would still proceed if there are queued
-            # or started builds.
             _log.info('No builds to download baselines from.')
 
         if args:
@@ -115,23 +112,36 @@ class RebaselineCL(AbstractParallelRebaselineCommand):
         return GitCL(self._tool)
 
     def trigger_jobs_for_missing_builds(self, builds):
-        """Returns True if jobs were triggered; False otherwise."""
-        builders_with_builds = {b.builder_name for b in builds}
-        builders_without_builds = set(self._try_bots()) - builders_with_builds
-        if not builders_without_builds:
-            return False
+        """Triggers try jobs for any builders that have no builds started.
 
-        _log.info('Triggering try jobs for:')
-        for builder in sorted(builders_without_builds):
-            _log.info('  %s', builder)
+        Args:
+          builds: A list of Build objects; if the build number of a Build is None,
+              then that indicates that the job is pending.
 
-        # If the builders may be under different masters, then they cannot
-        # all be started in one invocation of git cl try without providing
-        # master names. Doing separate invocations is slower, but always works
-        # even when there are builders under different master names.
-        for builder in sorted(builders_without_builds):
-            self.git_cl().run(['try', '-b', builder])
-        return True
+        Returns:
+            True if there are pending jobs to wait for, including jobs just started.
+        """
+        builders_with_builds = {b.builder_name for b in builds}
+        builders_without_builds = set(self._try_bots()) - builders_with_builds
+        builders_with_pending_builds = {b.builder_name for b in builds if b.build_number is None}
+
+        if builders_with_pending_builds:
+            _log.info('There are existing pending builds for:')
+            for builder in sorted(builders_with_pending_builds):
+                _log.info('  %s', builder)
+
+        if builders_without_builds:
+            _log.info('Triggering try jobs for:')
+            for builder in sorted(builders_without_builds):
+                _log.info('  %s', builder)
+            # If the builders may be under different masters, then they cannot
+            # all be started in one invocation of git cl try without providing
+            # master names. Doing separate invocations is slower, but always works
+            # even when there are builders under different master names.
+            for builder in sorted(builders_without_builds):
+                self.git_cl().run(['try', '-b', builder])
+
+        return bool(builders_with_pending_builds or builders_without_builds)
 
     def _test_prefix_list(self, issue_number, only_changed_tests):
         """Returns a collection of test, builder and file extensions to get new baselines for.
@@ -163,7 +173,7 @@ class RebaselineCL(AbstractParallelRebaselineCommand):
     def _builds_to_tests(self, issue_number):
         """Fetches a list of try bots, and for each, fetches tests with new baselines."""
         _log.debug('Getting results for Rietveld issue %d.', issue_number)
-        builds = self.rietveld.latest_try_job_results(issue_number, self._try_bots())
+        builds = self.rietveld.latest_try_jobs(issue_number, self._try_bots())
         if not builds:
             _log.debug('No try job results for builders in: %r.', self._try_bots())
         return {build: self._tests_to_rebaseline(build) for build in builds}

third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl_unittest.py

@@ -31,10 +31,12 @@ class RebaselineCLTest(BaseTestCase, LoggingTestCase):
                     {
                         'builder': 'MOCK Try Win',
                         'buildnumber': 5000,
+                        'result': 0,
                     },
                     {
-                        'builder': 'MOCK Mac Try',
+                        'builder': 'MOCK Try Mac',
                         'buildnumber': 4000,
+                        'result': 0,
                     },
                 ],
                 'files': {
@@ -107,7 +109,7 @@ class RebaselineCLTest(BaseTestCase, LoggingTestCase):
                         'option to download baselines for another existing CL.\n'])
 
     def test_execute_with_issue_number_from_branch(self):
-        git_cl = GitCL(MockExecutive2())
+        git_cl = GitCL(self.tool)
         git_cl.get_issue_number = lambda: '11112222'
         self.command.git_cl = lambda: git_cl
         self.command.execute(self.command_options(), [], self.tool)
@@ -176,3 +178,39 @@ class RebaselineCLTest(BaseTestCase, LoggingTestCase):
                   '--builder', 'MOCK Try Win', '--test', 'fast/dom/prototype-taco.html', '--build-number', '5000']],
                 [['python', 'echo', 'optimize-baselines', '--suffixes', 'txt', 'fast/dom/prototype-taco.html']]
             ])
+
+    def test_trigger_jobs_for_missing_builds_empty_list(self):
+        # Passing in no builds implies that no try jobs were started.
+        self.assertTrue(self.command.trigger_jobs_for_missing_builds([]))
+        self.assertEqual(
+            self.tool.executive.calls,
+            [['git', 'cl', 'try', '-b', 'MOCK Try Linux'], ['git', 'cl', 'try', '-b', 'MOCK Try Win']])
+        self.assertLog([
+            'INFO: Triggering try jobs for:\n',
+            'INFO:   MOCK Try Linux\n',
+            'INFO:   MOCK Try Win\n',
+        ])
+
+    def test_trigger_jobs_for_missing_builds_started_and_successful(self):
+        # A build number of None implies that a job has been started but not finished yet.
+        self.assertTrue(self.command.trigger_jobs_for_missing_builds([
+            Build('MOCK Try Linux', None),
+            Build('MOCK Try Win', 123),
+        ]))
+        self.assertEqual(self.tool.executive.calls, [])
+        self.assertLog([
+            'INFO: There are existing pending builds for:\n',
+            'INFO:   MOCK Try Linux\n',
+        ])
+
+    def test_trigger_jobs_for_missing_builds_one_started(self):
+        self.assertTrue(self.command.trigger_jobs_for_missing_builds([
+            Build('MOCK Try Linux', None),
+        ]))
+        self.assertEqual(self.tool.executive.calls, [['git', 'cl', 'try', '-b', 'MOCK Try Win']])
+        self.assertLog([
+            'INFO: There are existing pending builds for:\n',
+            'INFO:   MOCK Try Linux\n',
+            'INFO: Triggering try jobs for:\n',
+            'INFO:   MOCK Try Win\n',
+        ])

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

@@ -44,7 +44,7 @@ class W3CExpectationsLineAdder(object):
             return 1
 
         rietveld = Rietveld(self.host.web)
-        builds = rietveld.latest_try_job_results(issue_number, self.get_try_bots())
+        builds = rietveld.latest_try_jobs(issue_number, self.get_try_bots())
         _log.debug('Latest try jobs: %r', builds)
 
         if not builds: