1008914 - Add test to verify geolocation sharing option "Never 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 "Never Share Location" option
>> The location is not shared

3. Reload the page
>> The location sharing hanger is not displayed again and location is not 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 (location is not shared)

Comment 1

[Daniel Gherasim]

Attachment: v1.patch

This uses some functionality from bug 1008913 so that patch need to be applied first (along with it's dependencies).

Comment 2

[Andreea Matei [:AndreeaMatei]]

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

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

::: data/geolocation/position.html
@@ +22,5 @@
> +            break;
> +          case error.UNKNOWN_ERROR:
> +            result.innerHTML="unknown_error";
> +            break;
> +        }

Do we need a default case here?

::: firefox/tests/functional/restartTests/manifest.ini
@@ +7,5 @@
>  [include:testAddons_pluginDisabledAfterRestart/manifest.ini]
>  [include:testAddons_uninstallExtension/manifest.ini]
>  [include:testAddons_uninstallTheme/manifest.ini]
>  [include:testDefaultBookmarks/manifest.ini]
>  [include:testGeolocation_alwaysShareLocation/manifest.ini]

This is in remote now

::: firefox/tests/functional/restartTests/testGeolocation_neverShareLocation/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");

Leave a blank space between them and I would suggest to call it TEST_DATA_UPDATED

@@ +39,5 @@
> +    controller.waitForPageLoad();
> +  }, {type: "notification_popup"});
> +
> +  var neverLabel = utils.getProperty("chrome://browser/locale/browser.properties",
> +                                      "geolocation.neverShareLocation");

Indentation is a bit to the right.

@@ +42,5 @@
> +  var neverLabel = utils.getProperty("chrome://browser/locale/browser.properties",
> +                                      "geolocation.neverShareLocation");
> +  var neverAllowMenuItem = locationBar.getElement({type: "notification_menuItem",
> +                                                  subtype: "label",
> +                                                  value: neverLabel});

Here too, to the left. Should align with 'type'

@@ +49,5 @@
> +    neverAllowMenuItem.click();
> +  }, {type: "notification_popup", open: false});
> +
> +  // Check if the location is not displayed
> +

Please remove this empty line. Also the comment should read "Check the location is not displayed"

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

jsdoc please.

@@ +80,5 @@
> +      controller.waitForPageLoad();
> +    }, {type: "notification_popup"});
> +  }, errors.TimeoutError, "'Notification Popup' doesn't exists");
> +
> +  // Check if the location is not displayed

Same comment as above.

Comment 3

[Daniel Gherasim]

Attachment: v2.patch

Updated patch based on review done here & on bug 1008913.
I'll add the review flags once the first test is landed.

Comment 4

[Daniel Gherasim]

Attachment: v3.patch

First test is landed so updated patch based on reviews done there as it's similar.

Comment 5

[Andreea Matei [:AndreeaMatei]]

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

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

Some nits left.

::: firefox/tests/remote/restartTests/testGeolocation/testNeverShareLocation.js
@@ +14,5 @@
> +  "geoRequest_url1" : BASE_URL + "geolocation/position.html",
> +  "geoRequest_url2" : "http://mozqa.com/data/firefox/geolocation/position.html"
> +};
> +
> +

Please remove one line

@@ +83,5 @@
> +    locationBar.waitForNotificationPanel(aPanel => {
> +      locationBar.reload();
> +      targetPanel = aPanel;
> +    }, {type: "notification"});
> +  }, errors.TimeoutError, "'Notification Popup' doesn't exists");

nit: exist

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

Same here

@@ +123,5 @@
> +  expect.ok(notification, "The geolocation notification is opened");
> +}
> +
> +/**
> + * Check the location is not retrieved and returns sepcific 'denied' error code

nit: specific

Comment 6

[Daniel Gherasim]

Attachment: v3.1.patch

Thanks! Fixed the nits.

Comment 7

[Andreea Matei [:AndreeaMatei]]

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

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

This is similar to the Always share one, looks good now.

Comment 8

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

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

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

r- mainly because of the wrong test location. Lets get this fixed so that we finally can get rid of the restartTests subfolder.

::: data/geolocation/position.html
@@ +9,5 @@
>      }
>  
>      function handleError(error) {
> +      switch(error.code)
> +        {

Please keep the coding styles.

::: firefox/tests/remote/restartTests/testGeolocation/manifest.ini
@@ +1,2 @@
>  [testAlwaysShareLocation.js]
> +[testNeverShareLocation.js]

Please update the location of the test, similar as requested in my last comment on bug 1008913.

::: firefox/tests/remote/restartTests/testGeolocation/testNeverShareLocation.js
@@ +59,5 @@
> +  }, {type: "notification"});
> +
> +  checkGeoNotificationOpened();
> +
> +  // Wait for the geo notification to unload

You want to add the action we do here, so its better understandable.

@@ +127,5 @@
> + */
> +function checkLocationDataRequestDenied() {
> +  // Check if the location is not displayed
> +  var result = new findElement.ID(controller.tabs.activeTab, "result");
> +  expect.waitFor(() => result.getNode().textContent === "denied",

Mind putting brackets around the condition?

Comment 9

[Daniel Gherasim]

Attachment: v3.2.patch

Updated based on the last review.
This patch applies cleanly after patch from bug 1031263 gets landed.

Comment 10

[Daniel Gherasim]

Attachment: v3.3.patch

Updated the patch so it doesn't depend on any other bug now.

Comment 11

[Daniel Gherasim]

Attachment: v3.4.patch

Patch updated.
Applies cleanly on default.

Comment 12

[Andrei Eftimie]

Comment on attachment 8447867 [details] [diff] [review]
v3.4.patch

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

Just a few nits. Looks good to me otherwise.
Please request a review from Henrik once updated.

::: data/geolocation/position.html
@@ +8,5 @@
>        result.innerHTML = position.coords.latitude + " " + position.coords.longitude;
>      }
>  
>      function handleError(error) {
> +      switch(error.code) {

nit: there should be a space after the `switch` command.

@@ +10,5 @@
>  
>      function handleError(error) {
> +      switch(error.code) {
> +        case error.PERMISSION_DENIED:
> +          result.innerHTML="denied";

nit: please leave a space before and after the equal sign.

::: firefox/tests/remote/testGeolocation/testNeverShareLocation.js
@@ +112,5 @@
> +  checkLocationDataRequestDenied();
> +}
> +
> +/**
> + * Check that the geo notification is opened

nit: this should be "open", not "opened"

@@ +114,5 @@
> +
> +/**
> + * Check that the geo notification is opened
> + */
> +function checkGeoNotificationOpened() {

nit: same: "checkGeoNotificationOpen"

@@ +119,5 @@
> +  // Check the icon inside the popup notification exist
> +  var notification = locationBar.getElement({type: "notification_element",
> +                                             subtype: "geolocation-notification",
> +                                             parent: locationBar.getNotification()});
> +  expect.ok(notification, "The geolocation notification is opened");

nit: also here: "open"

@@ +127,5 @@
> + * Check the location is not retrieved and returns specific 'denied' error code
> + */
> +function checkLocationDataRequestDenied() {
> +  // Check if the location is not displayed
> +  var result = new findElement.ID(controller.tabs.activeTab, "result");

You can remove the `new` keyword.

Comment 13

[Daniel Gherasim]

Attachment: v3.5.patch

Filed bug 1032173 so we make the same updates in testcase-data.
Also pointed in bug 1000832 to also make the changes in this test when specific notifications can be retrieved.

Comment 14

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

Comment on attachment 8447958 [details] [diff] [review]
v3.5.patch

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

r=me with the nit fixed.

::: firefox/tests/remote/testGeolocation/testNeverShareLocation.js
@@ +60,5 @@
> +  }, {type: "notification"});
> +
> +  checkGeoNotificationOpen();
> +
> +  // Click on the "Nvever Share Location" item from the menu

'Never'

@@ +63,5 @@
> +
> +  // Click on the "Nvever Share Location" item from the menu
> +  // and wait for the geo notification to unload
> +  locationBar.waitForNotificationPanel(aPanel => {
> +    targetPanel = aPanel;

Make sure we are also getting the updated position of this line fixed for the already landed test.

Comment 15

[Daniel Gherasim]

Attachment: v4.patch

Patch applies cleanly on Default, Aurora & Beta.

Windows:
http://mozmill-crowd.blargon7.com/#/remote/report/59ac3f70f127c02da3a59eb1fc4a53a3
http://mozmill-crowd.blargon7.com/#/remote/report/59ac3f70f127c02da3a59eb1fc4d4017
http://mozmill-crowd.blargon7.com/#/remote/report/59ac3f70f127c02da3a59eb1fc4ce996

Linux:
http://mozmill-crowd.blargon7.com/#/remote/report/59ac3f70f127c02da3a59eb1fc4a8f1c
http://mozmill-crowd.blargon7.com/#/remote/report/59ac3f70f127c02da3a59eb1fc4b7172
http://mozmill-crowd.blargon7.com/#/remote/report/59ac3f70f127c02da3a59eb1fc4d4093

Beta:
http://mozmill-crowd.blargon7.com/#/remote/report/59ac3f70f127c02da3a59eb1fc4a77db
http://mozmill-crowd.blargon7.com/#/remote/report/59ac3f70f127c02da3a59eb1fc5205b4
http://mozmill-crowd.blargon7.com/#/remote/report/59ac3f70f127c02da3a59eb1fc520b90

Comment 16

[Andrei Eftimie]

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

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

All seems good, lets get this checked in:
https://hg.mozilla.org/qa/mozmill-tests/rev/5271dc83182e (default)

Comment 17

[Andrei Eftimie]

Tests look good.

Transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/07b61ec37679 (mozilla-aurora)
https://hg.mozilla.org/qa/mozmill-tests/rev/c111fa09ad5b (mozilla-beta)