Closed Bug 1343749 Opened 7 years ago Closed 7 years ago

Let MediaTestManager manage timeout of each test

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(3 files)

See bug 1313548 comment 39 for the motivation.

As we add more tests to run by the MediaTestManager, it is more likely for the total execution time to exceed the timeout set by the mochitest runner (which defaults to 5 mins).

One solution is to use SimpleTest.requestLongerTimeout() to request a greater timeout which however adds the maintenance effort and doesn't scale very well.  Moreover, it is often not obvious that a mochitest times out because there are simply too many tests to run by the MediaTestManager.

So the proposal here is to let MediaTestManager manage timeout of each test without relying on the global timeout set by the test runner. There are several benefits in doing this:

1. MediaTestManager won't get stuck on a single test timeout and sit idle until the whole mochitest times out.
2. MediaTestManager can force quit the timed out test and continue others to catch as many timeouts as possible in a single mochitest run.
3. MediaTestManager can provide useful debugging logs to indicate which test times out.
Assignee: nobody → jwwang
Priority: -- → P3
Thanks for opening this bug
Attachment #8842764 - Flags: review?(jyavenard)
Attachment #8842765 - Flags: review?(jyavenard)
Comment on attachment 8842764 [details]
Bug 1343749. Part 1 - Let MediaTestManager manage timeout of each test.

https://reviewboard.mozilla.org/r/116540/#review118242

::: dom/media/test/manifest.js:1675
(Diff revision 1)
> +    var onTimeout = function() {
> +      ok(false, `${token} timed out!`);
> +      this.finished(token);
> +    }.bind(this);
> +    // Default timeout to 30s for each test.
> +    this.timers[token] = setTimeout(onTimeout, 30000);

this is likely going to be too small...

Especially on android and simulator...
Attachment #8842764 - Flags: review?(jyavenard) → review+
Comment on attachment 8842765 [details]
Bug 1343749. Part 2 - remove the calls to SimpleTest.requestLongerTimeout() when MediaTestManager is used.

https://reviewboard.mozilla.org/r/116542/#review118244
Attachment #8842765 - Flags: review?(jyavenard) → review+
Comment on attachment 8842764 [details]
Bug 1343749. Part 1 - Let MediaTestManager manage timeout of each test.

https://reviewboard.mozilla.org/r/116540/#review118242

> this is likely going to be too small...
> 
> Especially on android and simulator...

Will default to 3mins instead.
Thanks for the review!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9c1478a17bf6
Part 1 - Let MediaTestManager manage timeout of each test. r=jya
https://hg.mozilla.org/integration/autoland/rev/04d6b4518dbf
Part 2 - remove the calls to SimpleTest.requestLongerTimeout() when MediaTestManager is used. r=jya
Blocks: 1344105
sorry had to backout for frequent failures in mda tests like https://treeherder.mozilla.org/logviewer.html#?job_id=81339000&repo=autoland
Flags: needinfo?(jwwang)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/77f475062c1d
Backed out changeset 04d6b4518dbf 
https://hg.mozilla.org/integration/autoland/rev/a4fedf44e1db
Backed out changeset 9c1478a17bf6 for frequent timeouts in mda tests
Comment on attachment 8843529 [details]
Bug 1343749. Part 3 - SimpleTest.js must be loaded before manifest.js which depends on SimpleTest.

https://reviewboard.mozilla.org/r/117232/#review119304

lgtm.
Attachment #8843529 - Flags: review?(jib) → review+
Thanks!
Flags: needinfo?(jwwang)
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bb160269c997
Part 1 - Let MediaTestManager manage timeout of each test. r=jya
https://hg.mozilla.org/integration/autoland/rev/64fcb6bf7295
Part 2 - remove the calls to SimpleTest.requestLongerTimeout() when MediaTestManager is used. r=jya
https://hg.mozilla.org/integration/autoland/rev/3fbab2a560ce
Part 3 - SimpleTest.js must be loaded before manifest.js which depends on SimpleTest. r=jib
You need to log in before you can comment on or make changes to this bug.