Wpt: When many tests are affected, CI stability jobs will time out

[foolip]

In https://github.com/w3c/web-platform-tests/pull/7654 I changed 778 files, and as a result all of the stability jobs failed, with messages like "No output has been received in the last 10m0s".

This is similar to https://github.com/w3c/web-platform-tests/issues/7073.

[jgraham]

There are two issues here. One is that we should have a periodic keepalive message when the tests are still running but not producing output. The other is that if we get no output for 10 minutes we're pretty much guaranteed to hit the hard timout (since that implies a minimum runtime of 100 minutes which is about twice the threshold).

[foolip]

Changed the title a bit because in https://github.com/w3c/web-platform-tests/pull/9641 not that many files were changed, but many included the changed file, and that's what matters. Also, not all browsers necessarily fail.

[gsnedders]

Does it make sense to run with --no-restart-on-unexpected in CI?

[foolip]

@jgraham, what kinds of things would you expect to blow up if we do that? And would that still restart between each of the 10 runs?

[jgraham]

Nothing should blow up; the default assumes that if you have one failing test then you don't want it to infect subsequent tests if it happens to set some bad state. In this case what you might see is that one unstable test would be more likely to make subsequent tests on the run look unstable. It won't affect restarting between runs at all.

An alternate apporach would be to set the expectation data after the first run and use that to avoid restarting on subequent runs unless something actually is unstable. At the moment the way we update the metadata is really slow, so that will itself have a performance impact.

[foolip]

@jgraham, as you can see, this is quite a common occurrence, requiring regular manual intervention, and our current setup isn't robust enough to build https://github.com/w3c/web-platform-tests/issues/7475 on top of. Are you open to revisiting the idea of running fewer iterations if we know we're going to exceed the timeout?

[jgraham]

So I'm not opposed to that, but it is a limitation of the current system that we should look to address in a better way.

But this issue seems to be about the "10m without output" problem which is different to the entire run timing out and requires a different solution.

[foolip]

I think most of the PRs I've linked to here are were actual timeouts close to 50 minutes, at least https://travis-ci.org/w3c/web-platform-tests/jobs/352958109 ended with just "The job exceeded the maximum time limit for jobs, and has been terminated".

Some of the timeouts have probably been to verbose logging (now fixed) and maybe I've misattributed some other things, but plain hangs that result in no output for 10 minutes isn't most of the problem, I think.

As for a better way, ultimately I think there are cases where it's just not a good use of resources to even try to run all affected tests 10 times, like if testharness.js has changed or if many tests are affected in some trivial way like in https://github.com/w3c/web-platform-tests/pull/9718.

No matter how much Travis capacity we have or if we are able to reuse the wpt.fyi running infra, I think it makes sense to put a cap on resource usage per PR and just do the most useful things that can be done within those constraints. It should be possible to run all tests once so that we can actually make testharness.js changes with confidence, and maybe twice to do with/without changes, but I wouldn't want to allocate more resources than that per PR.

I think the next step is to just buy more Travis capacity which will increase the timeout to 120 minutes. @plehegar is looking into that.

[foolip]

I think the next step for this issue is to fix https://github.com/w3c/web-platform-tests/issues/10269. Does that sound right, @kereliuk?

[gsnedders]

I think there's a fair bit of complexity here, as even with --stability or --verify, ultimately the time is almost all spent within the wptrunner execution code (as ultimately wptrunner is just called with some instruction to "run these tests in this mode"), hence we'd need some way to pass some deadline relatively deep into wptrunner where it actually runs tests.

If we want to modify the number of iterations based on current run time (or maybe just a target time per test), we need to know how long each individual test took to run (and if we're not restarting the browser between them then we need to monitor that somewhere), etc.

[foolip]

Does it not suffice to pass down a deadline in terms of the system clock, and consult that?

[jgraham]

I think the point is that a lot of the looping happens at a fairly low level, so how you express the deadline isn't as important as the fact that you have to pass that down through the layers and work out if the next action you take will cause it to be exceeded.

It's probably (somewhat) simpler for the current setup on travis because we don't do any loops without restarts. But actually we probably should since that's relatively cheap and likely finds a lot of problems with state.

[foolip]

@kereliuk @gsnedders now that https://github.com/w3c/web-platform-tests/issues/9874 is done, what's left to resolve this issue?

[gsnedders]

@foolip Implement, all the way down to the test execution loop, some way of giving a target time or something similar.

[foolip]

@jugglinmike, I'll go ahead and assign this to you, as simply taking these jobs off of Travis will resolve this issue in Q4.

[KyleJu]

Updating tests for Window onerror is not triggered bug fix. #13666

[foolip]

@KyleJu, thanks for linking instances of this to this issue. The PR will have to be admin merged, do you want me to do that, or is there something to investigate?

[KyleJu]

@foolip I think its a timeout issue and good to be admin merged.

[foolip]

Finally, after tons of affected PRs and much frustration, stability checks on Travis are no longer blocking, see https://github.com/web-platform-tests/wpt/pull/14096. Thanks @jgraham and @jugglinmike for working on the Taskcluster setup!

[KyleJu]

15133 chatted with the author and test failures are unrelated to the changes in the PR. Maybe Admin merged ?

[foolip]

@KyleJu did you comment on the wrong issue, or did this come up in rotation?

[KyleJu]

@foolip oops this came up in rotation but I filed a bug separately #15176 (commented on the wrong issue)

[KyleJu]

16219 caused by test timeout on Firefox Nightly. @Hexcles please admin merge

[KyleJu]

16596 aused by test timeout on Firefox Nightly. @Hexcles please admin merge

[KyleJu]

16653 caused by test timeout on Firefox Nightly. @Hexcles please admin merge

[foolip]

Reopening because this is still happening on Taskcluster, and we still need to admin merge, pointing back to this issue.

https://github.com/web-platform-tests/wpt/issues/20052 is an instance of this that I came across today.

[foolip]

@stephenmcgruer can you retriage this for priority? https://github.com/web-platform-tests/wpt/issues/7660#issuecomment-373023089 is the only idea I have for fixing this.

[stephenmcgruer]

I wrote a 'brief' document that summarizes the (I believe) generally agreed upon solution: https://docs.google.com/document/d/1dAlCSHUQldtgWDDTrGJR-ksm19FZZ3k8ppqc5-kSwIk/edit# . It's world-readable, comment-able by @chromiums, and I'm happy to give comment access to anyone else who wants it (just can't make it world comment-able due to spam).

[stephenmcgruer]

Update: unfortunately there was disagreement on the proposed approach for avoiding CI timeout and I haven't had the time/energy to drive it to a successful conclusion. I still hope to address this, hopefully sometime in Q3 once the Taskcluster Checks migration is comfortably landed.

[gsnedders]

FWIW, my view is still the ideal solution is to do something in the decision task that chooses to shard based on number of files changed (I don't think we have tests affected in the decision task?)

[Hexcles]

Well, even if we do that, we still have widely used resource files that affect way too many tests (such that we can't afford to run 10x).

[stephenmcgruer]

So we're in a better place to do this now, since we switched to repeat-only stability checks for the CI. We can check for an impending timeout between each iteration of the tests run (using previous runs to estimate running time), and bail if we're over our time limit.

Now just needs me (or someone) to put the time in to create a PR ;)