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)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla55
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 | ||
Updated•7 years ago
|
Assignee: nobody → jwwang
Priority: -- → P3
Comment 1•7 years ago
|
||
Thanks for opening this bug
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8842764 -
Flags: review?(jyavenard)
Attachment #8842765 -
Flags: review?(jyavenard)
Comment 4•7 years ago
|
||
mozreview-review |
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 5•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
Thanks for the review!
Comment 10•7 years ago
|
||
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
Comment 11•7 years ago
|
||
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)
Comment 12•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
mozreview-review |
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+
Comment 18•7 years ago
|
||
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
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bb160269c997 https://hg.mozilla.org/mozilla-central/rev/64fcb6bf7295 https://hg.mozilla.org/mozilla-central/rev/3fbab2a560ce
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 20•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/db731ff417a5 https://hg.mozilla.org/releases/mozilla-aurora/rev/3e0ce1278945 https://hg.mozilla.org/releases/mozilla-aurora/rev/c6633693bf86
status-firefox54:
--- → fixed
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•