Closed Bug 1008913 Opened 8 years ago Closed 8 years ago

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

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(firefox31 fixed, firefox32 fixed, firefox33 fixed)

RESOLVED FIXED
Tracking Status
firefox31 --- fixed
firefox32 --- fixed
firefox33 --- fixed

People

(Reporter: mihaelav, Assigned: danisielm)

References

Details

Attachments

(1 file, 13 obsolete files)

8.39 KB, patch
andrei
: review+
andrei
: checkin+
Details | Diff | Splinter Review
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
Whiteboard: [mentor=andreea.matei][lang=js]
Assignee: nobody → daniel.gherasim
Status: NEW → ASSIGNED
Depends on: 994040
Attached patch v1.patch (obsolete) — Splinter Review
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;
Attachment #8424766 - Flags: review?(andrei.eftimie)
Attachment #8424766 - Flags: review?(andreea.matei)
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
Attachment #8424766 - Flags: review?(andrei.eftimie)
Attachment #8424766 - Flags: review?(andreea.matei)
Attachment #8424766 - Flags: review-
(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 ?
Attached patch v1.1.patch (obsolete) — Splinter Review
Bug 1016343 filed for the reload method.
Patch here updated based on last review.
Attachment #8424766 - Attachment is obsolete: true
Attachment #8429249 - Flags: review?(andrei.eftimie)
Attachment #8429249 - Flags: review?(andreea.matei)
Depends on: 1016343
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.
Attachment #8429249 - Flags: review?(andrei.eftimie)
Attachment #8429249 - Flags: review?(andreea.matei)
Attachment #8429249 - Flags: review-
Attached patch v2.patch (obsolete) — Splinter Review
Updated patch as requested.
Attachment #8429249 - Attachment is obsolete: true
Attachment #8430744 - Flags: review?(andrei.eftimie)
Attachment #8430744 - Flags: review?(andreea.matei)
Blocks: 1008914
Blocks: 1008919
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
Attachment #8430744 - Flags: review?(andrei.eftimie)
Attachment #8430744 - Flags: review?(andreea.matei)
Attachment #8430744 - Flags: review-
Attachment #8430744 - Flags: feedback?(hskupin)
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.
Attachment #8430744 - Flags: feedback?(hskupin) → feedback+
> 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.
I would make this test part of the remote testrun for now, and utilize mozqa.com until we can have it using local resources.
(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.
Attached patch v3.patch (obsolete) — Splinter Review
All dependency bugs are fixed. Updated bug based on last feedbacks & discussions.
Attachment #8430744 - Attachment is obsolete: true
Attachment #8437543 - Flags: review?(andrei.eftimie)
Attachment #8437543 - Flags: review?(andreea.matei)
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
Attachment #8437543 - Flags: review?(andrei.eftimie)
Attachment #8437543 - Flags: review?(andreea.matei)
Attachment #8437543 - Flags: review-
Attached patch v3.1.patch (obsolete) — Splinter Review
Updated based on last review.
Attachment #8437543 - Attachment is obsolete: true
Attachment #8437651 - Flags: review?
Attachment #8437651 - Flags: review?(andrei.eftimie)
Attachment #8437651 - Flags: review?(andreea.matei)
Attachment #8437651 - Flags: review?
Comment on attachment 8437651 [details] [diff] [review]
v3.1.patch

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

This looks good now, thanks.
Attachment #8437651 - Flags: review?(hskupin)
Attachment #8437651 - Flags: review?(andrei.eftimie)
Attachment #8437651 - Flags: review?(andreea.matei)
Attachment #8437651 - Flags: review+
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.
Attachment #8437651 - Flags: review?(hskupin) → review-
Attached patch v4.patch (obsolete) — Splinter Review
Updated patch based on the last review.
Attachment #8437651 - Attachment is obsolete: true
Attachment #8439863 - Flags: review?(andrei.eftimie)
Attachment #8439863 - Flags: review?(andreea.matei)
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
Attachment #8439863 - Flags: review?(andrei.eftimie)
Attachment #8439863 - Flags: review?(andreea.matei)
Attachment #8439863 - Flags: review-
Attached patch v5.patch (obsolete) — Splinter Review
Updated!
Attachment #8439863 - Attachment is obsolete: true
Attachment #8442669 - Flags: review?(hskupin)
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.
Attachment #8442669 - Flags: review?(hskupin) → review-
Mentor: andrei.eftimie
Whiteboard: [mentor=andreea.matei][lang=js] → [lang=js]
Attached patch v6.patch (obsolete) — Splinter Review
(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.
Attachment #8442669 - Attachment is obsolete: true
Attachment #8443407 - Flags: review?(andrei.eftimie)
Attachment #8443407 - Flags: review?(andreea.matei)
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.
Attachment #8443407 - Flags: review?(andrei.eftimie)
Attachment #8443407 - Flags: review?(andreea.matei)
Attachment #8443407 - Flags: review-
Attached patch v6.1.patch (obsolete) — Splinter Review
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 ?
Attachment #8444418 - Flags: review?(hskupin)
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.
Attachment #8444418 - Flags: review?(hskupin) → review-
Attached patch v6.2.patch (obsolete) — Splinter Review
Updated based on Henrik's review.
Attachment #8443407 - Attachment is obsolete: true
Attachment #8444418 - Attachment is obsolete: true
Attachment #8445193 - Flags: review?(andrei.eftimie)
Attachment #8445193 - Flags: review?(andreea.matei)
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).
Attachment #8445193 - Flags: review?(andrei.eftimie)
Attachment #8445193 - Flags: review?(andreea.matei)
Attachment #8445193 - Flags: review+
Attached patch v6.3.patch (obsolete) — Splinter Review
Updated & tested across platforms.
Attachment #8445193 - Attachment is obsolete: true
Attachment #8445803 - Flags: review?(hskupin)
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.
Attachment #8445803 - Flags: review?(hskupin) → review+
Attached patch v6.4.patch (obsolete) — Splinter Review
Thanks, updated based on review.
Attachment #8445803 - Attachment is obsolete: true
Attachment #8445846 - Flags: review?(andrei.eftimie)
Attachment #8445846 - Flags: review?(andreea.matei)
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`
Attachment #8445846 - Flags: review?(andrei.eftimie)
Attachment #8445846 - Flags: review?(andreea.matei)
Attachment #8445846 - Flags: review-
Attached patch v6.5.patch (obsolete) — Splinter Review
Updates as requested.
Attachment #8445846 - Attachment is obsolete: true
Attachment #8446370 - Flags: review?(andrei.eftimie)
Attachment #8446370 - Flags: review?(andreea.matei)
Attached patch v6.6.patchSplinter Review
now applies cleanly
Attachment #8446370 - Attachment is obsolete: true
Attachment #8446370 - Flags: review?(andrei.eftimie)
Attachment #8446370 - Flags: review?(andreea.matei)
Attachment #8446377 - Flags: review?(andrei.eftimie)
Attachment #8446377 - Flags: review?(andreea.matei)
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)
Attachment #8446377 - Flags: review?(andrei.eftimie)
Attachment #8446377 - Flags: review?(andreea.matei)
Attachment #8446377 - Flags: review+
Attachment #8446377 - Flags: checkin+
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.
Flags: needinfo?(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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: needinfo?(andrei.eftimie)
OS: Linux → All
Hardware: x86 → All
Resolution: --- → FIXED
(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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [lang=js]
(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.
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
(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.
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.
Depends on: 1031263
No longer depends on: 1031263
Mentor: andrei.eftimie
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.