Bug 74562 – webkitpy: update unit tests in preparation for making host a mandatory parameter to Port objects

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

webkitpy: update unit tests in preparation for making host a mandatory parameter to Port objects

[Dirk Pranke]

Created attachment 119333 [details]
Patch

[Dirk Pranke]

Created attachment 119340 [details]
add in more unittest files

[Eric Seidel (no email)]

Comment on attachment 119340 [details]
add in more unittest files

View in context: https://bugs.webkit.org/attachment.cgi?id=119340&action=review

> Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:54
> +    def make_port(self, **kwargs):
> +        host = MockHost()
> +        if 'executive' in kwargs:
> +            host.executive = kwargs['executive']

Seems you should just use executive=None here instead of messing with kwargs.

> Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:223
> +        port._filesystem.write_text_file(port.layout_tests_dir() + '/canvas/test', '')
> +        port._filesystem.write_text_file(port.layout_tests_dir() + '/css2.1/test', '')

We may want to add some sort of touch_file() method at some point.  At least to the MockFileSystem. :)

> Tools/Scripts/webkitpy/layout_tests/port/dryrun.py:69
> -        self.__delegate = host.port_factory.get(**kwargs)
> +        self.__delegate = factory.PortFactory(host).get(**kwargs)

I'm confused why this change?  Isn't this already what host.port_factory returns?  Or are you just trying to make explicit that we use a real PortFactory here?

[Dirk Pranke]

Comment on attachment 119340 [details]
add in more unittest files

View in context: https://bugs.webkit.org/attachment.cgi?id=119340&action=review

>> Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:54
>> +            host.executive = kwargs['executive']
> 
> Seems you should just use executive=None here instead of messing with kwargs.

good idea.

>> Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:223
>> +        port._filesystem.write_text_file(port.layout_tests_dir() + '/css2.1/test', '')
> 
> We may want to add some sort of touch_file() method at some point.  At least to the MockFileSystem. :)

good idea.

>> Tools/Scripts/webkitpy/layout_tests/port/dryrun.py:69
>> +        self.__delegate = factory.PortFactory(host).get(**kwargs)
> 
> I'm confused why this change?  Isn't this already what host.port_factory returns?  Or are you just trying to make explicit that we use a real PortFactory here?

host.port_factory only exists on the Host object, not the SystemHost object; since Port only requires a SystemHost, I can't assume that function exists.

[Dirk Pranke]

Created attachment 119546 [details]
update w/ review feedback

[Dirk Pranke]

Please take another look?

[James Robinson]

Huh, anyone know why the watchlist system thought that I needed to be CC'd on this? My only watchlist entry is this one:
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/watchlist#L18

[Eric Seidel (no email)]

(In reply to comment #7)
> Huh, anyone know why the watchlist system thought that I needed to be CC'd on this? My only watchlist entry is this one:
> http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/watchlist#L18

Because you're amazing -- just the way you are.

[David Levin]

(In reply to comment #7)
> Huh, anyone know why the watchlist system thought that I needed to be CC'd on this? My only watchlist entry is this one:
> http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/watchlist#L18

fwiw, I applied this patch to ToT and ran the watchlist locally "./Tools/Scripts/webkit-patch apply-watchlist-local".

All I got was: Result of watchlist: cc "[e-mail], [e-mail], [e-mail]" messages ""

So I have no idea. It may have been that the reset of the local tree failed on the stylebot and then we ran the watchlist anyway and picked up an old change on the server. (I think that is possible -- pretty remote so I didn't invest hugely in doing lots to deal with this -- but possible).

[David Levin]

(In reply to comment #9)
> (In reply to comment #7)
> > Huh, anyone know why the watchlist system thought that I needed to be CC'd on this? My only watchlist entry is this one:
> > http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/watchlist#L18
> 
> fwiw, I applied this patch to ToT and ran the watchlist locally "./Tools/Scripts/webkit-patch apply-watchlist-local".
> 
> All I got was: Result of watchlist: cc "[e-mail], [e-mail], [e-mail]" messages ""
> 
> So I have no idea. It may have been that the reset of the local tree failed on the stylebot and then we ran the watchlist anyway and picked up an old change on the server. (I think that is possible -- pretty remote so I didn't invest hugely in doing lots to deal with this -- but possible).

btw, note that the stylebot went red on this run without anything added to the bug which supports this theory. If anyone wants to fix the underlying problem, I'm happy to give advice (but for now I can't do this):
https://bugs.webkit.org/show_bug.cgi?id=74735

[Eric Seidel (no email)]

Comment on attachment 119546 [details]
update w/ review feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=119546&action=review

Seems OK.  I'm not sure the TestWebKitPort change is a win.

> Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:55
> +        return Port(host, executive=executive, **kwargs)

This executive=executive isn't needed, right?

> Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:-50
> -        host = host or MockHost()
> -        WebKitPort.__init__(self, host=host, **kwargs)

Interesting.  I thought  this "implicit MockHost" was useful.

[Dirk Pranke]

(In reply to comment #11)
> (From update of attachment 119546 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119546&action=review
> 
> Seems OK.  I'm not sure the TestWebKitPort change is a win.
> 
> > Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:55
> > +        return Port(host, executive=executive, **kwargs)
> 
> This executive=executive isn't needed, right?
>

I'm not sure which question you're asking ... if you're asking if you need to specify both executive and kwargs, the answer is yes (executive isn't included in kwargs). If you're asking if Port still takes the executive parameter at all, the answer is also yes (the executive parameter can be removed after 74566 lands).

> > Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:-50
> > -        host = host or MockHost()
> > -        WebKitPort.__init__(self, host=host, **kwargs)
> 
> Interesting.  I thought  this "implicit MockHost" was useful.

My initial thinking was that I didn't want any Port constructor creating its own (Mock)Hosts, but given that this is test code and it takes other custom parameters, I can see your point. We could have a factory method instead, but that's probably not much better. I'll revert these changes.

[Dirk Pranke]

Created attachment 119897 [details]
update w/ review feedback

[Dirk Pranke]

Committed r103253: <http://trac.webkit.org/changeset/103253>