1161615 - gUM mochitest hang after catching errors

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Nils Ohlmeier [:drno]]

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.

Comment 1

[Nils Ohlmeier [:drno]]

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.

Comment 2

[Nils Ohlmeier [:drno]]

Attachment: MozReview Request: bz://1161615/drno

/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/

Comment 3

[Martin Thomson [:mt:]]

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.

Comment 4

[Nils Ohlmeier [:drno]]

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

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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...

Comment 6

[Nils Ohlmeier [:drno]]

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).

Comment 7

[Nils Ohlmeier [:drno]]

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/

Comment 8

[Nils Ohlmeier [:drno]]

Comment on attachment 8602365 [details]
MozReview Request: bz://1161615/drno

Carrying forward r+=mt,jib

Comment 9

[Martin Thomson [:mt:]]

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.

Comment 10

[Nils Ohlmeier [:drno]]

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/

Comment 11

[Nils Ohlmeier [:drno]]

Comment on attachment 8602365 [details]
MozReview Request: bz://1161615/drno

Implemented the fix mt suggested.

Carrying forward r+=mt,jib

Comment 12

[Nils Ohlmeier [:drno]]

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/

Comment 13

[Nils Ohlmeier [:drno]]

Comment on attachment 8602365 [details]
MozReview Request: bz://1161615/drno

Fixed the test_getUserMedia_callbacks test case.

Carrying forward r+=mt,jib

Comment 14

[Nils Ohlmeier [:drno]]

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/

Comment 15

[Nils Ohlmeier [:drno]]

Comment on attachment 8602365 [details]
MozReview Request: bz://1161615/drno

Fixed more test failures on XP and 10.6.

Carrying forward r+=mt,jib

Comment 16

[Martin Thomson [:mt:]]

We are going to have to do something about these spurious review requests.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4a454c2b1c3

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c4a454c2b1c3

Comment 20

[Nils Ohlmeier [:drno]]

Attachment: MozReview Request: Bug 1161615: clean up gUM mochitest to use promises