Refactor the buildbot module in webkitpy.

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

Index: third_party/WebKit/Tools/Scripts/webkitpy/common/net/buildbot.py
diff --git a/third_party/WebKit/Tools/Scripts/webkitpy/common/net/buildbot.py b/third_party/WebKit/Tools/Scripts/webkitpy/common/net/buildbot.py
index 77517e602904bed3a2d6c110e65ec912961b57c4..3896db7fc4ad2aee4930710eb5bcbe3af7001f5d 100644
--- a/third_party/WebKit/Tools/Scripts/webkitpy/common/net/buildbot.py
+++ b/third_party/WebKit/Tools/Scripts/webkitpy/common/net/buildbot.py
@@ -26,48 +26,49 @@
 # (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 re
 import urllib2
 
-import webkitpy.common.config.urls as config_urls
 from webkitpy.common.memoized import memoized
 from webkitpy.common.net.layouttestresults import LayoutTestResults
 from webkitpy.common.net.networktransaction import NetworkTransaction
 from webkitpy.common.system.logutils import get_logger
 
 
+RESULTS_URL_BASE = 'https://storage.googleapis.com/chromium-layout-test-archives'
+
 _log = get_logger(__file__)
 
 
-class Builder(object):
+class BuildBot(object):
+    """This class represents an interface to BuildBot-related functionality.
 
-    def __init__(self, builder_name, buildbot):
-        self._name = builder_name
-        self._buildbot = buildbot
+    This includes fetching layout test results from Google Storage.
+    """

[dcampb, 2016/07/15 17:33:26]

Whats the benefit of not having a __init__ in this class and passing all the
arguments as parameters for each function? 

[qyearsley, 2016/07/15 17:49:59]

Good question -- __init__ methods aren't necessary if there's nothing to set up
on instance attributes.

As it is, instances of this class have no attributes; they only serve as an
interface to the functionality provided by the methods.

How this class is used currently is that WebKitPatch has an instead of BuildBot,
which it uses to access buildbot-related functionality (downloading results for
specific builders and builds), and it uses the same BuildBot instance to
download results for different builders.

So, each time a method here is called it's called with a different argument, so
there's no benefit to having an __init__ method that takes that argument and
stores it as an attribute.

 
-    def name(self):
-        return self._name
+    def results_url(self, builder_name, build_number=None):
+        """Returns a URL for one set of archived layout test results.
 
-    def results_url(self):
-        return config_urls.chromium_results_url_base_for_builder(self._name)
+        If a build number is given, this will be results for a particular run;
+        otherwise it will be the accumulated results URL, which should have
+        the latest results.
+        """
+        if build_number:
+            url_base = self.builder_results_url_base(builder_name)
+            return "%s/%s/layout-test-results" % (url_base, build_number)
+        return self.accumulated_results_url_base(builder_name)
 
-    def latest_layout_test_results_url(self):
-        return config_urls.chromium_accumulated_results_url_base_for_builder(self._name)
+    def builder_results_url_base(self, builder_name):
+        return '%s/%s' % (RESULTS_URL_BASE, re.sub('[ .()]', '_', builder_name))

[wkorman, 2016/07/19 22:46:27]

What is this re sub doing and why? Perhaps add a comment.

[qyearsley, 2016/07/19 23:17:26]

I think that replacing parentheses, dots and spaces with underscores in builder
names for Google Storage directory names was originally done to make the
directory names more convenient and URL-friendly; now it has to be done to
maintain compatibility with existing directory names.

Note, this function was copied over from config/urls.py.

Added a docstring.

 
     @memoized
-    def latest_layout_test_results(self):
-        return self.fetch_layout_test_results(self.latest_layout_test_results_url())
-
-    def _fetch_file_from_results(self, results_url, file_name):
-        # It seems this can return None if the url redirects and then returns 404.
-        result = urllib2.urlopen("%s/%s" % (results_url, file_name))
-        if not result:
-            return None
-        # urlopen returns a file-like object which sometimes works fine with str()
-        # but sometimes is a addinfourl object.  In either case calling read() is correct.
-        return result.read()
+    def accumulated_results_url_base(self, builder_name):
+        return self.builder_results_url_base(builder_name) + "/results/layout-test-results"

[wkorman, 2016/07/19 22:46:27]

Is there a doc we can link to in class doc comment that gives an overview of the
layout test results json data format?

When I work on these scripts I always just have to reverse-engineer it from code
and sample data myself, figured worth asking in case you've seen something I
haven't and can help future eng.

[qyearsley, 2016/07/19 23:17:26]

The official doc is supposed to be
https://www.chromium.org/developers/the-json-test-results-format, I think -- and
for layout tests the code that's responsible for the layout test results output
is summarize_results in
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp....

I can't see any harm in mentioning this in a comment here. This also seems like
something that could be mentioned in layouttestresults.py.

 
     def fetch_layout_test_results(self, results_url):
+        """Returns a LayoutTestResults object for results fetched from a given URL."""
         # FIXME: This should cache that the result was a 404 and stop hitting the network.
+        # This may be able to be done by just adding a @memoized decorator.
         results_file = NetworkTransaction(convert_404_to_None=True).run(
             lambda: self._fetch_file_from_results(results_url, "failing_results.json"))
         revision = NetworkTransaction(convert_404_to_None=True).run(
@@ -76,24 +77,12 @@ class Builder(object):
             results_file = None

[dcampb, 2016/07/15 17:33:26]

Why is there a Builder object? Would it be beneficial to move all the functions
from Builder to BuildBot and pass the arguments as parameters to the functions?

[qyearsley, 2016/07/15 17:49:59]

That's what this CL does -- after this change, there should be no BuildBot
object.

         return LayoutTestResults.results_from_string(results_file, revision)
 
-    def build(self, build_number):
-        return Build(self, build_number=build_number)
-
-
-class Build(object):
-
-    def __init__(self, builder, build_number):
-        self._builder = builder
-        self._number = build_number
-
-    def results_url(self):
-        return "%s/%s/layout-test-results" % (self._builder.results_url(), self._number)
-
-    def builder(self):
-        return self._builder
-
-
-class BuildBot(object):
-
-    def builder_with_name(self, builder_name):
-        return Builder(builder_name, self)
+    def _fetch_file_from_results(self, results_url, file_name):
+        # 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" % (results_url, file_name))
+        if not result:
+            return None
+        # urlopen returns a file-like object which sometimes works fine with str()
+        # but sometimes is a addinfourl object.  In either case calling read() is correct.
+        return result.read()