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)

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

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

joel, any update?

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.

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

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 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.

> 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 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

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

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

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

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.

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.

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?

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 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

Pushed to m-c: fb8e2e65e917

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.

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.

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

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

lets open separate bugs for separate test cases.

verified via mxr.

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.

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 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 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 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 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 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

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)