Closed Bug 1352463 Opened 7 years ago Closed 7 years ago

Increase default timeout of wdspec WPT tests

Categories

(Remote Protocol :: Marionette, enhancement)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: ato, Assigned: ato)

Details

Attachments

(2 files)

Bug 1352463 - Define WPT timeouts on the test type level;
59 bytes, text/x-review-board-request
jgraham
: review+
Details
Bug 1352463 - Increase wdspec timeouts;
59 bytes, text/x-review-board-request
jgraham
: review+
Details 
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.
Assignee: nobody → ato
> 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?
(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).
Attachment #8853459 - Flags: review?(james)
Attachment #8853460 - Flags: review?(james)
Status: NEW → ASSIGNED
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?
Attachment #8853459 - Flags: review?(james) → review-
Comment on attachment 8853460 [details]
Bug 1352463 - Increase wdspec timeouts;

https://reviewboard.mozilla.org/r/125550/#review130320
Attachment #8853460 - Flags: review?(james) → review+
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 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.
Attachment #8853459 - Flags: review?(james) → review-
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 on attachment 8853459 [details]
Bug 1352463 - Define WPT timeouts on the test type level;

https://reviewboard.mozilla.org/r/125548/#review131466
Attachment #8853459 - Flags: review?(james) → review+
Judging by the try run, I clearly broke something else with regards to reftests (Wr).  I will investigate further before pushing this.
Pushed by atolfsen@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/59b7df82a6fc
https://hg.mozilla.org/mozilla-central/rev/8f81e8c98217
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: