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

RESOLVED FIXED in Firefox 43

Status

()

P2
normal
Rank:
25
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: drno, Assigned: drno)

Tracking

(Blocks: 1 bug)

Trunk
mozilla43
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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?
backlog: --- → webRTC+
Rank: 25
Priority: -- → P2
(Assignee)

Comment 1

4 years ago
Created 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
Attachment #8646611 - Flags: review?(docfaraday)
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?
Attachment #8646611 - Flags: review?(docfaraday) → review+
(Assignee)

Comment 3

4 years ago
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).
(Assignee)

Comment 4

4 years ago
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
Attachment #8646611 - Attachment description: MozReview Request: Bug 1193045: check selected attribute for all calls. r?bwc → MozReview Request: Bug 1193045: check selected attribute for all calls. r=bwc
(Assignee)

Updated

4 years ago
Assignee: nobody → drno
(Assignee)

Comment 5

4 years ago
Ha.. non-bundle calls have "obviously" more then one pair selected...
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. ;)
(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.
(Assignee)

Comment 8

4 years ago
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
(Assignee)

Comment 9

4 years ago
(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 :-)
(Assignee)

Comment 10

4 years ago
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?
Flags: needinfo?(jib)
Summary: mochitests should verify the presence of exactly one 'selected' attribute in the stats → Wait for mochitest test step results and add extra safety check for 'selected' attribute
Nils, here or separate bug is fine for me. You pick.
Flags: needinfo?(jib)
(Assignee)

Comment 12

4 years ago
(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.
(Assignee)

Comment 13

4 years ago
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
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Blocks: 1194496
https://hg.mozilla.org/mozilla-central/rev/bc7d2eccea16
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox43: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.