796888 - Create Mochitest for Video only connection (connect/disconnect)

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As part of the initial landing of the WebRTC feature some smoketests should ensure the basics for the Peer Connection feature are working. This bug covers the request for the following test:

Ensure a local video only connection works:

* The connection can be established
* Packages can be send/received
* Disconnect

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: WIP v1

It's a WIP and not ready. But it's something for Eric to play with and test. More work to come tomorrow.

Comment 2

[Eric Rescorla (:ekr)]

I think this may be working, but it's hard to tell. Here's what looks like where it fails:

 0:14.13 --DOMWINDOW == 14 (0x1180af2e8) [serial = 8] [outer = 0x11b1a23d0] [url = about:blank]
 0:14.13 --DOMWINDOW == 13 (0x11b2896c8) [serial = 11] [outer = 0x11b1a23d0] [url = about:blank]
 0:14.13 --DOMWINDOW == 12 (0x12088c058) [serial = 14] [outer = 0x10c87cdd0] [url = about:blank]
 0:15.25 ********** type: audio
 0:15.28 ********** type: video
 0:15.92 ********** type: audio
 0:15.93 ********** type: video
 0:15.97 10 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_peerConnection_basicVideo.html | [SimpleTest.finish()] No checks actually run. (You need to call ok(), is(), or similar functions at least once.  Make sure you use SimpleTest.waitForExplicitFinish() if you need it.)
 0:15.97 11 INFO TEST-END | /tests/dom/media/tests/mochitest/test_peerConnection_basicVideo.html | finished in 6327ms
 0:15.99 12 INFO TEST-START | Shutdown
 0:15.99 13 INFO Passed: 6
 0:15.99 14 INFO Failed: 1
 0:15.99 15 INFO Todo:   0
 0:15.99 16 INFO SimpleTest FINISHED
 0:15.99 17 INFO TEST-INFO | Ran 0 Loops
 0:15.99 18 INFO SimpleTest FINISHED


OTOH, we are getting the a/v streams, so that looks good.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Eric Rescorla (:ekr) from comment #2)
> [SimpleTest.finish()] No checks actually run. (You need to call ok(), is(),
> or similar functions at least once.  Make sure you use
> SimpleTest.waitForExplicitFinish() if you need it.)

Sorry, that was my fault. Missed to re-add this line after supplying the testcase for bug 818714. I will upload a fixed WIP for you.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: WIP v1.1

Comment 5

[Jason Smith [:jsmith]]

Couple of notes, although you might already know this:

1. Make sure to write this automation to be independent of the gum params type. If you do that and try to reuse it for the three combos (video, audio, video+audio), you'll kill three birds with one stone.

2. I'd pull the peer connection logic out of head.js and stick it into a separate JS file

3. Surprised the test that's a WIP doesn't leak. If it does (which I think it will probably will), stick it under the leaking tests.

4. I'd avoid doing the one cleanup for platform checks given that the android patch will fix this and follow a different path there

5. In your framework design, avoid forcing the code flow. That way, when you extend this automation in the future, you can easily change the flow of logic and reuse the code.

6. For some of the gum setup, you might want to reuse the media stream playback js logic. There's certain events you should wait for as you'll see in that file before moving on to doing more logic (e.g. canplaythrough)

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Jason Smith [:jsmith] from comment #5)
> Couple of notes, although you might already know this:

This is a WIP and only up for Ekr to use it. That's all, really.
 
> 1. Make sure to write this automation to be independent of the gum params
> type. If you do that and try to reuse it for the three combos (video, audio,
> video+audio), you'll kill three birds with one stone.

Not necessarily. Depending on what you want to test different checks have to be made. So it's not that easy as you might think now.

> 2. I'd pull the peer connection logic out of head.js and stick it into a
> separate JS file

No, head.js is the shared lib for our tests and we should stick with that. Any other shared code should be added to this file. If you don't believe me check MXR for other components. At maximum I would allow three files: head.js, media.js, and pc.js. But really, we should not introduce new files for each type of object.

> 3. Surprised the test that's a WIP doesn't leak. If it does (which I think
> it will probably will), stick it under the leaking tests.

As mentioned above the patch is here so Erik can easily run it without having to recompile with the flag introduced with bug 817709.

> 4. I'd avoid doing the one cleanup for platform checks given that the
> android patch will fix this and follow a different path there

Just a left-over I would have removed for the first real patch.

> 5. In your framework design, avoid forcing the code flow. That way, when you
> extend this automation in the future, you can easily change the flow of
> logic and reuse the code.

This object exists to cover probably more than 90% of our tests. I'm not going to write the same code again and again. Tests which will cover failures will have their own code for the handshake.

> 6. For some of the gum setup, you might want to reuse the media stream
> playback js logic. There's certain events you should wait for as you'll see
> in that file before moving on to doing more logic (e.g. canplaythrough)

My tests will not test gUM but the peer connection. So currently I can't say when I will use this code.

Thanks for the feedback.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #6)
> (In reply to Jason Smith [:jsmith] from comment #5)
> > Couple of notes, although you might already know this:
> 
> This is a WIP and only up for Ekr to use it. That's all, really.
>  
> > 1. Make sure to write this automation to be independent of the gum params
> > type. If you do that and try to reuse it for the three combos (video, audio,
> > video+audio), you'll kill three birds with one stone.
> 
> Not necessarily. Depending on what you want to test different checks have to
> be made. So it's not that easy as you might think now.
> 
> > 2. I'd pull the peer connection logic out of head.js and stick it into a
> > separate JS file
> 
> No, head.js is the shared lib for our tests and we should stick with that.
> Any other shared code should be added to this file. If you don't believe me
> check MXR for other components. At maximum I would allow three files:
> head.js, media.js, and pc.js. But really, we should not introduce new files
> for each type of object.

Well...your feedback on the other bug in gum automation implied the opposite actually that we should separate the test framework setup logic from the class object logic. And I did agree with the motivations there. I'd usually think it's better to lean on simple files for readability, so I'd actually move that into a separate file still. My opinion though.

> 
> > 3. Surprised the test that's a WIP doesn't leak. If it does (which I think
> > it will probably will), stick it under the leaking tests.
> 
> As mentioned above the patch is here so Erik can easily run it without
> having to recompile with the flag introduced with bug 817709.
> 
> > 4. I'd avoid doing the one cleanup for platform checks given that the
> > android patch will fix this and follow a different path there
> 
> Just a left-over I would have removed for the first real patch.
> 
> > 5. In your framework design, avoid forcing the code flow. That way, when you
> > extend this automation in the future, you can easily change the flow of
> > logic and reuse the code.
> 
> This object exists to cover probably more than 90% of our tests. I'm not
> going to write the same code again and again. Tests which will cover
> failures will have their own code for the handshake.
> 
> > 6. For some of the gum setup, you might want to reuse the media stream
> > playback js logic. There's certain events you should wait for as you'll see
> > in that file before moving on to doing more logic (e.g. canplaythrough)
> 
> My tests will not test gUM but the peer connection. So currently I can't say
> when I will use this code.

True, but some logic (like waiting for events to fire) you might end up doing as well. The tests probably not.

> 
> Thanks for the feedback.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As we agreed on today we want to have really simple tests first. That means any media flow checks will be implemented in a follow-up bug. Updating summary accordingly.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1

First real basic test which exercises the connect and disconnect part. It also checks at least for the right amount of streams being attached to the peer connection clients. More checks will come later.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

And I have submitted the test to try (for the alder branch):
https://tbpl.mozilla.org/?tree=Try&rev=0fadda3187f9

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 690603 [details] [diff] [review]
Patch v1

Feedback for now, given that there are still some unwanted changes in the patch and I would like to hear back from you if the general approach is good enough before starting with the next test.

Comment 12

[Jason Smith [:jsmith]]

Comment on attachment 690603 [details] [diff] [review]
Patch v1

Review of attachment 690603 [details] [diff] [review]:
-----------------------------------------------------------------

I've included my feedback too on a skim. Looks like you are on the right track.

Note - This is more of a perspective analysis.

::: dom/media/tests/mochitest/Makefile.in
@@ +24,5 @@
>  
>  # The following tests are leaking and cannot be run by default yet
>  ifdef MOZ_WEBRTC_LEAKING_TESTS
>  MOCHITEST_FILES += \
> +  test_peerConnection_basicVideo.html \

The makefile is a bit weird...are you synced with trunk?

::: dom/media/tests/mochitest/head.js
@@ +18,5 @@
>    SimpleTest.waitForExplicitFinish();
>  
>    // If this is a desktop supported test and we're on android or b2g,
>    // indicate that the test is not supported and skip the test
> +  if(aDesktopSupportedOnly && (['', 'Android'].indexOf(navigator.platform) !== -1)) {

You are know this, but this is just a small note to make sure to remove this on check in for the patch that's happening on the Android fix.

::: dom/media/tests/mochitest/mediaStreamPlayback.js
@@ -34,5 @@
>      var canPlayThroughFired = false;
>  
>      // Verifies we've received a correctly initialized LocalMediaStream
> -    ok(this.mediaStream instanceof LocalMediaStream,
> -      "Stream should be a LocalMediaStream");

Don't remove this - this is a contract between the caller to make sure a gum media stream is the correct object type

::: dom/media/tests/mochitest/pc.js
@@ +9,5 @@
> +
> +  handShake: function PC_handShake(aPCLocal, aPCRemote, aSuccessCallback) {
> +
> +    function onCreateOfferSuccess(aOffer) {
> +      pc1_offer = aOffer;

Might be worthwhile to do validation here with each function stated below. The type of validation you are aiming to in these functions very much follows a state machine - are the right values set in each particular step?

For a function callback that's returning an object:

* Do an object contract test here - make sure the object returned is of the type mozRTCSessionDescription (Randell double check me on the moz prefix piece)

Generally:

* Check that the attributes of aOffer are correct here
** What should type be at this point?
** What should sdp be at this point?

::: dom/media/tests/mochitest/test_peerConnection_basicVideo.html
@@ +8,5 @@
> +  <title>Basic video-only peer connection</title>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <script type="application/javascript" src="head.js"></script>
> +  <script type="application/javascript" src="mediaStreamPlayback.js"></script>

This import probably won't be necessary if you aren't using the function calls from it.

@@ +28,5 @@
> +
> +  var pcLocal;
> +  var pcRemote;
> +
> +  var test_data = {

Why use the dictionary? Why not use separate variables?

@@ +41,5 @@
> +
> +    pcLocal = new mozRTCPeerConnection();
> +    pcRemote = new mozRTCPeerConnection();
> +
> +    pcLocal.onaddstream = function (aStream) {

You should probably validate somewhere in here that this event did successfully get fired. And make sure remoteStreams is updated correctly.

@@ +45,5 @@
> +    pcLocal.onaddstream = function (aStream) {
> +      test_data['pcLocal'].push(aStream);
> +
> +      videoPCRemote.mozSrcObject = aStream.stream;
> +      videoPCRemote.play();

Take a look at the logic in media stream playback - depending on the circumstances, you may need to wait for an event to fire to deterministically know you are playing content.

@@ +48,5 @@
> +      videoPCRemote.mozSrcObject = aStream.stream;
> +      videoPCRemote.play();
> +    };
> +
> +    pcRemote.onaddstream = function (aStream) {

Same with the comment above - validate this event is fired. And make sure remoteStreams array gets updated.

@@ +62,5 @@
> +      navigator.mozGetUserMedia({video: true, fake: true}, function onSuccess(aRemoteInputStream) {
> +        pcRemote.addStream(aRemoteInputStream);
> +
> +        videoLocal.mozSrcObject = aLocalInputStream;
> +        videoLocal.play();

Random question - why do playback on three media elements?

@@ +88,5 @@
> +  function disconnect() {
> +    pcLocal.close();
> +    pcRemote.close();
> +
> +    info("We can't run any checks and clean-up code due to a crash (see bug 820072)");

You also need to cleanup the streams here - make sure to call stop() on each stream.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 690603 [details] [diff] [review]
Patch v1

Review of attachment 690603 [details] [diff] [review]:
-----------------------------------------------------------------

f+ with the onAddStream thing fixed

::: dom/media/tests/mochitest/test_peerConnection_basicVideo.html
@@ +44,5 @@
> +
> +    pcLocal.onaddstream = function (aStream) {
> +      test_data['pcLocal'].push(aStream);
> +
> +      videoPCRemote.mozSrcObject = aStream.stream;

You need to test the type of the stream (if it has video).  RIght now, you'll get two AddStreams, one audio and one video.  Luckily, audio will come first, and video will overwrite it in mozSrcObject - but it's fragile and implementation-dependent.  If it's audio+video (in the future) it should go directly to the video element.

@@ +51,5 @@
> +
> +    pcRemote.onaddstream = function (aStream) {
> +      test_data['pcRemote'].push(aStream);
> +
> +      videoPCLocal.mozSrcObject = aStream.stream;

Ditto

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Randell Jesup [:jesup] from comment #13)
> You need to test the type of the stream (if it has video).  RIght now,
> you'll get two AddStreams, one audio and one video.  Luckily, audio will
> come first, and video will overwrite it in mozSrcObject - but it's fragile
> and implementation-dependent.  If it's audio+video (in the future) it should
> go directly to the video element.

That's right. Not sure why I missed that. Thanks for catching it!

(In reply to Jason Smith [:jsmith] from comment #12)
> The makefile is a bit weird...are you synced with trunk?

That was my fault. All of those changes to different files will be reverted in the next version of the patch.

> >      // Verifies we've received a correctly initialized LocalMediaStream
> > -    ok(this.mediaStream instanceof LocalMediaStream,
> > -      "Stream should be a LocalMediaStream");
> 
> Don't remove this - this is a contract between the caller to make sure a gum
> media stream is the correct object type

We have to talk about it later given that this class is not usable by peer connection tests for remote streams. For now I reverted that and removed any reference to MediaStreamPlayback.

> > +    function onCreateOfferSuccess(aOffer) {
> > +      pc1_offer = aOffer;
> 
> Might be worthwhile to do validation here with each function stated below.
> The type of validation you are aiming to in these functions very much
> follows a state machine - are the right values set in each particular step?
> 
> For a function callback that's returning an object:
> 
> * Do an object contract test here - make sure the object returned is of the
> type mozRTCSessionDescription (Randell double check me on the moz prefix
> piece)

I don't see why a check for a specific type is necessary here. But I will indeed check properties on the object to ensure a valid state.

> > +    pcLocal.onaddstream = function (aStream) {
> 
> You should probably validate somewhere in here that this event did
> successfully get fired. And make sure remoteStreams is updated correctly.

All that is done in the final handShake callback.

> > +    pcLocal.onaddstream = function (aStream) {
> > +      test_data['pcLocal'].push(aStream);
> > +
> > +      videoPCRemote.mozSrcObject = aStream.stream;
> > +      videoPCRemote.play();
> 
> Take a look at the logic in media stream playback - depending on the
> circumstances, you may need to wait for an event to fire to
> deterministically know you are playing content.

As mentioned above we will not include any media stream checks in this test for now. This is really basic stuff for a pc connection.

> @@ +62,5 @@
> > +      navigator.mozGetUserMedia({video: true, fake: true}, function onSuccess(aRemoteInputStream) {
> > +        pcRemote.addStream(aRemoteInputStream);
> > +
> > +        videoLocal.mozSrcObject = aLocalInputStream;
> > +        videoLocal.play();
> 
> Random question - why do playback on three media elements?

This is a 1-1 copy of the manual testcase on webrtc-landing and might need the video element for checks later. For now it's just a visual indicator watching the tests.

> > +    info("We can't run any checks and clean-up code due to a crash (see bug 820072)");
> 
> You also need to cleanup the streams here - make sure to call stop() on each
> stream.

Please read my info() message and remind what we agreed on IRC. All of the existing tests need to clean up the media streams correctly. So that will be a follow-up bug.


I pushed again to try to check if the detected crash is gone after the big lock patch has been landed yesterday:

https://tbpl.mozilla.org/?tree=Try&rev=f389e5644dee

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v2

This should fix the nits from the last feedback request. Anything else will be done in a follow-up bug.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 691830 [details] [diff] [review]
Patch v2

https://hg.mozilla.org/integration/mozilla-inbound/rev/2fc7800b9847

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2fc7800b9847