913446 - WebRTC Mochitest changes to allow running under Steeplechase

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[(not currently active) Ted Mielczarek]

We're writing a test harness (bug 894559) we're calling Steplechase to run WebRTC tests on two different clients both behind NATs. ekr and I discussed the format of the HTML tests, and we decided it would be useful to be able to easily port the existing Mochitests to run in this new harness. Conveniently, the structure of the existing tests lent itself to this without a lot of pain. Attached are the changes I made to get this working. I tested that the Mochitests still passed on my local machine, and that test_peerConnection_basicAudio.html passes when run under Steeplechase.

There are a few classes of changes here:
Harness integration: Steeplechase is using a pretty minimal JS harness ( https://github.com/luser/steeplechase/blob/master/webharness/test.js ), so there's a little bit of impedence mismatch with Mochitest. I made a few things conditional on that. The biggest change there is how the test is invoked. Mochitest expects your test to start immediately upon load, but Steeplechase waits for the signalling channel to be setup (and the other client to have the test loaded) before starting the test (by calling run_test), so I modified runTest to accomodate that.

Splitting the test into local and remote sides: I gave PeerConnectionTest extra options: is_local and is_remote, both defaulting to true. This means that as a Mochitest, it will run with both is_local and is_remote, and run both halves of the test in the same page. Steeplechase passes in in "is_initiator" boolean to the run_test function, so it passes {is_local: is_initiator, is_remote: !is_initiator} as the options. This was the only thing I had to change in the actual test file, I made the runTests callback take an options parameter that it would pass to PeerConnectionTest. The other part of this was filtering the test commands so that PC_LOCAL commands don't run on the remote side, and PC_REMOTE commands don't run on the local side. There was also a little bit of conditional logic needed to make sure we're not trying to poke the pc{Local,Remote} directly on the other side when it's not there. Finally, I had to add steps to send and receive the offer/answer from the other side. This just shortcuts to basically the existing code in Mochitest, but uses the signalling channel (socket.io) to send and recieve in Steeplechase.

I'd like to land these so that in the near future we can have Steeplechase using these bits of the Mochitest harness directly, so that porting selected Mochitests to run in Steeplechase is trivial.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: WebRTC Mochitest changes to allow running under Steeplechase

Comment 2

[Jason Smith [:jsmith]]

Comment on attachment 800757 [details] [diff] [review]
WebRTC Mochitest changes to allow running under Steeplechase

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

Changes look fine to me overall. I'd like to get a 2nd review here from Clint, as he has been involved in the Steeplechase discussions, so he could provide input on a review from a Steeplechase perspective.

::: dom/media/tests/mochitest/pc.js
@@ +351,2 @@
>  
> +  if (options.is_local)

The one thing I'm debating here is wondering if using a null approach is a good for maintainability. Every time we reference pcLocal or pcRemote we'll have to remember to use appropriate if statements for null checks. Have you considered using a stub here instead for the else cases that generates no-ops? I'm wondering if that might be better than doing repeated null checks.

Comment 3

[(not currently active) Ted Mielczarek]

I did notice that it was a bit of a pain to do the null checks, but my worry otherwise is that tests would silently do the wrong thing. Written like this, tests that try to use pc{Local,Remote} on the wrong side of the connection will error trying to access properties on the null value. It seems much more likely that we'd hit confusing logic errors if the pc looked valid but wasn't.

Comment 4

[cmtalbert]

Comment on attachment 800757 [details] [diff] [review]
WebRTC Mochitest changes to allow running under Steeplechase

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

This looks good, just one nit about a pretty vague comment. 

r+

::: dom/media/tests/mochitest/templates.js
@@ +133,5 @@
>    [
>      'PC_LOCAL_CHECK_MEDIA_STREAMS',
>      function (test) {
> +      if (test.pcRemote) {
> +        //TODO: check this with steeplechase?

Did we check this? Can we put a better comment in here about this, what exactly are we worried about failing?

Comment 5

[(not currently active) Ted Mielczarek]

That particular test is trying to verify that the number of media streams in the PeerConnection matches what the other side requested. We could verify that in Steeplechase but it'd probably require an extra round trip message (or tacking that info on to the offer/answer message, maybe). I guess I can just see how hard that is to fix, I just punted on that.

Comment 6

[(not currently active) Ted Mielczarek]

Attachment: WebRTC Mochitest changes to allow running under Steeplechase.

Sorry, been hung up on Steeplechase harness work. I fixed the test Clint pointed out to send the right information over the signalling channel and check it. I'm going to land this patch as attached.

Comment 7

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70221b582ca1

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/70221b582ca1