rebaseline_cl.py: Detect step names

This CL makes rebaseline_cl.py detect the step name for a build
programmatically when retrieving results. Layout test results will
soon be always uploaded to a subdirectory of their step name, so
the step name needs to be known.

Bug: 882946
Change-Id: Iebf03c968f559c49872f97b25fa7e12f16ec5ad2
Reviewed-on: https://chromium-review.googlesource.com/c/1262425
Commit-Queue: Stephen Martinis <martiniss@chromium.org>
Reviewed-by: Robert Ma <robertma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597301}

third_party/blink/tools/blinkpy/common/net/buildbot.py

@@ -28,16 +28,20 @@
 
 import collections
 import logging
+import json
 import re
+import urllib
 import urllib2
 
 from blinkpy.common.memoized import memoized
 from blinkpy.common.net.layout_test_results import LayoutTestResults
 from blinkpy.common.net.network_transaction import NetworkTransaction
+from blinkpy.web_tests.layout_package import json_results_generator
 
 _log = logging.getLogger(__name__)
 
-RESULTS_URL_BASE = 'https://test-results.appspot.com/data/layout_results'
+TEST_RESULTS_SERVER = 'https://test-results.appspot.com'
+RESULTS_URL_BASE = '%s/data/layout_results' % TEST_RESULTS_SERVER
 
 
 class Build(collections.namedtuple('Build', ('builder_name', 'build_number'))):
@@ -58,7 +62,7 @@ class BuildBot(object):
         https://www.chromium.org/developers/the-json-test-results-format
     """
 
-    def results_url(self, builder_name, build_number=None):
+    def results_url(self, builder_name, build_number=None, step_name=None):
         """Returns a URL for one set of archived layout test results.
 
         If a build number is given, this will be results for a particular run;
@@ -68,6 +72,8 @@ class BuildBot(object):
         if build_number:
             assert str(build_number).isdigit(), 'expected numeric build number, got %s' % build_number
             url_base = self.builder_results_url_base(builder_name)
+            if step_name:
+                return '%s/%s/%s/layout-test-results' % (url_base, build_number, step_name)
             return '%s/%s/layout-test-results' % (url_base, build_number)
         return self.accumulated_results_url_base(builder_name)
 
@@ -102,7 +108,46 @@ class BuildBot(object):
         Uses full_results.json if full is True, otherwise failing_results.json.
         """
         return self.fetch_layout_test_results(
-            self.results_url(build.builder_name, build.build_number), full)
+            self.results_url(build.builder_name, build.build_number,
+                             step_name=self.get_layout_test_step_name(build)),
+            full)
+
+    @memoized
+    def get_layout_test_step_name(self, build):
+        url = '%s/testfile?%s' % (TEST_RESULTS_SERVER, urllib.urlencode({
+            'builder': build.builder_name,
+            'buildnumber': build.build_number,
+            'name': 'full_results.json',
+            # This forces the server to gives us JSON rather than an HTML page.
+            'callback': 'ADD_RESULTS',
+        }))
+        data = NetworkTransaction(return_none_on_404=True).run(
+            lambda: self.fetch_file(url))
+        if not data:
+            _log.debug('Got 404 response from:\n%s', url)
+            return None
+
+        # Strip out the callback
+        data = json.loads(json_results_generator.strip_json_wrapper(data))
+        suites = [
+            entry['TestType'] for entry in data
+            # Some suite names are like 'webkit_layout_tests on Intel GPU (with
+            # patch)'. Only make sure it starts with webkit_layout_tests and
+            # runs with a patch. This should be changed eventually to use actual
+            # structured data from the test results server.
+            if (entry['TestType'].startswith('webkit_layout_tests') and
+                entry['TestType'].endswith('(with patch)'))
+        ]
+        # In manual testing, I sometimes saw results where the same suite was
+        # repeated twice. De-duplicate here to try to catch this.
+        suites = list(set(suites))
+        if len(suites) != 1:
+            raise Exception(
+                'build %s on builder %s expected to only have one layout test '
+                'step, instead has %s' % (
+                    build.build_number, build.builder_name, suites))
+
+        return suites[0]
 
     @memoized
     def fetch_layout_test_results(self, results_url, full=False):
@@ -111,20 +156,20 @@ class BuildBot(object):
         """
         base_filename = 'full_results.json' if full else 'failing_results.json'
         results_file = NetworkTransaction(return_none_on_404=True).run(
-            lambda: self.fetch_file(results_url, base_filename))
+            lambda: self.fetch_file('%s/%s' % (results_url, base_filename)))
         if results_file is None:
             _log.debug('Got 404 response from:\n%s/%s', results_url, base_filename)
             return None
         revision = NetworkTransaction(return_none_on_404=True).run(
-            lambda: self.fetch_file(results_url, 'LAST_CHANGE'))
+            lambda: self.fetch_file('%s/LAST_CHANGE' % results_url))
         if revision is None:
             _log.debug('Got 404 response from:\n%s/LAST_CHANGE', results_url)
         return LayoutTestResults.results_from_string(results_file, revision)
 
-    def fetch_file(self, url_base, filename):
+    def fetch_file(self, url):
         # It seems this can return None if the url redirects and then returns 404.
         # FIXME: This could use Web instead of using urllib2 directly.
-        result = urllib2.urlopen('%s/%s' % (url_base, filename))
+        result = urllib2.urlopen(url)
         if not result:
             return None
         # urlopen returns a file-like object which sometimes works fine with str()

third_party/blink/tools/blinkpy/common/net/buildbot_unittest.py

@@ -26,6 +26,7 @@
 # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
+import json
 import logging
 import unittest
 
@@ -65,8 +66,8 @@ class BuilderTest(LoggingTestCase):
     def test_fetch_layout_test_results_with_no_results_fetched(self):
         buildbot = BuildBot()
 
-        def fetch_file(_, filename):
-            return None if filename == 'failing_results.json' else 'contents'
+        def fetch_file(url):
+            return None if url.endswith('failing_results.json') else 'contents'
 
         buildbot.fetch_file = fetch_file
         results = buildbot.fetch_layout_test_results(buildbot.results_url('B'))
@@ -76,6 +77,38 @@ class BuilderTest(LoggingTestCase):
             'https://test-results.appspot.com/data/layout_results/B/results/layout-test-results/failing_results.json\n'
         ])
 
+    def test_fetch_layout_test_results_weird_step_name(self):
+        buildbot = BuildBot()
+
+        def fetch_file(url):
+            if '/testfile' in url:
+                return ('ADD_RESULTS(%s);' % (json.dumps(
+                    [{"TestType": "webkit_layout_tests on Intel GPU (with patch)"},
+                     {"TestType": "base_unittests (with patch)"}])))
+            return json.dumps({'passed': True}) if url.endswith('failing_results.json') else 'deadbeef'
+
+        buildbot.fetch_file = fetch_file
+        results = buildbot.fetch_results(Build('builder', 123))
+        self.assertEqual(results._results, {  # pylint: disable=protected-access
+            'passed': True
+        })
+        self.assertLog([])
+
+    def test_get_step_name(self):
+        buildbot = BuildBot()
+
+        def fetch_file(_):
+            return ('ADD_RESULTS(%s);' % (json.dumps(
+                [{"TestType": "webkit_layout_tests (with patch)"},
+                 {"TestType": "site_per_process_webkit_layout_tests (with patch)"},
+                 {"TestType": "webkit_layout_tests (retry with patch)"},
+                 {"TestType": "base_unittests (with patch)"}])))
+
+        buildbot.fetch_file = fetch_file
+        step_name = buildbot.get_layout_test_step_name(Build('foo', 5))
+        self.assertEqual(step_name, 'webkit_layout_tests (with patch)')
+        self.assertLog([])
+
 
 class BuildBotHelperFunctionTest(unittest.TestCase):
 

third_party/blink/tools/blinkpy/tool/commands/rebaseline_test.py

@@ -4,6 +4,7 @@
 
 import json
 import logging
+import optparse
 
 from blinkpy.tool.commands.rebaseline import AbstractRebaseliningCommand
 
@@ -22,6 +23,10 @@ class RebaselineTest(AbstractRebaseliningCommand):
             self.builder_option,
             self.build_number_option,
             self.results_directory_option,
+            optparse.make_option(
+                '--step-name',
+                help=('Name of the step which ran the actual tests, and which '
+                      'should be used to retrieve results from.'))
         ])
 
     def execute(self, options, args, tool):
@@ -44,7 +49,9 @@ class RebaselineTest(AbstractRebaseliningCommand):
         if options.results_directory:
             results_url = 'file://' + options.results_directory
         else:
-            results_url = self._tool.buildbot.results_url(options.builder, build_number=options.build_number)
+            results_url = self._tool.buildbot.results_url(
+                options.builder, build_number=options.build_number,
+                step_name=options.step_name)
 
         for suffix in self._baseline_suffix_list:
             self._rebaseline_test(port_name, options.test, suffix, results_url)

third_party/blink/tools/blinkpy/tool/commands/rebaseline_test_unittest.py

@@ -21,7 +21,8 @@ class TestRebaselineTest(BaseTestCase):
             'test': 'userscripts/another-test.html',
             'suffixes': 'txt',
             'results_directory': None,
-            'build_number': None
+            'build_number': None,
+            'step_name': None,
         }, **kwargs))
 
     def test_rebaseline_test_internal_with_port_that_lacks_buildbot(self):
@@ -44,7 +45,8 @@ class TestRebaselineTest(BaseTestCase):
                 'verbose': True,
                 'test': 'failures/expected/image.html',
                 'results_directory': None,
-                'build_number': None
+                'build_number': None,
+                'step_name': None,
             })
             oc.capture_output()
             self.command.execute(options, [], self.tool)