Closed
Bug 1161615
Opened 6 years ago
Closed 6 years ago
gUM mochitest hang after catching errors
Categories
(Core :: WebRTC, defect)
Core
WebRTC
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: drno, Assigned: drno)
Details
Attachments
(1 file, 1 obsolete file)
As the test_getUserMedia_* tests just pass generateErrorCallback() into the .then() function they only translate errors into test failure, but then never call SimpleTest.finish() and therefore the test hangs.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → drno
Assignee | ||
Comment 1•6 years ago
|
||
Another but probably related problem is: if I pass constraints like {"videox":true, "fake":true} into gUM it appears just to hang in the mochitest. I don't know yet if that is a problem with the mochitests or from the actual implementation.
Assignee | ||
Comment 2•6 years ago
|
||
/r/8273 - Bug 1161615: clean up gUM mochitest to use promises Pull down this commit: hg pull -r 57d7684b548efb4a0b483ad5faf3b5c8fe0a6210 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8602365 -
Flags: review?(martin.thomson)
Attachment #8602365 -
Flags: review?(jib)
Comment 3•6 years ago
|
||
Comment on attachment 8602365 [details] MozReview Request: bz://1161615/drno https://reviewboard.mozilla.org/r/8271/#review7009 I see nothing complicated here. Make sure that you cut runTestWhenReady down, the catch block is atually dead code. However, it does need a new .catch added to the chain there. ::: dom/media/tests/mochitest/mediaStreamPlayback.js:233 (Diff revision 1) > + .then(() => SimpleTest.finish(), generateErrorCallback()); I suggested moving these up to the runTestWhenReady function. ::: dom/media/tests/mochitest/head.js:168 (Diff revision 1) > - } catch (e) { > + } catch (e) { > - ok(false, 'Error executing test: ' + e + > + ok(false, 'Error executing test: ' + e + > - ((typeof e.stack === 'string') ? > + ((typeof e.stack === 'string') ? > - (' ' + e.stack.split('\n').join(' ... ')) : '')); > + (' ' + e.stack.split('\n').join(' ... ')) : '')); > - } > + } You should move the catch block here to a .catch function instead. Since the test function runs inside a .then, it won't throw. Also note that this code is shared with pc.js tests. ::: dom/media/tests/mochitest/test_getUserMedia_constraints.html:88 (Diff revision 1) > - .catch(e => ok(false, "Unexpected failure: " + e.name + ", " + e.message)) > + .catch(e => ok(false, "Unexpected failure: " + e.name + ", " + e.message)); You can drop the catch here if you have a good catch in runTestWhenReady() ::: dom/media/tests/mochitest/test_getUserMedia_peerIdentity.html:31 (Diff revision 1) > - .catch(e => ok(false, 'error in test: ' + e)) > + .catch(e => ok(false, 'error in test: ' + e)); Drop the catch.
Attachment #8602365 -
Flags: review?(martin.thomson) → review+
Assignee | ||
Comment 4•6 years ago
|
||
https://reviewboard.mozilla.org/r/8271/#review7015 > I suggested moving these up to the runTestWhenReady function. I don't think that is so easy, because of the convoluted stuff with B2G network shutdown and steeplechase calling a different finish.
Comment 5•6 years ago
|
||
https://reviewboard.mozilla.org/r/8273/#review7019 ::: dom/media/tests/mochitest/head.js:118 (Diff revision 1) > // are in place before running it. Users of this file need to ensure that they > // also provide a promise called `scriptsReady` as well. Lose comment. ::: dom/media/tests/mochitest/mediaStreamPlayback.js:230 (Diff revision 1) > -function runTest(f) { > - return scriptsReady.then(() => runTestWhenReady(f)); > +function runTest(testFunction) { > + return scriptsReady.then(() => { > + return runTestWhenReady(testFunction) > + .then(() => SimpleTest.finish(), generateErrorCallback()); > + }); Please flatten. How about: var runTest = testFunction => scriptsReady .then(() => runTestWhenReady(testFunction)) .then(() => SimpleTest.finish(), generateErrorCallback()); ::: dom/media/tests/mochitest/head.js:164 (Diff revision 1) > function runTestWhenReady(testFunc) { > setupEnvironment(); > - return Promise.all([scriptsReady, testConfigured]).then(() => { > - try { > + try { > - return testConfigured.then(options => testFunc(options)); > + return testConfigured.then(options => testFunc(options)); > - } catch (e) { > + } catch (e) { > - ok(false, 'Error executing test: ' + e + > + ok(false, 'Error executing test: ' + e + I would change setupEnvironment() to return a promise instead of relying on global variables. setupEnvironment() is called even on steeplechase is it not? And flatten it: return setupEnvironment() .then(testFunc) .catch(e => ok(false, ...)); ::: dom/media/tests/mochitest/test_getUserMedia_basicAudio.html:18 (Diff revision 1) > - getUserMedia(constraints).then(aStream => { > + return getUserMedia(constraints).then(aStream => { > checkMediaStreamTracks(constraints, aStream); s/aStream/stream/ throughout? I thought such notation was frowned on in js...
Updated•6 years ago
|
Attachment #8602365 -
Flags: review?(jib) → review+
Assignee | ||
Comment 6•6 years ago
|
||
https://reviewboard.mozilla.org/r/8273/#review7097 > I would change setupEnvironment() to return a promise instead of relying on global variables. setupEnvironment() is called even on steeplechase is it not? > > And flatten it: > > return setupEnvironment() > .then(testFunc) > .catch(e => ok(false, ...)); This turned out to be more challenging then initially though. setupEnvironment resolves the promise for the regular Mochitest case, but the same promise gets resolve via run_test for the steeplechase case. The real problem is that these two functions take different input and pass it through to the test function. I agree that we should clean this mess up. But I suggest that we do this a part of bug 1020436 (splitting the mochitest chains/queues into two).
Assignee | ||
Comment 7•6 years ago
|
||
Comment on attachment 8602365 [details] MozReview Request: bz://1161615/drno /r/8273 - Bug 1161615: clean up gUM mochitest to use promises Pull down this commit: hg pull -r 3efb1ffdfe8a55dc9501b30c076c9480e840a1e1 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8602365 -
Flags: review?(martin.thomson)
Attachment #8602365 -
Flags: review?(jib)
Attachment #8602365 -
Flags: review+
Assignee | ||
Comment 8•6 years ago
|
||
Comment on attachment 8602365 [details]
MozReview Request: bz://1161615/drno
Carrying forward r+=mt,jib
Attachment #8602365 -
Flags: review?(martin.thomson)
Attachment #8602365 -
Flags: review?(jib)
Attachment #8602365 -
Flags: review+
Comment 9•6 years ago
|
||
Comment on attachment 8602365 [details] MozReview Request: bz://1161615/drno https://reviewboard.mozilla.org/r/8271/#review7121 ::: dom/media/tests/mochitest/mediaStreamPlayback.js:231 (Diff revision 2) > - return scriptsReady.then(() => runTestWhenReady(f)); > -} > + .then(() => runTestWhenReady(testFunction)) > + .then(() => finish()) > + .catch(e => > + ok(false, 'Error in test execution: ' + e + > + ((typeof e.stack === 'string') ? > + (' ' + e.stack.split('\n').join(' ... ')) : ''))); Technically, this is only ever going to surface issues that arise in finish(). You can probably delete it, or at least replace it with something a little less rigorous. Note that calling SimpleText.ok() after SimpleTest.finish() is not a good idea.
Attachment #8602365 -
Flags: review+
Assignee | ||
Comment 10•6 years ago
|
||
Comment on attachment 8602365 [details] MozReview Request: bz://1161615/drno /r/8273 - Bug 1161615: clean up gUM mochitest to use promises Pull down this commit: hg pull -r 0cb8dd30ab4445fcc0622a374449289f357ead1f https://reviewboard-hg.mozilla.org/gecko/
Attachment #8602365 -
Flags: review?(martin.thomson)
Attachment #8602365 -
Flags: review?(jib)
Attachment #8602365 -
Flags: review+
Assignee | ||
Comment 11•6 years ago
|
||
Comment on attachment 8602365 [details]
MozReview Request: bz://1161615/drno
Implemented the fix mt suggested.
Carrying forward r+=mt,jib
Attachment #8602365 -
Flags: review?(martin.thomson)
Attachment #8602365 -
Flags: review?(jib)
Attachment #8602365 -
Flags: review+
Assignee | ||
Comment 12•6 years ago
|
||
Comment on attachment 8602365 [details] MozReview Request: bz://1161615/drno /r/8273 - Bug 1161615: clean up gUM mochitest to use promises Pull down this commit: hg pull -r 15e3b5747e0f5b461944d0b43ab43cda11207f3b https://reviewboard-hg.mozilla.org/gecko/
Attachment #8602365 -
Flags: review?(martin.thomson)
Attachment #8602365 -
Flags: review?(jib)
Attachment #8602365 -
Flags: review+
Assignee | ||
Comment 13•6 years ago
|
||
Comment on attachment 8602365 [details]
MozReview Request: bz://1161615/drno
Fixed the test_getUserMedia_callbacks test case.
Carrying forward r+=mt,jib
Attachment #8602365 -
Flags: review?(martin.thomson)
Attachment #8602365 -
Flags: review?(jib)
Attachment #8602365 -
Flags: review+
Assignee | ||
Comment 14•6 years ago
|
||
Comment on attachment 8602365 [details] MozReview Request: bz://1161615/drno /r/8273 - Bug 1161615: clean up gUM mochitest to use promises Pull down this commit: hg pull -r e181129a0dc0a6df21846f067e31401e93972a67 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8602365 -
Flags: review?(martin.thomson)
Attachment #8602365 -
Flags: review?(jib)
Attachment #8602365 -
Flags: review+
Assignee | ||
Comment 15•6 years ago
|
||
Comment on attachment 8602365 [details]
MozReview Request: bz://1161615/drno
Fixed more test failures on XP and 10.6.
Carrying forward r+=mt,jib
Attachment #8602365 -
Flags: review?(martin.thomson)
Attachment #8602365 -
Flags: review?(jib)
Attachment #8602365 -
Flags: review+
Comment 16•6 years ago
|
||
We are going to have to do something about these spurious review requests.
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 17•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4a454c2b1c3
Keywords: checkin-needed
Comment 18•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c4a454c2b1c3
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Comment 19•6 years ago
|
||
Attachment #8602365 -
Attachment is obsolete: true
Attachment #8620233 -
Flags: review+
Assignee | ||
Comment 20•6 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•