489817 - migrate existing geolocation tests from mochitest to browser-chrome

Status: VERIFIED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

Per a discussion in IRC regarding geolocation tests, we decided they should be moved from the current mochitest implementation to browser-chrome.

Comment 1

[(not currently active) Ted Mielczarek]

Is this just because the tests need chrome privs to add the testing provider?

Comment 2

[Doug Turner (:dougt)]

right.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So we will have no testing that geolocation works in Gecko outside Firefox?

Comment 4

[Robert Kaiser]

SeaMonkey actually runs browser-chrome tests in common directories, but if we want to use *anything* in the tests that is implemented in browser/ they should not live in shared directories.

Comment 5

[Doug Turner (:dougt)]

I am not happy eith this solution really.  Basically to test some aspects of geolocation, we need a way to allow the mochitest to control the behavior the underlying geolocation provider.  For example, to test that the |timeout| options of the API works, you would need a provider that basically doesn't return data.  To support such a idea, i added a testing component:

http://mxr.mozilla.org/mozilla-central/source/dom/src/geolocation/test/TestGeolocationProvider.js

Mochitests can signal to this provider to do things like turn off set the position that it returns to a specific value.

This component must replace the actual geolocation provider that we ship in products.  (This real provider is tested via litmus).

The problem with the way it is setup right now:

1) this testing provider is built in all debug builds
   a)  macs don't package, so this testing component shows up for all mac developers.  It should only be in mochitest runs

2) mochitest as written have a bunch of universal permissions cruft to be able to signal the test location provider

3) motchitests also have to do a bunch of component registration to ensure that the testing component is used.


Is there a better way?

Thoughts?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Component registration in browser chrome tests doesn't seem that arduous, so I don't really understand why it's a problem.

That being said, can you test more of the shipping code and avoid the need for the test component by changing the provider pref to point to a localhost sjs that returns what you need? See https://developer.mozilla.org/En/Httpd.js/HTTP_server_for_unit_tests#SJS.3a_server-side_scripts .

Browser chrome tests can run in SeaMonkey, as Kairo points out, so that shouldn't be a problem.

Comment 7

[Doug Turner (:dougt)]

oh, that is a thought.  we could just write a network geolocation provider and use that -- somehow control it via mochitest (maybe my changing the URL we point to).  That might be the best solution, actually, as it tests much more of the code.

Any reason not to do this?  Clint?  Joel?

Comment 8

[cmtalbert]

(In reply to comment #7)
> oh, that is a thought.  we could just write a network geolocation provider and
> use that -- somehow control it via mochitest (maybe my changing the URL we
> point to).  That might be the best solution, actually, as it tests much more of
> the code.
> 
> Any reason not to do this?  Clint?  Joel?
On the surface this sounds like a good idea. Is this the change you want to make to avoid needing a test component?  Or would we still need a test geolocation component to do negative testing and we could use this network provider to test the real geolocation component?  Or could we use the network geolocation provider to also do negative testing?  Hmm...I'm not entirely clear about what you're proposing here.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

I like the idea in general as we would have our own real geolocation provider.  It is too bad we can't have an option in the browser to add a geo provider like we do search providers, but that is out of scope for the short term.

I am thinking we could create a provider that listens and lives on the mochitest stuff (http://localhost/<path>) and somehow configure the browser to point at that in the mochitest profile (just preferences?)  If needed we could chain this test provider off a separate port.

My biggest concern would be how do we control this provider?  We need to test start, stop, change data it returns, and timing.  

Another advantage of going this route is we might be able to have multiple providers that we create and can test multiple data sources and changing providers if we choose to allow that in the browser. [sorry for the over engineering here]

Is there a resource that outlines the interface a network geolocation provider has?

Comment 10

[Doug Turner (:dougt)]

geo.wifi.uri is the preference you need to tweak to point firefox at a different network geolocation provider.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #9)
> My biggest concern would be how do we control this provider?  We need to test
> start, stop, change data it returns, and timing.

You could probably control the data it returns using query string parameters (see e.g. http://mxr.mozilla.org/mozilla-central/source/netwerk/test/httpserver/test/data/sjs/object-state.sjs#1 ). You'd have to set the provider pref to different URLs for each test, but that shouldn't be a problem.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

I have spent some time looking into this and have run into some problems which I suspect are just a lack of skill with the codebase.

First off, I verified I could run dom/tests/mochitest/geolocation/test_allowCurrent.html by using the default network provider instead of the test provider.  This is done by:
editing http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/geolocation/geolocation_common.js#7 to be:
const testing_provider_cid  = Components.ID("{77da64d3-7458-4920-9491-86cc9914f904}");

instead of:
const testing_provider_cid = Components.ID("{10F622A4-6D7F-43A1-A938-5FFCBE2B1D1D}");


Then I ran the test again and saw passing results.  I also removed the TestGeolocationProvider.js from the related makefiles just to be sure I was using the network provider (and I verified with tcpdump).

Next, I started writing a network_geolocation.sjs file which has a has:
function handleRequest(request, response)
{
  var resp = {"location":{"latitude":"51.5","longitude":"-0.121","accuracy":"150.0","access_token":"2:jVhRZJ-j6PiRchH:RGMrR-W1BiwdZs12"}};

  response.setStatusLine("1.0", 200, "OK");
  response.ContentType = "application/x-javascript";
  response.write(resp);
}

Then I modified automation.py in the mochitest directory to set this user pref:
user_pref("geo.wifi.uri", "http://localhost:8888/tests/dom/tests/mochitest/geolocation/network_geolocation.sjs");

Then I ran the test again, and this time it never completes, but I see this in the console:
WARNING: We should have hit the document element...: file /home/mozilla/mozilla/src/layout/xul/base/src/nsBoxObject.cpp, line 185
JavaScript strict warning: file:///home/mozilla/mozilla/firefox_debug/dist/bin/components/NetworkGeolocationProvider.js, line 153: reference to undefined property Cc['@mozilla.org/wifi/monitor;1']
JavaScript strict warning: chrome://browser/content/places/utils.js, line 1322: reference to undefined property aPopup.childNodes[aPopup._startMarker + 1]
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "JSON.parse" {file: "file:///home/mozilla/mozilla/firefox_debug/dist/bin/components/NetworkGeolocationProvider.js" line: 209}]' when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
************************************************************


So I played around with setting the user_pref to be variations as I might have the wrong directory (starting to hack around here).  In the end, I have no luck and get the same results.

I saw a way to spit out messages to the console in the network provider:
http://mxr.mozilla.org/mozilla-central/source/dom/src/geolocation/NetworkGeolocationProvider.js#12

but after uncommenting the lines this didn't output anything to the console when running.


I suspect there is a small problem with my network_geolocation.sjs file which isn't sending the json back correctly, but it is hard to tell with limited methods for debugging.

Can somebody recommend a method for debugging so I can figure out what is going on here. Maybe there is an obvious flaw in my .sjs or json that I am not seeing and could get me going in again.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #12)
> function handleRequest(request, response)
> {
>   var resp =
> {"location":{"latitude":"51.5","longitude":"-0.121","accuracy":"150.0","access_token":"2:jVhRZJ-j6PiRchH:RGMrR-W1BiwdZs12"}};
> 
>   response.setStatusLine("1.0", 200, "OK");
>   response.ContentType = "application/x-javascript";
>   response.write(resp);

I think you want response.write(JSON.stringify(resp)) instead, object.toString() doesn't return valid JSON.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: Patch to use a local network server

This patch removes the TestGeolocationProvider.js and adds a network_geolocation.sjs which we access instead of the default network provider (google.com/loc/json).

Patch tested on a fresh m-c clone on linux.

Comment 15

[cmtalbert]

Comment on attachment 379734 [details] [diff] [review]
Patch to use a local network server

>diff --git a/dom/tests/mochitest/geolocation/geolocation_common.js b/dom/tests/mochitest/geolocation/geolocation_common.js
>--- a/dom/tests/mochitest/geolocation/geolocation_common.js
>+++ b/dom/tests/mochitest/geolocation/geolocation_common.js
>@@ -3,8 +3,8 @@ function ensure_geolocationProvider()
> function ensure_geolocationProvider()
> {
>     netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>-    
>-    const testing_provider_cid = Components.ID("{10F622A4-6D7F-43A1-A938-5FFCBE2B1D1D}");
>+
>+    const testing_provider_cid = Components.ID("{77da64d3-7458-4920-9491-86cc9914f904}");
What is this UID ^^? If this is the standard geolocation interface, then we shouldn't need to have this line in here at all.  We ought to be able to do the normal Cc[geocloatin_cid].Ci.nsIGeolocation or what-have-you.  I'd like all this component re-registration to be removed if possible.

The network_geolocation.sjs file is missing also.  Need to get that for further review.  Minusing on this one.

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: updated patch with network_geolocation.sjs and removed references to ensure_gelocation() [Checkin: See comment 19 & 20]

as per ctalbert's comments, I realized that I didn't have network_geolocation.sjs in this file (was a bug in hg 0.9.5 and I upgrade to 1.0.1 to resolve my issue).  I also removed all references to ensure_geolocation() as there is no need for this.

Verified on a fresh m-c clone, build, and full set of dom/tests/mochitest/geolocation tests.  Everything passes just fine in my testing.  Done on linux.

Comment 17

[cmtalbert]

Comment on attachment 379943 [details] [diff] [review]
updated patch with network_geolocation.sjs and removed references to ensure_gelocation()
[Checkin: See comment 19 & 20]

This looks really good.  In the future, you need to pass a -U 8 to the diff command (or specify it in the .hgrc file) so that we get 8 lines of context on these diffs.  

This approach looks good.I like that you removed all that ensure_geolocationProvider stuff this time.

Nice work! r=ctalbert

Comment 18

[Doug Turner (:dougt)]

Comment on attachment 379943 [details] [diff] [review]
updated patch with network_geolocation.sjs and removed references to ensure_gelocation()
[Checkin: See comment 19 & 20]

this is a good start.

lets also remove:

stop_geolocationProvider
resume_geolocationProvider
from geolocation_common.js.

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 379943 [details] [diff] [review]
updated patch with network_geolocation.sjs and removed references to ensure_gelocation()
[Checkin: See comment 19 & 20]


http://hg.mozilla.org/mozilla-central/rev/21ff59feef47

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 379943 [details] [diff] [review]
updated patch with network_geolocation.sjs and removed references to ensure_gelocation()
[Checkin: See comment 19 & 20]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6aca7b6b2ca8

after fixing context for
{
patching file dom/src/geolocation/Makefile.in
Hunk #1 FAILED at 84
1 out of 1 hunks FAILED
}

Comment 21

[Joel Maher ( :jmaher ) (UTC -8)]

comment #18 is fixed in 497053.  This is landed in m-c and passed unittests.  Should be landing in 1.9.1 this afternoon.

Comment 22

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Is there anything left to do here, or are geolocation tests now passing when run on packaged-builds?

Comment 23

[Doug Turner (:dougt)]

looks like everything landed.  we can cleanup the stuff I pointed out in comment 18 later.

Comment 24

[Joel Maher ( :jmaher ) (UTC -8)]

I had addressed comment @18 in bug 497053

Comment 25

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Excellent news, thanks!

Comment 26

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

Nothing has been turned the tree red in the last days. So lets mark this implementation as verified fixed on trunk and 1.9.1.