[webkit-reviews] review denied: [Bug 42832] webkit-patch command to find the ports covering a specific layout test : [Attachment 66656] proposed patch (reworked)

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 42832: webkit-patch command to find the ports covering a specific layout
test
https://bugs.webkit.org/show_bug.cgi?id=42832

Attachment 66656: proposed patch (reworked)
https://bugs.webkit.org/attachment.cgi?id=66656&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=66656&action=prettypatch

Mostly nits.  The fake test_expectations parser is my main complaint.

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:317
> +	       basename, extension = os.path.splitext(test_or_category)
> +	       if not extension and test_name.startswith(basename):
> +		   return True
Crazy.	We decide if something is a directory by whether it has an extension? 
It feels like there should be a better way to do this, but I don't know what it
is.

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:239
> +    def skipped_layout_tests(self):
> +	   return [line.split(':')[1].split("=")[0].strip()
> +		   for line in self.test_expectations().splitlines()
> +		   if line.find('SKIP') != -1 and not line.startswith('//')]
Surely we should use the test_expectation file parser instead of hacking one
together here.

> WebKitTools/Scripts/webkitpy/layout_tests/port/factory.py:37
> +ALL_PORT_NAMES = ['test', 'dryrun', 'mac', 'win', 'gtk', 'qt',
'chromium-mac',
> +		     'chromium-linux', 'chromium-win', 'google-chrome-win',
> +		     'google-chrome-mac', 'google-chrome-linux32',
'google-chrome-linux64']
This seems like a maintenance burden.  Can we get this information by crawling
the port classes that have been imported into memory?  You can see how we do
that to autoregister commands.

> WebKitTools/Scripts/webkitpy/tool/commands/queries.py:298
> +	   class Options:
> +	       pass
This seems like a ugly pattern.  Can't we just pass an empty dictionary to
represent no options?

> WebKitTools/Scripts/webkitpy/tool/main.py:80
> +	   self.port_factory = port.factory
Nice.  I'm glad these two subsystems are finally getting integrated.