482260 - add more comprehensive geolocation tests

Status: VERIFIED FIXED

(Core :: DOM: Geolocation, defect)

Description

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

Now that we have a geolocation test provider and a good set of tests, we should add more tests to increase our testing coverage.

Here are some tests we should add:
1) getCurrentPosition() calls in series (1000 or so)
2) getCurrentPosition() calls in parallel (100 or so)
3) watchPosition()/clearWatch() calls in series (1000 or so)
4) watchPosition() callbacks registered (1500 is the limit)
5) testcases where the geolocation provider returns bad data
6) verify the specific data returned from provider
7) test returning multiple values (will require the provider to change data)

Comment 1

[Doug Turner (:dougt)]

great job identifying these areas.  are you thinking about tackling these Joel?

Comment 2

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

yeah, I will get on this.  Probably next week or the week after though.

Comment 3

[Doug Turner (:dougt)]

joel, any update?

Comment 4

[Doug Turner (:dougt)]

also, we should add tests for:

where |a| is the objection returned to the geolocation APIs:

a.address.streetNumber
a.address.street 
a.address.premises 
a.address.city
a.address.county 
a.address.region
a.address.county
a.address.countryCode
a.address.postalCode

See bug 503942.

Comment 5

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

we need test cases for:
 - watchPosition(successCallback,errorCallback,options) <- the different options, and the various cases for the errorCallback.

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.1

Adds test cases for 1, 2, 3, 4, 5, and 6.


also, fixes a few problems with core code found when building these tests:

1) protects against the network location provider not returning data.  This can happen since some of the fields are optional.

2) max age can be zero.  we were preventing max age from being zero and setting it to 30s.


gavin, can you review the changes to dom/src/*
joel, can you review the mochitest stuff?

Comment 7

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

Comment on attachment 393678 [details] [diff] [review]
patch v.1

>+var gTestingEnabled = false;
this is adding a preference to the overall product.  This might be the only way to approach this, but it doesn't seem the safest method to take.  Any user can set the preference in about:config and turn this into testing mode.  Maybe there is no danger in that.

 
>+
>+    try {
>+        gTestingEnabled = this.prefService.getBoolPref("geo.wifi.testing");
>+    } catch (e) {}
>+
can we have the catch set gLoggingEnabled = false. I know it is not supposed to work that way, but neither is a lot of the stuff I hunt down.

>-      if (tempAge > 0)
>+      if (tempAge >= 0)
>         maximumAge = tempAge;

it is unclear why you are allowing this to be 0 now





> 
>+  if (getState("garbage") == "true") {
>+     // better way?
>+    var chars = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXTZabcdefghiklmnopqrstuvwxyz";
>+    position = "";
>+    var len = Math.floor(Math.random() * 5000);
>+
>+    for (var i=0; i< len; i++) {
>+        var c = Math.floor(Math.random() * chars.length);
>+        position += chars.substring(c, c+1);
>+    }
>+  }
>+
>+
this works great for random garbage.  I would like to test a variety of datatypes (char, null, largeint, float, int, zero) since this is data that can be spoofed over the internet.  



>+  if (completeCount==0) {
>+    ok(1, "all watchPosition successCallbacks called");
>+    SimpleTest.finish();
>+  } else {
>+    watchID = navigator.geolocation.watchPosition(successCallback, null, options);
>+    setTimeout(accept, 50);
>+  }

I had a lot of trouble getting this technique to work before.  This ran fine for me in testing, so your exact technique must be the right way to do it.


Overall this is good.  As you mentioned this covers scenarios 1-6.  Let me know your thoughts on the invalid/garbage data test.

Comment 8

[Doug Turner (:dougt)]

> it doesn't seem the safest method to take

please suggestion another approach?  I have no problem adding testing code to the product if it yields a better product.

> can we have the catch set gLoggingEnabled = false. I know it is not supposed to
work that way, but neither is a lot of the stuff I hunt down.

why?  the default value is set above.

> it is unclear why you are allowing this to be 0 now

Per spec.


> this works great for random garbage.

Can you file separate bugs?


mfinkle, can you also review the stuff in dom/src?

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 393678 [details] [diff] [review]
patch v.1

> WifiGeoCoordsObject.prototype = {

>     altitude: 0,
>     altitudeAccuracy: 0,
>-    heading: 0,
>-    speed: 0,
>+
> };

remove the blank line?

>+    this.coords = new WifiGeoCoordsObject(location.latitude,
>+                                          location.longitude,
>+                                          location.accuracy ? location.accuracy : 12450, // .5 * circumference of earth.
>+                                          location.altitude ? location.altitude : 0,
>+                                          location.altitude_accuracy ? location.altitude_accuracy : 0);


>+        this.address = new WifiGeoAddressObject(address.street_number ? address.street_number : null,
>+                                                address.street ? address.street : null,
>+                                                address.premises ? address.premises : null,
>+                                                address.city ? address.city : null,
>+                                                address.county ? address.county : null,
>+                                                address.region ? address.region : null,
>+                                                address.country ? address.country : null,
>+                                                address.country_code ? address.country_code : null,
>+                                                address.postal_code ? address.postal_code : null);

try using this pattern:

address.postal_code || null


>+        if (gTestingEnabled == false)
>+            this.timer.initWithCallback(this, 5000, this.timer.TYPE_ONE_SHOT);
>+        else
>+            this.timer.initWithCallback(this, 200, this.timer.TYPE_REPEATING_SLACK);
>+

I'm a little wishy-washy on putting testing support in the production code, but we didn't come up with a better work around after talking about it

Comment 10

[Doug Turner (:dougt)]

when I checked this in, the timeout test case randomly failed.

Comment 11

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

that case was random orange before.  it was quiet for almost 2 weeks, but showed up right before and right after this checkin.

Comment 12

[Doug Turner (:dougt)]

what?  i do not follow.  I can reproduce the timeout failure locally.

Comment 13

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

check out bug 497053.  the timeout showed up on 8-08 and 8-12.  I don't see a mention of it today at all.  If you can reproduce it locally, that is more than ctalbert or myself have been able to do.

Comment 14

[Doug Turner (:dougt)]

Attachment: patch v.2 [Checkin: Comment 18+29 & 28+30]

fixed the random orange on the timeout test.  The basic problem was that the sjs server isn't really stateful.  For whatever reason it seems to lose its shit resulting in problems for our tests.  For example, we tell it, via xhr, that it should not respond any longer.  yet, for some reason, when we do another GET request it happily responses.  wtf!.

basically what I have done is changed it around a bit.  we now will change the URL that we hit from the geolocation side such that no state needs to be saved in the server.  So, when we want the server to send garbage, we just change the URL to localhost/blah?responde-garbage.

works great; no orange as far as I can tell.

Comment 15

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

Doug, great find.  I took a quick look at your patch and it has the code from the first patch as well as any fixes for this new timeout patch.  I thought this was already checked in, could we just get a patch with the latest bits?

Comment 16

[Doug Turner (:dougt)]

the first patch was backed out because it made 497053 worse.  you can diff the two diffs to see what I changed.  basically it was teh sjs file and the geolocation_common file.

Comment 17

[cmtalbert]

Comment on attachment 394459 [details] [diff] [review]
patch v.2
[Checkin: Comment 18+29 & 28+30]

This looks good to me.  I'm excited about figuring out the problem with that [orange] timeout test.  Very awesome.  r=ctalbert

Comment 18

[cmtalbert]

Pushed to m-c: fb8e2e65e917

Comment 19

[cmtalbert]

I think we want to also take this on 1.9.2 and 1.9.1.  Setting 1.9.1-? status flag.  I will wait to ensure that we're all green before pushing to 1.9.2, and I'll wait on ss's direction for 1.9.1.

Comment 20

[cmtalbert]

Failures all over the place.

= Windows =
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1250295786.1250297991.4860.gz&fulltext=1

= Mac =
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1250296076.1250298197.6910.gz&fulltext=1

I'm reopening this bug, and I'll turn off the new tests that are making this worse in geolocation.

Comment 21

[cmtalbert]

Attachment: Further fix for orange to turn off the new tests [Backout: Comment 22]

This is a patch that will remove the new tests from the makefile since they seem to be making the orange worse.

Comment 22

[Doug Turner (:dougt)]

i re-enabled the tests a while ago without any trouble.

Comment 23

[Doug Turner (:dougt)]

lets open separate bugs for separate test cases.

Comment 24

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

verified via mxr.

Comment 25

[Daniel Veditz [:dveditz]]

Do we know why all the tests turned orange (comment 20) and what fixed them (comment 22)? If the fixes only landed on the trunk and/or 1.9.2 branch then we would still have problems landing this on the 1.9.1 branch.

Comment 26

[Doug Turner (:dougt)]

dveditz, two things:

1) the test were checked in with patch instead of hg import resulting in the new files not being pushed.

2) there was another regression at the time that was blamed on this changeset.

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 394459 [details] [diff] [review]
patch v.2
[Checkin: Comment 18+29 & 28+30]

Approved for 1.9.1.4, a=dveditz for release-drivers

Comment 28

[Serge Gautherie (:sgautherie)]

Comment on attachment 394459 [details] [diff] [review]
patch v.2
[Checkin: Comment 18+29 & 28+30]


http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0b25442d7d05

Comment 29

[Serge Gautherie (:sgautherie)]

Comment on attachment 394459 [details] [diff] [review]
patch v.2
[Checkin: Comment 18+29 & 28+30]


(In reply to comment #26)
> 1) the test were checked in with patch instead of hg import resulting in the
> new files not being pushed.

Damn: http://hg.mozilla.org/mozilla-central/rev/4f1a754a914b !

Comment 30

[Serge Gautherie (:sgautherie)]

Comment on attachment 394459 [details] [diff] [review]
patch v.2
[Checkin: Comment 18+29 & 28+30]


http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f6d1d3955a7b

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 394459 [details] [diff] [review]
patch v.2
[Checkin: Comment 18+29 & 28+30]

This patch doesn't apply, and apparently relies on additional feature changes made to 1.9.2 that don't exist in 1.9.1

If you want this in 1.9.1 we'll need a new patch, and if you're making feature changes please link to the old bugs where those were made so QA can verify them.

At this point this might be more appropriate for 1.9.1.5 -- getting late for 1.9.1.4

Comment 32

[Daniel Holbert [:dholbert]]

Doug, or Clint: can one of you backport this bug's patch to 1.9.1?  This seems to fix Bug 497053, which still causes a lot of failures on the 1.9.1 tinderboxen.

(see Bug 497053 comment 51)