Closed Bug 1008919 Opened 8 years ago Closed 8 years ago

Add test to verify geolocation sharing option "Not Now"

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, 7 obsolete files)

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
Whiteboard: [mentor=AndreeaMatei][lang=js]
Whiteboard: [mentor=AndreeaMatei][lang=js] → [mentor=andreea.matei][lang=js]
Assignee: nobody → daniel.gherasim
Status: NEW → ASSIGNED
Depends on: 1008913
Attached patch v1.patch (obsolete) — Splinter Review
(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.
Attachment #8430746 - Flags: review?(andrei.eftimie)
Attachment #8430746 - Flags: review?(andreea.matei)
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.
Attachment #8430746 - Flags: review?(andrei.eftimie)
Attachment #8430746 - Flags: review?(andreea.matei)
Attachment #8430746 - Flags: review-
Attached patch v1.1.patch (obsolete) — Splinter Review
Sorry for that!
Attachment #8430746 - Attachment is obsolete: true
Attachment #8434810 - Flags: review?(andrei.eftimie)
Attachment #8434810 - Flags: review?(andreea.matei)
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.
Attachment #8434810 - Flags: review?(andrei.eftimie)
Attachment #8434810 - Flags: review?(andreea.matei)
Attachment #8434810 - Flags: review-
Attached patch v2.patch (obsolete) — Splinter Review
Updated patch as requested. Will add the review flags once 1st test is done.
Attachment #8434810 - Attachment is obsolete: true
Mentor: andreea.matei
Whiteboard: [mentor=andreea.matei][lang=js] → [lang=js]
Attached patch v3.patch (obsolete) — Splinter Review
Patch is ready.
Attachment #8439029 - Attachment is obsolete: true
Attachment #8446475 - Flags: review?(andrei.eftimie)
Attachment #8446475 - Flags: review?(andreea.matei)
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
Attachment #8446475 - Flags: review?(andrei.eftimie)
Attachment #8446475 - Flags: review?(andreea.matei)
Attachment #8446475 - Flags: review-
Attached patch v4.patch (obsolete) — Splinter Review
Thanks, patch updated!
Attachment #8446475 - Attachment is obsolete: true
Attachment #8447136 - Flags: review?(andrei.eftimie)
Attachment #8447136 - Flags: review?(andreea.matei)
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.
Attachment #8447136 - Flags: review?(hskupin)
Attachment #8447136 - Flags: review?(andrei.eftimie)
Attachment #8447136 - Flags: review?(andreea.matei)
Attachment #8447136 - Flags: review+
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.
Attachment #8447136 - Flags: review?(hskupin) → review-
Attached patch v4.1.patch (obsolete) — Splinter Review
(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
Attachment #8447136 - Attachment is obsolete: true
Attachment #8447941 - Flags: review?(andrei.eftimie)
Attachment #8447941 - Flags: review?(andreea.matei)
Attached patch v4.2.patch (obsolete) — Splinter Review
Fixed some nits.
Attachment #8447941 - Attachment is obsolete: true
Attachment #8447941 - Flags: review?(andrei.eftimie)
Attachment #8447941 - Flags: review?(andreea.matei)
Attachment #8447943 - Flags: review?(andrei.eftimie)
Attachment #8447943 - Flags: review?(andreea.matei)
Attached patch v4.3.patchSplinter Review
Other small nit.
Attachment #8447943 - Attachment is obsolete: true
Attachment #8447943 - Flags: review?(andrei.eftimie)
Attachment #8447943 - Flags: review?(andreea.matei)
Attachment #8447944 - Flags: review?(andrei.eftimie)
Attachment #8447944 - Flags: review?(andreea.matei)
Comment on attachment 8447944 [details] [diff] [review]
v4.3.patch

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

Thanks Daniel.
Attachment #8447944 - Flags: review?(hskupin)
Attachment #8447944 - Flags: review?(andrei.eftimie)
Attachment #8447944 - Flags: review?(andreea.matei)
Attachment #8447944 - Flags: review+
Mentor: andreea.matei
Whiteboard: [lang=js]
Attachment #8447944 - Flags: review?(hskupin) → review+
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)
Attachment #8447944 - Flags: checkin+
Transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/daddd5c45709 (mozilla-aurora)
https://hg.mozilla.org/qa/mozmill-tests/rev/c505356ca261 (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.