478911 - better tests for geolocation

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[(not currently active) Ted Mielczarek]

Currently, since we don't ship a geolocation provider in Firefox, our geolocation tests are pretty lame there, and don't test much. We could use dougt's simple geo provider from his extension for basic testing that the DOM interfaces actually work.

Comment 1

[Doug Turner (:dougt)]

implementing 459425 would be an alternative to using the geolocation provider from my simple addon.

Comment 2

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

I would be interested in helping out with this.

Comment 3

[Doug Turner (:dougt)]

Attachment: patch v.1

1) removes the testing prompt and will test whatever prompt is built into the application.  if there isn't a prompt built into the application, the tests will fail, as expected.  Seamonkey, Fennec, and Firefox both have prompt, so this isn't a problem.

2) created a testing component (dom/src/geolocation/test/TestGeolocationProvider.js) that provides geolocation data.  This allows us to pause data generation so we can simulate a gps device that stops responding.

3) made the test cases much similar while continuing to test the same stuff (as well as testing the actual ui prompts).


of course, we need more tests, but hopefully this pass can help make that process much easier.

Comment 4

[Doug Turner (:dougt)]

Attachment: patch v.2

the last patch didn't wait long enough for the test geolocation provider to start sending data.

Comment 5

[Doug Turner (:dougt)]

Attachment: patch v.3

forgot the Makefile.in

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 365710 [details] [diff] [review]
patch v.3

joel, can you take a look?  Also, lets get clint to look too.

Comment 7

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

Comment on attachment 365710 [details] [diff] [review]
patch v.3

Overall this patch provides good coverage of the existing geolocation as well as a basic provider.  I have filed bug 482260 to address other tests that we should consider adding in the near future now that we have this infrastructure in place.

I tested this on Firefox and it doesn't work on Fennec as there is not a method for sending or ignoring the geolocation data to a website programatically inside of Fennec.

Comment 8

[cmtalbert]

Comment on attachment 365710 [details] [diff] [review]
patch v.3

Overall Doug, I like the approach, I think it's a lot more flexible and expandable for future testing.  Thanks for doing this.  I have some comments and nits, but the main reason I'm -'ing is because of test_cancelCurrent.html and test_cancelWatch.html look wrong. I have detailed comments below.

>diff --git a/dom/src/geolocation/test/TestGeolocationProvider.js b/dom/src/geolocation/test/TestGeolocationProvider.js
>+    classDescription: "teseting geolocation coords object",
nit: misspelled testing

>--- a/dom/tests/mochitest/geolocation/geolocation_common.js
>+++ b/dom/tests/mochitest/geolocation/geolocation_common.js
>@@ -1,36 +1,24 @@
>+  const Ci = Components.interfaces;
>+  
>+  function getChromeWindow(aWindow) {
>+      var chromeWin = aWindow 
>+          .QueryInterface(Ci.nsIInterfaceRequestor)
>+          .getInterface(Ci.nsIWebNavigation)
>+          .QueryInterface(Ci.nsIDocShellTreeItem)
>+          .rootTreeItem
>+          .QueryInterface(Ci.nsIInterfaceRequestor)
>+          .getInterface(Ci.nsIDOMWindow)
>+          .QueryInterface(Ci.nsIDOMChromeWindow);
>+      return chromeWin;

Since you've got enablePrivilege here, why not just use the nsIWindowMediator service to get the most recent window and (gives a DOMWindowInternal) and then QI it to nsIDOMChromeWindow?  Hmmm...checking on this, I'm not entirely sure my logic would pan out.  Is that why you chose this path? 

>+  // This is a bit of a hack. The notification doesn't have an API to
>+  // trigger buttons, so we dive down into the implementation and twiddle
>+  // the buttons directly.
It seems like the notification should expose the API here, and if not, it should at least expose the ID of the buttons.  The buttons do have anonid's but I'm not sure which button gets which anonid without digging deeper into this.  Either way, this ought to be more testable.  Please either file a follow on bug, or remind me to file one tomorrow. The code for these buttons is here: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/spinbuttons.xml

clickNotificationButton(getNotificationBox().currentNotification, "Exact Location (within 10 feet)");
> }
Boy I wish we had a better way to get those buttons other than their labels.

>--- a/dom/tests/mochitest/geolocation/test_allowCurrent.html
>+++ b/dom/tests/mochitest/geolocation/test_allowCurrent.html
>@@ -2,62 +2,46 @@
> <html>
> <!--
> https://bugzilla.mozilla.org/show_bug.cgi?id=
> -->
nit: Some of these bug statements have bugs, some don't.  I realize this is not code you touched, but while we're here we might as well fix these.  This is true for many of the files. I'm happy with setting them all to 478911, if that makes it easier.

>--- a/dom/tests/mochitest/geolocation/test_cancelCurrent.html
>+++ b/dom/tests/mochitest/geolocation/test_cancelCurrent.html

This test and test_cancelWatch.html (which has the same structure) are the reason I am going to minus this patch.  Unless I am missing something totally obvious, I don't think this test does what it appears to do.

>+var failureWasCalled = false;
This variable is never used.

>+function failureCallback(error) {
>+  ok(error.code == error.PERMISSION_DENIED, "Ensure that the error was PERMISSION_DENIED");
>+}
>+
>+function testDeclined() {
>+  ok(failureCallback, "Ensure that the failureCallback was called");
I think you intend this to be failureWasCalled
>   SimpleTest.finish();
> }
> 
>+function deny()
>+{
>+  failureCallback = true;
I think you intend this to be failureWasCalled
>+  clickDeny();
> }
> 
> /** Test for Bug  **/
> 
> SimpleTest.waitForExplicitFinish();
> 
>+navigator.geolocation.getCurrentPosition(successCallback, failureCallback, null);
> 
>+// click deny
>+setTimeout(clickDeny, 50);
> I think here, you mean to call deny() not clickDeny()

> // wait for position change
>+setTimeout(testDeclined, 1000);

Here is the problem with this structure.  Because the failureCallback is the name of a function  as well as the name of a variable the testDeclined function will never fail.  Generalizing this you can see it quickly:
Function a corresponds to failureCallback()
Function b corresponds to deny()
Function c corresponds to testDeclined()
js> function a() { print("yiihah"); }
js> function b() { a = true; }
js> function c() { if (a) print("a is true");
else print("a is not true"); }
js> c();
a is true
js> a
function a() {
    print("yiihah");
}
js> b()
js> a
true
js> c()
a is true


>--- a/dom/tests/mochitest/geolocation/test_clearWatch.html
>+++ b/dom/tests/mochitest/geolocation/test_clearWatch.html
>+setTimeout(clickAccept, 10);
>+setTimeout(clearWatch, 50);
This test feels like it is timing dependent. Is that true? Does it work the same on different platforms?

Comment 9

[Doug Turner (:dougt)]

> function getChromeWindow(aWindow)

This is the same code that I used in the geolocation prompt code for FF and Fennec.  it also is the same code that the password manager uses.  If you have better way of doing this, please let me know! 

http://mxr.mozilla.org/mozilla-central/search?string=getChromeWindow


> remind me to file one tomorrow

:-)  Using the label is terrible, but other mochitests do the same thing.  I think i pulled this code from:  http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/widgets/test_notificationbox.xul

I fixed up the cancel test cases.  good catches; thanks.

Comment 10

[Doug Turner (:dougt)]

Attachment: patch v.4

Comment 11

[Doug Turner (:dougt)]

Attachment: patch v.4

same as before, but I saved all files before diffing. :-)

Comment 12

[cmtalbert]

Comment on attachment 366807 [details] [diff] [review]
patch v.4

Thanks for the patch Doug.  This version looks really good.  I think the new patch makes it clearer that this stuff won't be timing dependent. r=ctalbert

Comment 13

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/54030de86b9c

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 366807 [details] [diff] [review]
patch v.4

a191=beltzner, yay tests!

Comment 15

[Doug Turner (:dougt)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9ef760885920