[NRWT] make nrwt able to test the same order like orwt

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Kristóf Kosztyó]

Created attachment 99045 [details]
WIP patch

It can be useful to get the same failed tests list.

[Peter Gal]

(In reply to comment #0)
> Created an attachment (id=99045) [details]


> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:566
> +            self._tests_with_http_lock = []

This is only used in 2 places (including this one). Will this be used later?

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:570
> +            for test_file in test_files:

you can ditch the 'i' variable from above if you use 'enumerate' in the for. Like this:
 for i, test_file in enumerate(test_files):

This way you don't need to manually increment the 'i' at the end.

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:575
> +                test_by_thousand.setdefault(j, [])

I think a better place for this would be in the 'if' case were you increment the 'j' variable,
there you can simply do the following:  test_by_thousand[j] = []

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:585
> +                test_lists.append(test_list_tuple)

Could you try this?:

test_lists = [(tests, test_list) for tests, test_list in test_by_thousand.iteritems()]

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:1388
> +            if re.search('LayoutTests.http', test):

For this a simple: 'LayoutTests.http' in test  is enough no need for the re.

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:1390
> +            elif re.search('LayoutTests.websocket', test):

ditto.

> Tools/Scripts/webkitpy/layout_tests/layout_package/worker.py:128
> +                    self.start_servers_with_lock()

I'm not sure but here you wanted to check if the 'list_name' is in the 'tests_with_http_lock' list?
If that's the case, then you should do it like this:

 if list_name in tests_with_http_lock:
     self.start_servers_with_lock()

[Kristóf Kosztyó]

Created attachment 99262 [details]
WIP patch

[Peter Gal]

(In reply to comment #2)
> Created an attachment (id=99262) [details]
> WIP patch

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:566
> +            i = 1

I think this is obsolete now, because of the enumerate in the for loop.

> Tools/Scripts/webkitpy/layout_tests/layout_package/worker.py:123
> +        elif self._options.use_orwt_order and 'http_lock' in str(list_name):

list_name is already string, or not? If so then you can ditch the str call.

[Kristóf Kosztyó]

Created attachment 99614 [details]
WIP patch

It is better?
The list_name was number, except when it contained the 'http_lock', so it needed to use the str()

[Peter Gal]

(In reply to comment #4)
> The list_name was number, except when it contained the 'http_lock', so it needed to use the str()
Oh I see, then you definitely need the str() somewhere.



> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:580
> +            test_lists = [('http_lock' + tests if tests in tests_with_http_lock else tests,

Try this here:
 'http_lock%d' % tests if tests in tests_with_http_lock else str(tests)

then you don't need those ugly str()-s above

[Kristóf Kosztyó]

Created attachment 99702 [details]
proposed fix

[Eric Seidel (no email)]

Comment on attachment 99702 [details]
proposed fix

We should just make it always use an ORWT compatible order.

[Kristóf Kosztyó]

Created attachment 99970 [details]
proposed fix

[Kristóf Kosztyó]

Created attachment 99971 [details]
proposed fix

[WebKit Review Bot]

Attachment 99970 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1

Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:665:  indentation is not a multiple of four  [pep8/E111] [5]
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:667:  indentation is not a multiple of four  [pep8/E111] [5]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dirk Pranke]

Comment on attachment 99971 [details]
proposed fix

This patch needs a test added to manager_unittest.py. I also need to stare at this a bit more, because I don't yet understand what it's doing (and I just spent a fair amount of time understanding how ORWT did things :).

[Dirk Pranke]

Comment on attachment 99971 [details]
proposed fix

Okay, I think I get it now. This patch is trying to do two things ...

1) Move the HTTP and websocket tests to the end

2) Create shards of tests, 1000 at a time.

It is true that ORWT does (1). 

I think (2) is wholly new, correct? ORWT didn't have a "sharding" concept, and sharding simply by a fixed number probably won't work all that well with intra-directory dependencies (you really want shard-by-directory instead, or possibly fully_parallel if you don't really care about how things are sharded). Note that the sharding is different from just running subsets of the tests. There are other flags that control that, prior to creating the shards.

In addition, one problem with moving the http tests to the end is that there no good way to coordinate this with the http locking code that we use to ensure that we don't have two NRWT processes running at once that are trying to start and stop web servers. Another problem is that the HTTP tests are the slowest tests, so you really want to start them right away when running multiple processes, to minimize the overall delay time. 

See bug 58625 for a fuller discussion of this. It's possible that we can fix the coordination of locking and unlocking and still put the http tests at the end, but that work will be nontrivial and, like I said, I don't think you want to put the tests at the end with multiple processes anyway (so if anything, I would suggest you add a patch that puts the http tests at the end only if in single-process mode, and decide that you don't care or need the concurrency).

If after all I've written you don't think NRWT will do what you want, maybe you can describe what the test conditions you are trying to replicate are and we can see what might make the most sense?

[Adam Barth]

Comment on attachment 99971 [details]
proposed fix

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

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:385
> +        optparse.make_option("--orwt-order",  dest="orwt_order",
> +            action="store_true", default=True,
> +            help="Run tests in ORWT's order (default)"),
> +        optparse.make_option("--no-orwt-order",  dest="orwt_order",
> +            action="store_false", help="Don't run tests in ORWT's order"),

We shouldn't be adding options like this.  ORWT won't mean anything in a year.

[Adam Barth]

This bug doesn't contain any explanation for why we'd want to do this.  What problem are we solving?

[Andras Becsi]

(In reply to comment #12)
> (From update of attachment 99971 [details])
> Okay, I think I get it now. This patch is trying to do two things ...
> 
> 1) Move the HTTP and websocket tests to the end
> 
> 2) Create shards of tests, 1000 at a time.
> 
> It is true that ORWT does (1). 
> 
> I think (2) is wholly new, correct? ORWT didn't have a "sharding" concept

While it is true that ORWT didn't have the concept of sharding, it did restart DumpRenderTree after every 1000 test. This behaviour made ORWT quite deterministic that is why we initiated the work on this.
The transition to NRWT however showed that using NRWT with --child-processes=1 the result is almost the same (except some minor flakyness) as with ORWT, so this feature is not that important as it was evaluated at first.
Although the mutual exclusion of tests which need httpd is important for the Qt bots since our concept with ORWT was to run multiple bots in parallel on the same server without using a vm or a chroot.
I do not fully understand the concept of shards regarding the http tests yet, but if BUG64092 gets fixed I think this bug really does not have any particular purpose.

[Dirk Pranke]

(In reply to comment #15)
> (In reply to comment #12)
> > (From update of attachment 99971 [details] [details])
> > Okay, I think I get it now. This patch is trying to do two things ...
> > 
> > 1) Move the HTTP and websocket tests to the end
> > 
> > 2) Create shards of tests, 1000 at a time.
> > 
> > It is true that ORWT does (1). 
> > 
> > I think (2) is wholly new, correct? ORWT didn't have a "sharding" concept
> 
> While it is true that ORWT didn't have the concept of sharding, it did restart DumpRenderTree after every 1000 test. This behaviour made ORWT quite deterministic that is why we initiated the work on this.

Ah, the --nthly flag. Yeah, we should probably add that to NRWT.

> The transition to NRWT however showed that using NRWT with --child-processes=1 the result is almost the same (except some minor flakyness) as with ORWT, so this feature is not that important as it was evaluated at first.
> Although the mutual exclusion of tests which need httpd is important for the Qt bots since our concept with ORWT was to run multiple bots in parallel on the same server without using a vm or a chroot.
> I do not fully understand the concept of shards regarding the http tests yet, but if BUG64092 gets fixed I think this bug really does not have any particular purpose.

Glad to hear it. That fix should land shortly :).

Shards are the units of parallelism - how work is divided up between multiple workers. Tests in a shard are run consecutively by the same worker, but different shards can be run by different workers (either serially or concurrently). Ideally every test would be its own shard, but some of our tests are broken and have inter-test dependencies, so we use the sharding concept to get around this.