862883 - Enable webrtc mochitest automation for FxAndroid

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Jason Smith [:jsmith]]

Turn on support for mochitests in webrtc on FxAndroid.

Comment 1

[Jason Smith [:jsmith]]

Attachment: Patch for try build testing - not for review

Comment 2

[Jason Smith [:jsmith]]

I've added a patch here so that we can grab this and run a try build run to find out what failures we get when we run the mochitests on Android.

Comment 3

[Jason Smith [:jsmith]]

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

Comment 4

[Jason Smith [:jsmith]]

Looking at the try run, looks like we passed on all gUM tests, so we can land a patch to turn those on right now.

However, every single peer connection test failed here with a reference error of "mozRTCPeerConnection is not defined." I thought the build patch that landed for WebRTC enabled PC support, but the tests are failing here on reference errors.

gcp - Any ideas why?

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Does the test put the media.peerconnection.enabled flag on Android?

Comment 6

[Jason Smith [:jsmith]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #5)
> Does the test put the media.peerconnection.enabled flag on Android?

Yes. See the attached patch - it runs all tests with media.peerconnection.enabled set to true.

Comment 7

[Jason Smith [:jsmith]]

Talked this over with Randell in the weekly meeting - he thinks the problem is that the build be generated on try with the mochitests isn't being compiled with the MOZ_WEBRTC flag, so the Peer Connection bits aren't being picked up.

I'll follow up with gcp in next week's meeting to find out why.

Comment 8

[Jason Smith [:jsmith]]

Putting needinfo on gcp for comment 7 in case he has ideas why the builds being ran in try are not building the WebRTC code on FxAndroid correctly.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

>Yes. See the attached patch - it runs all tests with media.peerconnection.enabled 
>set to true.

But it doesn't set media.navigator.enabled to true.

Comment 10

[Jason Smith [:jsmith]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #9)
> >Yes. See the attached patch - it runs all tests with media.peerconnection.enabled 
> >set to true.
> 
> But it doesn't set media.navigator.enabled to true.

That shouldn't matter. We landed a patch originally that removed the requirement that media.navigator.enabled had to be set.

If that was true, then I'd expect the gUM tests to be failing right now. But that's not happening here.

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Thing is I'm chasing mochitest crashes, and I don't have this problem at all:
https://tbpl.mozilla.org/?tree=Try&rev=43d69806d706

Comment 12

[Jason Smith [:jsmith]]

Hmm...okay. Maybe we changed something then after we preffed gUM or PC on in desktop. Okay, I'll modify the patch to enable both then.

Comment 13

[Gian-Carlo Pascutto [:gcp]]

So I had another try push today that didn't include the prefs flip:
https://tbpl.mozilla.org/?tree=Try&rev=286bad8d4740

Don't see the problem there at all.

Comment 14

[Jason Smith [:jsmith]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #13)
> So I had another try push today that didn't include the prefs flip:
> https://tbpl.mozilla.org/?tree=Try&rev=286bad8d4740
> 
> Don't see the problem there at all.

Okay. I'll update the above patch for something that can be landed then.

Comment 15

[Gian-Carlo Pascutto [:gcp]]

I just landed the code to make things pass on inbound, so a patch to enable them would be beat.

Comment 16

[Gian-Carlo Pascutto [:gcp]]

..neat.

Comment 17

[Jason Smith [:jsmith]]

Attachment: Enable WebRTC Crashtests on FxAndroid v1

Comment 18

[Jason Smith [:jsmith]]

Attachment: Enable WebRTC mochitests on FxAndroid v1

Comment 19

[Jason Smith [:jsmith]]

For people flagged down for review/feedback:

- review on gcp to ensure these tests then effectively will run on FxAndroid
- feedback on whimboo to see if he's okay with the framework changes to move towards the exclude list model, rather than the weird UA stuff we were doing originally
- feedback on mw22 to ensure I did the b2g.json setup correct

Comment 20

[Jason Smith [:jsmith]]

Comment on attachment 744701 [details] [diff] [review]
Enable WebRTC mochitests on FxAndroid v1

Apparently the perm pref is defined incorrectly.

Comment 21

[Jason Smith [:jsmith]]

Attachment: Enable WebRTC mochitests on FxAndroid v2

Comment 22

[Jason Smith [:jsmith]]

Comment on attachment 744706 [details] [diff] [review]
Enable WebRTC mochitests on FxAndroid v2

This time, with the right config pref.

Comment 23

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 744706 [details] [diff] [review]
Enable WebRTC mochitests on FxAndroid v2

Looks good to me, but you prolly want an r+ from an actual peer (I know next to nothing about the rests).

Comment 24

[Jason Smith [:jsmith]]

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

Comment 25

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

Comment on attachment 744706 [details] [diff] [review]
Enable WebRTC mochitests on FxAndroid v2

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

Looks fine to me except for the question you can find below.

::: dom/media/tests/mochitest/head.js
@@ +116,3 @@
>    SimpleTest.waitForExplicitFinish();
> +  SpecialPowers.pushPrefEnv({'set': [
> +    ['media.peerconnection.enabled', true],

We should also remove this preference given that peer connection is enabled by default.

::: testing/mochitest/b2g.json
@@ +203,5 @@
>      "dom/file/test/test_workers.html":"",
>      "dom/file/test/test_write_read_data.html":"",
>      "dom/imptests/editing/conformancetest/test_event.html":"",
>      "dom/imptests/editing/conformancetest/test_runtest.html":"",
> +    "dom/media/tests/mochitest":"",

Can we already run all our mochitests on B2G?

Comment 26

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #25)
> Comment on attachment 744706 [details] [diff] [review]
> Enable WebRTC mochitests on FxAndroid v2
> 
> Review of attachment 744706 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine to me except for the question you can find below.
> 
> ::: dom/media/tests/mochitest/head.js
> @@ +116,3 @@
> >    SimpleTest.waitForExplicitFinish();
> > +  SpecialPowers.pushPrefEnv({'set': [
> > +    ['media.peerconnection.enabled', true],
> 
> We should also remove this preference given that peer connection is enabled
> by default.

Actually, we can't right now. FxAndroid requires the preference to be set in order to use the mozRTCPeerConnection API right now (i.e. it's preffed off by default).

> 
> ::: testing/mochitest/b2g.json
> @@ +203,5 @@
> >      "dom/file/test/test_workers.html":"",
> >      "dom/file/test/test_write_read_data.html":"",
> >      "dom/imptests/editing/conformancetest/test_event.html":"",
> >      "dom/imptests/editing/conformancetest/test_runtest.html":"",
> > +    "dom/media/tests/mochitest":"",
> 
> Can we already run all our mochitests on B2G?

Don't think so. The WebRTC APIs are not implemented on B2G yet, so the tests will fail if we run them on B2G right now. This is being worked on though.

Comment 27

[Jason Smith [:jsmith]]

Try run looks green outside of the one failure we already know about in bug 866093. That's been fixed, so we should be good to land so long as I get the r+ from jesup.

Comment 28

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

(In reply to Jason Smith [:jsmith] from comment #26)
> > ::: testing/mochitest/b2g.json
> > @@ +203,5 @@
> > >      "dom/file/test/test_workers.html":"",
> > >      "dom/file/test/test_write_read_data.html":"",
> > >      "dom/imptests/editing/conformancetest/test_event.html":"",
> > >      "dom/imptests/editing/conformancetest/test_runtest.html":"",
> > > +    "dom/media/tests/mochitest":"",
> > 
> > Can we already run all our mochitests on B2G?
> 
> Don't think so. The WebRTC APIs are not implemented on B2G yet, so the tests
> will fail if we run them on B2G right now. This is being worked on though.

So what does it then mean when we add this line? Doesn't it include all of the mochitests in the current set of executable tests?

Comment 29

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #28)
> (In reply to Jason Smith [:jsmith] from comment #26)
> > > ::: testing/mochitest/b2g.json
> > > @@ +203,5 @@
> > > >      "dom/file/test/test_workers.html":"",
> > > >      "dom/file/test/test_write_read_data.html":"",
> > > >      "dom/imptests/editing/conformancetest/test_event.html":"",
> > > >      "dom/imptests/editing/conformancetest/test_runtest.html":"",
> > > > +    "dom/media/tests/mochitest":"",
> > > 
> > > Can we already run all our mochitests on B2G?
> > 
> > Don't think so. The WebRTC APIs are not implemented on B2G yet, so the tests
> > will fail if we run them on B2G right now. This is being worked on though.
> 
> So what does it then mean when we add this line? Doesn't it include all of
> the mochitests in the current set of executable tests?

It's the opposite. The line I'm adding here adds the directory of tests to the exclude list, not the include list.

See http://hg.mozilla.org/mozilla-central/file/da429c311864/testing/mochitest/b2g.json.

Comment 30

[Martijn Wargers (dead)]

Comment on attachment 744706 [details] [diff] [review]
Enable WebRTC mochitests on FxAndroid v2

Thanks!

Comment 31

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

Comment on attachment 744706 [details] [diff] [review]
Enable WebRTC mochitests on FxAndroid v2

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

::: dom/media/tests/mochitest/head.js
@@ +116,3 @@
>    SimpleTest.waitForExplicitFinish();
> +  SpecialPowers.pushPrefEnv({'set': [
> +    ['media.peerconnection.enabled', true],

We need to keep the ability for people to disable it, at least to provide a firestop for security issues

Comment 32

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/987686854a21

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/987686854a21