1352463 - Increase default timeout of wdspec WPT tests

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement)

Description

[Andreas Tolfsen ❲:ato❳]

The default timeout of WPT tests is 10 seconds (https://github.com/w3c/wptrunner/blob/master/wptrunner/wpttest.py#L5) and much too low for wdspec tests.  WebDriver-based tests are expensive and a saner timeout might be closer to 25 seconds if we structure tests sensibly.

Perhaps in the future we should allow the timeout to be configurable on a per-test or per-file level.  This is already possible to some extent with a meta tag in the file, but it is conceivable letting individual test functions set timeouts through pytest plugins such as pytest-timeout.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1352463 - Define WPT timeouts on the test type level;

Certain test types have a need for other defaults than the
wpttest.DEFAULT_TIMEOUT and wpttest.LONG_TIMEOUT values.  This patch
changes wptrunner to define default- and long timeouts on a test type
level.  This allows a test type to override the default durations defined
in the abstract Test.default_timeout and Test.long_timeout.

Concrete classes, such as ReftestTest and WdspecTest, may override these
class properties.

Review commit: https://reviewboard.mozilla.org/r/125548/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125548/

Comment 2

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1352463 - Increase wdspec timeouts;

Increases WPT wdspec timeouts to more realistic values.  Because wdspec
tests interact with the browser from an OOP program, they require more
time to run.  Interactive browser tests are also known for generally
being more expensive to run.

25 seconds for the default timeout and 120 seconds for the long timeout
are values picked out of the air and likely needs to be further refined
in the future.  It is however the current belief that this moves us in
the right direction.

Further improvements to this approach may involve letting wdspec tests
define timeouts on a per-file or a per-test function level through the
use of pytest-timeouts, but this is purely speculative at this point.
It is the current recommendation to adjust the number of tests and the
runtime duration of the tests in a file according to these new defaults.

Review commit: https://reviewboard.mozilla.org/r/125550/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125550/

Comment 3

[James Graham [:jgraham]]

> Perhaps in the future we should allow the timeout to be configurable on a per-test or per-file level

This is explicitly an anti-goal. We used to have this in testharness.js and it just caused problems. In general you can't know how fast tests will run on some random hardware, which is why we have the timeout multiplier setup.

It's also desirable that tests don't take too long to run as long-running tests are themselves problematic. What problem is this patch solving that can't be solved by splitting the tests across more files?

Comment 4

[Andreas Tolfsen ❲:ato❳]

(In reply to James Graham [:jgraham] from comment #3)
> > Perhaps in the future we should allow the timeout to be configurable on a per-test or per-file level
> 
> This is explicitly an anti-goal. We used to have this in testharness.js and
> it just caused problems. In general you can't know how fast tests will run
> on some random hardware, which is why we have the timeout multiplier setup.

This is true, but the timeout multiplier could be exposed for pytest to take into consideration.

But generally speaking I am also quite sceptical that timeouts defined on a per-test or per-file level would be a good idea.
 
> It's also desirable that tests don't take too long to run as long-running
> tests are themselves problematic. What problem is this patch solving that
> can't be solved by splitting the tests across more files?

WebDriver tests are inherently slower than testharness.js tests, and we ought to take this into consideration when defining the default timeouts for the test type.

The current timeouts cause almost every wdspec test to use the long timeout, and even that in some cases such as the action tests, are not enough.  For example, just adding a few more tests to either contexts.py or navigation.py will cause them to hit this limit, and the timeout is therefore causing tests to have to be split on more files for wdspec, than they have to be for other test types.

Much of the time spent in wdspec tests is spent starting and stopping the browser instance, and not doing the actual tests (action durations is a notable exception).

Comment 5

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8853459 [details]
Bug 1352463 - Define WPT timeouts on the test type level;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125548/diff/1-2/

Comment 6

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8853460 [details]
Bug 1352463 - Increase wdspec timeouts;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125550/diff/1-2/

Comment 7

[James Graham [:jgraham]]

Comment on attachment 8853459 [details]
Bug 1352463 - Define WPT timeouts on the test type level;

https://reviewboard.mozilla.org/r/125548/#review130318

::: testing/web-platform/harness/wptrunner/wpttest.py
(Diff revision 2)
>                        inherit_metadata,
>                        test_metadata,
>                        nodes=None,
>                        references_seen=None):
> -
> -        timeout = LONG_TIMEOUT if manifest_test.timeout == "long" else DEFAULT_TIMEOUT

Why did this get removed?

Comment 8

[James Graham [:jgraham]]

Comment on attachment 8853460 [details]
Bug 1352463 - Increase wdspec timeouts;

https://reviewboard.mozilla.org/r/125550/#review130320

Comment 9

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8853459 [details]
Bug 1352463 - Define WPT timeouts on the test type level;

https://reviewboard.mozilla.org/r/125548/#review130318

> Why did this get removed?

Sorry, my mistake.

Comment 10

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8853459 [details]
Bug 1352463 - Define WPT timeouts on the test type level;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125548/diff/2-3/

Comment 11

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8853460 [details]
Bug 1352463 - Increase wdspec timeouts;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125550/diff/2-3/

Comment 12

[James Graham [:jgraham]]

Comment on attachment 8853459 [details]
Bug 1352463 - Define WPT timeouts on the test type level;

https://reviewboard.mozilla.org/r/125548/#review131032

::: testing/web-platform/harness/wptrunner/wpttest.py
(Diff revision 3)
>          node = cls(manifest_test.source_file.tests_root,
>                     manifest_test.url,
>                     inherit_metadata,
>                     test_metadata,
>                     [],
> -                   timeout=timeout,

You  need to put the argument back here too.

Comment 13

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8853459 [details]
Bug 1352463 - Define WPT timeouts on the test type level;

https://reviewboard.mozilla.org/r/125548/#review131032

> You  need to put the argument back here too.

Oops.  It’s good you caught this.

Comment 14

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8853459 [details]
Bug 1352463 - Define WPT timeouts on the test type level;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125548/diff/3-4/

Comment 15

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8853460 [details]
Bug 1352463 - Increase wdspec timeouts;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125550/diff/3-4/

Comment 16

[James Graham [:jgraham]]

Comment on attachment 8853459 [details]
Bug 1352463 - Define WPT timeouts on the test type level;

https://reviewboard.mozilla.org/r/125548/#review131466

Comment 17

[Andreas Tolfsen ❲:ato❳]

Judging by the try run, I clearly broke something else with regards to reftests (Wr).  I will investigate further before pushing this.

Comment 18

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8853459 [details]
Bug 1352463 - Define WPT timeouts on the test type level;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125548/diff/4-5/

Comment 19

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8853460 [details]
Bug 1352463 - Increase wdspec timeouts;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125550/diff/4-5/

Comment 20

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8853459 [details]
Bug 1352463 - Define WPT timeouts on the test type level;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125548/diff/5-6/

Comment 21

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8853460 [details]
Bug 1352463 - Increase wdspec timeouts;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125550/diff/5-6/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59b7df82a6fc
Define WPT timeouts on the test type level; r=jgraham
https://hg.mozilla.org/integration/autoland/rev/8f81e8c98217
Increase wdspec timeouts; r=jgraham

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/59b7df82a6fc
https://hg.mozilla.org/mozilla-central/rev/8f81e8c98217