1008913 - Add test to verify geolocation sharing option "Always Share Location"

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Mihaela Velimiroviciu (:mihaelav)]

Steps:

1. Open a page which uses geolocation
>> The location sharing hanger is displayed

2. Select "Always Share Location" option
>> The location is shared

3. Reload the page
>> The location sharing hanger is not displayed again and location is automatically shared

4. Load other page which uses geolocation
>> The location sharing hanger is displayed

5. Restart the browser and open the page from step 1
>> The location sharing option is persisted and location is automatically shared

Comment 1

[Daniel Gherasim]

Attachment: v1.patch

Notes:
* using local location source so we don't depend on google when testing geolocation functionality;
* using the geolocation from mozqa for testing that notification is shown for another service;

Comment 2

[Andrei Eftimie]

Comment on attachment 8424766 [details] [diff] [review]
v1.patch

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

Looks good overall, but there are issues that need some attention:

I have some conflicts in firefox/lib/toolbars.js

::: firefox/tests/functional/restartTests/testGeolocation_alwaysShareLocation/test1.js
@@ +48,5 @@
> +    controller.waitForPageLoad();
> +  }, {type: "notification_popup"});
> +
> +  var alwaysLabel = utils.getProperty("chrome://browser/locale/browser.properties",
> +                                        "geolocation.alwaysShareLocation");

nit: indentation

@@ +53,5 @@
> +  var alwayAllowMenuItem = locationBar.getElement({type: "notification_menuItem",
> +                                                  subtype: "label",
> +                                                  value: alwaysLabel});
> +  // Wait for the geo notification to unload
> +  locationBar.waitForArrowPanel(() => {

This will have to be updated with the new name of this method from bug 994040

@@ +61,5 @@
> +  // Check if the location is displayed
> +  var result = new findElement.ID(controller.tabs.activeTab, "result");
> +  var regExp = /\d+(\.\d*)?\.\d+/;
> +  try {
> +    assert.waitFor(function () {

Fat arrow syntax please.

@@ +73,5 @@
> +  // No notification popup should appear on reloading the page
> +  assert.throws(() => {
> +    locationBar.waitForArrowPanel(() => {
> +      controller.open(TEST_DATA);
> +      controller.waitForPageLoad();

We can use the reload functionality from toolbars.locationbar

Comment 3

[Daniel Gherasim]

(In reply to Andrei Eftimie from comment #2)
> @@ +73,5 @@
> > +  // No notification popup should appear on reloading the page
> > +  assert.throws(() => {
> > +    locationBar.waitForArrowPanel(() => {
> > +      controller.open(TEST_DATA);
> > +      controller.waitForPageLoad();
> 
> We can use the reload functionality from toolbars.locationbar

I think we implemented this on the metro side & we don't have it in our current toolbars lib for firefox. Shoul I implement it in this bug ?

Comment 4

[Daniel Gherasim]

Attachment: v1.1.patch

Bug 1016343 filed for the reload method.
Patch here updated based on last review.

Comment 5

[Andrei Eftimie]

Comment on attachment 8429249 [details] [diff] [review]
v1.1.patch

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

You will also have to include /testGeolocation_alwaysShareLocation/manifest.ini in firefox/tests/functional/restartTests/manifest.ini

I am not sure why we need 2 tests here.
You have them in the restartTests folder, but this alone will not restart between them. You will have to call controller.restartApplication() to restart Firefox.
Also we could probably get away with a single test file. If you call controller.restartApplication("%name%") mozmill will run the function %name% after a restart.

::: data/geolocation/locations.json
@@ +2,5 @@
> +  "location": {
> +    "lat": 37.4133,
> +    "lng": -122.081,
> +    "accurancy": 42
> +  }

I see this location is in Montain View, but it points to the middle of a highway.

I'd suggest to customize it a bit, make an easter egg... I'd link the Mozilla Monument, which would be:
> lat: 37.789543
> lng: -122.388813
(tested these on both Open Street Map and Google Maps)

::: firefox/tests/functional/restartTests/testGeolocation_alwaysShareLocation/test1.js
@@ +11,5 @@
> +var utils = require("../../../../lib/utils");
> +
> +const BASE_URL = collector.addHttpResource("../../../../../data/");
> +const TEST_DATA = BASE_URL + "geolocation/position.html";
> +const TEST_DATA_2 = "http://mozqa.com/data/firefox/geolocation/position.html";

Since we still relying on an external webserver this test should be a remote one.
This is because the settings for Always/Never share location are domain based, not URL based, right?

Locally we should be able to use both 127.0.0.1 and localhost, I'd prefer if we're able to have this work than making this test a remote one.

@@ +16,5 @@
> +const LOCATION_DATA = BASE_URL + "geolocation/locations.json";
> +
> +const PREF_GEO_WIFI_URI = "geo.wifi.uri";
> +
> +const TIMEOUT_POSITION = 30000;

The name doesn't click for me. Since we don't have any other timeout values stored in this test we might as well leave it as TIMEOUT.
If we want to be more specific, we should do something like TIMEOUT_GEOLOCATE

@@ +18,5 @@
> +const PREF_GEO_WIFI_URI = "geo.wifi.uri";
> +
> +const TIMEOUT_POSITION = 30000;
> +
> +// Include required modules

This comment should probably not be here.

@@ +39,5 @@
> +/**
> + * Bug 1008913 : Add test to verify geolocation sharing option "Always Share Location"
> + */
> +function testAlwaysShareLocation() {
> +  persisted.test_data = TEST_DATA;

This should be set up in `setupModule`.

@@ +49,5 @@
> +  }, {type: "notification_popup"});
> +
> +  var alwaysLabel = utils.getProperty("chrome://browser/locale/browser.properties",
> +                                      "geolocation.alwaysShareLocation");
> +  var alwayAllowMenuItem = locationBar.getElement({type: "notification_menuItem",

nit: typo "always[...]"

@@ +51,5 @@
> +  var alwaysLabel = utils.getProperty("chrome://browser/locale/browser.properties",
> +                                      "geolocation.alwaysShareLocation");
> +  var alwayAllowMenuItem = locationBar.getElement({type: "notification_menuItem",
> +                                                  subtype: "label",
> +                                                  value: alwaysLabel});

nit: indentation

@@ +58,5 @@
> +    alwayAllowMenuItem.click();
> +  }, {type: "notification_popup", open: false});
> +
> +  // Check if the location is displayed
> +  var result = new findElement.ID(controller.tabs.activeTab, "result");

You don't need `new`

@@ +59,5 @@
> +  }, {type: "notification_popup", open: false});
> +
> +  // Check if the location is displayed
> +  var result = new findElement.ID(controller.tabs.activeTab, "result");
> +  var regExp = /\d+(\.\d*)?\.\d+/;

We should add a comment to what this will match. A regular expression is rarely clear by itself.

::: firefox/tests/functional/restartTests/testGeolocation_alwaysShareLocation/test2.js
@@ +28,5 @@
> +
> +function teardownModule(aModule) {
> +  prefs.preferences.clearUserPref(PREF_GEO_WIFI_URI);
> +
> +  aModule.tabBrowser.closeAllTabs();

We miss to clear all previously set location data.

Comment 6

[Daniel Gherasim]

Attachment: v2.patch

Updated patch as requested.

Comment 7

[Andrei Eftimie]

Comment on attachment 8430744 [details] [diff] [review]
v2.patch

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

I think this is a good use of restartApplication("next").
We might also skip the folder and just have the rest reside as firefox/tests/functional/restartTests/testGeolocation_alwaysShareLocation.js

::: firefox/tests/functional/restartTests/testGeolocation_alwaysShareLocation/test1.js
@@ +12,5 @@
> +
> +const BASE_URL = collector.addHttpResource("../../../../../data/");
> +const TEST_DATA = BASE_URL + "geolocation/position.html";
> +// Change local domain to make geolocation take it as another service request
> +const TEST_DATA_2 = TEST_DATA.replace("localhost", "127.0.0.1");

So we'll need something like this to make the test local.

If we want this test to be local, we'll need to use a hack like this, even if it's not pretty.
Since the other geolocation tests might want this as well we should have a helper function in utils to handle this transform.

Also interesting to note that httpd.js returns 'localhost' as canonical domain, while mozhttpd (which is currently in the works and will be available in the next mozmill version) returns '127.0.0.1'.
So the method will need to check which local address is used and return the other one, it will make a switch between localhost and 127.0.0.1 and vice-versa.

I'd like Henrik's input here since I think a hack like this is worth it since it allows us to have these GeoLocation tests locally, but this is still a hack.

@@ +52,5 @@
> +
> +  var alwaysLabel = utils.getProperty("chrome://browser/locale/browser.properties",
> +                                      "geolocation.alwaysShareLocation");
> +  var alwaysAllowMenuItem = locationBar.getElement({type: "notification_menuItem",
> +                                                   subtype: "label",

nit: indentation (need 1 more space)

@@ +74,5 @@
> +    controller.open(TEST_DATA_2);
> +    controller.waitForPageLoad();
> +  }, {type: "notification_popup"});
> +
> +  tabBrowser.closeAllTabs();

this is not needed here since you call it in setupTest

Comment 8

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

Comment on attachment 8430744 [details] [diff] [review]
v2.patch

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

::: firefox/lib/toolbars.js
@@ +543,5 @@
> +      case "notification_menuItem":
> +        nodeCollector.root = this.getElement({type: "notification_popup"}).getNode();
> +        nodeCollector.queryNodes("menuitem").filterByDOMProperty(spec.subtype,
> +                                                                 spec.value);
> +        break;

This is something bug 1000832 should get implemented.

::: firefox/tests/functional/restartTests/testGeolocation_alwaysShareLocation/test1.js
@@ +13,5 @@
> +const BASE_URL = collector.addHttpResource("../../../../../data/");
> +const TEST_DATA = BASE_URL + "geolocation/position.html";
> +// Change local domain to make geolocation take it as another service request
> +const TEST_DATA_2 = TEST_DATA.replace("localhost", "127.0.0.1");
> +const LOCATION_DATA = BASE_URL + "geolocation/locations.json";

All the above should go into a TEST_DATA object.

For the host part I'm not sure, but at least httpd.js should support fake domains. We should not be able to pass this through Mozmill yet. So might want to think about extending it, or making this test a remote one for now by using different subdomains on mozqa.com? Otherwise what about another HTML file on the same host? Shouldn't this also be enough?

@@ +97,5 @@
> +  // Regular expression pattern to match a float number
> +  var regExp = /\d+(\.\d*)?\.\d+/;
> +  try {
> +    assert.waitFor(() => regExp.test(result.getNode().textContent),
> +                   "", TIMEOUT_GEOLOCATE);

you want to use expect.match() here. I don't see a reason for assert.

Comment 9

[Andrei Eftimie]

> Otherwise what about another HTML file on the same host? Shouldn't this also be enough?
No, we need a different domain.

So if we want this test to be local, we'll either need to fiddle with the client OS `hosts` file, or try both `localhost` and `127.0.0.1` since these both are supported on both platforms.

Comment 10

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

I would make this test part of the remote testrun for now, and utilize mozqa.com until we can have it using local resources.

Comment 11

[Daniel Gherasim]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> Comment on attachment 8430744 [details] [diff] [review]
> v2.patch
> 
> Review of attachment 8430744 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/lib/toolbars.js
> @@ +543,5 @@
> > +      case "notification_menuItem":
> > +        nodeCollector.root = this.getElement({type: "notification_popup"}).getNode();
> > +        nodeCollector.queryNodes("menuitem").filterByDOMProperty(spec.subtype,
> > +                                                                 spec.value);
> > +        break;
> 
> This is something bug 1000832 should get implemented.
> 

I changed the logic a bit, to wait for a general menuItem, and we can give the panel as the parent node.
We can implement this here & we don't need for that bug to be fixe for our current geolocation tests.

> @@ +97,5 @@
> > +  // Regular expression pattern to match a float number
> > +  var regExp = /\d+(\.\d*)?\.\d+/;
> > +  try {
> > +    assert.waitFor(() => regExp.test(result.getNode().textContent),
> > +                   "", TIMEOUT_GEOLOCATE);
> 
> you want to use expect.match() here. I don't see a reason for assert.

We still need a timeout to wait for the geolocation data. 
Maybe an expect.waitFor the result to change & then using expect.match to test if it is the correct result.

Comment 12

[Daniel Gherasim]

Attachment: v3.patch

All dependency bugs are fixed. Updated bug based on last feedbacks & discussions.

Comment 13

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8437543 [details] [diff] [review]
v3.patch

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

There's a conflict in the manifest file. Otherwise, seems we're closer to land this.

::: firefox/tests/remote/restartTests/manifest.ini
@@ +12,5 @@
>  [include:testDiscoveryPane_installCollectionAddon/manifest.ini]
>  disabled = Bug 732353 - Disable all Discovery Pane tests due to unpredictable web dependencies
>  [include:testDiscoveryPane_installPickOfMonthAddon/manifest.ini]
>  disabled = Bug 657492 - 'Pick of the Month' add-ons are only compatible with Release and Beta builds
> +[include:testGeolocation/manifest.ini]

I think this should be after the Eula test, not sure why we have Discovery before Addons..

::: firefox/tests/remote/restartTests/testGeolocation/testAlwaysShareLocation.js
@@ +69,5 @@
> +  assert.throws(() => {
> +    locationBar.waitForNotificationPanel(() => {
> +      locationBar.reload();
> +    }, {type: "notification"});
> +  }, errors.TimeoutError, "'Notification Popup' doesn't exists");

nit: doesn't exist.

@@ +81,5 @@
> +  tabBrowser.closeAllTabs();
> +  controller.restartApplication("testAlwaysShareLocationPersisted");
> +}
> +
> +function testAlwaysShareLocationPersisted() {

Please add jsdoc here too

Comment 14

[Daniel Gherasim]

Attachment: v3.1.patch

Updated based on last review.

Comment 15

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8437651 [details] [diff] [review]
v3.1.patch

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

This looks good now, thanks.

Comment 16

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

Comment on attachment 8437651 [details] [diff] [review]
v3.1.patch

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

::: firefox/lib/toolbars.js
@@ +510,5 @@
>        case "identityBox":
>          return [new elementslib.ID(root, "identity-box")];
>        case "identityPopup":
>          return [new elementslib.ID(root, "identity-popup")];
> +      case "menuItem":

This is badly worded. It's not in the global scope of the location bar, but for the popup. So name it 'notificationPopup_menuItem'.

::: firefox/tests/remote/restartTests/testGeolocation/testAlwaysShareLocation.js
@@ +11,5 @@
> +var utils = require("../../../../lib/utils");
> +
> +const BASE_URL = collector.addHttpResource("../../../../../data/");
> +const TEST_DATA = [
> +  BASE_URL + "geolocation/position.html",

You may want to make this a dict, so you can name those specific entries.

@@ +13,5 @@
> +const BASE_URL = collector.addHttpResource("../../../../../data/");
> +const TEST_DATA = [
> +  BASE_URL + "geolocation/position.html",
> +  "http://mozqa.com/data/firefox/geolocation/position.html",
> +  BASE_URL + "geolocation/locations.json"

I wonder if we should stick this json file under another sub directory, called positions and gave it a better name. I assume we will get more of those. Also don't forget to get this finally pushed to the testcase-data repository! Usually we do that first.

@@ +42,5 @@
> +}
> +
> +/**
> + * Bug 1008913 : Add test to verify geolocation sharing option
> + *               "Always Share Location"

Please put the description in the next line.

@@ +49,5 @@
> +  // Wait for the geo notification to be opened
> +  locationBar.waitForNotificationPanel(() => {
> +    controller.open(TEST_DATA[0]);
> +    controller.waitForPageLoad();
> +  }, {type: "notification"});

This will also pass for other type of notifications. As long as bug 1000832 is not fixed, you have to check that the correct notification is visible.

@@ +60,5 @@
> +                                                      type: "menuItem",
> +                                                      subtype: "label",
> +                                                      value: alwaysLabel});
> +    alwaysAllowMenuItem.click();
> +  }, {type: "notification", open: false});

Same as above.

@@ +75,5 @@
> +  // With another geolocation service the notification popup should appear
> +  locationBar.waitForNotificationPanel(() => {
> +    controller.open(TEST_DATA[1]);
> +    controller.waitForPageLoad();
> +  }, {type: "notification"});

Same as above.

Comment 17

[Daniel Gherasim]

Attachment: v4.patch

Updated patch based on the last review.

Comment 18

[Andrei Eftimie]

Comment on attachment 8439863 [details] [diff] [review]
v4.patch

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

Looks good.
I'd like to update the Regular Expression though, inline for more info.

With the update, please ask a review from Henrik.

::: firefox/tests/remote/restartTests/testGeolocation/testAlwaysShareLocation.js
@@ +67,5 @@
> +                                                      value: alwaysLabel});
> +    alwaysAllowMenuItem.click();
> +
> +    assert.ok(aPanel.getNode().getAttribute("popupid"), "geolocation",
> +            "Correct notification is closing");

nit: indentation

@@ +119,5 @@
> +  expect.waitFor(() => result.getNode().textContent !== "undefined",
> +                 "Result content has changed", TIMEOUT_GEOLOCATE);
> +
> +  // Test if the result match a float number
> +  expect.match(result.getNode().textContent, /\d+(\.\d*)?\.\d+/,

Not sure about this expression.
Based on the response we receive in http://mozqa.com/data/firefox/geolocation/position.html this should be /\d+\.\d*/ based on:
1. mozmill's assertion .match method doesn't do anything with captured values (we can remove the parentheses)
2. we expect a float, that means one dot between numbers

Comment 19

[Daniel Gherasim]

Attachment: v5.patch

Updated!

Comment 20

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

Comment on attachment 8442669 [details] [diff] [review]
v5.patch

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

::: firefox/tests/remote/restartTests/testGeolocation/testAlwaysShareLocation.js
@@ +45,5 @@
> + * Bug 1008913
> + * Add test to verify geolocation sharing option "Always Share Location"
> + */
> +function testAlwaysShareLocation() {
> +  var correctPanel = null;

If we have to use that, I would name it targetPanel. You don't know if that is the correct one. Also please obey an empty line after declarations.

@@ +54,5 @@
> +    correctPanel = aPanel;
> +  }, {type: "notification"});
> +
> +  assert.ok(correctPanel.getNode().getAttribute("popupid"), "geolocation",
> +            "Correct notification was opened");

I would not rely on the panel as returned above. It doesn't have to be the correct one, because that we check after the callback has been finished. Inside the callback you only have a reference to the wanted notification group type, which has not been verified.

@@ +91,5 @@
> +  assert.ok(correctPanel.getNode().getAttribute("popupid"), "geolocation",
> +            "Correct notification was opened");
> +
> +  tabBrowser.closeAllTabs();
> +  controller.restartApplication("testAlwaysShareLocationPersisted");

Never initiate a restart within a test method! Whenever we fail before that line, the browser will not be restarted and it can fail in various ways, e.g. profile not reset.

Comment 21

[Daniel Gherasim]

Attachment: v6.patch

(In reply to Henrik Skupin (:whimboo) from comment #20)
> @@ +54,5 @@
> > +    correctPanel = aPanel;
> > +  }, {type: "notification"});
> > +
> > +  assert.ok(correctPanel.getNode().getAttribute("popupid"), "geolocation",
> > +            "Correct notification was opened");
> 
> I would not rely on the panel as returned above. It doesn't have to be the
> correct one, because that we check after the callback has been finished.
> Inside the callback you only have a reference to the wanted notification
> group type, which has not been verified.
> 

I changed the assert to check for the geo icon presence, as we do in the other testShareLocation test.

> @@ +91,5 @@
> > +  assert.ok(correctPanel.getNode().getAttribute("popupid"), "geolocation",
> > +            "Correct notification was opened");
> > +
> > +  tabBrowser.closeAllTabs();
> > +  controller.restartApplication("testAlwaysShareLocationPersisted");
> 
> Never initiate a restart within a test method! Whenever we fail before that
> line, the browser will not be restarted and it can fail in various ways,
> e.g. profile not reset.

Done that with a persisted object.

Comment 22

[Andrei Eftimie]

Comment on attachment 8443407 [details] [diff] [review]
v6.patch

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

Just one small change. we don't need to store "available" as a persisted value.

Ask for a review from Henrik with this update. It looks good to me otherwise.

::: firefox/tests/remote/restartTests/testGeolocation/testAlwaysShareLocation.js
@@ +38,5 @@
> +  aModule.tabBrowser.closeAllTabs();
> +}
> +
> +function teardownTest(aModule) {
> +  if(persisted.nextTest.avaible) {

nit: missing space after `if`

@@ +55,5 @@
> + * Bug 1008913
> + * Add test to verify geolocation sharing option "Always Share Location"
> + */
> +function testAlwaysShareLocation() {
> +  persisted.nextTest.avaible = true;

We don't need this.
Lets just use test if "persisted.nextText" is available and restart to it. Then remove the value.

Comment 23

[Daniel Gherasim]

Attachment: v6.1.patch

I also did a small change in the checkGeoNotificationOpened so we test that correct notification is opened.

I've tested this by opening the master password notification instead & works great.

Henrik, can you have a final look ?

Comment 24

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

Comment on attachment 8444418 [details] [diff] [review]
v6.1.patch

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

::: firefox/tests/remote/restartTests/testGeolocation/testAlwaysShareLocation.js
@@ +97,5 @@
> +/**
> + * Test if the always share property is persisted after a restart
> + */
> +function testAlwaysShareLocationPersisted() {
> +  delete persisted.nextTest;

Keep in mind that this will not run if test 1 fails. Better to reset this in teardownTest if it contains data.

@@ +105,5 @@
> +    locationBar.waitForNotificationPanel(aPanel => {
> +      controller.open(TEST_DATA["geoRequest_url1"]);
> +      controller.waitForPageLoad();
> +    }, {type: "notification"});
> +  }, errors.TimeoutError, "'Notification Popup' doesn't exist");

So what happens IF a notification appears? Our test will fail but it will not close the notification, which can cause trouble in follow-up tests.

@@ +128,5 @@
> +function waitForLocationRetrieved() {
> +  var result = findElement.ID(controller.tabs.activeTab, "result");
> +
> +  expect.waitFor(() => result.getNode().textContent !== "undefined",
> +                 "Result content has changed", TIMEOUT_GEOLOCATE);

You cannot check if the content has changed, but you can see if data is available.

@@ +131,5 @@
> +  expect.waitFor(() => result.getNode().textContent !== "undefined",
> +                 "Result content has changed", TIMEOUT_GEOLOCATE);
> +
> +  // Test if the result match a float number
> +  expect.match(result.getNode().textContent, /\d+\.\d*/,

Why a \d* as final part of the regex? I don't think we will ever have a value like '3.' returned, or? I think it should be a \d+ given that there will always be a number behind the dot.

Comment 25

[Daniel Gherasim]

Attachment: v6.2.patch

Updated based on Henrik's review.

Comment 26

[Andrei Eftimie]

Comment on attachment 8445193 [details] [diff] [review]
v6.2.patch

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

r+ from me with the mentioned item fixed.

::: firefox/tests/remote/restartTests/testGeolocation/testAlwaysShareLocation.js
@@ +49,5 @@
> +}
> +
> +function teardownModule(aModule) {
> +  prefs.preferences.clearUserPref(PREF_GEO_WIFI_URI);
> +

Only one thing. We should also delete persisted.nextTest here (even if it's null).

Comment 27

[Daniel Gherasim]

Attachment: v6.3.patch

Updated & tested across platforms.

Comment 28

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

Comment on attachment 8445803 [details] [diff] [review]
v6.3.patch

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

API and test coverage wise it looks fine now. There are still two things I would like to see addressed. It's enough to request review for that from Andreea or Andrei.

::: firefox/tests/remote/restartTests/testGeolocation/testAlwaysShareLocation.js
@@ +42,5 @@
> +  }
> +
> +  if (persisted.nextTest) {
> +    var test = persisted.nextTest;
> +    persisted.nextTest = null;

I see that this adds some duplication here. So why not combining this and from setupModule() and execute it in setupTest()? This way we have it reset each time, and we can get rid of this extra temporary variable.

@@ +144,5 @@
> +                 "Location data is avaible", TIMEOUT_GEOLOCATE);
> +
> +  // Test if the result match a float number
> +  expect.match(result.getNode().textContent, /\d+\.\d+/,
> +               "Correct location has been retrieved");

Something I missed last time and noticed now... When we say correct location, why don't we compare the values to the ones from the location file? Only that way we can ensure that the correct location has been retrieved.

Comment 29

[Daniel Gherasim]

Attachment: v6.4.patch

Thanks, updated based on review.

Comment 30

[Andrei Eftimie]

Comment on attachment 8445846 [details] [diff] [review]
v6.4.patch

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

Looks good.
Please fix the nits mentioned inline and I'll land it.

::: firefox/tests/remote/restartTests/testGeolocation/testAlwaysShareLocation.js
@@ +48,5 @@
> +
> +  if (persisted.nextTest) {
> +    controller.restartApplication(persisted.nextTest);
> +  }
> +

nit: please remove this empty line

@@ +146,5 @@
> +
> +  expect.waitFor(() => result.getNode().textContent !== "undefined",
> +                 "Location data is avaible", TIMEOUT_GEOLOCATE);
> +
> +  // Test if the result match a float number

Please update this comment so it reflects that we are actually testing the correct coordinates.

@@ +147,5 @@
> +  expect.waitFor(() => result.getNode().textContent !== "undefined",
> +                 "Location data is avaible", TIMEOUT_GEOLOCATE);
> +
> +  // Test if the result match a float number
> +  expect.match(result.getNode().textContent, LOCATION.lat + " " + LOCATION.lng,

We can update this to `expect.equal`

Comment 31

[Daniel Gherasim]

Attachment: v6.5.patch

Updates as requested.

Comment 32

[Daniel Gherasim]

Attachment: v6.6.patch

now applies cleanly

Comment 33

[Andrei Eftimie]

Comment on attachment 8446377 [details] [diff] [review]
v6.6.patch

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

Landed:
https://hg.mozilla.org/qa/mozmill-tests/rev/93a3aca4b881 (default)

Comment 34

[Andrei Eftimie]

Patch applies cleanly on mozilla-aurora and mozilla-beta as well. 
Lets monitor today's results, and if all is good I'll transplant it tomorrow.

Comment 35

[Andrei Eftimie]

Tests ran fine on the latest build.

Transplanted:
http://hg.mozilla.org/qa/mozmill-tests/rev/d60f624a2145 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/a4d2b65a4df6 (mozilla-beta)

Thanks Daniel.

Comment 36

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

(In reply to Andrei Eftimie from comment #35)
> http://hg.mozilla.org/qa/mozmill-tests/rev/a4d2b65a4df6 (mozilla-beta)

I have no idea why this patch has been landed on Beta! This caused all of our tests to fail, because the referenced locationbar.reload() method does not exist on that branch.

Backed out from mozilla-beta:
https://hg.mozilla.org/qa/mozmill-tests/rev/b61e666fff73

Comment 37

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #36)
> (In reply to Andrei Eftimie from comment #35)
> > http://hg.mozilla.org/qa/mozmill-tests/rev/a4d2b65a4df6 (mozilla-beta)
> 
> I have no idea why this patch has been landed on Beta!

How come you "have no idea"?
I specifically asked on IRC yesterday if we're to backport this patch:
> [10:18am] andrei_eftimie: whimboo: I just landed bug 1008913 on default
> [10:18am] _AutomationBot: Bug https://bugzilla.mozilla.org/show_bug.cgi?id=1008913 normal, ASSIGNED , Add test to verify geolocation sharing option "Always Share Location"
> [10:19am] andrei_eftimie: should we backport it down to beta? the general rule is that we usually don’t do it for new tests afair
> [...]
> [10:54am] whimboo: andrei_eftimie: in general yes. we may consider it for important ones
> [10:55am] andrei_eftimie: that was my question. should we backport it for this particular case?
> [...]
> [11:17am] whimboo: andrei_eftimie: this test is part of our goals, so I woudl consider it as very important
> [11:17am] andrei_eftimie: ok

I assume this is the case that we want this test to run against Beta, so I'm fixing the issue and re-enable it.

> This caused all of
> our tests to fail, because the referenced locationbar.reload() method does
> not exist on that branch.
> 
> Backed out from mozilla-beta:
> https://hg.mozilla.org/qa/mozmill-tests/rev/b61e666fff73

Sorry for this. Indeed I messed up by not testing it on Beta. We were missing a dependency which was not present on that branch.

I added the missing dependency on Beta from bug 1016343 and readded this test:
https://hg.mozilla.org/qa/mozmill-tests/rev/9c7b983eaa3c (mozilla-beta)

Functional: 
http://mozmill-crowd.blargon7.com/#/functional/report/883d8406dab55a771a00934158bce99f
Remote:
http://mozmill-crowd.blargon7.com/#/remote/report/883d8406dab55a771a00934158bceb4f

All should be fine now.

Comment 38

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

(In reply to Andrei Eftimie from comment #37)
> > I have no idea why this patch has been landed on Beta!
> 
> How come you "have no idea"?
> I specifically asked on IRC yesterday if we're to backport this patch:

Right. But it has not been tested at all. That's the point here. Beta is not a branch where we can handle check-ins loosely. So please get back to the rules, which we have enforced a long time ago.

Comment 39

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

I wonder why this test has been put under firefox/tests/remote/restartTests. That's not necessary anymore with Mozmill 2.x and having all restarts covered by a single test. So it should really have gone into firefox/tests/remote/testGeolocation. Daniel, please file a follow-up bug, so we can get the test moved.