Bug 104761 – support -wk2 port names properly in webkitpy.layout_tests.port

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dirk Pranke]

Unfortunately, we're not yet in a world where there is a 1:1 mapping between port names and the list of all the ports (and port configurations) that we care about. For example, the port names don't necessarily tell you if there are both release and debug bots for that port name, or wk1 vs. wk2. So, using all_port_names() as a proxy for "all of the port configurations and/or builders I need to care about" is probably doing the wrong thing.

This manifested itself in bug 102389, but it wouldn't surprise me if there were other bugs in the code resulting from this.

It's not immediately clear to me what the right way to fix this is, either ...

[Dirk Pranke]

Created attachment 179137 [details]
Patch

[Dirk Pranke]

The uploaded patch at least makes port names ending in '-wk2' work properly. I'm still not sure what to do about release/debug, but I'm not sure if there's any cases where that information is needed from the port name, so this should fix most if not all of the current issues.

(retitling the bug subject as well).

[Eric Seidel (no email)]

Comment on attachment 179137 [details]
Patch

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

Bleh.  This code all stinks. (not your fault).  But if this moves us closer to expected behavior, and you feel we've sufficiently tested this, then this is OK.  And we'll just keep making it better... eventually.

In my view some sort of PortFactory should be responsible for computing the full "name" (or config dictionary) for the Port and then creating the port with that.  Our current behavior of having the Port constructors do that doesn't work well. :(

> Tools/Scripts/webkitpy/layout_tests/port/base.py:115
> +        if self._name and '-wk2' in self._name:
> +            self._options.webkit_test_runner = True

Do you need to use your fancy set_default instead?

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 179137 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179137&action=review
> 
> Bleh.  This code all stinks. (not your fault).  But if this moves us closer to expected behavior, and you feel we've sufficiently tested this, then this is OK.  And we'll just keep making it better... eventually.
> 
> In my view some sort of PortFactory should be responsible for computing the full "name" (or config dictionary) for the Port and then creating the port with that.  Our current behavior of having the Port constructors do that doesn't work well. :(
> 

Agreed. The PortFactory is trying to do that now, but we're not quite there yet (this patch does get us closer).

I think the testing is sufficient. I guess we'll find out :)

> > Tools/Scripts/webkitpy/layout_tests/port/base.py:115
> > +        if self._name and '-wk2' in self._name:
> > +            self._options.webkit_test_runner = True
> 
> Do you need to use your fancy set_default instead?

No, we don't want this to be a default option; it should override whatever the default was, since -wk2 is being explicitly requested.

[Dirk Pranke]

Committed r137650: <http://trac.webkit.org/changeset/137650>