1193045 - Wait for mochitest test step results and add extra safety check for 'selected' attribute

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Nils Ohlmeier [:drno]]

Currently our mochitests only pay attention to the 'selected' attribute in the RTCP stats if a TURN server is provisioned. And even then they silently exit if nothing is selected.
We should probably change this:
1) error out if nothing is selected
2) error out if more then one thing is select
3) verify the connection type also for non-relayed calls?

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: MozReview Request: Bug 1193045: check selected attribute for all calls. r=bwc

Bug 1193045: check selected attribute for all calls. r?bwc

Comment 2

[Byron Campen [:bwc]]

Comment on attachment 8646611 [details]
MozReview Request: Bug 1193045: check selected attribute for all calls. r=bwc

https://reviewboard.mozilla.org/r/15807/#review14093

::: dom/media/tests/mochitest/pc.js:1715
(Diff revision 1)
> -    info("checkStatsIceConnectionType verifying: local=" +
> -         JSON.stringify(stats[lId]) + " remote=" + JSON.stringify(stats[rId]));
> +    ok(typeof lId !== 'undefined', "local candidatepair selected " +
> +       JSON.stringify(lId));
> +    ok(typeof rId !== 'undefined', "remote candidatepair selected " +
> +       JSON.stringify(rId));

This message is pretty cryptic. Maybe "Got local/remote candidate id for selected pair."

::: dom/media/tests/mochitest/pc.js:1719
(Diff revision 1)
>      if ((typeof stats[lId] === 'undefined') ||
>          (typeof stats[rId] === 'undefined')) {

I guess this would fail if nothing was selected, wouldn't it? If so, we've probably never observed this bug in CI.

::: dom/media/tests/mochitest/templates.js:464
(Diff revision 1)
> -    test.pcRemote.getStats().then(stats => {
> +    return test.pcRemote.getStats().then(stats => {

Wow, does this mean we've never waited for the getStats to finish before proceeding with the test?

Comment 3

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/15807/#review14093

> I guess this would fail if nothing was selected, wouldn't it? If so, we've probably never observed this bug in CI.

Yes exactly. So far this only had an info message. So CI never pointed it out to us if it was missing. Lets see what the try run will tell us.

The idea here is to double check that we have ID's for both candidates before proceeding. That we entries in the stats for both candidates. And to exit early if one of these fails (as mochitest happily continues after a single test failure).

> Wow, does this mean we've never waited for the getStats to finish before proceeding with the test?

Yes, this and all the once below were not waited for before exiting a test execution (since we switched to promises).

Comment 4

[Nils Ohlmeier [:drno]]

Comment on attachment 8646611 [details]
MozReview Request: Bug 1193045: check selected attribute for all calls. r=bwc

Bug 1193045: check selected attribute for all calls. r=bwc

Comment 5

[Nils Ohlmeier [:drno]]

Ha.. non-bundle calls have "obviously" more then one pair selected...

Comment 6

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

https://reviewboard.mozilla.org/r/15807/#review14107

::: dom/media/tests/mochitest/templates.js:463
(Diff revision 1)
>    function PC_REMOTE_CHECK_STATS(test) {
> -    test.pcRemote.getStats().then(stats => {
> +    return test.pcRemote.getStats().then(stats => {
>        test.pcRemote.checkStats(stats, test.steeplechase);
>      });
>    },

Yes, this is pretty terrible, and way too easy to miss.

Why don't we make the chain executor alert if a step does NOT return a promise? i.e. ok(result instanceof Promise, "...");

We could make an exception for functions ending in _SYNC.

Might even catch a few more cases.

::: dom/media/tests/mochitest/templates.js:470
(Diff revision 1)
> -    test.pcLocal.getStats().then(stats => {
> +    return test.pcLocal.getStats().then(stats => {

Too bad getStats for pcLocal and pcRemote wont run in parallel anymore. ;)

Comment 7

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #6)
> Too bad getStats for pcLocal and pcRemote wont run in parallel anymore. ;)

Should have looked at blame before cracking that joke.

Comment 8

[Nils Ohlmeier [:drno]]

Comment on attachment 8646611 [details]
MozReview Request: Bug 1193045: check selected attribute for all calls. r=bwc

Bug 1193045: check selected attribute for all calls. r=bwc

Comment 9

[Nils Ohlmeier [:drno]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #7)
> (In reply to Jan-Ivar Bruaroey [:jib] from comment #6)
> > Too bad getStats for pcLocal and pcRemote wont run in parallel anymore. ;)
> 
> Should have looked at blame before cracking that joke.

Except if we would finally start to race pc_local and pc_remote :-)

Comment 10

[Nils Ohlmeier [:drno]]

Turns out I don't know my own testing code any more: we always ran the verification of amount of candidatepair's with selected=true here https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/pc.js#1748

But this falls into the steps where we don't wait for the results, so the patch still should land.

jib: I like your idea of the safety check. Do you want me to add this here?

Comment 11

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

Nils, here or separate bug is fine for me. You pick.

Comment 12

[Nils Ohlmeier [:drno]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #11)
> Nils, here or separate bug is fine for me. You pick.

Created bug 1194496 for that, as it actually might require some work to get that running.

Comment 13

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/15807/#review14107

> Yes, this is pretty terrible, and way too easy to miss.
> 
> Why don't we make the chain executor alert if a step does NOT return a promise? i.e. ok(result instanceof Promise, "...");
> 
> We could make an exception for functions ending in _SYNC.
> 
> Might even catch a few more cases.

Deferred that to bug 1194496

Comment 14

[Pulsebot]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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