Bug 104761 - support -wk2 port names properly in webkitpy.layout_tests.port
Summary: support -wk2 port names properly in webkitpy.layout_tests.port
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords: NRWT
Depends on:
Blocks:
 
Reported: 2012-12-11 19:26 PST by Dirk Pranke
Modified: 2012-12-13 13:54 PST (History)
5 users (show)

See Also:


Attachments
Patch (13.36 KB, patch)
2012-12-12 15:49 PST, Dirk Pranke
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2012-12-11 19:26:13 PST
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 ...
Comment 1 Dirk Pranke 2012-12-12 15:49:54 PST
Created attachment 179137 [details]
Patch
Comment 2 Dirk Pranke 2012-12-12 15:51:24 PST
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).
Comment 3 Eric Seidel (no email) 2012-12-13 12:46:08 PST
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?
Comment 4 Dirk Pranke 2012-12-13 12:48:50 PST
(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.
Comment 5 Dirk Pranke 2012-12-13 13:54:05 PST
Committed r137650: <http://trac.webkit.org/changeset/137650>