Closed Bug 796888 Opened 9 years ago Closed 9 years ago

Create Mochitest for Video only connection (connect/disconnect)

Categories

(Core :: WebRTC, defect)

18 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [WebRTC][blocking-webrtc+][qa-])

Attachments

(1 file, 3 obsolete files)

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
Assignee: nobody → rwood
Whiteboard: [WebRTC], [blocking-webrtc+]
Depends on: 784515
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC][blocking-webrtc+][needs bug 784515]
Version: 14 Branch → 18 Branch
Assignee: rwood → administration
Assignee: administration → nobody
Attached patch WIP v1 (obsolete) — Splinter Review
It's a WIP and not ready. But it's something for Eric to play with and test. More work to come tomorrow.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
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.
(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.
Attached patch WIP v1.1 (obsolete) — Splinter Review
Attachment #689029 - Attachment is obsolete: true
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)
(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.
(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.
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.
Summary: Create Mochitest for Video only connection (send/receive/disconnect) → Create Mochitest for Video only connection (connect/disconnect)
Whiteboard: [WebRTC][blocking-webrtc+][needs bug 784515] → [WebRTC][blocking-webrtc+]
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #689166 - Attachment is obsolete: true
Attachment #690603 - Flags: review?(rjesup)
And I have submitted the test to try (for the alder branch):
https://tbpl.mozilla.org/?tree=Try&rev=0fadda3187f9
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.
Attachment #690603 - Flags: review?(rjesup) → feedback?(rjesup)
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.
Attachment #690603 - Flags: feedback+
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
Attachment #690603 - Flags: feedback?(rjesup) → feedback+
Depends on: 820709
(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
Depends on: 821292
Attached patch Patch v2Splinter Review
This should fix the nits from the last feedback request. Anything else will be done in a follow-up bug.
Attachment #690603 - Attachment is obsolete: true
Attachment #691830 - Flags: review?(rjesup)
Attachment #691830 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/2fc7800b9847
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Flags: in-testsuite+
Whiteboard: [WebRTC][blocking-webrtc+] → [WebRTC][blocking-webrtc+][qa-]
You need to log in before you can comment on or make changes to this bug.