Bug 102229 – Add the --order option to NRWT

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Zan Dobersek]

As discussed on the webkit-dev mailing list[1], a new option, '--order' shall be added to specify in detail in what order the gathered tests should be run.
At this moment three possible values have been somewhat agreed upon:
'natural', sorts the tests in natural order,
'random', sorts the tests in random order,
'none', keeps the order in which the tests were gathered.

The '--randomized-order' option shall be removed as its effect is now achievable through using '--order=random'.

I'll upload a patch in a moment.


[1] http://lists.webkit.org/pipermail/webkit-dev/2012-November/022750.html

[Zan Dobersek]

Created attachment 174186 [details]
Patch

[Zan Dobersek]

Comment on attachment 174186 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174186&action=review

The patch adds two new modules under webkitpy.common. The natural_order (could use a better name) contains methods for natural sorting and test key that were moved here from the Sharder class. The test_name method contains the TEST_PATH_SEPARATOR variable and the split_test_name method, both moved here from the Port class.

Also, the natural ordering is used as default for the --order option while I proposed no ordering as default on the mailing. This preserves the current behavior.

> 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.

> Tools/Scripts/webkitpy/thirdparty/ordered_set.py:89
> +if __name__ == '__main__':

I've copied the whole source file from the link below the header, but this part isn't really needed. Can/Should it be removed?

[Dirk Pranke]

Comment on attachment 174186 [details]
Patch

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.

[Dirk Pranke]

Comment on attachment 174186 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174186&action=review

>> 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.

Also, I'm not sure where how you got to the OrderedSet implementation, but there is this page on the web ...

http://www.peterbe.com/plog/uniqifiers-benchmark

from which one can glean that you can write an order-preserving list-de-duping function like this:

def dedup_seq(xs):  # aka f7()
   seen = set()
   return [x for x in xs if x not in seen and not seen.add(x)]

> Tools/Scripts/webkitpy/layout_tests/port/base.py:596
> +        return OrderedSet([self.relative_test_filename(f) for f in files])

Maybe this should just return the list?

>> Tools/Scripts/webkitpy/thirdparty/ordered_set.py:89
>> +if __name__ == '__main__':
> 
> I've copied the whole source file from the link below the header, but this part isn't really needed. Can/Should it be removed?

I would not use this module at all (as discussed above), but if we do decide to keep it, we should leave the file as close to the original as possible.


Also:

(In reply to comment #2)
> (From update of attachment 174186 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174186&action=review
> 
> The patch adds two new modules under webkitpy.common. The natural_order (could use a better name) contains methods for natural sorting and test key that were moved here from the Sharder class. The test_name method contains the TEST_PATH_SEPARATOR variable and the split_test_name method, both moved here from the Port class.
> 

What do you not like about "natural_order" as a name?

[Zan Dobersek]

Comment on attachment 174186 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174186&action=review

>> 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).

I'll then produce a separate patch for this module.

>>> 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.
> 
> Also, I'm not sure where how you got to the OrderedSet implementation, but there is this page on the web ...
> 
> http://www.peterbe.com/plog/uniqifiers-benchmark
> 
> from which one can glean that you can write an order-preserving list-de-duping function like this:
> 
> def dedup_seq(xs):  # aka f7()
>    seen = set()
>    return [x for x in xs if x not in seen and not seen.add(x)]

The OrderedSet implementation was retrieved from http://code.activestate.com/recipes/576694/. The OrderedDict was imported from the same place.

I used OrderedSet so the current behavior of specifying 'foo bar foo' would only run tests in 'foo bar' in the specified order, but I can understand advantages of using 'foo bar foo' to run tests in foo twice. Changing this behavior I don't think OrderedSet is required anymore.

[Zan Dobersek]

Created attachment 174481 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 174481 [details]
Patch

r- now, because it isn't applyable, and the half applied patch killed the Qt EWS :-/

Failed to run "['/storage/WebKit-qt-ews/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-i..." exit_code: 1

Traceback (most recent call last):
  File "/storage/WebKit-qt-ews/Tools/Scripts/webkit-patch", line 44, in <module>
    from webkitpy.tool.main import WebKitPatch
  File "/storage/WebKit-qt-ews/Tools/Scripts/webkitpy/tool/main.py", line 37, in <module>
    from webkitpy.common.host import Host
  File "/storage/WebKit-qt-ews/Tools/Scripts/webkitpy/common/host.py", line 38, in <module>
    from webkitpy.common.net.buildbot.chromiumbuildbot import ChromiumBuildBot
  File "/storage/WebKit-qt-ews/Tools/Scripts/webkitpy/common/net/buildbot/chromiumbuildbot.py", line 32, in <module>
    from webkitpy.layout_tests.port.builders import builder_path_from_name
  File "/storage/WebKit-qt-ews/Tools/Scripts/webkitpy/layout_tests/port/__init__.py", line 34, in <module>
    from base import Port  # It's possible we don't need to export this virtual baseclass outside the module.
  File "/storage/WebKit-qt-ews/Tools/Scripts/webkitpy/layout_tests/port/base.py", line 51, in <module>
    from webkitpy.common import find_files
  File "/storage/WebKit-qt-ews/Tools/Scripts/webkitpy/common/find_files.py", line 47, in <module>
    from webkitpy.common import natural_order
ImportError: cannot import name natural_order

[Dirk Pranke]

Comment on attachment 174481 [details]
Patch



View in context: https://bugs.webkit.org/attachment.cgi?id=174481&action=review

This patch is mostly okay, but having to split my comments between this bug, bug 102401, and bug 102396 does make things a bit more complicated (which is why I wanted to make sure we were all on the same page before you did too much more work, but I guess the time zones worked against me. Sorry!).

In short, re: bug102401 - if we're going to make a separate module for natural sorting, it should not have anything to do w/ test names. 

re: bug102396 - I do not want to create a separate module for test names at this time. 

Once you sort those two issues out, then the comments on the rest of this patch are fairly straightforward ...

I apologize for the back-and-forth.

> Tools/Scripts/webkitpy/common/find_files.py:84
> +            paths_to_walk.append(path)

Since we're now using lists, you can now replace lines 76-84 with

paths_to_walk = itertools.chain(filesystem.glob(path) for path in paths)

I think :). I'm not sure how much clearer that actually is; I usually find itertools.chain* confusing. But, calling glob every time and getting rid of the if is probably still an improvement.

> Tools/Scripts/webkitpy/common/find_files.py:89
> +        files.sort(key=natural_order.test_name_key)

You need to sort the files here in order to have files sorted properly when using --order=none but paths contain directories, right (since both --order=random and --order=natural will re-sort later in manager._prepare())?

See my comments in bug 102396 about test_names and the Port class (i.e., leave them in the port class); I think if you also move the test_name_key function to the Port object this will make the layering easier (and then I think the runner doesn't need to know about TEST_PATH_SEPARATOR either)

> Tools/Scripts/webkitpy/common/find_files.py:90
> +        all_files.extend(files)

Not that you could also use an itertools.chain() here, but I wouldn't necessarily recommend it :).

[Zan Dobersek]

(In reply to comment #8)
> In short, re: bug102401 - if we're going to make a separate module for natural sorting, it should not have anything to do w/ test names. 
> 
> re: bug102396 - I do not want to create a separate module for test names at this time. 

Right, understood. I'll create a new bug for moving the natural sorting to the base Port interface for now, as recommended in #102401.(In reply to comment #7)


> (From update of attachment 174481 [details])
> r- now, because it isn't applyable, and the half applied patch killed the Qt EWS :-/
> 
> Failed to run "['/storage/WebKit-qt-ews/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-i..." exit_code: 1
> ...

Ugh, sorry about that. I'll look into why the EWS gets into problems when a patch isn't applied properly.

[Zan Dobersek]

Comment on attachment 174481 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174481&action=review

>> Tools/Scripts/webkitpy/common/find_files.py:89
>> +        files.sort(key=natural_order.test_name_key)
> 
> You need to sort the files here in order to have files sorted properly when using --order=none but paths contain directories, right (since both --order=random and --order=natural will re-sort later in manager._prepare())?
> 
> See my comments in bug 102396 about test_names and the Port class (i.e., leave them in the port class); I think if you also move the test_name_key function to the Port object this will make the layering easier (and then I think the runner doesn't need to know about TEST_PATH_SEPARATOR either)

Yes, that's the reason why natural sorting is used here.

Unfortunately, moving the natural sorting and the test name key based on it to the Port interface and then using them here, in find_files, causes an import loop. Can the pure natural ordering functionality be moved into a separate module and then have the test name key definitions duplicated where required until the test name/path logic is moved into a separate module?

[Dirk Pranke]

(In reply to comment #10)
> (From update of attachment 174481 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174481&action=review
> 
> >> Tools/Scripts/webkitpy/common/find_files.py:89
> >> +        files.sort(key=natural_order.test_name_key)
> > 
> > You need to sort the files here in order to have files sorted properly when using --order=none but paths contain directories, right (since both --order=random and --order=natural will re-sort later in manager._prepare())?
> > 
> > See my comments in bug 102396 about test_names and the Port class (i.e., leave them in the port class); I think if you also move the test_name_key function to the Port object this will make the layering easier (and then I think the runner doesn't need to know about TEST_PATH_SEPARATOR either)
> 
> Yes, that's the reason why natural sorting is used here.
> 
> Unfortunately, moving the natural sorting and the test name key based on it to the Port interface and then using them here, in find_files, causes an import loop. Can the pure natural ordering functionality be moved into a separate module and then have the test name key definitions duplicated where required until the test name/path logic is moved into a separate module?

Ah. Well, you can move the natural ordering functionality into a separate module, but I don't think it should know anything about test_names.

To get around the import loop we should modify find files to take an optional sort function parameter and pass it in from the call in base.Port. That will avoid any duplication and probably is the right thing to do anyway since the logic being used to sort files is very specific to the tests (and one can imagine plenty of uses for find_files that don't need sorting at all).

Or, we could just move find_files back into the Port class as well; it's not clear that it really makes sense as a standalone module since it's using the logic specific to test names and uses filesystem for the heavy lifting.

[Zan Dobersek]

Created attachment 175490 [details]
Patch

[WebKit Review Bot]

Comment on attachment 175490 [details]
Patch

Attachment 175490 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14966080

New failing tests:
platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html

[Dirk Pranke]

Comment on attachment 175490 [details]
Patch

Close. Making Port.split_test a static method seems to defeat the point of the abstraction, and it doesn't seem like it's really needed anywhere? Looks fine otherwise, though.

[Zan Dobersek]

(In reply to comment #14)
> (From update of attachment 175490 [details])
> Close. Making Port.split_test a static method seems to defeat the point of the abstraction, and it doesn't seem like it's really needed anywhere? Looks fine otherwise, though.

It's now used in Port.test_key, which is static as well. Ergo, due to this use case, it should be possible to call split_test without a Port class instance.

How about making it a class method?

[Dirk Pranke]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 175490 [details] [details])
> > Close. Making Port.split_test a static method seems to defeat the point of the abstraction, and it doesn't seem like it's really needed anywhere? Looks fine otherwise, though.
> 
> It's now used in Port.test_key, which is static as well. Ergo, due to this use case, it should be possible to call split_test without a Port class instance.
> 
> How about making it a class method?

Yeah, but the only place where Port.test_key is used and you don't have access to a port object is in the unit tests, and it's certainly easy enough to create one there. I don't think classmethods actually help here, since the only way you'd know which class method to call would be to have a port object already.

[Zan Dobersek]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > (From update of attachment 175490 [details] [details] [details])
> > > Close. Making Port.split_test a static method seems to defeat the point of the abstraction, and it doesn't seem like it's really needed anywhere? Looks fine otherwise, though.
> > 
> > It's now used in Port.test_key, which is static as well. Ergo, due to this use case, it should be possible to call split_test without a Port class instance.
> > 
> > How about making it a class method?
> 
> Yeah, but the only place where Port.test_key is used and you don't have access to a port object is in the unit tests, and it's certainly easy enough to create one there. I don't think classmethods actually help here, since the only way you'd know which class method to call would be to have a port object already.

Ah, yes, I can just use TestWebKitPort in the unit tests.

[Zan Dobersek]

Created attachment 175817 [details]
Patch

[Dirk Pranke]

Comment on attachment 175817 [details]
Patch

Looks good. Thanks for bearing with me!

[Zan Dobersek]

Comment on attachment 175817 [details]
Patch

Clearing flags on attachment: 175817

Committed r135653: <http://trac.webkit.org/changeset/135653>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.