Closed Bug 758187 Opened 12 years ago Closed 11 years ago

Write a new mozmill test for checking location sharing

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Product:
Mozilla QA Graveyard
Component:
Mozmill Tests
Type:
defect

Tracking

(firefox27 fixed, firefox28 fixed, firefox29 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox27 --- fixed
firefox28 --- fixed
firefox29 --- fixed

People

(Reporter: vladmaniac, Assigned: AndreeaMatei)

References

(
URL
)

Details

(Whiteboard: s=130304 u=new c=geolocation p=1)

Attachments

(3 files, 21 obsolete files)

test that getNotificationIcon works as expected
1.05 KB, application/javascript
Details
patch v9
6.43 KB, patch
andrei
: review+
andrei
: checkin+
Details | Diff | Splinter Review
[Beta] patch v1
7.31 KB, patch
andrei
: review-
Details | Diff | Splinter Review 
We need some better coverage for smoketests and this test is a good candidate IMO 
Litmus: https://litmus.mozilla.org/show_test.cgi?id=17083
Assignee: nobody → vlad.mozbugs
Status: NEW → ASSIGNED
Whiteboard: [mozmill-functional]
Vlad, what's the outcome from the discussion with developers if that really has to be a manual and/or Mozmill test? I don't see anything why this cannot be automated via browser chrome tests.
(In reply to Henrik Skupin (:whimboo) from comment #1)
> Vlad, what's the outcome from the discussion with developers if that really
> has to be a manual and/or Mozmill test? I don't see anything why this cannot
> be automated via browser chrome tests.

There was no discussion so far (Should we have those? Never had them before), yet still:  

We are targeting a coverage here on the smoketests by having automated tests which comply exactly with the litmus test, so that the manual team would not run this in the fast betas cycle. 
I am 100% sure that browser chrome tests are not being developed following the litmus test steps. 
Furthermore, this is a UI test which should IMO be covered in Mozmill 

I still need to finish a regex there an I have a first patch on this one. If you consider it to be useless please close this bug :) 

Thanks
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #2)
> There was no discussion so far (Should we have those? Never had them
> before), yet still:  

I'm not sure who started with the smoketests but there should have been a discussion between developers and feature owners if that's necessary to have as manual test. 

> We are targeting a coverage here on the smoketests by having automated tests
> which comply exactly with the litmus test, so that the manual team would not
> run this in the fast betas cycle. 

Right, but the question here really is if we waste time on creating manual and Mozmill tests if those are already automated. Remember the other bug which has been marked wontfix lately.

> I am 100% sure that browser chrome tests are not being developed following
> the litmus test steps. 

No, because those are independent. But if all those test scenarios are already covered by automated tests, even if those are multiple ones, there is no need for us to create manual / Mozmill tests.

> Furthermore, this is a UI test which should IMO be covered in Mozmill 

No, browser chrome also tests the ui pieces of Firefox. If we would use keyboard shortcuts or have to restart Firefox that would be a case for a manual/Mozmill test.

> I still need to finish a regex there an I have a first patch on this one. If
> you consider it to be useless please close this bug :) 

Do it and we get it landed when its ready. But we should really get better in qualifying what's necessary. As mentioned in last weeks QA meeting we will come up with a documentation soon.
Hi Vlad, here is my recommendations:

Speak with Doug Turner (:dougt) about what existing test coverage exists for this Litmus test. If this functionality is 100% covered via unit tests, browser-chrome tests, etc, then I don't think we need to develop a Mozmill test for this.

Once we know that information, we should make note of it for when we port the Litmus test over to MozTrap. In MozTrap we should be tracking all test coverage, not just Mozmill.
Thanks for clarifications Henrik and Anthony

cc-ing Doug Turner. 
Doug - are there any existing frameworks which cover this test, or should we implement a Mozmill test? Lots of thanks!
Existing geolocation automated tests do not follow our litmus steps and do little testing on the UI, 

http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/ 

Or do I get the above link wrong? 
If I'm right, we totally need this in Mozmill
If that's the case and also the feedback you got from Doug, please go ahead and work on the Mozmill test.
Attached patch initial patch (obsolete) — — Splinter Review 
initial patch for this test 

Henrik, you can review the test whenever you have the time, but please hold off check-in until I can get the feedback from Doug on this bug. Thanks
Attachment #629136 - Flags: review?(hskupin)
Attached patch patch v1.1 (obsolete) — — Splinter Review 
Sorry for missing to add the test folder under the manifest.ini file 
Fixed this in patch v1.1
Attachment #629136 - Attachment is obsolete: true
Attachment #629136 - Flags: review?(hskupin)
Attachment #629142 - Flags: review?(hskupin)
We have the go from Doug on this one - we can make good usage of this test 
Henrik, can you please take the review when you have the time?
Comment on attachment 629142 [details] [diff] [review]
patch v1.1

>+[include:testGeolocation/manifest.ini]

Where is that file? It's not included in this patch.

>+const BROWSER_PROPERTIES = "chrome://browser/locale/browser.properties";

>+const TIMEOUT = 5000;

Why?

>+  // Check if the Geolocation icon appears in the left of the website favicon
>+  var geoIcon = new elementslib.ID(controller.window.document, "notification-popup-box");

Just call the variable 'icon'. Also what you want to see here is a new element entry in the locationbar class for this box. Keep in mind that we do not want to use any elementlib methods directly in our tests. 

>+  controller.waitFor(function(){
>+    return !geoIcon.getNode().hidden;
>+  });

This is not testing that the geolocation icon is shown. The usage of the above mentioned method will ensure that. But better if you would really check the child 'geo-notification-icon' for the showing attribute.

nit: also please always add a space between function and opening brackets. You know that. :)
	
>+  // Get Geolocation popup notification
>+  var popUpNotification = new elementslib.ID(controller.window.document, 
>+					     "geolocation-notification");	

Please make use of the getNotification() method from locationBar class in toolbars.js.

>+  // Check if a Geolocation icon appears in the notification
>+  assert.equal(popUpNotification.getNode().childNodes[0].hidden, false);

You should be able to use getNotificationElement() to retrieve the real element. I don't want to see the usage of childNodes here.

>+  // Get message DTD for popup		
>+  var shareWithSiteDTD = /[^\%S\?$]*/.exec(utils.getProperty(BROWSER_PROPERTIES,
>+                                        "geolocation.shareWithSite"))[0];

What is that doing? I would appreciate a way better comment here. Also get the property value separately.

>+  // Check if message: "Would you like to share your location with <domain>?" appears	
>+  assert.equal(popUpNotification.getNode().notification.message,
>+	       shareWithSiteDTD + domain + '?');

Use expect here because that's not a blocking assertion. Also get the message first via getNotificationElement() and replace equal with match() to do a regex assertion.

>+  // Check if a Share Location button appears
>+  var nodeCollector = new domUtils.nodeCollector(controller.window);
>+
>+  nodeCollector.queryNodes("#geolocation-notification");
>+  nodeCollector.root = nodeCollector.nodes[0];
>+  nodeCollector.queryAnonymousNode("type", "menu-button");
>+
>+  var shareLocationButton = nodeCollector.elements[0];

No need for nodeCollector here. Use getNotificationElement() with a Lookup string again.

>+  // Check if the button displays the correct message  
>+  assert.equal(shareLocationButton.getNode().label, shareLocationDTD);

Move the comment as message into the equal call.

>+  // Check if the location is displayed 
>+  var result = new elementslib.ID(controller.window.document, "result");

You are working in a tab and have to use controller.tabs.activeTab.

>+  controller.waitFor(function () {
>+    return result.getNode().innerHTML !== "undefined";
>+  }, "Geolocation position has been found");
>+}

Does that really work? Have you made a negative test just to ensure? I think the geolocation test under privatebrowsing is wrong too. innerHTML is always defined. Probably we should update the test file and move the information text to a separate paragraph.
Attachment #629142 - Flags: review?(hskupin) → review-
Sorry for leaving this test in a backlog, I will have a patch with fixed revision requests as soon as I have a free cycle
Attached patch patch v1.2 (obsolete) — — Splinter Review 
Requested changes fixed
Attachment #629142 - Attachment is obsolete: true
Attachment #636612 - Flags: review?(hskupin)
> 
> Does that really work? Have you made a negative test just to ensure? I think
> the geolocation test under privatebrowsing is wrong too. innerHTML is always
> defined. Probably we should update the test file and move the information
> text to a separate paragraph.

Do we need a separate bug to fix the geolocation test under private browsing also?
Attachment #636612 - Flags: review?(dave.hunt)
(In reply to Henrik Skupin (:whimboo) from comment #11)
> Comment on attachment 629142 [details] [diff] [review]
> patch v1.1
> 
> >+[include:testGeolocation/manifest.ini]
> 
> Where is that file? It's not included in this patch.

The new patch fixes this

> 
> >+const BROWSER_PROPERTIES = "chrome://browser/locale/browser.properties";
> 
> >+const TIMEOUT = 5000;
> 
> Why?

Sorry my bad, that was just left there by mystake

> 
> >+  // Check if the Geolocation icon appears in the left of the website favicon
> >+  var geoIcon = new elementslib.ID(controller.window.document, "notification-popup-box");
> 
> Just call the variable 'icon'. Also what you want to see here is a new
> element entry in the locationbar class for this box. Keep in mind that we do
> not want to use any elementlib methods directly in our tests. 

fixed in the follow up patch

> 
> >+  controller.waitFor(function(){
> >+    return !geoIcon.getNode().hidden;
> >+  });
> 
> This is not testing that the geolocation icon is shown. The usage of the
> above mentioned method will ensure that. But better if you would really
> check the child 'geo-notification-icon' for the showing attribute.
> 

fixed

> nit: also please always add a space between function and opening brackets.
> You know that. :)
> 	
> >+  // Get Geolocation popup notification
> >+  var popUpNotification = new elementslib.ID(controller.window.document, 
> >+					     "geolocation-notification");	
> 
> Please make use of the getNotification() method from locationBar class in
> toolbars.js.
> 
> >+  // Check if a Geolocation icon appears in the notification
> >+  assert.equal(popUpNotification.getNode().childNodes[0].hidden, false);
> 
> You should be able to use getNotificationElement() to retrieve the real
> element. I don't want to see the usage of childNodes here.

fixed

> 
> >+  // Get message DTD for popup		
> >+  var shareWithSiteDTD = /[^\%S\?$]*/.exec(utils.getProperty(BROWSER_PROPERTIES,
> >+                                        "geolocation.shareWithSite"))[0];
> 
> What is that doing? I would appreciate a way better comment here. Also get
> the property value separately.

Better comment added in the patch, please check and follow up if there are problems 

> 
> >+  // Check if message: "Would you like to share your location with <domain>?" appears	
> >+  assert.equal(popUpNotification.getNode().notification.message,
> >+	       shareWithSiteDTD + domain + '?');
> 
> Use expect here because that's not a blocking assertion. Also get the
> message first via getNotificationElement() and replace equal with match() to
> do a regex assertion.

fixed

> 
> >+  // Check if a Share Location button appears
> >+  var nodeCollector = new domUtils.nodeCollector(controller.window);
> >+
> >+  nodeCollector.queryNodes("#geolocation-notification");
> >+  nodeCollector.root = nodeCollector.nodes[0];
> >+  nodeCollector.queryAnonymousNode("type", "menu-button");
> >+
> >+  var shareLocationButton = nodeCollector.elements[0];
> 
> No need for nodeCollector here. Use getNotificationElement() with a Lookup
> string again.

fixed

> 
> >+  // Check if the button displays the correct message  
> >+  assert.equal(shareLocationButton.getNode().label, shareLocationDTD);
> 
> Move the comment as message into the equal call.
> 

fixed

> >+  // Check if the location is displayed 
> >+  var result = new elementslib.ID(controller.window.document, "result");
> 
> You are working in a tab and have to use controller.tabs.activeTab.
> 

fixed

> >+  controller.waitFor(function () {
> >+    return result.getNode().innerHTML !== "undefined";
> >+  }, "Geolocation position has been found");
> >+}
> 
> Does that really work? Have you made a negative test just to ensure? I think
> the geolocation test under privatebrowsing is wrong too. innerHTML is always
> defined. Probably we should update the test file and move the information
> text to a separate paragraph.

this was changed to use textContent please see the follow up patch

Dave, the patch contains these changes, please ask for more if necessary
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #14)
> > 
> > Does that really work? Have you made a negative test just to ensure? I think
> > the geolocation test under privatebrowsing is wrong too. innerHTML is always
> > defined. Probably we should update the test file and move the information
> > text to a separate paragraph.
> 
> Do we need a separate bug to fix the geolocation test under private browsing
> also?

When we do the change we have to fix it directly. We can't leave a broken test behind.
(In reply to Henrik Skupin (:whimboo) from comment #16)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #14)
> > > 
> > > Does that really work? Have you made a negative test just to ensure? I think
> > > the geolocation test under privatebrowsing is wrong too. innerHTML is always
> > > defined. Probably we should update the test file and move the information
> > > text to a separate paragraph.
> > 
> > Do we need a separate bug to fix the geolocation test under private browsing
> > also?
> 
> When we do the change we have to fix it directly. We can't leave a broken
> test behind.

So can you please validate if my patch is correct so that I can follow up with a new one containing also the fix for the privatebrowsing test which needs geolocation?
Comment on attachment 636612 [details] [diff] [review]
patch v1.2

>+      case "geoIcon":
>+        elem = new elementslib.ID(this._controller.window.document, "geo-notification-icon");
>+        break;

We want to have clear and understandable names without any acronyms. Also I would like to see a helper method which retrieves the currently active notification icon, e.g. activeNotificationIcon().

>+++ b/tests/functional/testGeolocation/testShareLocation.js
>+var domUtils = require("../../../lib/dom-utils");
>+var prefs = require("../../../lib/prefs");

Those modules are not used. Please remove them.

>+function setupModule(){
>+  controller = mozmill.getBrowserController();
>+  locationBar =  new toolbars.locationBar(controller);
>+}

Ensure to close all tabs before doing anything.

>+  // Open geolocation page
>+  controller.open(LOCAL_TEST_PAGE);
>+  controller.waitForPageLoad();

I see those comments as not useful. Please only add them when the code is not clear and needs more details.

>+  // Check if the Geolocation icon appears in the left of the website favicon
>+  var icon = locationBar.getElement({type: "geoIcon"});
>+
>+  controller.waitFor(function() {
>+    return icon.getNode().clientHeight === 16;
>+  });

This is not the right check. Instead have a look at the DOM attributes and JS properties for all the notification icons. There is a better one. Also keep the above mentioned task in mind.

>+  // Check if a Geolocation icon appears in the notification
>+  var geoImage = locationBar.getNotificationElement(
>+			     "geolocation-notification",
>+			     '/anon({"popupid":"geolocation"})');
>+  controller.assertNode(geoImage);

Use expect here and fix the indentation for the getNotificationElement call. The comment should be part of the expect call.

>+  // Get message displayed on popup notification from DTD file		
>+  var dtdProperty = utils.getProperty(BROWSER_PROPERTIES,
>+			  "geolocation.shareWithSite");

Indentation again. Please check our coding styles. I don't want to mention that part again. 

>+  var shareWithSiteDTD = /[^\%S\?$]*/.exec(dtdProperty)[0];
>+
>+  var domain = controller.window.content.location.hostname;

No need for a blank line. Also I would suggest you move this down below the message declaration.

>+  // Check if message: "Would you like to share your location with <domain>?" appears	
>+  var notification = locationBar.getNotificationElement("geolocation-notification");
>+  var message = notification.getNode().getAttribute("label");

This is still not right. You want to get the message via getNotificationElement by specifying the lookup string to the description.

>+  expect.match(message, shareWithSiteDTD + domain + '?');

I do not see a comment for this match() call. Also not sure why match() has been chosen here. There is no regex involved.

>+  // Get caption for Share location button from DTD
>+  var shareLocationDTD = utils.getProperty(BROWSER_PROPERTIES,
>+  			       "geolocation.shareLocation");

This is not getting an entity from a DTD file. So not sure what you want here.

>+  // Check if the button displays the correct message  
>+  expect.equal(shareLocationButton.getNode().label, shareLocationDTD,
>+		       "Button does not display the correct message");

The message wrongly states the expected state.

>+  // Check if the bubble notification window dissapeared
>+  controller.waitFor(function() {
>+    return icon.getNode().clientHeight === 0;
>+  });

Again. Please use a message for the waitFor call instead of a comment and obey the above mentioned comment re visibility. I have mentioned the commenting thingy a couple of time and that's how we have to proceed in the future. Also fix the nit: disappeared.

>+  // Check if the location is displayed 
>+  var result = new elementslib.ID(controller.tabs.activeTab, "result");
>+
>+  controller.waitFor(function () {
>+    return result.getNode().textContent !== "undefined";
>+  }, "Geolocation position has been found");
>+}

If you don't want to fix the failure I have mentioned in this bug please file a new one right away and we can move that over.
Attachment #636612 - Flags: review?(hskupin)
Attachment #636612 - Flags: review?(dave.hunt)
Attachment #636612 - Flags: review-
Attached patch patch v2.0 (obsolete) — — Splinter Review 
Changes addressed
Attachment #636612 - Attachment is obsolete: true
Attachment #640571 - Flags: review?(hskupin)
Adding a test to see that the new helper method getNotificationIcon() in toolbar.js module passes and fails as expected
Attachment #640572 - Flags: feedback?(hskupin)
Re the failure in the geolocation test under private browsing, I consider to be a task to do in other bug, just for the good tracking of things
aouch - I see the indentation being wrong again, this is because of the editor. Will submit a good patch in a sec
Comment on attachment 640571 [details] [diff] [review]
patch v2.0

Cancelling review request for the moment
Attachment #640571 - Flags: review?(hskupin)
Attached patch patch v2.1 (obsolete) — — Splinter Review 
Fixed problem with the editor adding extra spaces. Hope it looks as expected now
Attachment #640571 - Attachment is obsolete: true
Attachment #640573 - Flags: review?(hskupin)
Attachment #640573 - Flags: review?(dave.hunt)
Comment on attachment 640573 [details] [diff] [review]
patch v2.1

>   /**
>+   * Retrieves the active notification icon
>+   * 
>+   * @return The active notification icon element
>+   * @type {ElemBase}
>+   */
>+  getNotificationIcon : function locationBar_getNotificationIcon() {

Based on Henrik's suggestion I would call this getActiveNotificationIcon.

>+    var notificationPopupBox = new elementslib.ID(this._controller.window.document, 
>+                                                  "notification-popup-box"); 
>+    var notificationIconChilds = notificationPopupBox.getNode().childNodes;
>+    var activeIcon = [];    
>+
>+    for (var i = 0; i < notificationIconChilds.length; i++) {
>+      activeIcon.push(notificationPopupBox.getNode().childNodes[i].hasAttribute("showing"));
>+    }
>+    if (activeIcon.indexOf(true) !== -1) {
>+      return notificationPopupBox.getNode().childNodes[activeIcon.indexOf(true)];
>+    }
>+    else throw new Error(arguments.callee.name + ": Notification icon not found");
>+  },

Couldn't you simply return the first notification icon child node that has attribute 'showing'? Is there a need to build up an array of boolean values?

>+  // Get message displayed on popup notification from DTD file		
>+  var dtdProperty = utils.getProperty(BROWSER_PROPERTIES,
>+			               "geolocation.shareWithSite");

This does not appear to be getting anything from a DTD, also the variable name should be more descriptive.

>+  var domain = controller.window.content.location.hostname;
>+	
>+  // Check if message: "Would you like to share your location with <domain>?" appears	
>+  var notification = locationBar.getNotificationElement("geolocation-notification");
>+  var message = notification.getNode().getAttribute("label");

Henrik's previous comment does not apply to have been addressed here: "This is still not right. You want to get the message via getNotificationElement by specifying the lookup string to the description."

>+  var shareWithSiteDTD = /[^\%S\?$]*/.exec(dtdProperty)[0];

What does the 'DTD' refer to in this variable name?
Attachment #640573 - Flags: review?(hskupin)
Attachment #640573 - Flags: review?(dave.hunt)
Attachment #640573 - Flags: review-
> 
> Based on Henrik's suggestion I would call this getActiveNotificationIcon.

Alright. Also the position of the method would remain the same in the file in this case, because it does not make sense to move it alphabetically in this case, because it refers to 'Notification' and should be IMHO after the getNotificationElement method. Correct me if I'm wrong please. 
 

> Couldn't you simply return the first notification icon child node that has
> attribute 'showing'? Is there a need to build up an array of boolean values?

I need to position of the child to get the proper element and return it. If I were to give up on the boolean array, I would need to get to position index from the 'for' loop. Between adding more stuff in a for loop and declaring an array of 7 elements I choose the second. That was the reason I did it, please correct it with a better solution otherwise :)
> 
> >+  // Get message displayed on popup notification from DTD file		
> >+  var dtdProperty = utils.getProperty(BROWSER_PROPERTIES,
> >+			               "geolocation.shareWithSite");
> 
> This does not appear to be getting anything from a DTD, also the variable
> name should be more descriptive.

The name is not right, agree. But its getting stuff from a DTD. Please see what BROWSER_PROPERTIES points to.  

> 
> >+  var domain = controller.window.content.location.hostname;
> >+	
> >+  // Check if message: "Would you like to share your location with <domain>?" appears	
> >+  var notification = locationBar.getNotificationElement("geolocation-notification");
> >+  var message = notification.getNode().getAttribute("label");
> 
> Henrik's previous comment does not apply to have been addressed here: "This
> is still not right. You want to get the message via getNotificationElement
> by specifying the lookup string to the description."
> 

Can't say upfront I'll look into but I sense you are right

> >+  var shareWithSiteDTD = /[^\%S\?$]*/.exec(dtdProperty)[0];

> 
> What does the 'DTD' refer to in this variable name?

again, the strings are from a DTD file, please double check that.
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #26)
> > 
> > Based on Henrik's suggestion I would call this getActiveNotificationIcon.
> 
> Alright. Also the position of the method would remain the same in the file
> in this case, because it does not make sense to move it alphabetically in
> this case, because it refers to 'Notification' and should be IMHO after the
> getNotificationElement method. Correct me if I'm wrong please. 

I'll defer to Henrik on that one. I'm not sure how strict we are on alphabetically listing methods.

> > Couldn't you simply return the first notification icon child node that has
> > attribute 'showing'? Is there a need to build up an array of boolean values?
> 
> I need to position of the child to get the proper element and return it. If
> I were to give up on the boolean array, I would need to get to position
> index from the 'for' loop. Between adding more stuff in a for loop and
> declaring an array of 7 elements I choose the second. That was the reason I
> did it, please correct it with a better solution otherwise :)

Well the following is working for me, so I'm not sure why it wouldn't also work for you:

getNotificationIcon : function locationBar_getNotificationIcon() {
  var notificationPopupBox = new elementslib.ID(this._controller.window.document, 
                                                "notification-popup-box"); 
  var notificationIcons = notificationPopupBox.getNode().childNodes;

  for (var i = 0; i < notificationIcons.length; i++) {
    if (notificationIcons[i].hasAttribute("showing")) {
      return notificationIcons[i]
    }
  }
  throw new Error(arguments.callee.name + ": Notification icon not found");
},

> > 
> > >+  // Get message displayed on popup notification from DTD file		
> > >+  var dtdProperty = utils.getProperty(BROWSER_PROPERTIES,
> > >+			               "geolocation.shareWithSite");
> > 
> > This does not appear to be getting anything from a DTD, also the variable
> > name should be more descriptive.
> 
> The name is not right, agree. But its getting stuff from a DTD. Please see
> what BROWSER_PROPERTIES points to.  

A properties file is not a DTD. See https://developer.mozilla.org/en/XUL_Tutorial/Localization#DTD_Files

> > >+  var shareWithSiteDTD = /[^\%S\?$]*/.exec(dtdProperty)[0];
> 
> > 
> > What does the 'DTD' refer to in this variable name?
> 
> again, the strings are from a DTD file, please double check that.

As above, this does not look like a DTD to me.

As mentioned on IRC the patch does not apply cleanly for me (the tests/functional/manifest.ini has conflicts), and the test often fails when run on Mac OS X.

I also noticed trailing spaces and tabs where we should be using spaces. Please make sure your patches have the appropriate white space.
Attached patch patch v2.3 (obsolete) — — Splinter Review 
Fixed: 

* re-name variables which had 'DTD' in the name
* inserted a waitFor and TIMEOUT const and the test is not failing intermittently any more. The extra timeout is needed because the notification is dismissed when the geolocation info appears: this may vary in time 
* re Henrik's suggestions: getNotificationElement was changed to elementslib.ID because the element has an id and this is much better than a lookup IMHO
Attachment #640573 - Attachment is obsolete: true
Attachment #642567 - Flags: review?(dave.hunt)
Almost forgot, also the manifest would not cause problems on the patch anymore, a local hg pull -u was needed to fix that. Thanks
Comment on attachment 642567 [details] [diff] [review]
patch v2.3

>+  getActiveNotificationIcon : function locationBar_getActiveNotificationIcon() {

Please put this in the right alphabetical order. We do not have sections in this class yet. Also I would use a getter here.

>+    var notificationPopupBox = new elementslib.ID(this._controller.window.document, 
>+                                                  "notification-popup-box"); 

*Never* access elements directly outside of the getElement(s) method. This has to be moved over there and also nodeCollector has to be used here.

>+  controller.waitFor(function () {
>+    activeNotificationIcon = locationBar.getActiveNotificationIcon();
>+    return (activeNotificationIcon !== undefined);  

This can be done in a single line.

>+  controller.waitFor(function () {
>+    return activeNotificationIcon.getAttribute("id") === "geo-notification-icon";
>+  }, "The geolocation icon appears in the left of the website favicon");

Not sure why this has to be a waitFor. It's clearly an expect.equal() call. Also you do not test anything related to the orientation, so please remove that from the comment. In RTL locales it will be on the right side. 

>+  var notification = new elementslib.ID(controller.window.document, "geolocation-notification");

Again, what I have already mentioned in a former review: we will never access elements directly but in the appropriate lib.

>+  // Get caption for Share location button
>+  var shareLocationProperty = utils.getProperty(BROWSER_PROPERTIES,
>+  			                         "geolocation.shareLocation");
>+	
>+  // Check if a Share Location button appears and displays the correct message
>+  var shareLocationButton = locationBar.getNotificationElement("geolocation-notification",
>+  					                        '/anon({"anonid":"button"})');

Not sure on this one and if it has to be checked. It's clearly an l10n related test. Shouldn't be it enough for us to test that the button is visible? Please discuss this with Anthony too.

>+  controller.waitThenClick(shareLocationButton);

You can click directly.

>+  controller.waitFor(function () {
>+    return !locationBar.getNotification().getNode().hasAttribute("panelopen");
>+  }, TIMEOUT, "The bubble notification window disappeared");

'TIMEOUT' for what? Please give it a clearer description.
Attachment #642567 - Flags: review?(dave.hunt) → review-
(In reply to Henrik Skupin (:whimboo) from comment #30)
> Comment on attachment 642567 [details] [diff] [review]
> patch v2.3
> 
> >+  getActiveNotificationIcon : function locationBar_getActiveNotificationIcon() {
> 
> Please put this in the right alphabetical order. We do not have sections in
> this class yet. Also I would use a getter here.

I do not understand why would we need a getter because we define and implement the function in the locationBar class. Do we want it somewhere else and then use a getter in the locationBar class? 
Can you please elaborate a bit? Thanks - we have some time to discuss it while Anthony gives his input re the l10n check in the test
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #31)
> I do not understand why would we need a getter because we define and
> implement the function in the locationBar class. Do we want it somewhere
> else and then use a getter in the locationBar class? 

That's correct.
Attachment #640572 - Flags: feedback?(hskupin)
>>+  // Check if a Share Location button appears and displays the correct message
>>+  var shareLocationButton = locationBar.getNotificationElement("geolocation-notification",
>>+  					                        '/anon({"anonid":"button"})');
>
> Not sure on this one and if it has to be checked. It's clearly an l10n related test. 
> Shouldn't be it enough for us to test that the button is visible? 
> Please discuss this with Anthony too.

I think I agree with this assessment. The functional test needs only check that the button has appeared. The l10n test would make sure the button is properly localized.
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #33)
> >>+  // Check if a Share Location button appears and displays the correct message
> >>+  var shareLocationButton = locationBar.getNotificationElement("geolocation-notification",
> >>+  					                        '/anon({"anonid":"button"})');
> >
> > Not sure on this one and if it has to be checked. It's clearly an l10n related test. 
> > Shouldn't be it enough for us to test that the button is visible? 
> > Please discuss this with Anthony too.
> 
> I think I agree with this assessment. The functional test needs only check
> that the button has appeared. The l10n test would make sure the button is
> properly localized.

Thanks for the feedback Anthony! 

Henrik - one more thing needed here. re adding metadata with the moztrap id, I've found the id of the test but I do not really know how this works, is it somewhere documented?
Also how would you suggest to check that the button is visible? 
The UI code for this element is here 
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#927 

and it has only seven attributes which can be easily checked by dumping 
shareLocationButton.getNode().attributes.length in the terminal

Also, the .visible JS property does not apply here
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #34)
> Henrik - one more thing needed here. re adding metadata with the moztrap id,
> I've found the id of the test but I do not really know how this works, is it
> somewhere documented?

We have to set the id via the meta property on the test function itself. For Litmus we did:

testStopAndReload.meta = {litmusids : [8030]};

I would suggest the following format for moztrap:

testStopAndReload.meta = {moztrap_testid: 645};

Usually we will only have a single test id, but in some cases it can be that different tests exist for different platforms or their versions. But logic for that will have to be implemented beside this line, so it will not affect your code here.
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #35)
> and it has only seven attributes which can be easily checked by dumping 
> shareLocationButton.getNode().attributes.length in the terminal
> 
> Also, the .visible JS property does not apply here

As you can see in the XBL widget the button is a usual xul:button, so all its properties apply here. Keep in mind that 'visible' is not the only existing property but 'hidden' and others also exist.

Looks like we can extend assertElementVisible() in utils.js here.
Attached patch patch v3.0 (obsolete) — — Splinter Review 
fixed all change requests
Attachment #642567 - Attachment is obsolete: true
Attachment #644206 - Flags: review?(hskupin)
Comment on attachment 644206 [details] [diff] [review]
patch v3.0

adding Dave
Attachment #644206 - Flags: review?(dave.hunt)
This patch was tested on all platforms, I've insisted on a win XP VM which is a bit degraded 
Test behaves as expected 

http://mozmill-crowd.blargon7.com/#/functional/reports?branch=All&platform=All&from=2012-07-19&to=2012-07-20
Comment on attachment 644206 [details] [diff] [review]
patch v3.0

>+  get activeNotificationIcon() {
>+    return activeNotificationIcon(this._controller);
>+  },

Why has the code for activeNotificationIcon been moved out to the global scope? This is part of the locationbar class. Also I don't see a reason why we should create a wrapper when it simply calls activeNotificationIcon(). Kill the activeNotificationIcon() method entirely.

>+      case "notification_geolocation":
>+        elem = new elementslib.ID(this._controller.window.document, "geolocation-notification");
>+        break;

Please don't reference this element directly but use the subtype of this method to specify which notification you want to grab. In that case the type has to be 'notification_icon' and as subtype we want to throw in 'geolocation' for example. There are way more notifications we want to handle.

>+  var notificationIcons = notificationPopupBox.getNode().childNodes;
>+
>+  for (var i = 0; i < notificationIcons.length; i++) {
>+    if (notificationIcons[i].hasAttribute("showing")) {
>+      return notificationIcons[i];
>+    }

Use nodeCollector here please as mentioned already before. Also you should return an element and not a node.

>+  var activeNotificationIcon = locationBar.activeNotificationIcon;
>+
>+  expect.equal(activeNotificationIcon.getAttribute("id"), "geo-notification-icon", 
>+               "The geolocation icon appears in the website favicon");

>+  var geoImage = locationBar.getNotificationElement("geolocation-notification",
>+			                             '/anon({"popupid":"geolocation"})');

Is there no better anon attribute available? This could be anything below the notification element but doesn't exactly reference the icon. Or has the icon the popupid attribute only? I wouldn't think so. Also please fix the indentation, and could we call it simply icon instead of geoImage?

>+  // Get message displayed on popup notification		
>+  var shareWithSiteProperty = utils.getProperty(BROWSER_PROPERTIES,
>+			                         "geolocation.shareWithSite");
>+  var domain = controller.window.content.location.hostname;
>+	
>+  // Check if message: "Would you like to share your location with <domain>?" appears	
>+  var notification = locationBar.getElement({type: "notification_geolocation"});
>+  var message = notification.getNode().getAttribute("label");
>+  var shareWithSite = /[^\%S\?$]*/.exec(shareWithSiteProperty)[0];
>+
>+  expect.equal(message, shareWithSite + domain + '?',
>+               "Triggered confirmation message is correct");

Given that we do not check for the text of the button I do not think we need this.

>+  // Check if a Share Location button is visible
>+  var shareLocationButton = locationBar.getNotificationElement("geolocation-notification",
>+  					                        '/anon({"anonid":"button"})');

Try to keep your variable names short. There is only a single button in this notification, so why not calling it 'button'?

>+  expect.ok(!shareLocationButton.getNode().hidden, "Share location button is visible");

So what about using the method in utils.js I have mentioned before? While thinking more about I'm even not sure if we need this check at all. If the button is not visible the event will not be fired and the following tests will fail.

>+  // XXX: The notification panel unloads lazily so additional timeout is needed
>+  controller.waitFor(function () {
>+    return activeNotificationIcon === null;
>+  }, TIMEOUT, "The bubble notification window disappeared");

With the above fix this will not work anymore because activeNotificationIcon is of type element.

>+/**
>+ * Map test functions to moztrap tests
>+ */

>+// testLarryBlue.meta = {moztrap_testid: 329};

Checking the moztrap test you are testing way more in this automated test as it was requested by the manual test writer. While that doesn't have be wrong we possibly have duplicated code with other test frameworks. I assume that the icon details and others are already tested via browser-chrome tests. Therefore analyze first what's really necessary in the Mozmill test and don't make it too complicated.
Attachment #644206 - Flags: review?(hskupin)
Attachment #644206 - Flags: review?(dave.hunt)
Attachment #644206 - Flags: review-
> 
> >+  // XXX: The notification panel unloads lazily so additional timeout is needed
> >+  controller.waitFor(function () {
> >+    return activeNotificationIcon === null;
> >+  }, TIMEOUT, "The bubble notification window disappeared");
> 
> With the above fix this will not work anymore because activeNotificationIcon
> is of type element.
> 
Before testing patch is ready, I want to clarify that this will work because the activeNotificationIcon getter has a fallback to null, meaning the line 

    return null; 

We return null if no active notification is available.
Attached patch patch v4.0 (obsolete) — — Splinter Review 
Changes: 

* implemented the getter in the locationBar class
* used nodeCollector to get elements 
* deleted code which checked for strings (that is l10n) 
* please note that some review requests are not addressed due to code deletion
Attachment #644206 - Attachment is obsolete: true
Attachment #644252 - Flags: review?(hskupin)
> Is there no better anon attribute available? This could be anything below
> the notification element but doesn't exactly reference the icon. Or has the
> icon the popupid attribute only? I wouldn't think so. Also please fix the
> indentation, and could we call it simply icon instead of geoImage?

The attribute is only for the icon this can easily be verified with the DOM Inspector. 
Sorry for the indentation, its fixed in the new patch
Comment on attachment 644252 [details] [diff] [review]
patch v4.0

adding Dave
Attachment #644252 - Flags: review?(dave.hunt)
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #42)
> > >+  // XXX: The notification panel unloads lazily so additional timeout is needed
> > >+  controller.waitFor(function () {
> > >+    return activeNotificationIcon === null;
> > >+  }, TIMEOUT, "The bubble notification window disappeared");
> > 
> > With the above fix this will not work anymore because activeNotificationIcon
> > is of type element.
> > 
> Before testing patch is ready, I want to clarify that this will work because
> the activeNotificationIcon getter has a fallback to null, meaning the line 
> 
>     return null; 
> 
> We return null if no active notification is available.

Not sure about this statement. It's simply wrong because we have to return a valid element even if its node is null. So this *will* not work!
Comment on attachment 644252 [details] [diff] [review]
patch v4.0

This patch is failing regularly when Firefox does not have focus.
Attachment #644252 - Flags: review?(hskupin)
Attachment #644252 - Flags: review?(dave.hunt)
Attachment #644252 - Flags: review-
Vlad, as we agreed on this bug should be a priority for you. Now there are 2 days without updates. Please continue to work on this bug so we can get it landed soonish. Thanks.
(In reply to Henrik Skupin (:whimboo) from comment #48)
> Vlad, as we agreed on this bug should be a priority for you. Now there are 2
> days without updates. Please continue to work on this bug so we can get it
> landed soonish. Thanks.

Glad you said, I knew that we wanted to land it because of the mozTrap migration, but I was giving it a priority equal to the other test development bugs. Perhaps we should keep a list of priorities somewhere, these times I kinda miss Pivotal Tracker. 

Anyways, coming back on the bug again, re comment 46 - which element we want to return? 
Right now we are collecting all elements and filter for the one which has the "showing" attribute and if found we return that element. Instead of returning the null object, we want to return an element. Probably I need to re-think the whole method from the beginning ?
The method as is to retrieve the node(s) you shouldn't use as mentioned already twice on this bug. Again, make use of nodeCollector to retrieve the appropriate node/element. It will automatically wrap a null object into an element afair.
Attached patch patch v5.0 (obsolete) — — Splinter Review 
Modified ActiveNotificationIcon getter to return an element always
Attachment #644252 - Attachment is obsolete: true
Attachment #646094 - Flags: review?(hskupin)
Attachment #646094 - Flags: review?(dave.hunt)
Dave, before giving the review verdict, mind testing the patch see if it fails for you? 
@my side it looks pretty good but I would really need you to give the go
Attached patch patch v5.1 (obsolete) — — Splinter Review 
So tested locally on a very heavy loaded environment on mac os x lion and the test failed. did not fail if tested on standard system loading. 

so a simple waitFor does the trick and fixes this issue

Dave - please test this patch on your system let's see how it goes
Attachment #646094 - Attachment is obsolete: true
Attachment #646094 - Flags: review?(hskupin)
Attachment #646094 - Flags: review?(dave.hunt)
Attachment #646525 - Flags: review?(dave.hunt)
Comment on attachment 646525 [details] [diff] [review]
patch v5.1

First off, this still fails for me (Mac OS X 10.74) if Firefox does not have focus.

>Bug 758187 - Write a new mozmill test for checking location sharing. r=hskupin

Please add me to the list of reviewers.

>+  controller.waitFor(function () {
>+    activeNotificationIcon = locationBar.activeNotificationIcon;
>+
>+    return (activeNotificationIcon !== undefined);
>+  }, "The geolocation icon has been found");

I'm not sure this is the most accurate message. At this point you're verifying that an active notification icon has been found, not specifically that it is the geolocation icon (that's the next check).

>+  expect.equal(activeNotificationIcon.getNode().getAttribute("id"), "geo-notification-icon", 
>+               "The geolocation icon appears in the website favicon");

Is favicon relevant here? Perhaps the message should be "The geolocation icon appears in the awesome bar"? Please correct me if I'm missing something here.

I also noticed whitespace issues (trailing whitespace, and tabs instead of spaces).
Attachment #646525 - Flags: review?(dave.hunt) → review-
Its very easy to make the test fail without Firefox on focus, but I cannot make other tests which use notifications fail in this situation. So I'm afraid we have a single case for this notification IMHO

Dave, do you have any suggestions?
Please provide details of other tests that interact with notifications but do not suffer from the focus issue.
(In reply to Dave Hunt (:davehunt) from comment #56)
> Please provide details of other tests that interact with notifications but
> do not suffer from the focus issue.

Please see preferences tests like 
  tests/functional/testPreferences/testPasswordSavedAndDeleted.js 
or just do a grep to identify all. 

I was testing with Firefox without focus, meaning focusing the terminal and leave the test to run and they are a PASS. If doing the same with the geolocation test, it fails.
Its no wonder the test is failing if Firefox does not have focus - because the position is not retrieved in this case. 

Its easily reproducible manually with the testcase on mozqa.com, the one used in the mozmill test also http://mozqa.com/data/firefox/geolocation/position.html 

Steps to reproduce: 

1. Open test page
2. Select something else on the screen so that Firefox would loose focus. Note: this would have to be done quite fast 

I am currently working with the testpage see if something could be done there. The next step would be to search/log Firefox bugs re this problem
I think the behavior of the geolocation notification dialog is normal : these anchor dialogs are design to be easily dismissed, but you can always click on the icon to trigger them again. 

The problem here, and my final question would be - Why focusmanager.testmode pref being set to 'true' by mozprofile does not take action for this particular notification? 

cc-ing Doug on this one. Doug, would you mind giving us some feedback?
Thanks for looking into this Vlad. If the geolocation notification acts differently to other notifications, then we should certainly see if we can avoid focus issues when tests run with Firefox not in focus.
(In reply to Dave Hunt (:davehunt) from comment #60)
> Thanks for looking into this Vlad. If the geolocation notification acts
> differently to other notifications, then we should certainly see if we can
> avoid focus issues when tests run with Firefox not in focus.

No worries Dave - the test will have no value if Firefox has no focus, so probably we would want to 'invent' something to keep it in focus at the runtime of this test - mouse click some coordinates on firefox to gain focus would do imo
No, I would expect the focus manager test mode to assist here, however such notifications may need to be made aware of it. Hopefully Doug can help out here, or CC someone more suitable.
I would talk to Gavin Sharp about this issue. He probably has an answer for it.
Gavin, would you mind giving me some feedback on comment 59? I would really appreciate. We need this to land soonish. Lots of thanks!
I don't of any reasons offhand why the geolocation PopupNotification would behave differently than other PopupNotifications, wrt focus. Are you sure it's not some other timing-related issue?

I'm not really familiar with focus manager test mode, or how it would possibly impact popupnotification panels, but Neil might have some ideas.
Priority: -- → P2
Whiteboard: [mozmill-functional]
This bug is a quarterly goal so please finish it off by end of this week.
Whiteboard: s=q3 u=new c=geolocation p=1
Flagging Neil for feedback.
Flags: needinfo?(enndeakin)
The test testPasswordSavedAndDeleted.js has a line
  controller.type(userField, "bar")
which causes window.focus() to be called.

The test you've added in this patch doesn't, so the popup doesn't appear as the window is in the background.

Is the test framework intenionally opening Firefox as a background window? If so, you'll need to 'focus' the firefox window before running any tests by calling window.focus(). If focusmanager.testmode is enabled, this will set the window as focused internally without making it actually focused by the platform.
Flags: needinfo?(enndeakin)
(In reply to Neil Deakin from comment #68)
> calling window.focus(). If focusmanager.testmode is enabled, this will set
> the window as focused internally without making it actually focused by the
> platform.

Neil, the focusmanager.testmode pref is always true for the Mozmill framework. And as mentioned above other tests for other type of doorhanger notifications work pretty well.

And as I have also stated I can see this behavior when doing the steps manually. The location sharing notification is the only one which is not shown when Firefox is in the background.
The testmode is designed in the situation when you want to run the tests in the background. The only behavioural change it makes is to not bring windows to the front or focus them while the test is running. The test mode does not magically make features (such as popups) work that aren't supposed to work while not in a foreground window.

If you have a test that will only pass if the window is focused, you need to focus the window (typically by using window.focus()). The test mode will ensure that the window doesn't physically appear in front but is instead 'virtually in front'.

If you run a test manually, and lower the window yourself, then the geolocation test should fail as the window is not in the foreground as the geolocation test nor the test framework doesn't ever focus the window such the testmode makes the window 'virtually in front'.

The testPasswordSavedAndDeleted.js *does* call window.focus() so it passes fine.

Note that I am running the tests as follows:

mozmill -b /builds/moz2/ready/output/dist/NightlyDebug.app -t tests/functional/testGeolocation/testShareLocation.js

I deduced all of the above analysis from focus logging.
(In reply to Neil Deakin from comment #70)
> If you have a test that will only pass if the window is focused, you need to
> focus the window (typically by using window.focus()). The test mode will
> ensure that the window doesn't physically appear in front but is instead
> 'virtually in front'.

Oh, that part was really useful, Neil! If it's only virtually in front that would be the behavior we want to have. Vlad, please check that and come up with a new patch if that works.
Whiteboard: s=q3 u=new c=geolocation p=1
Attached patch patch v5.2 (obsolete) — — Splinter Review 
* works for me, tested with Linux and Mac Os X, Dave can you also confirm?
* addressed Dave's requests from the previous review cycle
Attachment #646525 - Attachment is obsolete: true
Attachment #673125 - Flags: review?(hskupin)
Attachment #673125 - Flags: review?(dave.hunt)
Comment on attachment 673125 [details] [diff] [review]
patch v5.2

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

Good to hear. Some things we have to address through. Leaving review request for Dave open so he can check the other changes made.

::: lib/toolbars.js
@@ +341,5 @@
> +    nodeCollector.queryNodes("#notification-popup-box");
> +    nodeCollector.root = nodeCollector.nodes[0];
> +    nodeCollector.queryNodes(".notification-anchor-icon[showing='true']");
> +
> +    return nodeCollector.elements[0]; 

Do we really have to use nodeCollector? Why cna't we do this simply with elementslib.Selector()?

Also you have again introduced whitespace issues which have to be caught by your internal review!

::: tests/functional/testGeolocation/testShareLocation.js
@@ +23,5 @@
> +/**
> + * Test displaying geolocation notification
> + */
> +function testVerifyDisplayGeolocationNotification() {
> +  controller.window.focus();

This should be in setupModule() but ultimately this line has to be called in Mozmill itself. I would propose we fix the constructor of MozMillController and release a new 1.5 version.
Attachment #673125 - Flags: review?(hskupin) → review-
Attached patch patch v5.3 (obsolete) — — Splinter Review 
* no more whitespaces
* passed internal reviews
* switched from nodeCollector to elementslib.Selector so that we do iterate for all the nodes in the DOM as there will be only one active notification at a given time
* also update controller.waitFor() into assert.waitFor()
Attachment #673125 - Attachment is obsolete: true
Attachment #673125 - Flags: review?(dave.hunt)
Attachment #673160 - Flags: review?(hskupin)
Attachment #673160 - Flags: review?(dave.hunt)
Comment on attachment 673160 [details] [diff] [review]
patch v5.3

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

Looks good and passes every time for me now. Just one thing to address before we can land this.

::: tests/functional/testGeolocation/testShareLocation.js
@@ +27,5 @@
> +function testVerifyDisplayGeolocationNotification() {
> +  controller.open(LOCAL_TEST_PAGE);
> +  controller.waitForPageLoad();
> +
> +  controller.waitFor(function () {

You've changed the other instances of controller.waitFor to use assert/expect. We should also change this one.
Attachment #673160 - Flags: review?(hskupin)
Attachment #673160 - Flags: review?(dave.hunt)
Attachment #673160 - Flags: review-
Attached patch patch v5.4 (obsolete) — — Splinter Review 
* done
Attachment #673160 - Attachment is obsolete: true
Attachment #673212 - Flags: review?(hskupin)
Attachment #673212 - Flags: review?(dave.hunt)
Comment on attachment 673212 [details] [diff] [review]
patch v5.4

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

::: tests/functional/testGeolocation/testShareLocation.js
@@ +16,5 @@
> +  controller = mozmill.getBrowserController();
> +  locationBar =  new toolbars.locationBar(controller);
> +  tabBrowser = new tabs.tabBrowser(controller);
> +
> +  controller.window.focus();

Personally I wouldn't take this but fix it in Mozmill right away. It's a quick fix through. So please get my last review comments addressed Vlad.
Attachment #673212 - Flags: review?(hskupin)
Comment on attachment 673212 [details] [diff] [review]
patch v5.4

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

We need bug 803492 solved first to be able to put together the final patch. Clearing review request.
Attachment #673212 - Flags: review?(dave.hunt)
Assignee: vlad.mozbugs → nobody
Status: ASSIGNED → NEW
Assignee: nobody → andreea.matei
Whiteboard: s=130304 u=new c=geolocation p=1
Status: NEW → ASSIGNED
Attached patch patch v5.5 (obsolete) — — Splinter Review 
I'm taking the element in the getter through it's ID and waiting for the showing attribute to be available.

Tested on all platforms (Win 8, XP, OS X 10.8.2 and Ubuntu 12.04), over the weekend I've let it run for 3000 times on each and it did not failed.

Here are passing reports:
OS X: http://mozmill-crowd.blargon7.com/#/functional/report/db924715258381629c26ddbad6ba9335
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/db924715258381629c26ddbad6bd2599
Win XP: http://mozmill-crowd.blargon7.com/#/functional/report/db924715258381629c26ddbad6c29a88
Win 8: http://mozmill-crowd.blargon7.com/#/functional/report/db924715258381629c26ddbad6c86217
Attachment #673212 - Attachment is obsolete: true
Attachment #728968 - Flags: review?(hskupin)
Attachment #728968 - Flags: review?(dave.hunt)
Comment on attachment 728968 [details] [diff] [review]
patch v5.5

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

::: lib/toolbars.js
@@ +334,5 @@
> +   * @type {ElemBase}
> +   */
> +  get activeNotificationIcon() {
> +    var activeNotificationIcon = new elementslib.ID(this._controller.window.document,
> +                                                    "geo-notification-icon");

Can we please shorten the line and just call the variable `icon`? That's clear enough here given that no other icon is present.

Also why do you reference the geo notification icon here? This is very specific and will not work in such a more general method. I would like to see a method which really waits for the notification to appear. Most likely it should be part of `getNotification()` if that references the same kind of notification. Otherwise we would need a `waitForNotification()` method.

At the end we would have to refactor the old notifications out of location bar given that those are not part of it. But that can be done in a follow-up.
Attachment #728968 - Flags: review?(hskupin)
Attachment #728968 - Flags: review?(dave.hunt)
Attachment #728968 - Flags: review-
Attached patch patch v6 (obsolete) — — Splinter Review 
Updated the patch, moved the geolocationIcon in getElements() and waiting for the "showing" attribute in the test.
Updated getNotification() method to take as parameter the state we want to check (the popup has opened or closed).
With this approved, we can have a follow-up bug to refactor some tests that use this state as testPasswordManager/testPasswordNotificationBar.js [1] or testPasswordSavedAndDeleted.js [2]

[1] http://hg.mozilla.org/qa/mozmill-tests/file/095113e860f1/tests/functional/testPasswordManager/testPasswordNotificationBar.js#l58
[2] http://hg.mozilla.org/qa/mozmill-tests/file/095113e860f1/tests/functional/testPreferences/testPasswordSavedAndDeleted.js#l57
Attachment #728968 - Attachment is obsolete: true
Attachment #730092 - Flags: review?(hskupin)
Attachment #730092 - Flags: review?(dave.hunt)
If it helps, I pattern searched through the default repo and found the occurrences of getNotification() that Andreea found, plus I think maybe one more

restartTests/testPreferences_masterPassword/test1.js

Unrelated, I noticed the diff in patch v6 could potentially use the upcoming BASE_URL,TEST_URL happening in bug 848649 if you guys want; but I will be re-checking the entire repo again anyway and can refactor it as part of that change, no problems.
I entered a marker bug for those potential test changes, as bug 855427, in case it's useful.
Blocks: 855427
Comment on attachment 730092 [details] [diff] [review]
patch v6

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

::: lib/toolbars.js
@@ +477,5 @@
>          elem = new elementslib.ID(this._controller.window.document, "feed-button");
>          break;
> +      case "geolocationIcon":
> +        elem = new elementslib.ID(this._controller.window.document, "geo-notification-icon");
> +        break;

I don't think we need this when we do the more generalized method I have proposed. There are more than the geolocation notification available, which we should cover here.

Also why can't we use the `visibility` CSS property to determine the visibility state? If it's not collapse the notification is active.

@@ +537,5 @@
> +    var notificationPopup = this.getElement({type: "notification_popup"});
> +    assert.waitFor(function () {
> +      return notificationPopup.getNode().state === (aState ? 'opened' : 'closed');
> +    }, "Notification popup state has been changed");
> +

Not sure I understand this change. As proposed in my last review we should have a waitForNotification() like method, which waits for the specified open/closed state. I think that every invoke of getNotification() will always ensure that it is visible.

::: tests/functional/testGeolocation/testShareLocation.js
@@ +7,5 @@
> +var tabs = require("../../../lib/tabs");
> +var toolbars = require("../../../lib/toolbars");
> +
> +const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../data/');
> +const LOCAL_TEST_PAGE = LOCAL_TEST_FOLDER + 'geolocation/position.html';

Please update those lines to be in sync with jfrench's work for TEST_URL.

@@ +9,5 @@
> +
> +const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../data/');
> +const LOCAL_TEST_PAGE = LOCAL_TEST_FOLDER + 'geolocation/position.html';
> +
> +const TIMEOUT = 15000;

Which timeout? A more descriptive name would be good to have here.

@@ +33,5 @@
> +
> +  assert.waitFor(function () {
> +    geolocationIcon = locationBar.getElement({type: "geolocationIcon"});
> +    return geolocationIcon.getNode().hasAttribute("showing");
> +  }, "An active notification icon has been found");

See my comment in the lib module. I would like to see something like `locationBar.waitForNotification("geo")`. This will also lessen the confusion with the other geolocation icon below.
Attachment #730092 - Flags: review?(hskupin)
Attachment #730092 - Flags: review?(dave.hunt)
Attachment #730092 - Flags: review-
With the changes in bug 803492, I'd like to get this done in order to test the focus completely.

(In reply to Henrik Skupin (:whimboo) from comment #84)
> 
> @@ +537,5 @@
> > +    var notificationPopup = this.getElement({type: "notification_popup"});
> > +    assert.waitFor(function () {
> > +      return notificationPopup.getNode().state === (aState ? 'opened' : 'closed');
> > +    }, "Notification popup state has been changed");
> > +
> 
> Not sure I understand this change. As proposed in my last review we should
> have a waitForNotification() like method, which waits for the specified
> open/closed state. I think that every invoke of getNotification() will
> always ensure that it is visible.
It was my understanding from comment 80 that we can add the waitFor() in the getNotification() method if it's referencing the same kind of notification. And it is - notification-popup.
If we want it more general, than I'll make another method (waitForNotification) where we can take a notification type from getElement() as parameter.

> > +
> > +  assert.waitFor(function () {
> > +    geolocationIcon = locationBar.getElement({type: "geolocationIcon"});
> > +    return geolocationIcon.getNode().hasAttribute("showing");
> > +  }, "An active notification icon has been found");
> 
> See my comment in the lib module. I would like to see something like
> `locationBar.waitForNotification("geo")`. This will also lessen the
> confusion with the other geolocation icon below.

This one was waiting for the icon in the location bar, not the notification. I can wait for the CSS visibility instead, it will harder to get to it (querySelector, nodeCollector) compared to "showing" attribute, but would still need the element in getElements() or in the test itself.
(In reply to Andreea Matei [:AndreeaMatei] from comment #85)
> > > +    var notificationPopup = this.getElement({type: "notification_popup"});
> > > +    assert.waitFor(function () {
> > > +      return notificationPopup.getNode().state === (aState ? 'opened' : 'closed');
> > > +    }, "Notification popup state has been changed");
> > > +
> > 
> > Not sure I understand this change. As proposed in my last review we should
> > have a waitForNotification() like method, which waits for the specified
> > open/closed state. I think that every invoke of getNotification() will
> > always ensure that it is visible.
>
> It was my understanding from comment 80 that we can add the waitFor() in the
> getNotification() method if it's referencing the same kind of notification.
> And it is - notification-popup.

I think that would be good. Otherwise we would have to recreate the element each time we want to change for a change of the notifications open state.

> If we want it more general, than I'll make another method
> (waitForNotification) where we can take a notification type from
> getElement() as parameter.

We should have this, yes.
 
> > > +  assert.waitFor(function () {
> > > +    geolocationIcon = locationBar.getElement({type: "geolocationIcon"});
> > > +    return geolocationIcon.getNode().hasAttribute("showing");
> > > +  }, "An active notification icon has been found");
> > 
> > See my comment in the lib module. I would like to see something like
> > `locationBar.waitForNotification("geo")`. This will also lessen the
> > confusion with the other geolocation icon below.
> 
> This one was waiting for the icon in the location bar, not the notification.
> I can wait for the CSS visibility instead, it will harder to get to it
> (querySelector, nodeCollector) compared to "showing" attribute, but would
> still need the element in getElements() or in the test itself.

Could this become a separate method then like waitForNotificationIcon()? Do we have to really wait for it or can we directly wait for the popup, and check the icon after the popup has been opened?
Attached patch patch v6.1 (obsolete) — — Splinter Review 
Indeed, no need to wait for the location bar geolocation icon, it's visible right after loading the page, with popup open or closed.

I've let the waitForNotification() method take care of all kinds of notifications.

On 2.0, there's an issue with waitForPageLoad() (again) on Nightly and Linux only (aurora works). To be able to test it, I replaced it with a sleep and the geolocation check works as expected.

Reports for 1.5:
Linux:
http://mozmill-crowd.blargon7.com/#/functional/report/90a0b225333a4e0c868fcb2b445d3534
OS X:
http://mozmill-crowd.blargon7.com/#/functional/report/90a0b225333a4e0c868fcb2b445d12e3
Windows:
http://mozmill-crowd.blargon7.com/#/functional/report/90a0b225333a4e0c868fcb2b445d257b
Attachment #730092 - Attachment is obsolete: true
Attachment #753230 - Flags: review?(hskupin)
Attachment #753230 - Flags: review?(dave.hunt)
Comment on attachment 753230 [details] [diff] [review]
patch v6.1

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

Thanks Andreea. One nit, and one suggestion.

::: lib/toolbars.js
@@ +574,5 @@
> +
> +  /**
> +   * Waits for the given notification popup
> +   *
> +   * @param {ElemBase} aElement

This isn't an element, it's a string reference to an element.

@@ +581,5 @@
> +   *        True if notification is open
> +   *        [optional - default: false]
> +   */
> +  waitForNotification : function locationBar_waitForNotification(aElement, aState) {
> +    aState = (aState == undefined) ? false : aState;

Should we default to "notification_popup"?
Attachment #753230 - Flags: review?(hskupin)
Attachment #753230 - Flags: review?(dave.hunt)
Attachment #753230 - Flags: review-
Comment on attachment 753230 [details] [diff] [review]
patch v6.1

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

::: tests/functional/testGeolocation/testShareLocation.js
@@ +30,5 @@
> +function testVerifyDisplayGeolocationNotification() {
> +  controller.open(TEST_DATA);
> +  controller.waitForPageLoad();
> +
> +  locationBar.waitForNotification("notification_popup", true);

I don't think this general approach is good. Instead of 'notification_popup' we should use the type of popup we are waiting for. Otherwise we will work on a different kind of popup but not the one we want to.

@@ +33,5 @@
> +
> +  locationBar.waitForNotification("notification_popup", true);
> +
> +  // Check the geolocation icon is visible in the location bar
> +  var geolocationIcon = locationBar.getElement({type: "geolocationIcon"});

We have icons for each kind of popup. Those are really bound together. I think we should directly check its existence inside of waitForNotification. It would be good to get a mentored follow-up bug filed which can cover all the other cases.

@@ +44,5 @@
> +  expect.ok(icon, "The geolocation icon appears in the notification popup");
> +
> +  // Check if a Share Location button is visible
> +  var button = locationBar.getNotificationElement("geolocation-notification",
> +                                                  '/anon({"anonid":"button"})');

There is no other attribute we could make use of which describes the button a bit better?
Attachment #753230 - Flags: review-
Depends on: 888252
No longer depends on: 888252
Depends on: 882485
Giving the blocking bug was fixed for default, I'd like to finish up the patch here. Just a question:
I've noticed on OS X we have now a dialog that's asking permission to enable Location Services to allow Nightly to use the current location.
This is a OS dialog that we can't handle, so if we Cancel it we can't finish up the test. 
Do we want to fully skip it on mac or just add an if for the final check of the position? I'm thinking we can still check the notification popup, the icons and button.
(In reply to Andreea Matei [:AndreeaMatei] from comment #90)
> Giving the blocking bug was fixed for default, I'd like to finish up the
> patch here. Just a question:
> I've noticed on OS X we have now a dialog that's asking permission to enable
> Location Services to allow Nightly to use the current location.
> This is a OS dialog that we can't handle, so if we Cancel it we can't finish
> up the test. 
> Do we want to fully skip it on mac or just add an if for the final check of
> the position? I'm thinking we can still check the notification popup, the
> icons and button.

If we can still run the majority of the test on Mac, I would suggest only making the final part conditional.
(In reply to Andreea Matei [:AndreeaMatei] from comment #90)
> I've noticed on OS X we have now a dialog that's asking permission to enable
> Location Services to allow Nightly to use the current location.
> This is a OS dialog that we can't handle, so if we Cancel it we can't finish
> up the test. 

Hm, strange. Since when do we have that. Please do a regression test. I believe there should be a way we can disable that. Most likely via a preference.
URL: https://litmus.mozilla.org/show_test.... → http://mozqa.com/data/firefox/geoloca...
After a bit of Bugzilla searching I found bug 874587, which looks like it may have introduced this. From the changes, it would appear that setting preference "geo.provider.testing" to true may work for us.

See http://hg.mozilla.org/mozilla-central/file/582ffcd0459a/testing/profiles/prefs_general.js#l146
Attached patch geolocationTest.patch (obsolete) — — Splinter Review 
Thanks Dave, that pref does the trick! :)

Reports:
Linux:
http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d61d669da
OS X:
http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d61d61f1f
Windows:
http://mozmill-crowd.blargon7.com/#/functional/report/180cf2548ef2865af3ae441d61d6a334

> > +  locationBar.waitForNotification("notification_popup", true);
> 
> I don't think this general approach is good. Instead of 'notification_popup'
> we should use the type of popup we are waiting for. Otherwise we will work
> on a different kind of popup but not the one we want to.
There I don't pass in the id of the notification but the type (which in this case is the same) from the getElements switch. waitForNotification() method takes the notification element as a parameter.

> @@ +33,5 @@
> > +
> > +  locationBar.waitForNotification("notification_popup", true);
> > +
> > +  // Check the geolocation icon is visible in the location bar
> > +  var geolocationIcon = locationBar.getElement({type: "geolocationIcon"});
> 
> We have icons for each kind of popup. Those are really bound together. I
> think we should directly check its existence inside of waitForNotification.
> It would be good to get a mentored follow-up bug filed which can cover all
> the other cases.
Good point, done that in a switch to have other icons added in the follow up bug I'll file after we check this in.

> @@ +44,5 @@
> > +  expect.ok(icon, "The geolocation icon appears in the notification popup");
> > +
> > +  // Check if a Share Location button is visible
> > +  var button = locationBar.getNotificationElement("geolocation-notification",
> > +                                                  '/anon({"anonid":"button"})');
> 
> There is no other attribute we could make use of which describes the button
> a bit better?
Yes, I'm using label now, which is more specific: "Share Location".
Attachment #753230 - Attachment is obsolete: true
(In reply to Andreea Matei [:AndreeaMatei] from comment #94)
> Thanks Dave, that pref does the trick! :)

So I think this should be added as a default pref to Mozmill/Mozbase. Can you file a bug for it please?
Depends on: 897891
Added bug 897891 for that, sorry I missed to add the review flag here but it turned out well since I have to update the patch anyway after we have the preference :)

Just a FWY, the test is still blocked by bug 888252 for Aurora, just Nightly works for now.
Comment on attachment 780856 [details] [diff] [review]
geolocationTest.patch

Giving that Mozmill 1.5.22 will take longer to be released, we discussed in "Ask an expert" to leave this pref in the test for now and have a follow up later to remove it.
Attachment #780856 - Flags: review?(hskupin)
Attachment #780856 - Flags: review?(dave.hunt)
Comment on attachment 780856 [details] [diff] [review]
geolocationTest.patch

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

::: lib/toolbars.js
@@ +476,5 @@
>          elem = new elementslib.ID(this._controller.window.document, "feed-button");
>          break;
> +      case "geolocationIcon":
> +        elem = new elementslib.ID(this._controller.window.document, "geo-notification-icon");
> +        break;

I think that I already said this earlier but I'm not really sure what we finally decided. But IMO this should still be made more general so it can be used for all the other notification icons too, without having to write more and more cases:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#499

I would like to see:

case "notificationIcon":
  elem = new elementslib.ID(this._controller.window.document, subtype + "-notification-icon");

So we can use getElement({type: "notification-icon", subtype: "geo"});

@@ +576,5 @@
> +  /**
> +   * Waits for the given notification popup
> +   *
> +   * @param {String} aElement
> +   *        Notification attribute to wait for

It's not an attribute but a child element.

@@ +578,5 @@
> +   *
> +   * @param {String} aElement
> +   *        Notification attribute to wait for
> +   * @param {Boolean} aState
> +   *        True if notification is open

nit: should be open?

@@ +582,5 @@
> +   *        True if notification is open
> +   *        [optional - default: false]
> +   * @param {Object} aIcon
> +   *        Icon linked to the notification
> +   *        [optional - default: undefined]

The brackets already say that this parameter is optional. So it should be:

{Object} [aIcon=undefined]

@@ +585,5 @@
> +   *        Icon linked to the notification
> +   *        [optional - default: undefined]
> +   */
> +  waitForNotification : function locationBar_waitForNotification(aElement, aState, aIcon) {
> +    aState = (aState == undefined) ? false : aState;

I don't see that this is necessary. Just make a !! comparison below.

@@ +590,5 @@
> +
> +    var notification = this.getElement({type: aElement});
> +    assert.waitFor(function () {
> +      return notification.getNode().state === (aState ? 'open' : 'closed');
> +    }, "Notification popup state has been changed");

'visibility state'

@@ +601,5 @@
> +                    "The geolocation icon is visible in the location bar");
> +          break;
> +        default:
> +          throw new Error(arguments.callee.name + ": Unknown element type - " + spec.type);
> +      }

I would not use a switch here, but directly forward to the getElement with the right type and subtype. If the icon does not exist you will get back a null element.

::: tests/functional/testGeolocation/testShareLocation.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

"use strict";

@@ +5,5 @@
> +// Include required modules
> +var { assert, expect } = require("../../../lib/assertions");
> +var tabs = require("../../../lib/tabs");
> +var prefs = require("../../../lib/prefs");
> +var toolbars = require("../../../lib/toolbars");

As usual please keep the alphabetical order.

@@ +10,5 @@
> +
> +const BASE_URL = collector.addHttpResource('../../../data/');
> +const TEST_DATA = BASE_URL + 'geolocation/position.html';
> +
> +const GEOLOCATION_PREF = "geo.provider.testing";

I would call this PREF_OS_GEO_PROVIDER. Keep in mind that PREF_ is a prefix and not a suffix.

@@ +12,5 @@
> +const TEST_DATA = BASE_URL + 'geolocation/position.html';
> +
> +const GEOLOCATION_PREF = "geo.provider.testing";
> +
> +const POSITION_TIMEOUT = 15000;

Same for TIMEOUT_

@@ +24,5 @@
> +
> +  // OS X launches a system dialog and requires approval for Nightly
> +  // in order to share the location
> +  if (mozmill.isMac) {
> +    prefs.preferences.setPref(GEOLOCATION_PREF, true);

I would always set this pref and not make is OS X dependent.

@@ +66,5 @@
> +  // The position updates lazily so additional timeout is needed
> +  var result = new elementslib.ID(controller.tabs.activeTab, "result");
> +  assert.waitFor(function () {
> +    return result.getNode().textContent !== "undefined" &&
> +           result.getNode().textContent !== "";

I wonder if we should better do a regex here to have a qualified check.
Attachment #780856 - Flags: review?(hskupin)
Attachment #780856 - Flags: review?(dave.hunt)
Attachment #780856 - Flags: review-
Priority: P2 → P1
Attached patch geolocationTest.patch (obsolete) — — Splinter Review 
Updated as requested.
I couldn't use regex because the location is slowly generated and the assert.match doesn't work like a waitFor, to keep checking it for the set timeout.
It only worked if I had a long sleep before the match.

Reports:
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/b3aefd29a5c41bda1cc43616d3e7f79c
OS X: http://mozmill-crowd.blargon7.com/#/functional/report/b3aefd29a5c41bda1cc43616d3e71d29
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/b3aefd29a5c41bda1cc43616d3e952dc
Attachment #780856 - Attachment is obsolete: true
Attachment #784337 - Flags: review?(hskupin)
Attachment #784337 - Flags: review?(dave.hunt)
(In reply to Andreea Matei [:AndreeaMatei] from comment #99)
> Created attachment 784337 [details] [diff] [review]
> geolocationTest.patch

+const TIMEOUT_POSITION = 15000;

I was curious about this w.r.t. another marker bug I entered (bug 898873) for some potential cleanup. Is there a naming convention when only one timeout is used in a test. Just wondering.
I don't know any convention regarding that, but I feel that sometimes, even if it's only one, it might be good to name it more specifically than just TIMEOUT. In this case it's showing the fact that the position is retrieved lazily.
Comment on attachment 784337 [details] [diff] [review]
geolocationTest.patch

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

::: lib/toolbars.js
@@ +577,5 @@
> +  /**
> +   * Waits for the given notification popup
> +   *
> +   * @param {String} aElement
> +   *        Notification attribute to wait for

As Henrik pointed out this is an element, and not an attribute.

::: tests/functional/testGeolocation/testShareLocation.js
@@ +63,5 @@
> +  // Check if the location is displayed
> +  // The position updates lazily so additional timeout is needed
> +  var result = new elementslib.ID(controller.tabs.activeTab, "result");
> +  assert.waitFor(function () {
> +    return result.getNode().textContent !== "undefined" &&

You should be able to use a regular expression to check this, simply by returning the result of regexp.test. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/test
Attachment #784337 - Flags: review?(hskupin)
Attachment #784337 - Flags: review?(dave.hunt)
Attachment #784337 - Flags: review-
Attached patch geolocationTest.patch (obsolete) — — Splinter Review 
Thanks Dave for the hint, didn't know about that! :) Using the regex now.

Reports, the shown errors are from bug 905960 and bug 907101:
OS X: http://mozmill-crowd.blargon7.com/#/functional/report/e503b7b0a70a3839c66c64572e8b09ea
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/e503b7b0a70a3839c66c64572e8d2d62
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/e503b7b0a70a3839c66c64572e8cf41b
Attachment #784337 - Attachment is obsolete: true
Attachment #792783 - Flags: review?(hskupin)
Attachment #792783 - Flags: review?(dave.hunt)
Comment on attachment 792783 [details] [diff] [review]
geolocationTest.patch

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

::: tests/functional/testGeolocation/testShareLocation.js
@@ +12,5 @@
> +
> +const BASE_URL = collector.addHttpResource('../../../data/');
> +const TEST_DATA = BASE_URL + 'geolocation/position.html';
> +
> +const PREF_OS_GEO_PROVIDER = "geo.provider.testing";

I would say lets get mozmill 1.5.22 released so we don't have to modify the preference in the test. That means please remove all traces here.

@@ +64,5 @@
> +  // The position updates lazily so additional timeout is needed
> +  var result = new elementslib.ID(controller.tabs.activeTab, "result");
> +  assert.waitFor(function () {
> +    var regExp = /\d+(\.\d*)?\.\d+/;
> +    return regExp.test(result.getNode().textContent);

I would move the regex definition out of the loop. There is no need to re-create each time when it stays the same.
Attachment #792783 - Flags: review?(hskupin)
Attachment #792783 - Flags: review?(dave.hunt)
Attachment #792783 - Flags: review-
Attached patch geolocationTest.patch (obsolete) — — Splinter Review 
Updated as requested, but we need to wait for the 1.5.22 release in order to properly work on OS X.
Attachment #792783 - Attachment is obsolete: true
Attachment #794575 - Flags: review?(andrei.eftimie)
Comment on attachment 794575 [details] [diff] [review]
geolocationTest.patch

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

Works fine.
Not going into details because it has been heavily reviewed by now.

Just a couple of small nits:

::: tests/functional/testGeolocation/testShareLocation.js
@@ +9,5 @@
> +var tabs = require("../../../lib/tabs");
> +var toolbars = require("../../../lib/toolbars");
> +
> +const BASE_URL = collector.addHttpResource('../../../data/');
> +const TEST_DATA = BASE_URL + 'geolocation/position.html';

nit: double quotes for consistency on both lines above

@@ +15,5 @@
> +const TIMEOUT_POSITION = 15000;
> +
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.locationBar =  new toolbars.locationBar(aModule.controller);

nit: 2 spaces after the assignment operand
Attachment #794575 - Flags: review?(andrei.eftimie) → review+
Attached patch geolocationTest.patch (obsolete) — — Splinter Review 
Now with Mozmill 1.5.22 we can land this.
Updated and added reports:
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a101f08c
OS X: http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a101c751
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/3c3cd991de01b704f3f65783a101eff6

Andrei, would you like to test your checkin permissions with this? :)
Attachment #794575 - Attachment is obsolete: true
Attachment #796608 - Flags: review+
Attachment #796608 - Flags: checkin?(andrei.eftimie)
Somehow the setting needed for this test is available in mozmill 1.5 but is missing for mozmill 2.0

Lets first try to get this to work under 2.0 also as we are now really close to a green run there.

I'll track this in bug 897891
(In reply to Andrei Eftimie from comment #108)
> I'll track this in bug 897891

No. As discussed on IRC please file a mozbase bug so we can bump the mozprofile version and any other affected package. Sadly we will have to wait for the final mozmill version for that.
Depends on: 911076
This works well in 1.5 and at the moment we don't run 2.0 on our CI. We are aware of the issue there and it will be fixed in the final 2.0 release, so I don't see why we should be blocked in having this landed now.
Well, not just now cause we're back on 1.5.21 :)
All dependencies have been fixed.
This should work fine now.

Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/bd6acde69667 (default)

Andreea, do we want to run this on older branches?
status-firefox26: --- → fixed
Attachment #796608 - Flags: checkin?(andrei.eftimie) → checkin+
Backed out: http://hg.mozilla.org/qa/mozmill-tests/rev/146f8f27073e (default)

Fails on localised builds (thanks Mario for catching this before it fails on CI) with:
> Expression "{"label":"Share Location"}" returned null. Anonymous == true

This should probably be taked out from a dtd.
We missed this :(
CI started a testrun before the backout, and we fail with:
"Geolocation position has been found"

http://mozmill-daily.blargon7.com/#/functional/report/fb97b6210ae70da1b9ace67445dcdb0d

We should investigate and fix that.
I have fixed the label issue, sorry for missing it. Lesson learned to test on different locales and always look for dtds.

Regarding the other failure, I'm testing it now several times in a loop and check if I can catch that. When I tested with a 10000 timeout, I did get some failure once in a while. With 15000 seemed pretty stable, didn't failed once. I'll try with the machines CPU heavily loaded.
We might want to check on a machine from CI.
The response we are waiting for might not get through firewall or proxy settings...
Attached patch patch v9 — — Splinter Review 
We're retrieving the label property now.

Works well with 2.0 too, I'll add reports tomorrow when I can check one remote mac machine to see why it failed there the last time.

Regarding backporting, we usually don't do it when it's a new test, but since I see wa-beta-blocking tag, not sure if we need it in beta sooner.
Attachment #796608 - Attachment is obsolete: true
Attachment #809208 - Flags: review?(andrei.eftimie)
Comment on attachment 809208 [details] [diff] [review]
patch v9

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

Looks good now. Works fine on localised builds.

See if you can reproduce the failure mentioned in comment 114, we might need to increase the timeout value if that is still failing on CI.
Attachment #809208 - Flags: review?(andrei.eftimie) → review+
I managed to check the test on mm-osx-107-3 and it works fine, also did a testrun:

http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee697219c85600

Not sure why failed the first time, might be that bug 888252 wasn't fully fixed on OS X at that point.
Comment on attachment 809208 [details] [diff] [review]
patch v9

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

Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/580ec7cca527 (default)
Attachment #809208 - Flags: checkin+
Lets let it run over the weekend, if it sticks we'll backport it on monday.
Just curious if it ran ok over the weekend? I have a related patch in bug 855427 which I think will need to be landed for related tests to pass, using the updated getNotification() method.
It ran over the weekend, it had one failure on a Win Vista machine, not being able to retrieve the position, but it might have been just a network issue as it didn't reproduce again. I'll check backporting on aurora and beta today.
Priority: P1 → P2
Attached patch [Beta] patch v1 — — Splinter Review 
Combined patches for Beta, with timeout increased.
Attachment #831527 - Flags: review?(andrei.eftimie)
Comment on attachment 831527 [details] [diff] [review]
[Beta] patch v1

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

Sorry I missed this review request earlier.
We'll need to update it to the new folder structure.
Attachment #831527 - Flags: review?(andrei.eftimie) → review-
This ended up in beta as well due to the merge. We have established it will be backported down to beta, cause was daily-beta-qa blocking, so closing it now.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
status-firefox26: affected → ---
status-firefox29: --- → fixed
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: