Replace webkitpy standalone builders functions with instantiable class.

third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py

 # Copyright (C) 2011 Google Inc. All rights reserved.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
 # met:
 #
 #     * Redistributions of source code must retain the above copyright
 # notice, this list of conditions and the following disclaimer.
 #     * Redistributions in binary form must reproduce the above
 # copyright notice, this list of conditions and the following disclaimer
@@ skipping 10 matching lines @@
 # OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
 # SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
 # LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
 # DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
 # THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
 # (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
 
-# In this dictionary, each item stores:
-# * port_name -- a fully qualified port name
-# * rebaseline_override_dir -- (optional) directory to put baselines in instead of where you would normally put them.
-#      This is useful when we don't have bots that cover particular configurations; so, e.g., you might
-#      support mac-mountainlion but not have a mac-mountainlion bot yet, so you'd want to put the mac-lion
-#      results into platform/mac temporarily.
-# * specifiers -- TestExpectation specifiers for that config. Valid values are found in
-#      TestExpectationsParser._configuration_tokens_list
 
-_exact_matches = {
-    "WebKit Win7": {"port_name": "win-win7", "specifiers": ['Win7', 'Release']},
-    "WebKit Win7 (dbg)": {"port_name": "win-win7", "specifiers": ['Win7', 'Debug']},
-    "WebKit Win10": {"port_name": "win-win10", "specifiers": ['Win10', 'Release']},
-    # FIXME: Rename this to 'WebKit Linux Precise'
-    "WebKit Linux": {"port_name": "linux-precise", "specifiers": ['Precise', 'Release']},
-    "WebKit Linux Trusty": {"port_name": "linux-trusty", "specifiers": ['Trusty', 'Release']},
-    "WebKit Linux (dbg)": {"port_name": "linux-precise", "specifiers": ['Precise', 'Debug']},
-    "WebKit Mac10.9": {"port_name": "mac-mac10.9", "specifiers": ['Mac10.9', 'Release']},
-    "WebKit Mac10.10": {"port_name": "mac-mac10.10", "specifiers": ['Mac10.10', 'Release']},
-    "WebKit Mac10.11": {"port_name": "mac-mac10.11", "specifiers": ['10.11', 'Release']},
-    "WebKit Mac10.11 (dbg)": {"port_name": "mac-mac10.11", "specifiers": ['10.11', 'Debug']},
-    "WebKit Mac10.11 (retina)": {"port_name": "mac-retina", "specifiers": ['Retina', 'Release']},
-    "WebKit Android (Nexus4)": {"port_name": "android", "specifiers": ['Android', 'Release']},
-}
+class Builders(object):
 

[qyearsley, 2016/05/02 21:56:36]

Optionally, here (or above) there could be a short description of what an
instance of Builders represents -- does it represent a record of all different
waterfall builders where layout tests are run? And usually the set of builders
is the set that's hard-coded in the constructor, but this can be overridden for
tests?

[bokan, 2016/05/04 11:58:47]

I'm probably not the best person to do this since I don't really know but that's
my understanding. I added a fairly generic comment with a TODO for someone more
knowledgeable to expand. If Dirk knows I can replace this with a better
explanation now.

[Dirk Pranke, 2016/05/04 22:42:36]

Builders is the list of the main chromium builders where the layout tests are
run. There are other
waterfalls that also run them (v8, webrtc), but we don't care about those.

+    def __init__(self):
 
-_ports_without_builders = [
-]
+        # In this dictionary, each item stores:
+        # * port_name -- a fully qualified port name
+        # * rebaseline_override_dir -- (optional) directory to put baselines in instead of where you would normally put them.
+        #      This is useful when we don't have bots that cover particular configurations; so, e.g., you might
+        #      support mac-mountainlion but not have a mac-mountainlion bot yet, so you'd want to put the mac-lion
+        #      results into platform/mac temporarily.
+        # * specifiers -- TestExpectation specifiers for that config. Valid values are found in
+        #      TestExpectationsParser._configuration_tokens_list
+        self._exact_matches = {
+            "WebKit Win7": {"port_name": "win-win7", "specifiers": ['Win7', 'Release']},
+            "WebKit Win7 (dbg)": {"port_name": "win-win7", "specifiers": ['Win7', 'Debug']},
+            "WebKit Win10": {"port_name": "win-win10", "specifiers": ['Win10', 'Release']},
+            # FIXME: Rename this to 'WebKit Linux Precise'
+            "WebKit Linux": {"port_name": "linux-precise", "specifiers": ['Precise', 'Release']},
+            "WebKit Linux Trusty": {"port_name": "linux-trusty", "specifiers": ['Trusty', 'Release']},
+            "WebKit Linux (dbg)": {"port_name": "linux-precise", "specifiers": ['Precise', 'Debug']},
+            "WebKit Mac10.9": {"port_name": "mac-mac10.9", "specifiers": ['Mac10.9', 'Release']},
+            "WebKit Mac10.10": {"port_name": "mac-mac10.10", "specifiers": ['Mac10.10', 'Release']},
+            "WebKit Mac10.11": {"port_name": "mac-mac10.11", "specifiers": ['10.11', 'Release']},
+            "WebKit Mac10.11 (dbg)": {"port_name": "mac-mac10.11", "specifiers": ['10.11', 'Debug']},
+            "WebKit Mac10.11 (retina)": {"port_name": "mac-retina", "specifiers": ['Retina', 'Release']},
+            "WebKit Android (Nexus4)": {"port_name": "android", "specifiers": ['Android', 'Release']},
+        }
 
+        self._ports_without_builders = [
+        ]
 
-def builder_path_from_name(builder_name):
-    return re.sub(r'[\s().]', '_', builder_name)
+    def builder_path_from_name(self, builder_name):
+        return re.sub(r'[\s().]', '_', builder_name)
 
+    def all_builder_names(self):
+        return sorted(set(self._exact_matches.keys()))
 
-def all_builder_names():
-    return sorted(set(_exact_matches.keys()))
+    def all_port_names(self):
+        return sorted(set(map(lambda x: x["port_name"], self._exact_matches.values()) + self._ports_without_builders))
 
+    def rebaseline_override_dir(self, builder_name):

[Dirk Pranke, 2016/05/03 23:16:38]

I think we don't use rebaseline_override_dirs any more, so I would get rid of
this.

[bokan, 2016/05/04 11:58:47]

It looks like there's a usage in rebaseline.py that's ultimately used from
RebaselineTest and CopyExistingBaselinesInternal. Or are you saying those are
dead code paths?

[Dirk Pranke, 2016/05/04 22:42:36]

I think they might be dead code, but am not positive. Maybe file a follow-up bug
to check?

[bokan, 2016/05/05 14:20:54]

Done. Filed crbug.com/609472

+        return self._exact_matches[builder_name].get("rebaseline_override_dir", None)
 
-def all_port_names():
-    return sorted(set(map(lambda x: x["port_name"], _exact_matches.values()) + _ports_without_builders))
+    def port_name_for_builder_name(self, builder_name):
+        return self._exact_matches[builder_name]["port_name"]
 
+    def specifiers_for_builder(self, builder_name):

[Dirk Pranke, 2016/05/03 23:16:38]

Is this function needed/used?

[bokan, 2016/05/04 11:58:47]

In one place, BotTestExpectations uses it in __init__

[Dirk Pranke, 2016/05/04 22:42:36]

Acknowledged.

+        return self._exact_matches[builder_name]["specifiers"]
 
-def rebaseline_override_dir(builder_name):
-    return _exact_matches[builder_name].get("rebaseline_override_dir", None)
+    def builder_name_for_port_name(self, target_port_name):
+        debug_builder_name = None
+        for builder_name, builder_info in self._exact_matches.items():
+            if builder_info['port_name'] == target_port_name:
+                if 'dbg' in builder_name:
+                    debug_builder_name = builder_name
+                else:
+                    return builder_name
+        return debug_builder_name
 
+    def builder_name_for_specifiers(self, version, build_type):

[Dirk Pranke, 2016/05/03 23:16:38]

Is this function even needed/used? The prior version of the file didn't have it,
right?

[bokan, 2016/05/04 11:58:47]

Ah, yes, this was added and used in my update-test-expectations patch. I've
removed it from here and will land it as part of that work.

+        for builder_name, info in self._exact_matches.items():
+            specifiers = info['specifiers']
+            if specifiers[0].lower() == version.lower() and specifiers[1].lower() == build_type.lower():
+                return builder_name

[qyearsley, 2016/05/02 21:56:36]

Here we're implicitly requiring the specifiers list to be [OS Version, Build
type], but we didn't really make that requirement explicit anywhere. In
rebaseline_unittest, we're using a fake specifier collection
`set(["mock-specifier"])`.

Would it make sense for this function to take a collection of specifiers, and
make sure they're all in the specifiers list for the builder? e.g.

    def builder_name_for_specifiers(self, query_specifiers):
        for builder_name, info in self._exact_matches.items():
            builder_specifiers = [s.lower() for s in info['specifiers']]
            if all(s.lower() in builder_specifiers for s in query_specifiers):
                return builder_name
        return ''

[bokan, 2016/05/04 11:58:47]

Good point. I've removed this method for the purposes of this patch since it's
only needed once my update-test-expectations patch lands but I'll do this on the
other CL.

 
-def port_name_for_builder_name(builder_name):
-    return _exact_matches[builder_name]["port_name"]
+        return ''
 
-
-def specifiers_for_builder(builder_name):
-    return _exact_matches[builder_name]["specifiers"]
-
-
-def builder_name_for_port_name(target_port_name):
-    debug_builder_name = None
-    for builder_name, builder_info in _exact_matches.items():
-        if builder_info['port_name'] == target_port_name:
-            if 'dbg' in builder_name:
-                debug_builder_name = builder_name
-            else:
-                return builder_name
-    return debug_builder_name
-
-
-def builder_path_for_port_name(port_name):
-    builder_path_from_name(builder_name_for_port_name(port_name))
+    def builder_path_for_port_name(self, port_name):
+        self.builder_path_from_name(self.builder_name_for_port_name(port_name))