Replace webkitpy standalone builders functions with instantiable class.

Replace webkitpy standalone builders functions with instantiable class.

This patch replaces the standalone functions and global _exact_matches
dictionary with an instantiable Builders class. By default, a new Builders
object uses the dictionary that was previously globally defined so there should
be no behavioral change. Subclasses of Builders can provide a new dictionary
for tests.

I've replaced uses of the global by instantiating a Builders object in the Host
class and modifying BotTestExpectations and BotTestExpectationsFactory to take
a Builders as an argument.

This was suggested in https://codereview.chromium.org/1766583002/ and makes the
builders more straightfoward to fake in unit tests.

BUG=595414
Committed: https://crrev.com/2e290a5019d0e50540503c18f660cb44d117e0d6
Cr-Commit-Position: refs/heads/master@{#391805}

[bokan, 2016-05-02 16:52:08]

Description was changed from

==========
Replace webkitpy standalone builders functions with instantiable class.

This was suggested in https://codereview.chromium.org/1766583002/ and makes the
builders more straightfoward to fake in unit tests.

BUG=595414
==========

to

==========
Replace webkitpy standalone builders functions with instantiable class.

This patch replaces the standalone functions and global _exact_matches
dictionary with an instantiable Builders class owned by the Host object. By
default, a new Builders object uses the dictionary that was previously globally
defined. Subclasses of Builders can provide a new dictionary for tests.

This was suggested in https://codereview.chromium.org/1766583002/ and makes the
builders more straightfoward to fake in unit tests.

BUG=595414
==========

[bokan, 2016-05-02 16:52:08]

bokan@chromium.org changed reviewers:
+ dpranke@chromium.org

[bokan, 2016-05-02 16:54:59]

Description was changed from

==========
Replace webkitpy standalone builders functions with instantiable class.

This patch replaces the standalone functions and global _exact_matches
dictionary with an instantiable Builders class owned by the Host object. By
default, a new Builders object uses the dictionary that was previously globally
defined. Subclasses of Builders can provide a new dictionary for tests.

This was suggested in https://codereview.chromium.org/1766583002/ and makes the
builders more straightfoward to fake in unit tests.

BUG=595414
==========

to

==========
Replace webkitpy standalone builders functions with instantiable class.

This patch replaces the standalone functions and global _exact_matches
dictionary with an instantiable Builders class. By default, a new Builders
object uses the dictionary that was previously globally defined so there should
be no behavioral change. Subclasses of Builders can provide a new dictionary
for tests.

I've replaced uses of the global by instantiating a Builders object in the Host
class and modifying BotTestExpectations and BotTestExpectationsFactory to take
a Builders as an argument.

This was suggested in https://codereview.chromium.org/1766583002/ and makes the
builders more straightfoward to fake in unit tests.

BUG=595414
==========

[bokan, 2016-05-02 17:18:08]

Hi Dirk,

PTAL, you asked for something like this in
https://codereview.chromium.org/1766583002/. Is this along the lines you were
thinking?

https://codereview.chromium.org/1939843002/diff/1/third_party/WebKit/Tools/Sc...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py
(right):

https://codereview.chromium.org/1939843002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/layout_package/bot_test_expectations.py:155:
# specifiers arg is used in unittests to avoid the static dependency on
builders.
This comment seems to be out of date even before this patch, we already depend
on builders here. Should I just remove the comment? Can we get rid of the
specifiers arg?

https://codereview.chromium.org/1939843002/diff/1/third_party/WebKit/Tools/Sc...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/factory_unittest.py
(right):

https://codereview.chromium.org/1939843002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/factory_unittest.py:52:
host = MockHost(os_name=os_name, os_version=os_version)
PortFactory takes a Host in a production situation so this seems more
appropriate

[qyearsley, 2016-05-02 21:56:35]

qyearsley@chromium.org changed reviewers:
+ qyearsley@chromium.org

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

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/common/host.py (right):

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/host.py:64: self.builders =
Builders()
In general, does it make sense for builders to be a member attribute of Host?
(Does Host represent the specific machine that's running a command or tests?)

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py
(right):

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py:33: 
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?

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py:95:
return builder_name
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 ''

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/factory.py
(right):

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/factory.py:36: from
webkitpy.layout_tests.port.builders import Builders
This import may be unnecessary now, since builders is an attribute of host, we
don't use it directly in this module now I think.

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/flakytests.py
(right):

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/flakytests.py:35: from
webkitpy.layout_tests.port.builders import Builders
This import may be unnecessary now. I don't see where it's used in this module
currently -- does importing it have some side-effect that is required for this
command?

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

Yup, this is pretty much what I had in mind. I would probably add some comments
as qyearsley@ suggests, and address the comments I had as well, but otherwise it
should be good to go.

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py
(right):

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py:72: def
rebaseline_override_dir(self, builder_name):
I think we don't use rebaseline_override_dirs any more, so I would get rid of
this.

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py:78: def
specifiers_for_builder(self, builder_name):
Is this function needed/used?

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py:91: def
builder_name_for_specifiers(self, version, build_type):
Is this function even needed/used? The prior version of the file didn't have it,
right?

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

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/common/host.py (right):

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/host.py:64: self.builders =
Builders()
On 2016/05/02 21:56:36, qyearsley wrote:
> In general, does it make sense for builders to be a member attribute of Host?
> (Does Host represent the specific machine that's running a command or tests?)

Host seems to be a general global state object. I'm not sure where else this
could be put so it would be accessible where needed. Port is the other obvious
candidate but we need the list of builders in order to construct a Port so that
can't be it. Dirk might know more if this is appropriate.

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py
(right):

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py:33: 
On 2016/05/02 21:56:36, qyearsley wrote:
> 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?

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.

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py:72: def
rebaseline_override_dir(self, builder_name):
On 2016/05/03 23:16:38, Dirk Pranke wrote:
> I think we don't use rebaseline_override_dirs any more, so I would get rid of
> this.

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?

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py:78: def
specifiers_for_builder(self, builder_name):
On 2016/05/03 23:16:38, Dirk Pranke wrote:
> Is this function needed/used?

In one place, BotTestExpectations uses it in __init__

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py:91: def
builder_name_for_specifiers(self, version, build_type):
On 2016/05/03 23:16:38, Dirk Pranke wrote:
> Is this function even needed/used? The prior version of the file didn't have
it,
> right?

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.

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py:95:
return builder_name
On 2016/05/02 21:56:36, qyearsley wrote:
> 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 ''

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.

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/factory.py
(right):

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/factory.py:36: from
webkitpy.layout_tests.port.builders import Builders
On 2016/05/02 21:56:36, qyearsley wrote:
> This import may be unnecessary now, since builders is an attribute of host, we
> don't use it directly in this module now I think.

Done.

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/flakytests.py
(right):

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/flakytests.py:35: from
webkitpy.layout_tests.port.builders import Builders
On 2016/05/02 21:56:36, qyearsley wrote:
> This import may be unnecessary now. I don't see where it's used in this module
> currently -- does importing it have some side-effect that is required for this
> command?

Done.

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

lgtm. 

I've described the classes below, so you could update the comments if so
desired.

Also, we should probably move the builders.py file out of
webkitpy/layout_tests/port (maybe just up into layout_tests), since there's
really nothing port-specific about it.

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/common/host.py (right):

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/host.py:64: self.builders =
Builders()
On 2016/05/04 11:58:47, bokan wrote:
> On 2016/05/02 21:56:36, qyearsley wrote:
> > In general, does it make sense for builders to be a member attribute of
Host?
> > (Does Host represent the specific machine that's running a command or
tests?)
> 
> Host seems to be a general global state object. I'm not sure where else this
> could be put so it would be accessible where needed. Port is the other obvious
> candidate but we need the list of builders in order to construct a Port so
that
> can't be it. Dirk might know more if this is appropriate.

Host is appropriate enough; we don't need to over-think this.

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py
(right):

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py:33: 
On 2016/05/04 11:58:47, bokan wrote:
> On 2016/05/02 21:56:36, qyearsley wrote:
> > 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?
> 
> 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.

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.

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py:72: def
rebaseline_override_dir(self, builder_name):
On 2016/05/04 11:58:47, bokan wrote:
> On 2016/05/03 23:16:38, Dirk Pranke wrote:
> > I think we don't use rebaseline_override_dirs any more, so I would get rid
of
> > this.
> 
> 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?

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

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py:78: def
specifiers_for_builder(self, builder_name):
On 2016/05/04 11:58:47, bokan wrote:
> On 2016/05/03 23:16:38, Dirk Pranke wrote:
> > Is this function needed/used?
> 
> In one place, BotTestExpectations uses it in __init__

Acknowledged.

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

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py
(right):

https://codereview.chromium.org/1939843002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/builders.py:72: def
rebaseline_override_dir(self, builder_name):
On 2016/05/04 22:42:36, Dirk Pranke wrote:
> On 2016/05/04 11:58:47, bokan wrote:
> > On 2016/05/03 23:16:38, Dirk Pranke wrote:
> > > I think we don't use rebaseline_override_dirs any more, so I would get rid
> of
> > > this.
> > 
> > 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?
> 
> I think they might be dead code, but am not positive. Maybe file a follow-up
bug
> to check?

Done. Filed crbug.com/609472

[bokan, 2016-05-05 14:21:06]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-05-05 14:21:07]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1939843002/#ps60001
(title: "Moved builders out of port, updated comment")

[commit-bot: I haz the power, 2016-05-05 14:21:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1939843002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1939843002/60001

[commit-bot: I haz the power, 2016-05-05 15:25:58]

Description was changed from

==========
Replace webkitpy standalone builders functions with instantiable class.

This patch replaces the standalone functions and global _exact_matches
dictionary with an instantiable Builders class. By default, a new Builders
object uses the dictionary that was previously globally defined so there should
be no behavioral change. Subclasses of Builders can provide a new dictionary
for tests.

I've replaced uses of the global by instantiating a Builders object in the Host
class and modifying BotTestExpectations and BotTestExpectationsFactory to take
a Builders as an argument.

This was suggested in https://codereview.chromium.org/1766583002/ and makes the
builders more straightfoward to fake in unit tests.

BUG=595414
==========

to

==========
Replace webkitpy standalone builders functions with instantiable class.

This patch replaces the standalone functions and global _exact_matches
dictionary with an instantiable Builders class. By default, a new Builders
object uses the dictionary that was previously globally defined so there should
be no behavioral change. Subclasses of Builders can provide a new dictionary
for tests.

I've replaced uses of the global by instantiating a Builders object in the Host
class and modifying BotTestExpectations and BotTestExpectationsFactory to take
a Builders as an argument.

This was suggested in https://codereview.chromium.org/1766583002/ and makes the
builders more straightfoward to fake in unit tests.

BUG=595414
==========

[commit-bot: I haz the power, 2016-05-05 15:25:59]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-05 15:27:29]

Description was changed from

==========
Replace webkitpy standalone builders functions with instantiable class.

This patch replaces the standalone functions and global _exact_matches
dictionary with an instantiable Builders class. By default, a new Builders
object uses the dictionary that was previously globally defined so there should
be no behavioral change. Subclasses of Builders can provide a new dictionary
for tests.

I've replaced uses of the global by instantiating a Builders object in the Host
class and modifying BotTestExpectations and BotTestExpectationsFactory to take
a Builders as an argument.

This was suggested in https://codereview.chromium.org/1766583002/ and makes the
builders more straightfoward to fake in unit tests.

BUG=595414
==========

to

==========
Replace webkitpy standalone builders functions with instantiable class.

This patch replaces the standalone functions and global _exact_matches
dictionary with an instantiable Builders class. By default, a new Builders
object uses the dictionary that was previously globally defined so there should
be no behavioral change. Subclasses of Builders can provide a new dictionary
for tests.

I've replaced uses of the global by instantiating a Builders object in the Host
class and modifying BotTestExpectations and BotTestExpectationsFactory to take
a Builders as an argument.

This was suggested in https://codereview.chromium.org/1766583002/ and makes the
builders more straightfoward to fake in unit tests.

BUG=595414

Committed: https://crrev.com/2e290a5019d0e50540503c18f660cb44d117e0d6
Cr-Commit-Position: refs/heads/master@{#391805}
==========

[commit-bot: I haz the power, 2016-05-05 15:27:30]

Patchset 4 (id:??) landed as
https://crrev.com/2e290a5019d0e50540503c18f660cb44d117e0d6
Cr-Commit-Position: refs/heads/master@{#391805}