[webkit-reviews] review denied: [Bug 102229] Add the --order option to NRWT : [Attachment 174186] Patch

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has denied Zan Dobersek
<zandobersek at gmail.com>'s request for review:
Bug 102229: Add the --order option to NRWT
https://bugs.webkit.org/show_bug.cgi?id=102229

Attachment 174186: Patch
https://bugs.webkit.org/attachment.cgi?id=174186&action=review

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


Thanks for working on this! R-'ing for now so we can get on the same page about
the overall design for this.

> Tools/Scripts/webkitpy/common/test_name.py:30
> +TEST_PATH_SEPARATOR = '/'

Why are you splitting this out into a separate module? I can see the argument
that all of the test name logic is generic and shouldn't be a part of the Port
interface, but I would actually prefer to address that as a separate cleanup
effort (how tests are named should be combined with other aspects of test
lookup).

>> Tools/Scripts/webkitpy/common/test_name.py:38
>> +	return (test_name[0:index], test_name[index:])
> 
> With a test name 'a/b/c.html', this returns ('a/b', '/c.html'). Is the
preceding / in the basename required? The unit tests don't complain, but I'd
like someone to confirm.

That seems wrong (or at least bad). I would expect it to split into 'a/b' and
'c.html', and I can't think that the slash is required on the front of the
basename. Please make that change.

> Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_finder.py:51
> +	   paths = OrderedSet(paths)

It's not actually clear to me that we should be using an ordered set here. It
seems like it would be useful (and a good idea) for "run-webkit-tests foo bar
foo" to actually run the tests in foo twice. This would also allow us to use
test lists to reproduce test runs with --repeat-each or --iterations more
easily.