Closed Bug 1008914 Opened 8 years ago Closed 8 years ago

Add test to verify geolocation sharing option "Never 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, 8 obsolete files)

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)
Whiteboard: [mentor=andrei.eftimie][lang=js]
Assignee: nobody → daniel.gherasim
Status: NEW → ASSIGNED
Attached patch v1.patch (obsolete) — Splinter Review
This uses some functionality from bug 1008913 so that patch need to be applied first (along with it's dependencies).
Attachment #8430745 - Flags: review?(andrei.eftimie)
Attachment #8430745 - Flags: review?(andreea.matei)
Depends on: 1008913
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.
Attachment #8430745 - Flags: review?(andrei.eftimie)
Attachment #8430745 - Flags: review?(andreea.matei)
Attachment #8430745 - Flags: review-
Attached patch v2.patch (obsolete) — Splinter Review
Updated patch based on review done here & on bug 1008913.
I'll add the review flags once the first test is landed.
Attachment #8430745 - Attachment is obsolete: true
Mentor: andrei.eftimie
Whiteboard: [mentor=andrei.eftimie][lang=js] → [lang=js]
Attached patch v3.patch (obsolete) — Splinter Review
First test is landed so updated patch based on reviews done there as it's similar.
Attachment #8439019 - Attachment is obsolete: true
Attachment #8446397 - Flags: review?(andrei.eftimie)
Attachment #8446397 - Flags: review?(andreea.matei)
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
Attachment #8446397 - Flags: review?(andrei.eftimie)
Attachment #8446397 - Flags: review?(andreea.matei)
Attachment #8446397 - Flags: review-
Attached patch v3.1.patch (obsolete) — Splinter Review
Thanks! Fixed the nits.
Attachment #8446397 - Attachment is obsolete: true
Attachment #8446468 - Flags: review?(andrei.eftimie)
Attachment #8446468 - Flags: review?(andreea.matei)
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.
Attachment #8446468 - Flags: review?(hskupin)
Attachment #8446468 - Flags: review?(andrei.eftimie)
Attachment #8446468 - Flags: review?(andreea.matei)
Attachment #8446468 - Flags: review+
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?
Attachment #8446468 - Flags: review?(hskupin) → review-
Attached patch v3.2.patch (obsolete) — Splinter Review
Updated based on the last review.
This patch applies cleanly after patch from bug 1031263 gets landed.
Attachment #8446468 - Attachment is obsolete: true
Attachment #8447124 - Flags: review?(andrei.eftimie)
Attachment #8447124 - Flags: review?(andreea.matei)
Depends on: 1031263
No longer depends on: 1031263
Attached patch v3.3.patch (obsolete) — Splinter Review
Updated the patch so it doesn't depend on any other bug now.
Attachment #8447124 - Attachment is obsolete: true
Attachment #8447124 - Flags: review?(andrei.eftimie)
Attachment #8447124 - Flags: review?(andreea.matei)
Attachment #8447832 - Flags: review?(andrei.eftimie)
Attachment #8447832 - Flags: review?(andreea.matei)
Attached patch v3.4.patch (obsolete) — Splinter Review
Patch updated.
Applies cleanly on default.
Attachment #8447832 - Attachment is obsolete: true
Attachment #8447832 - Flags: review?(andrei.eftimie)
Attachment #8447832 - Flags: review?(andreea.matei)
Attachment #8447867 - Flags: review?(andrei.eftimie)
Attachment #8447867 - Flags: review?(andreea.matei)
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.
Attachment #8447867 - Flags: review?(andrei.eftimie)
Attachment #8447867 - Flags: review?(andreea.matei)
Attachment #8447867 - Flags: review-
Attached patch v3.5.patch (obsolete) — Splinter Review
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.
Attachment #8447867 - Attachment is obsolete: true
Attachment #8447958 - Flags: review?(hskupin)
Mentor: andrei.eftimie
Whiteboard: [lang=js]
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.
Attachment #8447958 - Flags: review?(hskupin) → review+
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)
Attachment #8448588 - Flags: review?(andrei.eftimie)
Attachment #8448588 - Flags: review?(andreea.matei)
Attachment #8448588 - Flags: review+
Attachment #8448588 - Flags: checkin+
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)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.