1008919 - Add test to verify geolocation sharing option "Not Now"

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 "Not Now" option
>> The location is not shared

3. Reload the page
>> The location sharing hanger is displayed again

4. Restart the browser and open the page from step 1
>> The location hanger is displayed again

Comment 1

[Daniel Gherasim]

Attachment: v1.patch

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #0)
> 2. Select "Not Now" option
> >> The location is not shared
Not Now option doesn't actually trigger any event. So just waiting to see if a location is retrieved doesn't make sense.

Comment 2

[Andreea Matei [:AndreeaMatei]]

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

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

This doesn't have the test included in the patch.

Comment 3

[Daniel Gherasim]

Attachment: v1.1.patch

Sorry for that!

Comment 4

[Andreea Matei [:AndreeaMatei]]

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

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

::: firefox/tests/functional/restartTests/testGeolocation_notNowShareLocation/test1.js
@@ +26,5 @@
> +  utils.sanitize({siteSettings: true});
> +}
> +
> +/**
> + * Bug 1008913 : Add test to verify geolocation sharing option "Always Share Location"

The option is "Not now"

@@ +40,5 @@
> +  var locationLabel = utils.getProperty("chrome://browser/locale/browser.properties",
> +                                        "geolocation.shareLocation");
> +  var button = locationBar.getNotificationElement("geolocation-notification",
> +                                                  {type: "label",
> +                                                  value: locationLabel});

value should align with type.

@@ +59,5 @@
> +  controller.restartApplication("testNotNowShareLocationPersisted");
> +}
> +
> +function testNotNowShareLocationPersisted() {
> +  // No notification should appear

Actually the comment is wrong, the notification should appear, "not now" it's only temporary.

Comment 5

[Daniel Gherasim]

Attachment: v2.patch

Updated patch as requested. Will add the review flags once 1st test is done.

Comment 6

[Daniel Gherasim]

Attachment: v3.patch

Patch is ready.

Comment 7

[Andrei Eftimie]

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

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

Please align this more with the already landed test from bug 1008913.
We're missing to test that the geolocation notification is open: // the checkGeoNotificationOpened() helper function.
If we needed that there, I assume we want the same test here...

Other than that, this looks fine.

::: firefox/tests/functional/restartTests/testGeolocation/testNotNowShareLocation.js
@@ +54,5 @@
> +    controller.waitForPageLoad();
> +    targetPanel = aPanel;
> +  }, {type: "notification"});
> +
> +  // Check if a Share Location button is visible

This comment appears to be out of place. We're actually clicking on the "Not Now" button

@@ +70,5 @@
> +
> +    var notNowAllowMenuItem = locationBar.getElement({type: "notificationPopup_menuItem",
> +                                                      value: notNowLabel,
> +                                                      parent: menupopup});
> +    // Open menupopup from button element

Enhance this comment to explain that we're clicking on the small arrow in the right part of the button to have the menu open

Comment 8

[Daniel Gherasim]

Attachment: v4.patch

Thanks, patch updated!

Comment 9

[Andreea Matei [:AndreeaMatei]]

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

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

Looks fine now, lets get a final look from Henrik.

Comment 10

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

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

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

::: firefox/tests/functional/testGeolocation/testNotNowShareLocation.js
@@ +66,5 @@
> +                                          "geolocation.shareLocation");
> +    var shareLocationButton = locationBar.getNotificationElement("geolocation-notification",
> +                                                                 {type: "label",
> +                                                                  value: locationLabel});
> +    var menupopup = locationBar.getElement({type: "notificationPopup_menu",

Please name it 'notificationPopup_buttonMenu'. That would be better understandable here.

@@ +78,5 @@
> +
> +    // Click on the small arrow from the left of the 'Share Location' button
> +    // to open the menu popup so we can click on 'Not Now' option
> +    shareLocationButton.click(shareLocationButton.getNode().clientWidth - 2,
> +                              shareLocationButton.getNode().clientHeight/2);

I don't really like that. Is there no way to get the dropdown marker? Beside that we use the middle by default, so the 2nd parameter for the height can be left out.

@@ +80,5 @@
> +    // to open the menu popup so we can click on 'Not Now' option
> +    shareLocationButton.click(shareLocationButton.getNode().clientWidth - 2,
> +                              shareLocationButton.getNode().clientHeight/2);
> +    notNowAllowMenuItem.click();
> +    targetPanel = aPanel;

You should really assign this first in the callback. Otherwise if a click fails above, the popup will not be closed in teardownTest. Please also check the other patches, and the already landed test. This may apply to any of the callback methods.

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

Lets make sure we get all those method instances out of the tests in the toolbar module as part of bug 1000832.

Comment 11

[Daniel Gherasim]

Attachment: v4.1.patch

(In reply to Henrik Skupin (:whimboo) from comment #10)
> Comment on attachment 8447136 [details] [diff] [review]
> v4.patch
> 
> Review of attachment 8447136 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +78,5 @@
> > +
> > +    // Click on the small arrow from the left of the 'Share Location' button
> > +    // to open the menu popup so we can click on 'Not Now' option
> > +    shareLocationButton.click(shareLocationButton.getNode().clientWidth - 2,
> > +                              shareLocationButton.getNode().clientHeight/2);
> 
> I don't really like that. Is there no way to get the dropdown marker? Beside
> that we use the middle by default, so the 2nd parameter for the height can
> be left out.

Couldn't find any other solution. That <menupopup> it's wrapped in the <button> element & I don't see a way to trigger an event to that arrow.

> @@ +114,5 @@
> > +  // Check the icon inside the popup notification exists
> > +  var notification = locationBar.getElement({type: "notification_element",
> > +                                             subtype: "geolocation-notification",
> > +                                             parent: locationBar.getNotification()});
> > +  expect.ok(notification, "The geolocation notification is opened");
> 
> Lets make sure we get all those method instances out of the tests in the
> toolbar module as part of bug 1000832.

Pointed that in bug 1000832

Comment 12

[Daniel Gherasim]

Attachment: v4.2.patch

Fixed some nits.

Comment 13

[Daniel Gherasim]

Attachment: v4.3.patch

Other small nit.

Comment 14

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8447944 [details] [diff] [review]
v4.3.patch

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

Thanks Daniel.

Comment 15

[Daniel Gherasim]

Patch applies cleanly on Default, Aurora & Beta.
Windows:
http://mozmill-crowd.blargon7.com/#/functional/report/59ac3f70f127c02da3a59eb1fc4a50ca
http://mozmill-crowd.blargon7.com/#/functional/report/59ac3f70f127c02da3a59eb1fc4c0e21
http://mozmill-crowd.blargon7.com/#/functional/report/59ac3f70f127c02da3a59eb1fc4ea3ee

Linux:
http://mozmill-crowd.blargon7.com/#/functional/report/59ac3f70f127c02da3a59eb1fc4a874a
http://mozmill-crowd.blargon7.com/#/functional/report/59ac3f70f127c02da3a59eb1fc4aa3ec
http://mozmill-crowd.blargon7.com/#/functional/report/59ac3f70f127c02da3a59eb1fc4cdfb9

Mac:
http://mozmill-crowd.blargon7.com/#/functional/report/59ac3f70f127c02da3a59eb1fc4a6adc
http://mozmill-crowd.blargon7.com/#/functional/report/59ac3f70f127c02da3a59eb1fc4cf32a
http://mozmill-crowd.blargon7.com/#/functional/report/59ac3f70f127c02da3a59eb1fc5239eb

Comment 16

[Andrei Eftimie]

Comment on attachment 8447944 [details] [diff] [review]
v4.3.patch

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

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

Comment 17

[Andrei Eftimie]

Transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/daddd5c45709 (mozilla-aurora)
https://hg.mozilla.org/qa/mozmill-tests/rev/c505356ca261 (mozilla-beta)