-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove timeout in async_test for html tests #14087
Remove timeout in async_test for html tests #14087
Conversation
Travis has failed because "No output has been received in the last 10m0s". However, wpt-chrome-dev-stability and wpt-firefox-nightly-stability on Taskcluster passed, so at least the tests aren't flaky. I do wonder if any of them are slow though. Looking at https://queue.taskcluster.net/v1/task/Vw3E0As5R1W9TebRCNcLxQ/runs/0/artifacts/public/results/wpt_report.json.gz I see that /html/browsers/browsing-the-web/navigating-across-documents/010.html took 15 seconds. @qiuzhong, can you check if any other tests are made slow, and was that test in particular made slower with this change? |
I saw those "no output" errors on several of the PRs removing the test-specific timeout values. |
It happens more now because we filtered out a lot of the process output from Firefox in order to avoid overflowing the logs, but the flip side is we experience more timeouts. If we wanted to fix it we'd presumably have to arrange for a periodic ping to be logged. |
With #14096 merged it should be possible to just rebase this or otherwise retrigger checks. |
Remove all the timeout in `async_test` for html tests. Affected tests: 125 Before: Pass: 64, Failed: 61 After: Pass: 64, Failed: 61 Related: web-platform-tests#11120
19e5d31
to
cb656d1
Compare
Found lots of errors like this:
|
@@ -6,7 +6,7 @@ | |||
<pre id="step_log"></pre> | |||
<iframe id="test"></iframe> | |||
<script> | |||
var t = async_test(undefined, {timeout:10000}); | |||
var t = async_test(undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also remove the undefined arguments? They were only there to make it possible to pass in the second argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I'll remove all of them.
After remove the timeout parameter from async_test, the first parameter `undefined` is no use and should be removed.
Thanks @qiuzhong, one down, a few more PRs to go :) |
https://tools.taskcluster.net/groups/DWgMz-OFQOKp5iktSe1gSg unfortunately failed so a clean before/after diff view for this PR isn't possible. |
Filed #14207 for the fact that Taskcluster is failing. Once there's another working run I'll compare, but then more changes will be lumped in. |
Ah, but iframe_sandbox_popups_escaping-1.html wasn't modified, so that's probably a flaky test. |
Remove all the timeout in
async_test
for html tests.Affected tests: 125
Before: Pass: 64, Failed: 61
After: Pass: 64, Failed: 61
Related: #11120