Bug 58625 – new-run-webkit-tests: runs tests in a different order than ORWT

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Dirk Pranke]

It turns out that ORWT has a fancy sort function that is aware of numeric substrings in a test name, so that test-10.html will run after test-2.html.

NRWT needs equal fanciness :)

[Dirk Pranke]

Created attachment 89717 [details]
Patch

[Dirk Pranke]

Comment on attachment 89717 [details]
Patch

Not marking this for review just yet. While I think the patch is ready for review, I may want to leave this bug open as a tracking bug as there may be other issues affecting the test run order, and additional flakiness may ensue on either the mac or chromium bots as we change the order tests are run in.

[Eric Seidel (no email)]

Comment on attachment 89717 [details]
Patch

Nice!

[Dirk Pranke]

moving the numeric compare patch into a sub-bug (bug 58691).

[Dirk Pranke]

Okay, I've worked out all of the issues causing us to run a different list of test in a different order than ORWT, and am filing them as dependent bugs here:


1) ORWT will run all of the tests in a directory before it descends into subdirectories. NRWT sorts things slightly differently to account for the chunking arguments (--run-chunk, --run-part, --batch-size, etc.)

2) ORWT uses "natural sorting" on filenames and path names

3) ORWT, for some reason, will sort directories such that "inspector-enabled" is run before "inspector". I can't really think of a good reason for this.

4) NRWT will sort the shards by the number of tests in the shard. ORWT, since it has no real concept of sharding, just runs them in sort order

5) NRWT doesn't support webarchives, and it also has some outdated logic for feature detection and skipping (e.g., for mathml, xhtmlmp).

6) NRWT can deal with MHT tests, ORWT can't

7) NRWT can run reftests, ORWT will just skip them

8) NRWT runs the http/ tests first, ORWT runs them last

Issues (1), (2), and (3) will be addressed by the latest patch I'll post to bug 58691.

Issue (5) will be addressed by the patch in bug 58293.

I can't decide if it's really worth "fixing" (or at least changing) (4) and (8), but perhaps it makes sense to fix them in the nonsharded code path (what you get when you run in a single thread or in experimental fully parallel).

Presumably we're not going to fix (6) and (7), but that seems wholly unrelated to this bug and I suggest we treat them separately.

[Adam Roben (:aroben)]

(In reply to comment #5)
> 8) NRWT runs the http/ tests first, ORWT runs them last

This seems like an extremely user-visible issue when you have a small number of shards. I'd recommend fixing this one just to give people fewer things to complain about (especially the people who are mostly interested in seeing if "early" tests fail).

[Dirk Pranke]

(In reply to comment #6)
> (In reply to comment #5)
> > 8) NRWT runs the http/ tests first, ORWT runs them last
> 
> This seems like an extremely user-visible issue when you have a small number of shards. I'd recommend fixing this one just to give people fewer things to complain about (especially the people who are mostly interested in seeing if "early" tests fail).

I think this is much less of an issue when you are running multiple shards in parallel, in which I think you will probably still want to run the http tests as soon as possible, since they take so long.

[Ojan Vafai]

I think it makes sense to special-case the single-process case to run the http tests last. If you have >2 shards, not doing them first causes the tests to take longer to run.

[Dirk Pranke]

(In reply to comment #8)
> I think it makes sense to special-case the single-process case to run the http tests last. If you have >2 shards, not doing them first causes the tests to take longer to run.

We could do that, but I don't expect people to run the single-process case very often, so I'm not sure if it's worth it. That said, it's probably a one-line change ...

[Dirk Pranke]

(In reply to comment #9)
> (In reply to comment #8)
> > I think it makes sense to special-case the single-process case to run the http tests last. If you have >2 shards, not doing them first causes the tests to take longer to run.
> 
> We could do that, but I don't expect people to run the single-process case very often, so I'm not sure if it's worth it. That said, it's probably a one-line change ...

Okay, given the changes I've been making so that we can run more than one http test concurrently, this isn't really a one-line change any more, if you want to run the non-http tests without holding the http lock. See the comments I've made in the patch on bug 63112, and I'm unconvinced that it's worth it.

For now, I'm closing this as WONTFIX (and clearing the ownership). If anyone really thinks this needs to be fixed, please feel free to reopen.