RESOLVED FIXED 102229
Add the --order option to NRWT
https://bugs.webkit.org/show_bug.cgi?id=102229
Summary Add the --order option to NRWT
Zan Dobersek
Reported 2012-11-14 07:21:49 PST
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
Attachments
Patch (45.57 KB, patch)
2012-11-14 10:03 PST, Zan Dobersek
no flags
Patch (16.41 KB, patch)
2012-11-15 10:24 PST, Zan Dobersek
no flags
Patch (31.71 KB, patch)
2012-11-21 11:41 PST, Zan Dobersek
no flags
Patch (29.41 KB, patch)
2012-11-23 09:20 PST, Zan Dobersek
no flags
Zan Dobersek
Comment 1 2012-11-14 10:03:47 PST
Zan Dobersek
Comment 2 2012-11-14 10:27:55 PST
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 3 2012-11-14 11:38:39 PST
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 4 2012-11-14 11:56:23 PST
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 5 2012-11-15 08:07:04 PST
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
Comment 6 2012-11-15 10:24:24 PST
Csaba Osztrogonác
Comment 7 2012-11-15 11:08:27 PST
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 8 2012-11-15 17:47:49 PST
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
Comment 9 2012-11-15 22:54:00 PST
(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 10 2012-11-19 04:10:30 PST
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
Comment 11 2012-11-19 11:53:51 PST
(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
Comment 12 2012-11-21 11:41:10 PST
WebKit Review Bot
Comment 13 2012-11-21 13:43:07 PST
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 14 2012-11-21 13:57:50 PST
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
Comment 15 2012-11-22 00:47:59 PST
(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
Comment 16 2012-11-22 10:27:59 PST
(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
Comment 17 2012-11-22 12:14:13 PST
(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
Comment 18 2012-11-23 09:20:39 PST
Dirk Pranke
Comment 19 2012-11-23 12:43:55 PST
Comment on attachment 175817 [details] Patch Looks good. Thanks for bearing with me!
Zan Dobersek
Comment 20 2012-11-24 09:15:47 PST
Comment on attachment 175817 [details] Patch Clearing flags on attachment: 175817 Committed r135653: <http://trac.webkit.org/changeset/135653>
Zan Dobersek
Comment 21 2012-11-24 09:15:57 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.