New issue
Advanced search Search tips

Issue 843213 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner: ----
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Treat testharness.js test with <meta name=timeout content=long> as slow

Project Member Reported by lukasza@chromium.org, May 15 2018

Issue description

In  issue 842995  a flaky timeout of external/wpt/html/semantics/scripting-1/the-script-element/execution-timing/078.html was reported.  As a short-term solution, we can add this test to third_party/WebKit/LayoutTests/SlowTests.  However - maybe we can streamline this by having the framework *automatically* recognize some tests as slow?  For example, shouldn't the call below be sufficient to mark the test as slow?

    var t = async_test(undefined, { timeout: 10000 })

 
This is the "Timeouts in Tests" feature of WPT (https://web-platform-tests.org/writing-tests/testharness-api.html)

I don't know any (easy) way to change the timeout when running a test in our test runner. And statically parsing the source code seems like a bad idea, either.

A quick grep shows we have 100~200 of these. I feel like this is a feature we should only use sparingly. Perhaps some or many of them can be given a long timeout on the file level? And for the rest, perhaps unfortunately the current workaround is to manually add them to SlowTests or even skip them. 

Comment 2 by foolip@chromium.org, May 16 2018

Status: ExternalDependency (was: Untriaged)
If we wanted to parse this information from the tests and put it in the manifest, we'd need to put it <meta something=something> and change existing tests. But that would be a file-level time out, if any of the individual tests within had a longer timeout that would be nonsensical.

Filed https://github.com/w3c/web-platform-tests/issues/11029 and marking this blocked on that.

Comment 3 by foolip@chromium.org, May 16 2018

Cc: foolip@chromium.org
Owner: ----
Status: Available (was: ExternalDependency)
Summary: Treat testharness.js test with <meta name=timeout content=long> as slow (was: Should |async_test(..., { timeout: <big number> }| automatically increase layout test harness's timeout)
OK, outcome of https://github.com/w3c/web-platform-tests/issues/11029 is that one shouldn't actually use `async_test(undefined, { timeout: 10000 })` and that wptrunner already honors <meta name=timeout content=long>.

In https://chromium-review.googlesource.com/c/chromium/src/+/1061753 I'm trying to see how many tests use the per-test timeout mechanism, and I'm renaming this issue to reflect what a fix should be. Marking it as available, and leaving as P3, which means that nothing will happen unless someone volunteers, or until we have more cases of this being annoying to justify increasing the priority.
run_web_tests already supports this (and I actually just fixed an edge case for this last week). (In case you're curious, we don't parse HTML or JS, but rely on MANIFEST.json to extract the timeout info instead.)

So does this issue track the work of converting explicit per-subtest timeout to file-level meta tags?

Comment 5 by foolip@chromium.org, May 16 2018

Hmm, then I guess there's actually no Blink>Infra work to be done, but just the upstream conversion work? Should I file an upstream issue instead?

Comment 6 by foolip@chromium.org, May 16 2018

Does this also mean that we can add <meta name=timeout content=long> to tests instead of putting them in SlowTests?
Re #6: yes. Both <meta> (in .html) and // meta (in .js) are recognized as slow tests automatically without the need of adding them to SlowTests. i.e. there's no work to be done on the infra side. The remaining task is to convert the existing legacy use of the timeout argument.

Comment 8 by foolip@chromium.org, May 24 2018

Status: Fixed (was: Available)
Upstream issue is https://github.com/w3c/web-platform-tests/issues/11120.

Sign in to add a comment