795367 - Establish makefiles/directories for gUM/webrtc test automation and add a simple gUM error test

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Comment 1

[Jason Smith [:jsmith]]

Attachment: Directory Structure Patch

Comment 2

[Jason Smith [:jsmith]]

Attachment: Directory Structure Patch

Comment 3

[Jason Smith [:jsmith]]

Either Henrik, Rob, or Dave Hunt can review this. It's a simple patch that just puts out the directory structure for where mochitests will be put along with the MakeFile built.

Comment 4

[Jason Smith [:jsmith]]

Comment on attachment 665957 [details] [diff] [review]
Directory Structure Patch

Apparently mochitest doesn't appear to like makefiles that have no mochitests in them. Probably an easy work-around is just to land the "working pieces" of the other automated test I had along with the directory structure. That way, I don't have to fight no mochitest in makefile issue and I land an automated test with it.

Comment 5

[Jason Smith [:jsmith]]

Attachment: Establish directory, makefile, and sample error test

Updated the patch. I've added a sample test that does very bare basic error checking on gUM along with establishing the directory and makefile structure for webrtc & gUM automated tests. Tested this on my local machine by building and running mochitests a couple of times and hit no issues (tests passed consistently). 

- Need Henrik to review to confirm if he's alright with the makefile and directory structure
- Need Anant to review the mochitest portion

Note - The mochitest implemented here is by no means extensive to all error cases. I'll probably file followup bugs to add more tests down the line, but the initial goal here is to establish the directory, makefile, and get a sample test up and running.

Comment 6

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

Comment on attachment 666256 [details] [diff] [review]
Establish directory, makefile, and sample error test

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

I think that looks like a good start to get the infrastructure for Mochitests landed for WebRTC. But there are some parts which are not that clear right now and where I would like to get more information on. 

Regarding the naming schema. Which prefix do we want to make use of?

test_gum_ for gUM tests?
test_pc_ for peer connection?

If not I would tend to say we should create sub folders so we can separate those tests.

::: dom/tests/mochitest/Makefile.in
@@ -34,5 @@
>  #DIRS	+= notification
>  #endif
>  
>  include $(topsrcdir)/config/rules.mk
> -

Please keep this empty line at the end of the file.

::: dom/tests/mochitest/media/Makefile.in
@@ +6,5 @@
> +topsrcdir = @top_srcdir@
> +srcdir = @srcdir@
> +VPATH = @srcdir@
> +relativesrcdir = @relativesrcdir@
> +

I think that you should make use of tabs in all of the makefiles.

For an example see:
http://hg.mozilla.org/projects/alder/file/02555ba14aba/dom/tests/mochitest/bugs/Makefile.in#l6

@@ +11,5 @@
> +include $(DEPTH)/config/autoconf.mk
> +
> +MOCHITEST_FILES	= \
> +  test_getusermedia_errors.html \
> +  $(NULL)

I would intend with a tab and 4 characters here too.

::: dom/tests/mochitest/media/test_getusermedia_errors.html
@@ +34,5 @@
> +function checkNotSupportedErr() {
> +  var errNeeded = "NOT_SUPPORTED_ERR";
> +
> +  function onError(err) {
> +    ok(err == errNeeded, "Needed: " + errNeeded + ", got: " + err);

If we want to do a loosely comparison you should use is() otherwise ise(). I would prefer the latter. Then you can also remove 'needed.... got' which gets added automatically.

@@ +42,5 @@
> +    onError);
> +  navigator.mozGetUserMedia({picture: true, audio: true}, unexpectedCall,
> +    onError);
> +  navigator.mozGetUserMedia({video: true, audio: true, picture: true},
> +    unexpectedCall, onError);

Mind giving me a bit of information here? What should be the results of those tests? You always assign the same callbacks. Does it mean we end-up with "NOT_SUPPORTED_ERR" for any of those navigator.mozGetUserMedia() calls? A comment before the code would help through.

Do we have to take care of:
https://developer.mozilla.org/en-US/docs/Mochitest#What_if_my_tests_aren.27t_done_when_onload_fires.3F ?

Means do we have to wait until the page has been finished loading before we can start the test?

@@ +56,5 @@
> +    navigator.mozGetUserMedia();
> +  } catch(err) {
> +    exception = true;
> +  }
> +  ok(exception, "Exception should fire - not enough arguments");

Please make use of the exact failure message, e.g. 'No arguments raises an exception". Otherwise it's unnecessary harder to find the causing line. This applies to all of the lines.

@@ +73,5 @@
> +  } catch(err) {
> +    exception = true;
> +  }
> +
> +  ok(exception, "Exception should fire - not enough arguments");

nit: Unnecessary empty line above.

@@ +108,5 @@
> +}
> +
> +checkNotSupportedErr();
> +checkNumberOfArguments();
> +checkObjectTypes();

I'm not sure yet if the first test really fits in this test file or if it should better be a separate one. I hope with your feedback it's more clear.

Comment 7

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

Jason, do you have an update for us? It's already late in the week and it would be great have this code checked-in yesterday. Thanks.

Comment 8

[Jason Smith [:jsmith]]

Attachment: Establish directory, makefile, and sample error test v2

Comment 9

[Jason Smith [:jsmith]]

Attachment: Establish directory, makefile, and sample error test v2

Comment 10

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #6)
> Comment on attachment 666256 [details] [diff] [review]
> Establish directory, makefile, and sample error test
> 
> Review of attachment 666256 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think that looks like a good start to get the infrastructure for
> Mochitests landed for WebRTC. But there are some parts which are not that
> clear right now and where I would like to get more information on. 
> 
> Regarding the naming schema. Which prefix do we want to make use of?
> 
> test_gum_ for gUM tests?
> test_pc_ for peer connection?

I'd rather be explicit to the API under test specifically:

test_getusermedia_ <-- gUM tests
test_peerconnection_ <-- peer connection tests

It'll probably result in longer file names, but the tradeoff is readability, which I think has value.

> 
> If not I would tend to say we should create sub folders so we can separate
> those tests.

I think this is good idea to take care of when the complexity of the folder warrants a greater breakdown of tests by type. Right now though, I don't expect that to be a concern. If it becomes a concern, let's address when we need to.

> 
> ::: dom/tests/mochitest/Makefile.in
> @@ -34,5 @@
> >  #DIRS	+= notification
> >  #endif
> >  
> >  include $(topsrcdir)/config/rules.mk
> > -
> 
> Please keep this empty line at the end of the file.

Done.

> 
> ::: dom/tests/mochitest/media/Makefile.in
> @@ +6,5 @@
> > +topsrcdir = @top_srcdir@
> > +srcdir = @srcdir@
> > +VPATH = @srcdir@
> > +relativesrcdir = @relativesrcdir@
> > +
> 
> I think that you should make use of tabs in all of the makefiles.
> 
> For an example see:
> http://hg.mozilla.org/projects/alder/file/02555ba14aba/dom/tests/mochitest/
> bugs/Makefile.in#l6

Done.

> 
> @@ +11,5 @@
> > +include $(DEPTH)/config/autoconf.mk
> > +
> > +MOCHITEST_FILES	= \
> > +  test_getusermedia_errors.html \
> > +  $(NULL)
> 
> I would intend with a tab and 4 characters here too.

Done.

> 
> ::: dom/tests/mochitest/media/test_getusermedia_errors.html
> @@ +34,5 @@
> > +function checkNotSupportedErr() {
> > +  var errNeeded = "NOT_SUPPORTED_ERR";
> > +
> > +  function onError(err) {
> > +    ok(err == errNeeded, "Needed: " + errNeeded + ", got: " + err);
> 
> If we want to do a loosely comparison you should use is() otherwise ise(). I
> would prefer the latter. Then you can also remove 'needed.... got' which
> gets added automatically.

Done.

> 
> @@ +42,5 @@
> > +    onError);
> > +  navigator.mozGetUserMedia({picture: true, audio: true}, unexpectedCall,
> > +    onError);
> > +  navigator.mozGetUserMedia({video: true, audio: true, picture: true},
> > +    unexpectedCall, onError);
> 
> Mind giving me a bit of information here? What should be the results of
> those tests? You always assign the same callbacks. Does it mean we end-up
> with "NOT_SUPPORTED_ERR" for any of those navigator.mozGetUserMedia() calls?
> A comment before the code would help through.

Added a comment to explain this. Basically, I'm expecting an error callback to fire here with NOT_SUPPORTED_ERR, as these combinations of parameters into mozGetUserMedia is not allowed.

> 
> Do we have to take care of:
> https://developer.mozilla.org/en-US/docs/Mochitest#What_if_my_tests_aren.
> 27t_done_when_onload_fires.3F ?
> 
> Means do we have to wait until the page has been finished loading before we
> can start the test?

I believe Clint mentioned that we did not have deal with this case.

> 
> @@ +56,5 @@
> > +    navigator.mozGetUserMedia();
> > +  } catch(err) {
> > +    exception = true;
> > +  }
> > +  ok(exception, "Exception should fire - not enough arguments");
> 
> Please make use of the exact failure message, e.g. 'No arguments raises an
> exception". Otherwise it's unnecessary harder to find the causing line. This
> applies to all of the lines.

Done.

> 
> @@ +73,5 @@
> > +  } catch(err) {
> > +    exception = true;
> > +  }
> > +
> > +  ok(exception, "Exception should fire - not enough arguments");
> 
> nit: Unnecessary empty line above.

Done.

> 
> @@ +108,5 @@
> > +}
> > +
> > +checkNotSupportedErr();
> > +checkNumberOfArguments();
> > +checkObjectTypes();
> 
> I'm not sure yet if the first test really fits in this test file or if it
> should better be a separate one. I hope with your feedback it's more clear.

My original intention of using one file here was to focus on errors (both exception-based and expected errors) that immediately happen upon calling mozGetUserMedia. They would not focus on errors that would happen after a success callback happened. So I focused test file from a perspective of a caller who errors out immediately on the call. In that case, I actually did group exception-based tests with expected error code callbacks.

If in the future, if the complexity of this file starts to become unwieldy or the test file demonstrates a loss of cohesion of what's intended to be tested, then I think we could break this out into separate files. But I think that could be done when the complexity warrants file separation.

Comment 11

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

Comment on attachment 668290 [details] [diff] [review]
Establish directory, makefile, and sample error test v2

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

This looks great. I wonder if we should push this through try server or if we can land directly.

Randell, what's the default process on the project branch? If we can land directly, would you mind doing it? I might not have the right level to do so. Thanks.

Comment 12

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #11)
> Comment on attachment 668290 [details] [diff] [review]
> Establish directory, makefile, and sample error test v2
> 
> Review of attachment 668290 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks great. I wonder if we should push this through try server or if
> we can land directly.
> 
> Randell, what's the default process on the project branch? If we can land
> directly, would you mind doing it? I might not have the right level to do
> so. Thanks.

Btw - we'll also want to land this on mozilla-central too (the automated test that's here should be able to run on central right now).

Comment 13

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

Comment on attachment 668290 [details] [diff] [review]
Establish directory, makefile, and sample error test v2

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

While checking the patch outside of splinter review I have seen that your patch contains a lot of DOS endings. Please clean those up so we only have Linux line endings which is a requirement afair. Sorry for missing that earlier.

::: dom/tests/mochitest/media/Makefile.in
@@ +11,5 @@
> +include $(DEPTH)/config/autoconf.mk
> +
> +MOCHITEST_FILES	= \
> +		test_getusermedia_errors.html \
> +		$(NULL)

Beside the above please also kill the extra tab you put in here.

Comment 14

[Jason Smith [:jsmith]]

Attachment: Establish directory, makefile, and sample error test v3

Comment 15

[Jason Smith [:jsmith]]

Comment on attachment 668472 [details] [diff] [review]
Establish directory, makefile, and sample error test v3

Ran the diff through notepad++ eol converter to UNIX and removed the one tab on that one makefile.

Comment 16

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

Comment on attachment 668472 [details] [diff] [review]
Establish directory, makefile, and sample error test v3

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

Good to have tryserver! So this failed again because we do not have valid devices on the build machines. See the tbpl results:

15067 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/media/test_getusermedia_errors.html | NOT_SUPPORTED_ERR should have been found - got NO_DEVICES_FOUND, expected NOT_SUPPORTED_ERR

So in all the above cases when you call GetUserMedia you will have to use faked media devices.

Comment 17

[Jason Smith [:jsmith]]

Attachment: Establish directory, makefile, and sample error test v4

Haven't tested this yet, but this should fix the problem seen on the try server.

Comment 18

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

Comment on attachment 668530 [details] [diff] [review]
Establish directory, makefile, and sample error test v4

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

::: dom/tests/mochitest/media/test_getusermedia_errors.html
@@ +37,5 @@
> +  function onError(err) {
> +    is(err, errNeeded, "NOT_SUPPORTED_ERR should have been found");
> +  }
> +
> +	// All function calls here should throw a NOT_SUPPORTED_ERR

What's caused this indentation change?

@@ +38,5 @@
> +    is(err, errNeeded, "NOT_SUPPORTED_ERR should have been found");
> +  }
> +
> +	// All function calls here should throw a NOT_SUPPORTED_ERR
> +  navigator.mozGetUserMedia({fake: true, picture: true, video: true}, unexpectedCall,

I would propose to put fake at the end, because it's lesser important as the media type and will be true for all the Mochitests.

@@ +63,5 @@
> +  exception = false;
> +  try {
> +    navigator.mozGetUserMedia({video: true});
> +  } catch(err) {
> +    exception = true;

Which exception gets thrown here? If it's 'NO_DEVICES_FOUND' you will have to also add fake.

@@ +71,5 @@
> +  exception = false;
> +  try {
> +    navigator.mozGetUserMedia({video: true}, unexpectedCall);
> +  } catch(err) {
> +    exception = true;

Same here.

@@ +93,5 @@
> +  exception = false;
> +  try {
> +    navigator.mozGetUserMedia({video: true}, 1, unexpectedCall);
> +  } catch(err) {
> +    exception = true;

Same here.

@@ +101,5 @@
> +  exception = false;
> +  try {
> +    navigator.mozGetUserMedia({video: true}, unexpectedCall, 1);
> +  } catch(err) {
> +    exception = true;

And here.

Comment 19

[Jason Smith [:jsmith]]

Attachment: Establish directory, makefile, and sample error test v5

let's try again with the right cleanup this time.

Comment 20

[Jason Smith [:jsmith]]

Attachment: Establish directory, makefile, and sample error test v5

Comment 21

[Jason Smith [:jsmith]]

This time, I'm going to wait and test this to make sure I do this right.

Comment 22

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

Jason, at the same time please try to save the file with the UTF-8 encoding.

Comment 23

[Jason Smith [:jsmith]]

Attachment: Establish directory, makefile, and sample error test v6

Comment 24

[Jason Smith [:jsmith]]

Updated patch to address the above feedback. Realized after digging into this for a while that my issues I was running into yesterday include some odd hg issue on my machine (which I'll figure out separately) and the fact that I do not have try server access, so I can't push this to try.

Comment 25

[Jason Smith [:jsmith]]

Comment on attachment 668811 [details] [diff] [review]
Establish directory, makefile, and sample error test v6

Someone will need to execute a try run on this.

Comment 26

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

I have pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=d14ff033ebdf

Once everything is fine I will do the final review. I'm hesitated to do it before given the overall new experience for that component.

Comment 27

[Mozilla RelEng Bot]

Try run for e9571214479a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e9571214479a
Results (out of 16 total builds):
    exception: 15
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e9571214479a

Comment 28

[Jason Smith [:jsmith]]

Comment on attachment 668811 [details] [diff] [review]
Establish directory, makefile, and sample error test v6

Just saw the linux test results - looks like it's failing because we've found a bug through the test when we add the fake: true into the playing field:

- {picture: true, video: true, fake: true} - returns a success callback, but should have had an error of NOT_SUPPORTED_ERR
- {picture: true, audio: true, fake: true} - returns a success callback, but should have had an error of NOT_SUPPORTED_ERR
- {video: true, audio: true, picture: true, fake: true} - returns NOT_IMPLEMENTED error, but should have returned NOT_SUPPORTED_ERR, as we do not support callbacks with video, audio, and picture all equal to true

I'll file bugs for these. For now, I'll have to probably add a todo for those tests, as they won't work right now.

Comment 29

[Jason Smith [:jsmith]]

Attachment: Establish directory, makefile, and sample error test v7

Changed the is to todo_is, as that test won't pass as of right now due to a known bug. So long as the try passes for the other tests, we should be okay.

Comment 30

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

Jason, thank you for the quick update. When you file the bugs please ensure to add them to the dependency list. That will give us direct insight when behavior is changing. I will push the updated patch to try now. Watch out for results. Hopefully it will work this time.

> 15066 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/media/test_getusermedia_errors.html | Unexpected success - we shouldn't get here

Is there a chance that we can get more information here? What does the obj parameter contain. Would be good to know e.g. which media types have been requested.

Comment 31

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

Just as a note, please add a comment to the todo_is call which references the bugs and explain why we have to do it.

Comment 32

[Mozilla RelEng Bot]

Try run for d14ff033ebdf is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d14ff033ebdf
Results (out of 124 total builds):
    success: 93
    warnings: 31
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d14ff033ebdf

Comment 33

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

We are still getting:

> 15066 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/media/test_getusermedia_errors.html | Unexpected success - we shouldn't get here
> 15067 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/media/test_getusermedia_errors.html | Unexpected success - we shouldn't get here

I would say we kill this sub test for now and add it later. I really want to get this patch landed so that we are unblocked.

Comment 34

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

Once you have something, I can land it.  Uplift is (I think) tomorrow morning.  I can land on m-c directly if that makes sense.  If it doesn't need to make Aurora uplift, we have more options. We can probably uplift the patch after Aurora is pulled.

Comment 35

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #33)
> We are still getting:
> 
> > 15066 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/media/test_getusermedia_errors.html | Unexpected success - we shouldn't get here
> > 15067 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/media/test_getusermedia_errors.html | Unexpected success - we shouldn't get here
> 
> I would say we kill this sub test for now and add it later. I really want to
> get this patch landed so that we are unblocked.

Oh, I see the error I did. I left behind the success call against the ok call, so the test still fails. I'll just kill the test as you said.

Comment 36

[Jason Smith [:jsmith]]

Attachment: Establish directory, makefile, and sample error test v8

Comment 37

[Jason Smith [:jsmith]]

Comment on attachment 668980 [details] [diff] [review]
Establish directory, makefile, and sample error test v8

The diff file here is corrupt, rebuilding now.

Comment 38

[Jason Smith [:jsmith]]

Attachment: Establish directory, makefile, and sample error test v8

Comment 39

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

Pushed to try again, just to ensure we are good now:
https://tbpl.mozilla.org/?tree=Try&rev=7eeebad97c7a

(In reply to Randell Jesup [:jesup] from comment #34)
> Once you have something, I can land it.  Uplift is (I think) tomorrow
> morning.  I can land on m-c directly if that makes sense.  If it doesn't
> need to make Aurora uplift, we have more options. We can probably uplift the
> patch after Aurora is pulled.

If you can land it directly that would be appreciated. Not sure about uplift yet. I think we should talk about once a couple more tests have been made their way into m-c. But given that WebRTC is prefed off on aurora I think we should concentrate our work on m-c, unless you are having in mind to enable the pref later on that branch.

Comment 40

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

While talking about the pref there is one more thing which flipped through here. If gUM is disabled on the aurora branch the test would have to enable the preference before it can actually start. So we probably want to wait with the landing after the merge and then get a shared module landed which can do the initial work. I'm currently working on it as part of my patch for bug 796890.

Comment 41

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

Comment on attachment 668984 [details] [diff] [review]
Establish directory, makefile, and sample error test v8

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

The tests are passing this time, which is great. And I also don't see anything else which blocks us from landing except the merge. So lets wait and get it checked-in later today.

Comment 42

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

Comment on attachment 668984 [details] [diff] [review]
Establish directory, makefile, and sample error test v8

Actually this also needs a peer review.

Comment 43

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #41)
> Comment on attachment 668984 [details] [diff] [review]
> Establish directory, makefile, and sample error test v8
> 
> Review of attachment 668984 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The tests are passing this time, which is great. And I also don't see
> anything else which blocks us from landing except the merge. So lets wait
> and get it checked-in later today.

gUM is disabled on every branch (behind a preference), which would imply this test shouldn't be passing. But based on the try run, it is passing. Why is that the case?

Comment 44

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

(In reply to Jason Smith [:jsmith] from comment #43)
> gUM is disabled on every branch (behind a preference), which would imply
> this test shouldn't be passing. But based on the try run, it is passing. Why
> is that the case?

Because we run this against the alder branch and not mozilla-central. As given by derf on IRC it's even disabled at compile-time for Android. So that would add another level of conditional skip here. Would you have the time to investigate that or should I take over from now on?

Comment 45

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #44)
> (In reply to Jason Smith [:jsmith] from comment #43)
> > gUM is disabled on every branch (behind a preference), which would imply
> > this test shouldn't be passing. But based on the try run, it is passing. Why
> > is that the case?
> 
> Because we run this against the alder branch and not mozilla-central. As
> given by derf on IRC it's even disabled at compile-time for Android. So that
> would add another level of conditional skip here. Would you have the time to
> investigate that or should I take over from now on?

This try run should have been ran against mozilla-central and alder, as this test applies to both trees (primarily mozilla-central). That explains why the test didn't get flagged as a failure. I'll look into it.

Comment 46

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

Jason, you should make use of SpecialPowers.pushPrefEnv() to set the pref for the specific test. That should be all for the prefed-off feature.

Comment 47

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

Resetting [blocking-webrtc-] flag for now given that it has been triaged wrongly. This bug has been setup to get the initial code landed which is necessary to run Mochitests. Without it none of our other [blocking-webrtc+] Mochitest smoketests bugs can be landed.

So please reconsider the decision in your next meeting. This bug needs a higher priority.

Comment 48

[Jason Smith [:jsmith]]

Apparently my build of central for webrtc is acting up. If anyone has a build that's working and feels the urgency to finish off this bug, then feel free to steal. Otherwise, I'll finish this off when I get help from Randell to diagnose why build is acting up.

Comment 49

[Jason Smith [:jsmith]]

The two things that are left to do here include:

- Enabling the gUM tests only on Desktop and Android through the makefile
- Setting the special powers preference for media.navigator.enabled to true

After that, one last try run needs to be executed on mozilla central to ensure that this works on central.

Comment 50

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

I will take it now and hope we can get it landed by today.

Comment 51

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

I pushed an updated patch to try. Lets see if it fails on Android now. The reason why it was probably passing is that we do not check the exception type for all of the tests and this hides failures if gUM is not available.

https://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]

If we have failures I will update the makefiles to not run the tests on Android yet.

Comment 52

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

This is strange. Even with the changes made all the tests are passing for any Android build:

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

TEST-PASS | /tests/dom/tests/mochitest/media/test_getusermedia_errors.html | No arguments should fire a XPC_NOT_ENOUGH_ARGS exception

So I'm currently not sure if we really have to disable the tests for mobile.

Randell and Eric, I would really appreciate your feedback on it.

Comment 53

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #52)
> This is strange. Even with the changes made all the tests are passing for
> any Android build:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=2b677146cbe0
> 
> TEST-PASS | /tests/dom/tests/mochitest/media/test_getusermedia_errors.html |
> No arguments should fire a XPC_NOT_ENOUGH_ARGS exception
> 
> So I'm currently not sure if we really have to disable the tests for mobile.
> 
> Randell and Eric, I would really appreciate your feedback on it.

mozGetUserMedia exists on Android as well. Only picture: true support currently exists. It's expected that that mochitest should be able to be ran on Android, as it isn't really doing anything that's desktop or android specific - it's just number of args + object type tests (not OS dependent).

Comment 54

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

That is information I haven't had before. So we could indeed have the initial error test run on all devices and only have to exclude all the others which do more than checking for the parameters. That way we can have at least a really minimal coverage. I will push another time with a real number and type of parameters to try and see how it looks like on Android. Thanks Jason.

Comment 55

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

(In reply to Jason Smith [:jsmith] from comment #53)
> mozGetUserMedia exists on Android as well. Only picture: true support
> currently exists. It's expected that that mochitest should be able to be ran
> on Android, as it isn't really doing anything that's desktop or android
> specific - it's just number of args + object type tests (not OS dependent).


This doesn't seem to be true. By adding the following code I still get a pass on Android:

  navigator.mozGetUserMedia({video: true, fake: true}, function (stream) {
    ok(stream, "Got a video stream");
  }, function error() {
    ok(false, "No error should have been thrown");
  });

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

TEST-PASS | /tests/dom/tests/mochitest/media/test_getusermedia_errors.html | Got a video stream

So I really question now what can be done and what not. I would really appreciate to get this information from the developers. Randell please help us.

Comment 56

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

By using a peer connection in the test we timeout on Android:
https://tbpl.mozilla.org/?tree=Try&rev=41c48e9ff697

So I would suggest we are trying to get as much as possible tests landed on all platforms and only exclude those tests from Android which are failing right now.

Comment 57

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

Probably a bug in the argument parsing on Android - not generating a correct error; I suspect the stream was NULL.

Comment 58

[Jason Smith [:jsmith]]

(In reply to Randell Jesup [:jesup] from comment #57)
> Probably a bug in the argument parsing on Android - not generating a correct
> error; I suspect the stream was NULL.

Sounds like we've hit bug 786466.

Comment 59

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #55)
> (In reply to Jason Smith [:jsmith] from comment #53)
> > mozGetUserMedia exists on Android as well. Only picture: true support
> > currently exists. It's expected that that mochitest should be able to be ran
> > on Android, as it isn't really doing anything that's desktop or android
> > specific - it's just number of args + object type tests (not OS dependent).
> 
> 
> This doesn't seem to be true. By adding the following code I still get a
> pass on Android:
> 
>   navigator.mozGetUserMedia({video: true, fake: true}, function (stream) {
>     ok(stream, "Got a video stream");
>   }, function error() {
>     ok(false, "No error should have been thrown");
>   });
> 
> https://tbpl.mozilla.org/?tree=Try&rev=4f00e7d40c19
> 
> TEST-PASS | /tests/dom/tests/mochitest/media/test_getusermedia_errors.html |
> Got a video stream
> 
> So I really question now what can be done and what not. I would really
> appreciate to get this information from the developers. Randell please help
> us.

Anyways - my argument still stands that the mochitest could still be ran on Android even with bug 786466 there. We aren't planning to fix that bug anytime soon, as the behavior that occurs as a result of requesting video or audio true isn't catastrophic (it just gives us a NULL stream). The test is still doing generic checks even with bug 786466 existing.

It might be worth adding a comment in the test about that bug that the test will be needing to be updated if bug 786466 gets fixed.

Comment 60

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

(In reply to Jason Smith [:jsmith] from comment #59)
> It might be worth adding a comment in the test about that bug that the test
> will be needing to be updated if bug 786466 gets fixed.

No, you will notice that when running the test. But I will make a note on bug 786466 for sure.

Comment 61

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

Attachment: Patch v9

This updates the test and adds a head.js module to make it easier to write Mochitests for WebRTC in the future. More methods will be added soon. Right now I can't push to try for a final check, so I wonder if I can get a review in between.

Comment 62

[Jason Smith [:jsmith]]

Comment on attachment 670974 [details] [diff] [review]
Patch v9

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

Yuck. test_getUserMedia_errors.html looks like a readability mess. I liked the way it was written before because it would give clear direction to the what the test was actually aiming to do and what the expected results are. But although this functionally reduces the code size, it drops the readability of the test significantly.  I usually lean on the side of aiming for tests that give clear indication of what's being tested at some sacrifice of occasional small code reuse, as this gives clear direction to someone who picks up the code later.

We went down this route early on with the mozapps tests with too much obsecure abstraction, which ended up leaving people entirely confused how to maintain the code. I'd rather lean on the side of having a clear direction on what's actually being tested and the results of those tests.

Comment 63

[Jason Smith [:jsmith]]

I spoke with ctalbert on this - both approaches are technically okay, although if you go down a less boilerplate approach like what's seen in this patch, then you need to add a bit more comments in your code to still give clarification to exactly what's going on in this code. There definitely needs to be a lot more comments in here to give clarification of what's going on.

Although I will mention that it would have probably have been easier to stick with the original approach that already worked with only one small piece to add to the code about the special powers.

Comment 64

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #62)
> Comment on attachment 670974 [details] [diff] [review]
> Patch v9
> 
> Review of attachment 670974 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yuck. test_getUserMedia_errors.html looks like a readability mess. I liked
> the way it was written before because it would give clear direction to the
> what the test was actually aiming to do and what the expected results are.
> But although this functionally reduces the code size, it drops the
> readability of the test significantly.  I usually lean on the side of aiming
> for tests that give clear indication of what's being tested at some
> sacrifice of occasional small code reuse, as this gives clear direction to
> someone who picks up the code later.
> 
> We went down this route early on with the mozapps tests with too much
> obsecure abstraction, which ended up leaving people entirely confused how to
> maintain the code. I'd rather lean on the side of having a clear direction
> on what's actually being tested and the results of those tests.

Sorry, the wording here I think came across a bit harsh. What I really was intending to describe here is that we need still need to maintain the readability of the test, even if it's switched to a less boilerplate approach.

Comment 65

[Jason Smith [:jsmith]]

Attachment: Makefile and directory establishment with gum exception test v10

Comment 66

[Jason Smith [:jsmith]]

Attachment: Makefile and directory establishment with gum exception test v10

Comment 67

[Jason Smith [:jsmith]]

Comment on attachment 671132 [details] [diff] [review]
Makefile and directory establishment with gum exception test v10

I understand the urgency for this patch so I finished off the changes based on the comments I made above. I also fixed a possible bug in this patch that might cause this test become orange:

- Added a bunch of comments to give some clarity to what's going on
- Fixed the test harness to also disable the permission prompt for mozGetUserMedia

Comment 68

[Jason Smith [:jsmith]]

Comment on attachment 671132 [details] [diff] [review]
Makefile and directory establishment with gum exception test v10

Actually, at this point, this probably only needs Randell's review.

Comment 69

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

Comment on attachment 671132 [details] [diff] [review]
Makefile and directory establishment with gum exception test v10

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

::: dom/tests/mochitest/media/Makefile.in
@@ +10,5 @@
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +MOCHITEST_FILES	= \
> +	test_getUserMedia_exceptions.html \

Two spaces (I don't care that much, but some do)

Comment 70

[Jason Smith [:jsmith]]

Attachment: Makefile and directory establishment with gum exception test v11

Comment 71

[Jason Smith [:jsmith]]

Comment on attachment 671181 [details] [diff] [review]
Makefile and directory establishment with gum exception test v11

Carry forward Randell's r+, as this just a whitespace change in the makefile.

Comment 72

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/796eea064cd4

Comment 73

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/796eea064cd4

Comment 74

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

Thanks Jason for taking care of follow-up work. Good to see it has been landed. Just one thing, this patch again is based on files saved in DOS but not Linux mode. I have mentioned that a while back. So please fix that. For this case we have to update the code with a follow-up patch. :( So in the future please ask for review from Rob, Dave, or myself before a test gets landed. Thanks.

Comment 75

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

Looks like the push fixed the white-space issues. So we should be ok.