[webkit-reviews] review denied: [Bug 91754] [NRWT] should have a way to restrict pixel tests for individual directories : [Attachment 154101] proposed patch, test included

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has denied Balazs Kelemen
<kbalazs at webkit.org>'s request for review:
Bug 91754: [NRWT] should have a way to restrict pixel tests for individual
directories
https://bugs.webkit.org/show_bug.cgi?id=91754

Attachment 154101: proposed patch, test included
https://bugs.webkit.org/attachment.cgi?id=154101&action=review

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=154101&action=review


r-'ing this since it sounds like you've got another patch on the way.

>>> Tools/Scripts/webkitpy/layout_tests/port/base.py:755
>>> +	     return relative_path if
self._filesystem.exists(self._filesystem.join(self.layout_tests_dir(),
relative_path)) else None
>> 
>> It looks like you're adding this function so that we can support one of
three ways of specifying tests ...
>> 
>> 1) absolute paths, e.g. '/checkout/LayoutTests/foo
>> 2) paths relative to the checkout, e.g., 'LayoutTests/foo'
>> 3) paths relative to 'LayoutTests', e.g., 'foo'
>> 
>> right?
>> 
>> I don't know that supporting (1) is worth it or needed, but maybe you have
some need for it I'm not aware of? Most of the other ways of specifying tests
and directories don't support this method, so I would prefer not to support it,
for consistency.
>> 
>> Supporting (2) is done for NRWT in the manager code (in collect_tests() and
strip_test_dir_prefixes()). It would be good to combine that code with this.
>> 
>> Can we constrain this patch to just supporting (3), and then support (2) in
a separate patch? If you would prefer to leave things the way they are, can you
at least add a FIXME: comment to combine the code later?
>> 
>> Lastly, this name is too similar to relative_test_filename(); I don't think
callers would know when to use one or the other, so we should find a different
name. We should also figure out if we can just combine the two routines - does
anything *rely* on relatlive_test_filename only having the third type of
filename passed as input?
> 
> I think collect_tests supports all the three way, so I was thinking I should
do the same. Yes, it's better to leave it to a follow-up patch, so now I
restricted to the third form as you suggested.

Nope, collect_tests() won't accept absolute paths (at least in the case I just
tested, and I didn't think it worked, either). We could certainly change that,
but I've never heard someone actually request it.