Automate WebRTC-in-Chrome test cases

Adding two test cases to the existing webrtc content_browser tests:
1) Verify that setLocalDescription fails when trying to negotiate an unsupported video codec
 
- Verify that setLocalDescription fails when trying to negotiate with a client that does not support encryption
 

BUG=303035
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235798

[elham, 2013-11-01 21:31:53]

Hi,
can you please review this

[phoglund_chromium, 2013-11-04 09:34:56]

https://codereview.chromium.org/47923023/diff/20001/content/browser/media/web...
File content/browser/media/webrtc_browsertest.cc (right):

https://codereview.chromium.org/47923023/diff/20001/content/browser/media/web...
content/browser/media/webrtc_browsertest.cc:236: // This test will modify the
sdp offer to an unsupported codec to verify that
Nit: This sentence is a bit convoluted and hard to read. What about "This test
will modify the SDP offer to use an unsupported codec, which should cause
SetLocalDescription to fail.

Also, remember to end comments with period.

https://codereview.chromium.org/47923023/diff/20001/content/browser/media/web...
content/browser/media/webrtc_browsertest.cc:250: // This test will modify the
sdp offer to verify that SetLocalDescription
Nit: rewrite this sentence like the one above.

https://codereview.chromium.org/47923023/diff/20001/content/test/data/media/p...
File content/test/data/media/peerconnection-call.html (right):

https://codereview.chromium.org/47923023/diff/20001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:12: var gTestNonCrypto = false;
You don't need these. See more below.

https://codereview.chromium.org/47923023/diff/20001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:103: // Test that we can't
setup a call with an unsupported video codec
Nit: indentation

https://codereview.chromium.org/47923023/diff/20001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:108:
addStreamToBothConnectionsAndNegotiate, printGetUserMediaError);
It's weird to pass in addStreamToBothConnectionsAndNegotiate as the success
callback here, as you'd want the test to fail if the success callback gets
called!

Instead, you could pass in something like this:

function(stream) { 
  raise "Didn't expect to have success callback called!";
}

https://codereview.chromium.org/47923023/diff/20001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:408: // Called if
setLocaDescription fails.
Nit: setLocalDescription.

https://codereview.chromium.org/47923023/diff/20001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:409: function
printLocalDescriptionError(error) {
It's misleading to call this function printLocalDescriptionError since it
doesn't do that at all. Rather, it succeeds or fails the test depending on what
we are looking for.

You should split this function in two functions. Currently you use this function
it in both negotiateUnsupportedVideoCodec and negotiateNonCryptoCall, but
there's really no reason to. I'd split it and call one
verifyErrorIsFailedToUpdateSessionState and the other
verifyErrorIsCalledSdpWithoutCrypto. E.g. something like this:

function negotiateNonCryptoCall() {
   // ...
   navigator.webkitGetUserMedia({audio: true, video: true},
       ...,
       verifyErrorIsCalledSdpWithoutCrypto);
}

function verifyErrorIsCalledSdpWithoutCrypto(error) {
  if (error == 'SetLocalDescription failed: Called with a SDP without crypto
enabled') {
    document.title = 'OK';
  }
}

The other verification function will look very similar. Obviously, you'll pass
that into the webkitGetUserMedia call for negotiateUnsupportedVideoCodec
instead. You can get rid of the new global variables
(gTestUnsupportedVideoCodec, etc) at this point ase well since they become
unnecessary. That's great, because global variables are bad and we should avoid
them.

https://codereview.chromium.org/47923023/diff/20001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:424: // Called if
setLocalDescription success
This doesn't seem to be used.

https://codereview.chromium.org/47923023/diff/20001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:510: if (gTestNonCrypto) {
Good news and bad news: bemasc@ just refactored this in
https://codereview.chromium.org/39033006/diff/350001/content/test/data/media/....
So instead of looking at this global variable, you'll set a transform function
in your test function. Think about it as "I want to modify the offer SDP in this
way in this test."

So in your case, after rebasing in Ben's patch, your transform code should move
to the the test case function. You should end up with something like the
following:

function negotiateNonCryptoCall() {
  // ...
  transformSdp = function(sdp) {  // Gets called on offer SDP.
    sdp = sdp.replace(/a=crypto.*\r\n/g, 'a=Xcrypto\r\n');
    return offer.sdp.replace(/a=fingerprint.*\r\n/g, '');
  };
  navigator.getUserMedia(...);
}

Override transformSdp in the same way but with the other SDP modification in
unsupported video codec case.

[elham, 2013-11-07 23:48:00]

Hi, 
I did a rebase and made some of changes that you suggested.

https://codereview.chromium.org/47923023/diff/20001/content/browser/media/web...
File content/browser/media/webrtc_browsertest.cc (right):

https://codereview.chromium.org/47923023/diff/20001/content/browser/media/web...
content/browser/media/webrtc_browsertest.cc:236: // This test will modify the
sdp offer to an unsupported codec to verify that
On 2013/11/04 09:34:56, phoglund wrote:
> Nit: This sentence is a bit convoluted and hard to read. What about "This test
> will modify the SDP offer to use an unsupported codec, which should cause
> SetLocalDescription to fail.
> 
> Also, remember to end comments with period.

Done.

https://codereview.chromium.org/47923023/diff/20001/content/browser/media/web...
content/browser/media/webrtc_browsertest.cc:250: // This test will modify the
sdp offer to verify that SetLocalDescription
On 2013/11/04 09:34:56, phoglund wrote:
> Nit: rewrite this sentence like the one above.

Done.

https://codereview.chromium.org/47923023/diff/20001/content/test/data/media/p...
File content/test/data/media/peerconnection-call.html (right):

https://codereview.chromium.org/47923023/diff/20001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:103: // Test that we can't
setup a call with an unsupported video codec
On 2013/11/04 09:34:56, phoglund wrote:
> Nit: indentation

Done.

https://codereview.chromium.org/47923023/diff/20001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:108:
addStreamToBothConnectionsAndNegotiate, printGetUserMediaError);
On 2013/11/04 09:34:56, phoglund wrote:
> It's weird to pass in addStreamToBothConnectionsAndNegotiate as the success
> callback here, as you'd want the test to fail if the success callback gets
> called!
> 
> Instead, you could pass in something like this:
> 
> function(stream) { 
>   raise "Didn't expect to have success callback called!";
> }

No GetUserMedia had nothing to do with offer.sdp and is not expected to fail.
Also I need to negotiate the offer and that's why I need to call
addStreamToBothConnectionsAndNegotiate

https://codereview.chromium.org/47923023/diff/20001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:408: // Called if
setLocaDescription fails.
On 2013/11/04 09:34:56, phoglund wrote:
> Nit: setLocalDescription.

I still need this, since I wanted to check if we get a error callback from
setLocalDescription

https://codereview.chromium.org/47923023/diff/20001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:424: // Called if
setLocalDescription success
On 2013/11/04 09:34:56, phoglund wrote:
> This doesn't seem to be used.

It will be used every time setLocalDescription succeeds

https://codereview.chromium.org/47923023/diff/20001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:510: if (gTestNonCrypto) {
On 2013/11/04 09:34:56, phoglund wrote:
> Good news and bad news: bemasc@ just refactored this in
>
https://codereview.chromium.org/39033006/diff/350001/content/test/data/media/....
> So instead of looking at this global variable, you'll set a transform function
> in your test function. Think about it as "I want to modify the offer SDP in
this
> way in this test."
> 
> So in your case, after rebasing in Ben's patch, your transform code should
move
> to the the test case function. You should end up with something like the
> following:
> 
> function negotiateNonCryptoCall() {
>   // ...
>   transformSdp = function(sdp) {  // Gets called on offer SDP.
>     sdp = sdp.replace(/a=crypto.*\r\n/g, 'a=Xcrypto\r\n');
>     return offer.sdp.replace(/a=fingerprint.*\r\n/g, '');
>   };
>   navigator.getUserMedia(...);
> }
> 
> Override transformSdp in the same way but with the other SDP modification in
> unsupported video codec case.

Done.

[phoglund_chromium, 2013-11-08 09:17:29]

https://codereview.chromium.org/47923023/diff/20001/content/test/data/media/p...
File content/test/data/media/peerconnection-call.html (right):

https://codereview.chromium.org/47923023/diff/20001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:108:
addStreamToBothConnectionsAndNegotiate, printGetUserMediaError);
You are right: my bad.

https://codereview.chromium.org/47923023/diff/20001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:409: function
printLocalDescriptionError(error) {
On 2013/11/04 09:34:56, phoglund wrote:
> It's misleading to call this function printLocalDescriptionError since it
> doesn't do that at all. Rather, it succeeds or fails the test depending on
what
> we are looking for.
> 
> You should split this function in two functions. Currently you use this
function
> it in both negotiateUnsupportedVideoCodec and negotiateNonCryptoCall, but
> there's really no reason to. I'd split it and call one
> verifyErrorIsFailedToUpdateSessionState and the other
> verifyErrorIsCalledSdpWithoutCrypto. E.g. something like this:
> 
> function negotiateNonCryptoCall() {
>    // ...
>    navigator.webkitGetUserMedia({audio: true, video: true},
>        ...,
>        verifyErrorIsCalledSdpWithoutCrypto);
> }
> 
> function verifyErrorIsCalledSdpWithoutCrypto(error) {
>   if (error == 'SetLocalDescription failed: Called with a SDP without crypto
> enabled') {
>     document.title = 'OK';
>   }
> }
> 
> The other verification function will look very similar. Obviously, you'll pass
> that into the webkitGetUserMedia call for negotiateUnsupportedVideoCodec
> instead. You can get rid of the new global variables
> (gTestUnsupportedVideoCodec, etc) at this point ase well since they become
> unnecessary. That's great, because global variables are bad and we should
avoid
> them.
Oh, you are right again. You are passing this callback into setLocalDescription
rather than getUserMedia, which I thought for some reason. I still think the
comment applies though. Let me make a new comment on the latest patch set.

https://codereview.chromium.org/47923023/diff/20001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:424: // Called if
setLocalDescription success
On 2013/11/07 23:48:00, elham wrote:
> On 2013/11/04 09:34:56, phoglund wrote:
> > This doesn't seem to be used.
> 
> It will be used every time setLocalDescription succeeds
Yes, except you pass in the error handler function twice into
setLocalDescription. I'll make a new comment in the new patch set.

[phoglund_chromium, 2013-11-08 09:29:26]

Starting to look good, you just need to get rid of the globals and introduce a
new transform function, which your new test cases will override separately.

https://codereview.chromium.org/47923023/diff/80001/content/test/data/media/p...
File content/test/data/media/peerconnection-call.html (right):

https://codereview.chromium.org/47923023/diff/80001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:12: var gTestNonCrypto = false;
I still think you can get rid of these. See more below.

https://codereview.chromium.org/47923023/diff/80001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:23: var transformCandidate =
function(candidate) { return candidate; };
Introduce a new "transform" function here called onLocalDescriptionError, which
you override in your two new test cases. By default, it should do nothing.

https://codereview.chromium.org/47923023/diff/80001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:128: transformSdp =
removeVideoCodec;
Here you'll have something like

onLocalDescriptionError = function(error) {
  var expectedMsg = 'SetLocalDescription failed: Failed to' +
      ' update session state: ERROR_CONTENT';
  expectEquals(expectedMsg, error);

  // Got the right message, test succeeded.
  document.title = 'OK';
}

https://codereview.chromium.org/47923023/diff/80001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:130:
addStreamToBothConnectionsAndNegotiate, printGetUserMediaError);
Nit: should be indented four spaces (this is wrong in the functions above;
please fix those too).

https://codereview.chromium.org/47923023/diff/80001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:552:
caller.setLocalDescription(offer, printLocalDescriptionError,
Pass in an empty function which does nothing as the first argument, and the new
onSetLocalDescriptionError function as the second argument.

[elham, 2013-11-08 23:30:39]

Hi Patrik,

I have made the modification that you suggested, can you please take a look
again.

https://codereview.chromium.org/47923023/diff/80001/content/test/data/media/p...
File content/test/data/media/peerconnection-call.html (right):

https://codereview.chromium.org/47923023/diff/80001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:12: var gTestNonCrypto = false;
On 2013/11/08 09:29:26, phoglund wrote:
> I still think you can get rid of these. See more below.

Done.

https://codereview.chromium.org/47923023/diff/80001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:23: var transformCandidate =
function(candidate) { return candidate; };
On 2013/11/08 09:29:26, phoglund wrote:
> Introduce a new "transform" function here called onLocalDescriptionError,
which
> you override in your two new test cases. By default, it should do nothing.

Done.

https://codereview.chromium.org/47923023/diff/80001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:128: transformSdp =
removeVideoCodec;
On 2013/11/08 09:29:26, phoglund wrote:
> Here you'll have something like
> 
> onLocalDescriptionError = function(error) {
>   var expectedMsg = 'SetLocalDescription failed: Failed to' +
>       ' update session state: ERROR_CONTENT';
>   expectEquals(expectedMsg, error);
> 
>   // Got the right message, test succeeded.
>   document.title = 'OK';
> }

Done.

https://codereview.chromium.org/47923023/diff/80001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:130:
addStreamToBothConnectionsAndNegotiate, printGetUserMediaError);
On 2013/11/08 09:29:26, phoglund wrote:
> Nit: should be indented four spaces (this is wrong in the functions above;
> please fix those too).

Done.

https://codereview.chromium.org/47923023/diff/80001/content/test/data/media/p...
content/test/data/media/peerconnection-call.html:552:
caller.setLocalDescription(offer, printLocalDescriptionError,
On 2013/11/08 09:29:26, phoglund wrote:
> Pass in an empty function which does nothing as the first argument, and the
new
> onSetLocalDescriptionError function as the second argument.

Done.

[phoglund_chromium, 2013-11-11 08:00:19]

lgtm

https://codereview.chromium.org/47923023/diff/200001/content/test/data/media/...
File content/test/data/media/peerconnection-call.html (right):

https://codereview.chromium.org/47923023/diff/200001/content/test/data/media/...
content/test/data/media/peerconnection-call.html:127:
addStreamToBothConnectionsAndNegotiate, printGetUserMediaError);
Nit: should be indented 4 spaces. In general,
someReallyReallyLongFunctionName(withParametersThatMakeTheLineWrap,
    andTheSecondParameterIsTooLongToLineUpWithTheLeftParen);

This is also wrong on lines 117 and 143: please fix that while you're at it.

https://codereview.chromium.org/47923023/diff/200001/content/test/data/media/...
content/test/data/media/peerconnection-call.html:572: offerSdp=
offerSdp.replace('a=rtpmap:100 VP8/90000\r\n',
Nit: space between offerSdp and =.

https://codereview.chromium.org/47923023/diff/200001/content/test/data/media/...
content/test/data/media/peerconnection-call.html:573: 'a=rtpmap:100
XVP8/90000\r\n');
Nit: should line up with the other 'a=...' string.

[elham, 2013-11-11 17:13:09]

Fixed the indention and ready to submit

https://codereview.chromium.org/47923023/diff/200001/content/test/data/media/...
File content/test/data/media/peerconnection-call.html (right):

https://codereview.chromium.org/47923023/diff/200001/content/test/data/media/...
content/test/data/media/peerconnection-call.html:127:
addStreamToBothConnectionsAndNegotiate, printGetUserMediaError);
On 2013/11/11 08:00:20, phoglund wrote:
> Nit: should be indented 4 spaces. In general,
> someReallyReallyLongFunctionName(withParametersThatMakeTheLineWrap,
>     andTheSecondParameterIsTooLongToLineUpWithTheLeftParen);
> 
> This is also wrong on lines 117 and 143: please fix that while you're at it.
>     

Done.

https://codereview.chromium.org/47923023/diff/200001/content/test/data/media/...
content/test/data/media/peerconnection-call.html:572: offerSdp=
offerSdp.replace('a=rtpmap:100 VP8/90000\r\n',
On 2013/11/11 08:00:20, phoglund wrote:
> Nit: space between offerSdp and =.

Done.

https://codereview.chromium.org/47923023/diff/200001/content/test/data/media/...
content/test/data/media/peerconnection-call.html:573: 'a=rtpmap:100
XVP8/90000\r\n');
On 2013/11/11 08:00:20, phoglund wrote:
> Nit: should line up with the other 'a=...' string.

Done.

[commit-bot: I haz the power, 2013-11-11 17:13:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/elham@google.com/47923023/250001

[commit-bot: I haz the power, 2013-11-11 17:30:36]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[elham, 2013-11-11 17:38:25]

Adding wjia owner of media_browsertest.cc

[wjia(left Chromium), 2013-11-11 17:59:14]

Could you also try these news tests on Android? If they can't pass or are flaky,
you need disable these tests on Android. You can use the same method used in
content/browser/media/webrtc_browsertest.cc, or add those tests in
build/android/pylib/gtest/filter/content_browsertests_disabled.

[elham, 2013-11-14 21:14:35]

On 2013/11/11 17:59:14, wjia wrote:
> Could you also try these news tests on Android? If they can't pass or are
flaky,
> you need disable these tests on Android. You can use the same method used in
> content/browser/media/webrtc_browsertest.cc, or add those tests in
> build/android/pylib/gtest/filter/content_browsertests_disabled.

I have disabled the two new test on Android.

[wjia(left Chromium), 2013-11-14 21:30:20]

On 2013/11/14 21:14:35, elham wrote:
> On 2013/11/11 17:59:14, wjia wrote:
> > Could you also try these news tests on Android? If they can't pass or are
> flaky,
> > you need disable these tests on Android. You can use the same method used in
> > content/browser/media/webrtc_browsertest.cc, or add those tests in
> > build/android/pylib/gtest/filter/content_browsertests_disabled.
> 
> I have disabled the two new test on Android.

Thanks! I have sent the patch to trybots. Will lg when try jobs are green.

[wjia(left Chromium), 2013-11-14 23:45:25]

lgtm

[commit-bot: I haz the power, 2013-11-15 00:56:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/elham@google.com/47923023/370001

[commit-bot: I haz the power, 2013-11-15 01:15:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/elham@google.com/47923023/370001

[commit-bot: I haz the power, 2013-11-15 03:48:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/elham@google.com/47923023/370001

[commit-bot: I haz the power, 2013-11-15 09:51:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/elham@google.com/47923023/370001

[commit-bot: I haz the power, 2013-11-15 11:44:37]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-18 17:23:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/elham@google.com/47923023/370001

[commit-bot: I haz the power, 2013-11-18 19:51:26]

Change committed as 235798

[elham1, 2013-12-11 21:11:57]

A revert of this CL has been created in
https://codereview.chromium.org/105113008/ by elham@chromium.org.

The reason for reverting is: by mistake I added it here to this closed issue
number. I should have created a new branch and uploaded my changes.

[elham1, 2013-12-11 22:37:10]

On 2013/12/11 21:11:57, elham1 wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/105113008/ by mailto:elham@chromium.org.
> 
> The reason for reverting is: by mistake I added it here to this closed issue
> number. I should have created a new branch and uploaded my changes.

I deleted Patch set 8 from this issue. I will create a new branch and upload my
new changes.