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

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.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 62309: proposed patch
https://bugs.webkit.org/attachment.cgi?id=62309&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
This seems like a useful command. This patch needs a unittest.

> +	   webkit-patch command to find the ports covering a specific layout
test

I don't think using the word "cover" is very clear. How about s/covering/not
skipping/?

> +class WhatCovers(AbstractDeclarativeCommand):
> +    name = "what-covers"

This name isn't very clear. Instead of giving the list of ports that don't skip
the test, how about returning the list of ports that do skip it. Usually, that
list will be much shorter. Then this command could be called "skipped-ports" or
something like htat.

> +    def execute(self, options, args, tool):
> +	   platform_directory = os.path.join(tool.scm().cwd, "LayoutTests",
"platform")

Ideally this wouldn't depend on being run from a specific directory. scm.py has
some bits to find to root directory of the checkout.

> +	   results = dict([(test, []) for test in args])
> +	   for port_name in os.listdir(platform_directory):
> +	       skipped_file = os.path.join(platform_directory, port_name,
"Skipped")

Can you put a FIXME to also check
LayoutTests/platform/chromium/test_expectations.txt? Or, even better, just add
the code to parse it. That file has a different syntax. Each test is on it's
own line, but it's not just skipped. The format for the line is:
MODIFIERS : path/to/test = EXPECTATIONS

If the test is skipped, the one of the MODIFIERS will be SKIP.