1190574 - Test harness functions like test.chain.replace aren't asserting inputs, hiding bad tests.

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

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

With a patch that catches this, I found these broken tests (which I've fixed):

> 21114 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_forwarding_basicAudioVideoCombined.html | Error executing test: Error: Unknown test: PC_REMOTE_CHECK_MEDIA_FLOW_PRESENT
> 25134 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_relayOnly.html | Error executing test: Error: Unknown test: PC_LOCAL_SETURP_ICE_HANDLER
> 30551 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_replaceTrack.html | Error executing test: Error: Unknown test: PC_REMOTE_CHECK_MEDIA_FLOW_PRESENT
> 31887 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_syncSetDescription.html | Error executing test: Error: Unknown test: [object Object]
> 31903 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_forwarding_basicAudioVideoCombined.html | Error executing test: Error: Unknown test: PC_REMOTE_CHECK_MEDIA_FLOW_PRESENT

Comment 1

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

Attachment: MozReview Request: Bug 1190574 - make test.chain.replace and cohorts throw on unknown test name + fix broken tests

Bug 1190574 - make test.chain.replace and cohorts throw on unknown test name + fix broken tests

Comment 2

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

Ignore dom/media/tests/mochitest/test_peerConnection_relayOnly.html, that's one I was testing with in Bug 1187775, which is how I stumbled on this.

Comment 3

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

Comment on attachment 8642618 [details]
MozReview Request: Bug 1190574 - make test.chain.replace and cohorts throw on unknown test name + fix broken tests

Bug 1190574 - make test.chain.replace and cohorts throw on unknown test name + fix broken tests

Comment 4

[Nils Ohlmeier [:drno]]

Comment on attachment 8642618 [details]
MozReview Request: Bug 1190574 - make test.chain.replace and cohorts throw on unknown test name + fix broken tests

https://reviewboard.mozilla.org/r/14739/#review13335

::: dom/media/tests/mochitest/test_selftest.html:28
(Diff revision 2)
> +  is(catcher(() => test.chain.replace("PC_LOCAL_SET_LOCAL_DESCRIPTION", TEST)),
> +     null, "test.chain.replace works");
> +  is(catcher(() => test.chain.replace("FOO", TEST)),
> +     "Unknown test: FOO", "test.chain.replace catches typos");

This makes me wonder if we should have something more detailed, where we verify that every single of the chain functions works as expected.

Comment 5

[Nils Ohlmeier [:drno]]

Comment on attachment 8642618 [details]
MozReview Request: Bug 1190574 - make test.chain.replace and cohorts throw on unknown test name + fix broken tests

https://reviewboard.mozilla.org/r/14739/#review13337

Ship It!

Comment 6

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

(In reply to Nils Ohlmeier [:drno] from comment #4)
> This makes me wonder if we should have something more detailed, where we
> verify that every single of the chain functions works as expected.

I think that's a good idea. Feel free to expound on it or open a followup bug.

Comment 7

[Nils Ohlmeier [:drno]]

I filled bug 1203683 as a follow up.

jib: I thought you land this after my r+. Any reason not to land it?

Comment 8

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

Comment on attachment 8642618 [details]
MozReview Request: Bug 1190574 - make test.chain.replace and cohorts throw on unknown test name + fix broken tests

Bug 1190574 - make test.chain.replace and cohorts throw on unknown test name + fix broken tests

Comment 9

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

See Bug 1202046 comment 2 for why I didn't land this. Basically https://treeherder.mozilla.org/#/jobs?repo=try&revision=099d31b82ace showed "PeerConnectionWrapper (pcRemote): legal ICE state transition from new to failed" perma-failing when I enabled the missing tests. I've done a rebase and a new try to see if things have improved.

Comment 10

[Nils Ohlmeier [:drno]]

Oh sorry. I mis-read comment 2. I thought this would fix it. But you said it runs into the same error.

Hmm so if the rebasing doesn't magically fix it, we basically have a perma orange ICE connection issue. Let me know if that is the case. I would be interested in trying out a couple of things if that is the case.

Comment 11

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

Yup still perma-orange: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f89edd89af4f

Yeah comment 2 was a different error which is gone. This is all Bug 1202046 now it seems.

Comment 12

[Nils Ohlmeier [:drno]]

Attachment: MozReview Request: Bug 1190574 - make test.chain.replace and cohorts throw on unknown test name + fix broken tests

Bug 1190574 - make test.chain.replace and cohorts throw on unknown test name + fix broken tests

Comment 13

[Nils Ohlmeier [:drno]]

Attachment: MozReview Request: Bug 1190574: added missing calls to release stored ICE candidates. r+jib

Bug 1190574: really made sLD/sRD calls synchronos. r?jib

Comment 14

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

Comment on attachment 8661084 [details]
MozReview Request: Bug 1190574: added missing calls to release stored ICE candidates. r+jib

https://reviewboard.mozilla.org/r/19291/#review17227

::: dom/media/tests/mochitest/test_peerConnection_syncSetDescription.html:19
(Diff revision 1)
> -  test.pcLocal._pc.setLocalDescription(test.originalOffer);
> +  return new Promise((resolve, reject) =>
> +    test.pcLocal._pc.setLocalDescription(test.originalOffer, resolve, reject));

No, the whole point of this test is to verify that we support calling these methods without waiting for them to succeed. It's the only reason peerConnections still has an "operations" array [1]. It's a legacy thing back to the callback days when people signaled after createOffer rather than after setLocalDescription [2].

[1] http://w3c.github.io/webrtc-pc/#operation
[2] http://dev.w3.org/2011/webrtc/editor/webrtc-20120720.html#simple-example

Comment 15

[Eric Rescorla (:ekr)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #14)
> Comment on attachment 8661084 [details]
> MozReview Request: Bug 1190574: really made sLD/sRD calls synchronos. r?jib
> 
> https://reviewboard.mozilla.org/r/19291/#review17227
> 
> ::: dom/media/tests/mochitest/test_peerConnection_syncSetDescription.html:19
> (Diff revision 1)
> > -  test.pcLocal._pc.setLocalDescription(test.originalOffer);
> > +  return new Promise((resolve, reject) =>
> > +    test.pcLocal._pc.setLocalDescription(test.originalOffer, resolve, reject));
> 
> No, the whole point of this test is to verify that we support calling these
> methods without waiting for them to succeed. It's the only reason
> peerConnections still has an "operations" array [1]. It's a legacy thing
> back to the callback days when people signaled after createOffer rather than
> after setLocalDescription [2].

As far as I know, whether you call with Promises or with Callbacks, you are
still required to queue.

Comment 16

[Eric Rescorla (:ekr)]

Also, there's absolutely no reason you have to wait till after setLocalDescription() to signal, promises or otherwise.

Comment 17

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

Fair enough, but all our tests signal after setLocalDescription, except this one, so if we wait for success on this one as well, then we lose coverage.

Comment 18

[Nils Ohlmeier [:drno]]

You are right I missed what the point of this test is. I removed the promises. But the reason why the test fails without my latest patch is that the stored ICE candidates are never added on the far ends PeerConnection.
So if I add one single success callback to sRD() to release the ICE candidates into the PC then the test starts to fail. It appears the reason is that the wrapping of legacy calls with onSuccess callbacks into a Promise basically no longer guarantees that the wrapping Promise from sRD gets executed before the native promise based call into createAnswer().

Comment 19

[Nils Ohlmeier [:drno]]

Comment on attachment 8661084 [details]
MozReview Request: Bug 1190574: added missing calls to release stored ICE candidates. r+jib

Bug 1190574: added missing calls to release stored ICE candidates.

Comment 20

[Nils Ohlmeier [:drno]]

So my latest patch only adds the releasing of the ICE candidates, which is the reason the test keeps failing.
But it now fails for the above mentioned reason. Jib can you take a look how to fix this best in PeerConnection.js?
I know we are mixing now old style with and without callback, plus Promise based createOffer and createAnswer. But we should probably fix it I think.

Comment 21

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

Comment on attachment 8661084 [details]
MozReview Request: Bug 1190574: added missing calls to release stored ICE candidates. r+jib

https://reviewboard.mozilla.org/r/19291/#review17315

::: dom/media/tests/mochitest/test_peerConnection_syncSetDescription.html:24
(Diff revision 2)
> -  test.pcRemote._pc.setRemoteDescription(test._local_offer);
> +  test.pcRemote._pc.setRemoteDescription(test._local_offer,
> +      test.pcRemote.releaseIceCandidates);

The legacy api is fickle. You can't call setLocal/RemoteDescription with two arguments. 1 (new) or 3 (old), but not 2. i.e. you need to add the required error callback here. Unfortunately, due to webidl overload limitations, the error about this is hidden. If you were to add:

    .catch(e => console.error(e))

to the tail of that call, before the semicolon, then you would see:

TypeError: Not enough arguments to mozRTCPeerConnection.setLocalDescription.

Hopefully, we wont be writing a lot of new legacy code after this test.

Comment 22

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

I'm fairly confident PeerConnection.js is working here. Here's a fiddle showing sync setLD/RD in action, albeit with new promise methods https://jsfiddle.net/23mzs084/

Comment 23

[Nils Ohlmeier [:drno]]

Comment on attachment 8661084 [details]
MozReview Request: Bug 1190574: added missing calls to release stored ICE candidates. r+jib

Bug 1190574: added missing calls to release stored ICE candidates. r?jib

Comment 24

[Nils Ohlmeier [:drno]]

Good catch. Yeah adding the error callback seem to make it work.
So here is the patch which fixes the missing relay of ICE candidates for this test.

Comment 25

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

Comment on attachment 8661084 [details]
MozReview Request: Bug 1190574: added missing calls to release stored ICE candidates. r+jib

https://reviewboard.mozilla.org/r/19291/#review17357

::: dom/media/tests/mochitest/test_peerConnection_syncSetDescription.html:25
(Diff revision 3)
> +      test.pcRemote.releaseIceCandidates, function() {});

We probably want generateErrorCallback there in case there's a problem.

Comment 26

[Maire Reavy [:mreavy]]

Nils - is this ready to land?  See also bug 1202046 which used to go perma-orange with this patch.

Comment 27

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

Comment on attachment 8642618 [details]
MozReview Request: Bug 1190574 - make test.chain.replace and cohorts throw on unknown test name + fix broken tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/14739/diff/3-4/

Comment 28

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

Attachment: MozReview Request: Bug 1190574: added missing calls to release stored ICE candidates. r?jib

Bug 1190574: added missing calls to release stored ICE candidates. r?jib

Comment 29

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

Comment on attachment 8683325 [details]
MozReview Request: Bug 1190574: added missing calls to release stored ICE candidates. r?jib

Just a rebase.

Comment 30

[Nils Ohlmeier [:drno]]

Comment on attachment 8661083 [details]
MozReview Request: Bug 1190574 - make test.chain.replace and cohorts throw on unknown test name + fix broken tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19289/diff/1-2/

Comment 31

[Nils Ohlmeier [:drno]]

Comment on attachment 8661084 [details]
MozReview Request: Bug 1190574: added missing calls to release stored ICE candidates. r+jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19291/diff/3-4/

Comment 32

[Nils Ohlmeier [:drno]]

Comment on attachment 8661084 [details]
MozReview Request: Bug 1190574: added missing calls to release stored ICE candidates. r+jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19291/diff/4-5/

Comment 33

[Nils Ohlmeier [:drno]]

Sorry I did not realized that this was still depending on me. Pushed the one change jib requested. Try run is on its way...

Comment 34

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

Seeing this in try in bug 1202046 comment 10:

> 15:00:37     INFO -  4275 INFO TEST-PASS | dom/media/tests/mochitest/test_selftest.html | test.chain.replace catches typos
> 15:00:37     INFO -  4276 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_selftest.html | Test timed out.

Comment 35

[Nils Ohlmeier [:drno]]

The try run in comment 32 apparently did not execute anything. Time to go back to bigger guns again...

Comment 36

[Nils Ohlmeier [:drno]]

Comment on attachment 8661084 [details]
MozReview Request: Bug 1190574: added missing calls to release stored ICE candidates. r+jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19291/diff/5-6/

Comment 37

[Nils Ohlmeier [:drno]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #34)
> Seeing this in try in bug 1202046 comment 10:
> 
> > 15:00:37     INFO -  4275 INFO TEST-PASS | dom/media/tests/mochitest/test_selftest.html | test.chain.replace catches typos
> > 15:00:37     INFO -  4276 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_selftest.html | Test timed out.

If we never call finish() in a test then it times out :-)

Lets see if the latest patch will turn green...

Comment 38

[Nils Ohlmeier [:drno]]

Try run looks sane

Comment 39

[Carsten Book [:Tomcat]]

does 8c850071a79d	Bug 1190574 - make test.chain.replace and cohorts throw on unknown test name + fix broken tests	also need checkin ?

Comment 40

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

Yes, thanks.

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/689ca8a01248
https://hg.mozilla.org/integration/mozilla-inbound/rev/203a48eb1726

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/689ca8a01248
https://hg.mozilla.org/mozilla-central/rev/203a48eb1726

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/689ca8a01248
https://hg.mozilla.org/mozilla-central/rev/203a48eb1726