1088638 - Add automated test to install, disable, enable and remove services

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Cosmin Malutan, [:cosmin-malutan]]

https://moztrap.mozilla.org/manage/case/11503/
1 Launch Firefox.
2 Access the following URL: http://mixedpuppy.github.io/socialapi-demo/.
  -> The user is redirected to the Demo Social Service github page.
3 Press the "Demo Social Service" button and then click on the "Enable Services" button displayed in the doorhanger.
  -> Pressing the "Enable Services" button installs the service. The service's sidebar is displayed.
4 Click on the "Share Location" button from the displayed notification.
5 Open the Add-ons Manager.
6 Click on the "Services" menu item.
  -> The "Demo Social Service 1" service is displayed, with two options available ("Disable" and "Remove").
7 Click the "Disable" button.
  -> The service is disabled, the sidebar is removed.
8 Click the "Enable" button.
  -> The service is enabled, the sidebar is restored.
9 Click the "Remove" button.
  -> The service is removed along with the sidebar.
10 Click the "Undo" link.
  ->The service is restored, along with the sidebar.

This requires to install an remote service so it should be done with mozmill.

Comment 1

[Andrei Eftimie]

Attachment: v1.patch

yey for `browserWindow.getProperty` :)

I feel this might still be compacted a bit, but nothing stands out where this can be improved right now.

Also I've used a nasty selector: "#testbuttons li:last-child button" as there's no better way of selecting the specific button.
We do check the ShareLocation panel, but we don't check if there's any output from that.

Otherwise the test runs fine on Nightly / OSX:
http://mozmill-crowd.blargon7.com/#/remote/report/635fcffb06b246be47416301bd51bd2a

Comment 2

[Andreea Matei [:AndreeaMatei]]

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

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

Beside the comments, this works great for me.

::: firefox/tests/remote/testAddons/testServiceInstallDisableEnableRemove.js
@@ +26,5 @@
> +  aModule.browserWindow.tabs.closeAllTabs();
> +  aModule.browserWindow.controller.stopApplication(true);
> +}
> +
> +function testInstall() {

JSdoc please and maybe more suggestive name, what we're installing.

@@ +84,5 @@
> +
> +  // Disable the addon
> +  addonsManager.disableAddon({addon: addon});
> +  assert.ok(!addonsManager.isAddonEnabled({addon: addon}),
> +            "The addon is disabled");

What I also see in the expected behaviour and we're not checking is that the sidebar should be closed when disabled/removed, and back on when enabled. We should assert that too.

Comment 3

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

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

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

I hope you don't mind my drive-by feedback here. It looks fine beside the comments I made inline.

::: firefox/tests/remote/testAddons/testServiceInstallDisableEnableRemove.js
@@ +10,5 @@
> +var utils = require("../../../../lib/utils");
> +
> +const ADDON = {
> +  id: "[e-mail]",
> +  url: "https://socialapi-demo.paas.allizom.org/"

Why can't we have this as a local addon? I would really prefer a functional test.

@@ +18,5 @@
> +  aModule.browserWindow = new browser.BrowserWindow();
> +
> +  aModule.addonsManager = new addons.AddonsManager(aModule.browserWindow.controller);
> +  aModule.locationBar = new toolbars.locationBar(aModule.browserWindow.controller);
> +  aModule.doc = browserWindow.controller.window.document;

We should never cache the document. What you should better do is to enhance the browser class for a document property.

@@ +31,5 @@
> +  browserWindow.controller.open(ADDON.url);
> +  browserWindow.controller.waitForPageLoad();
> +
> +  // Install the service
> +  let demoButton = findElement.Selector(doc, "#provider-list button");

We do not want to retrieve chrome elements directly in the test. So why aren't those part of a ui library?

Comment 4

[Andreea Matei [:AndreeaMatei]]

(In reply to Henrik Skupin (:whimboo) from comment #3)
> Comment on attachment 8525319 [details] [diff] [review]
> v1.patch
> 
> Review of attachment 8525319 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I hope you don't mind my drive-by feedback here. It looks fine beside the
> comments I made inline.
> 
> ::: firefox/tests/remote/testAddons/testServiceInstallDisableEnableRemove.js
> @@ +10,5 @@
> > +var utils = require("../../../../lib/utils");
> > +
> > +const ADDON = {
> > +  id: "[e-mail]",
> > +  url: "https://socialapi-demo.paas.allizom.org/"
> 
> Why can't we have this as a local addon? I would really prefer a functional
> test.

Don't think we'll have this as functional since it uses remote services, that's the purpose of the testcase. I'd propose we use this page on allizom and if we find it brings us issues, consider then to try and have it at least on mozqa.
I'll update the patch for now, won't require much time either way.

Comment 5

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

Which services does it test? We could always have local mockups to mimic their behavior. Please see what we do for local search engines.

Comment 6

[Cosmin Malutan, [:cosmin-malutan]]

The only reason we test this with mozmill is because we test a remote service, otherwise it should have been a browser chrome test. Also on step 4 we have to "Share the location", this also means that we need to access the remote resources. We could try to mockup this but without any certainty. On my opinion we should go with this  approach and if it's possible for the service to be mocked up to do that later, meanwhile we would have coverage for this test. It would be bad to have this blocked for couple of months, meanwhile it should be covered by the QA.
Regarding the search engines we used real ones for a long time until we mocked them up, I don't see why we cant do that here too.

Comment 7

[Andreea Matei [:AndreeaMatei]]

Please see comment 0 for the steps. We mostly want to test the sidebar is present, click sign in and the disable/enable/remove service from the services category in the addons manager. 
Share location it's required as part of activating the social service, but we don't use it beyond that, neither all the buttons in the sidebar. We just care to test disable/enable/remove the service in addons manager, once activated.

Comment 8

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: mockup_service

After investigating this a bit it seems it's quite easy to mock-up this service. This patch adds a mockup service which prompts user to share the location.

Comment 9

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.0

Here are the merged patches.
(In reply to Henrik Skupin (:whimboo) from comment #3)
> > +  id: "[e-mail]",
> > +  url: "https://socialapi-demo.paas.allizom.org/"
> 
> Why can't we have this as a local addon? I would really prefer a functional
> test.
Done that.

> > +  aModule.doc = browserWindow.controller.window.document;
> 
> We should never cache the document. What you should better do is to enhance
> the browser class for a document property.
No need to cache it, I did cached the controller as we do in most of the testes so it's more readable.

> @@ +31,5 @@
> > +  browserWindow.controller.open(ADDON.url);
> > +  browserWindow.controller.waitForPageLoad();
> > +
> > +  // Install the service
> > +  let demoButton = findElement.Selector(doc, "#provider-list button");
> 
> We do not want to retrieve chrome elements directly in the test. So why
> aren't those part of a ui library?
The point of UI libraries are to retrieve elements that are used in more tests. This is a specific extension that it's being tested and it won't be used in other tests, I checked and I didn't found library methods for other addons, but we retrieve the elements directly in test. If you strongly want this I can make a library for this specific extension.

Comment 10

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8532436 [details] [diff] [review]
patch v1.0

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

::: data/services/empty_service/sidebar.html
@@ +27,5 @@
> +          default:
> +            result.innerHTML = "Unknown error code: " + aError.code;
> +        }
> +      }
> +      function geoLocationShare() {

A blank line above please.

::: firefox/tests/functional/testAddons/testServiceInstallDisableEnableRemove.js
@@ +26,5 @@
> +  aModule.browserWindow.tabs.closeAllTabs();
> +  aModule.controller.stopApplication(true);
> +}
> +
> +function testInstall() {

JS doc please, for all functions.

@@ +42,5 @@
> +    "servicesInstall-notification", {type: "label", value: enableLabel}
> +  );
> +  locationBar.waitForNotificationPanel(() => {
> +    enableButton.click();
> +  }, {type: "notification", open: false});

Lets add a comment describing these 2 actions, enableButton is a the one for enabling the service, inside the notification.

@@ +69,5 @@
> +
> +function testDisableEnableRemoveUndo() {
> +  addonsManager.open();
> +
> +  // Get the extensions pane

services pane

@@ +82,5 @@
> +  // Disable the addon
> +  addonsManager.disableAddon({addon: addon});
> +  assert.ok(!addonsManager.isAddonEnabled({addon: addon}),
> +            "The addon is disabled");
> +

Please add a check for the sidebar too, that it's removed when the service gets disabled/removed.

Comment 11

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.0

Thanks for review, done.

Comment 12

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8532496 [details] [diff] [review]
patch v2.0

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

I think the term "service" would be a more appropriate choice to use instead of "addon" in the test, see inline comments.

::: firefox/tests/functional/testAddons/testServiceInstallDisableEnableRemove.js
@@ +10,5 @@
> +
> +const BASE_URL = collector.addHttpResource("../../../../data/");
> +const TEST_DATA = {
> +  "url" : BASE_URL + "services/install.html",
> +  "addon" : "Testing Social Service"

I think "service" would be more appropriate here (and everywhere else across the test), instead of "addon"

@@ +26,5 @@
> +  aModule.browserWindow.tabs.closeAllTabs();
> +  aModule.controller.stopApplication(true);
> +}
> +
> +/*

/**

@@ +52,5 @@
> +  // When installing the service the sidebar should be displayed
> +  var sidebar = findElement.ID(controller.window.document, "social-sidebar-box");
> +  assert.waitFor(() => {
> +    return sidebar.exists() && utils.isDisplayed(controller, sidebar);
> +  },"Sidebar has been displayed");

Add a whitespace before the comment, please

@@ +54,5 @@
> +  assert.waitFor(() => {
> +    return sidebar.exists() && utils.isDisplayed(controller, sidebar);
> +  },"Sidebar has been displayed");
> +}
> +/**

Blank line before this, please

@@ +62,5 @@
> +  // Share Location
> +  var shareLocation = findElement.ID(controller.window.document, "shareLocation");
> +  assert.waitFor(() => {
> +    return shareLocation.exists() && utils.isDisplayed(controller, shareLocation);
> +  },"Share geolocation button has been found");

Add a whitespace before the comment, please

@@ +88,5 @@
> +  addonsManager.setCategory({
> +    category: addonsManager.getCategoryById({id: "service"})
> +  });
> +
> +  // Get the addon by name

// Get the service by name

@@ +89,5 @@
> +    category: addonsManager.getCategoryById({id: "service"})
> +  });
> +
> +  // Get the addon by name
> +  var addon = addonsManager.getAddons({attribute: "name",

I'd go for "service" as the var name, instead of addon

@@ +92,5 @@
> +  // Get the addon by name
> +  var addon = addonsManager.getAddons({attribute: "name",
> +                                       value: TEST_DATA.addon})[0];
> +
> +  // Disable the addon

// Disable the service

@@ +95,5 @@
> +
> +  // Disable the addon
> +  addonsManager.disableAddon({addon: addon});
> +  assert.ok(!addonsManager.isAddonEnabled({addon: addon}),
> +            "The addon is disabled");

"The service is disabled"

@@ +103,5 @@
> +  assert.waitFor(() => {
> +    return sidebar.exists() && !utils.isDisplayed(controller, sidebar);
> +  },"Sidebar has been hidden.");
> +
> +  // Enable the addon

// Enable the service

@@ +106,5 @@
> +
> +  // Enable the addon
> +  addonsManager.enableAddon({addon: addon});
> +  assert.ok(addonsManager.isAddonEnabled({addon: addon}),
> +            "The addon is enabled");

"The service is enabled"
Also, you should check that the sidebar appears back when enabling the service.

@@ +108,5 @@
> +  addonsManager.enableAddon({addon: addon});
> +  assert.ok(addonsManager.isAddonEnabled({addon: addon}),
> +            "The addon is enabled");
> +
> +  // Remove the addon

// Remove the service

@@ +111,5 @@
> +
> +  // Remove the addon
> +  addonsManager.removeAddon({addon: addon});
> +  assert.equal(addon.getNode().getAttribute("pending"), "uninstall",
> +               "Addon was marked for uninstall");

"Service was marked for uninstall"
Also check that the sidebar is not shown

@@ +113,5 @@
> +  addonsManager.removeAddon({addon: addon});
> +  assert.equal(addon.getNode().getAttribute("pending"), "uninstall",
> +               "Addon was marked for uninstall");
> +
> +  // Undo addon removal

// Undo service removal

@@ +116,5 @@
> +
> +  // Undo addon removal
> +  addonsManager.undo({addon: addon});
> +  assert.equal(addon.getNode().getAttribute("pending"), "",
> +               "Addon is no longer marked for uninstall");

"Service is no longer marked for uninstalling"
Check that the sidebar appears back, too

Comment 13

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.1

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #12)
> I think the term "service" would be a more appropriate choice to use instead
> of "addon" in the test, see inline comments.
Done that.

> > +  // Enable the addon
> > +  addonsManager.enableAddon({addon: addon});
> > +  assert.ok(addonsManager.isAddonEnabled({addon: addon}),
> > +            "The addon is enabled");
> 
> "The service is enabled"
> Also, you should check that the sidebar appears back when enabling the
> service.
Sidebar doesn't come back after a re-enabling the service, I think I talked with Andreea about this and she said it's ok. 

Thanks

Comment 14

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

(In reply to Cosmin Malutan from comment #9)
> > Why can't we have this as a local addon? I would really prefer a functional
> > test.
> Done that.

Wow, great to hear that! Thanks a lot!

> > We should never cache the document. What you should better do is to enhance
> > the browser class for a document property.
> No need to cache it, I did cached the controller as we do in most of the
> testes so it's more readable.

Great. My point was not only about readability but also about stale elements you would have when caching the document.

> addons, but we retrieve the elements directly in test. If you strongly want
> this I can make a library for this specific extension.

Thanks for the clarification. It was not clear by the code. So think about to maybe add a global comment to the test which explains that.

Comment 15

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8532530 [details] [diff] [review]
patch v2.1

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

This looks good to me. Andreea may want to take a look over this.

Comment 16

[Andreea Matei [:AndreeaMatei]]

Looks good to me too. Was this tested on all platforms Cosmin?

Comment 17

[Cosmin Malutan, [:cosmin-malutan]]

Yes, here are the reports.

http://mozmill-crowd.blargon7.com/#/functional/report/6ceccc4004d363e02f4bec5e0e7c6391
http://mozmill-crowd.blargon7.com/#/functional/report/6ceccc4004d363e02f4bec5e0e7c6adf
http://mozmill-crowd.blargon7.com/#/functional/report/6ceccc4004d363e02f4bec5e0e7c873c

Comment 18

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8532530 [details] [diff] [review]
patch v2.1

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

Lets see how it behaves before backporting:
http://hg.mozilla.org/qa/mozmill-tests/rev/1f826c03f734 (default)

Comment 19

[Cosmin Malutan, [:cosmin-malutan]]

Everything ran smoothly with today's Nightly testruns.
Andreea can you please backport this to aurora and beta?

Comment 20

[Andreea Matei [:AndreeaMatei]]

It's not transplanting cleanly and please let me know if it tested well on all platforms. Thanks.

Comment 21

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.1 aurora

Auroara 
http://mozmill-crowd.blargon7.com/#/functional/report/8469cd03fbfbd17b47a7e8cdb37bc213
http://mozmill-crowd.blargon7.com/#/functional/report/8469cd03fbfbd17b47a7e8cdb37bd1a2
http://mozmill-crowd.blargon7.com/#/functional/report/8469cd03fbfbd17b47a7e8cdb380c9af

Comment 22

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.1 beta

Beta
http://mozmill-crowd.blargon7.com/#/functional/report/8469cd03fbfbd17b47a7e8cdb37cd7cd
http://mozmill-crowd.blargon7.com/#/functional/report/8469cd03fbfbd17b47a7e8cdb381528e
http://mozmill-crowd.blargon7.com/#/functional/report/8469cd03fbfbd17b47a7e8cdb37da1d3

Comment 23

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8535543 [details] [diff] [review]
patch v2.1 aurora

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

http://hg.mozilla.org/qa/mozmill-tests/rev/d694ddbde3c6 (aurora)

Comment 24

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8535547 [details] [diff] [review]
patch v2.1 beta

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

http://hg.mozilla.org/qa/mozmill-tests/rev/38de40f25872 (beta)

Thanks Cosmin!

Comment 25

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

Comment on attachment 8532530 [details] [diff] [review]
patch v2.1

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

::: data/services/empty_service/sidebar.html
@@ +31,5 @@
> +
> +      function geoLocationShare() {
> +        result = document.getElementById("geoLocationResult");
> +        navigator.geolocation.getCurrentPosition(handlePosition, handleError,
> +                                                 {enableHighAccuracy: false, timeout: 30000});

You retrieve the position here but never assign it to the results variable? How does this currently work?

::: firefox/tests/functional/testAddons/testServiceInstallDisableEnableRemove.js
@@ +72,5 @@
> +
> +  var locationLabel = browserWindow.getProperty("geolocation.shareLocation");
> +  var shareButton = locationBar.getNotificationElement(
> +    "geolocation-notification", {type: "label", value: locationLabel}
> +  );

nit: This is not a valid style (indentation) for function parameters.

@@ +75,5 @@
> +    "geolocation-notification", {type: "label", value: locationLabel}
> +  );
> +  locationBar.waitForNotificationPanel(() => {
> +    shareButton.click();
> +  }, {type: "notification", open: false});

Why aren't we checking if the geolocation has been correctly acquired? You would have seen the bug.

@@ +101,5 @@
> +
> +  // Sidebar should be hidden when the service is disabled
> +  var sidebar = findElement.ID(controller.window.document, "social-sidebar-box");
> +  assert.waitFor(() => {
> +    return sidebar.exists() && !utils.isDisplayed(controller, sidebar);

Doesn't check isDisplayed itself if the element exists?

Comment 26

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #25)
> > +
> > +      function geoLocationShare() {
> > +        result = document.getElementById("geoLocationResult");
> > +        navigator.geolocation.getCurrentPosition(handlePosition, handleError,
> > +                                                 {enableHighAccuracy: false, timeout: 30000});
> 
> You retrieve the position here but never assign it to the results variable?
> How does this currently work?
From the moztrap in comment 0, it says we have to check only for notification, we don't really care about the location that's tested somewhere else. All that we care here is to call the geolocation API correctly so the notification will be displayed from inside of service.
That's what I had in mind when I didn't add checked for location result.

> :::
> firefox/tests/functional/testAddons/testServiceInstallDisableEnableRemove.js
> @@ +72,5 @@
> > +
> > +  var locationLabel = browserWindow.getProperty("geolocation.shareLocation");
> > +  var shareButton = locationBar.getNotificationElement(
> > +    "geolocation-notification", {type: "label", value: locationLabel}
> > +  );
> 
> nit: This is not a valid style (indentation) for function parameters.
This would exceed the the 80 limit other way, we have other cases like this, but I have no problem on changing it, just that it will be ugly.
http://hg.mozilla.org/qa/mozmill-tests/file/530536be4b4e/firefox/lib/tabs.js#l668
http://hg.mozilla.org/qa/mozmill-tests/file/530536be4b4e/firefox/tests/functional/restartTests/testPreferences_masterPassword/test1.js#l69
http://hg.mozilla.org/qa/mozmill-tests/file/530536be4b4e/firefox/tests/functional/testPreferences/testPasswordSavedAndDeleted.js#l68


> > +  // Sidebar should be hidden when the service is disabled
> > +  var sidebar = findElement.ID(controller.window.document, "social-sidebar-box");
> > +  assert.waitFor(() => {
> > +    return sidebar.exists() && !utils.isDisplayed(controller, sidebar);
> 
> Doesn't check isDisplayed itself if the element exists?
No it doesn't if the element hasn't have a node it will fail with undefined. http://hg.mozilla.org/qa/mozmill-tests/file/530536be4b4e/lib/utils.js#l385

Henrik what do you say, do we need to check for the location, and should I change the nit above or could I leave it as it is?

Comment 27

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

(In reply to Cosmin Malutan from comment #26)
> From the moztrap in comment 0, it says we have to check only for
> notification, we don't really care about the location that's tested
> somewhere else. All that we care here is to call the geolocation API
> correctly so the notification will be displayed from inside of service.
> That's what I had in mind when I didn't add checked for location result.

For manual tests the eye of the tester covers lots of other states in the UI, and I'm sure the position is used to verify it. Anyway, if we really dont need it, why the retrieval of the result element?

> > > +  var locationLabel = browserWindow.getProperty("geolocation.shareLocation");
> > > +  var shareButton = locationBar.getNotificationElement(
> > > +    "geolocation-notification", {type: "label", value: locationLabel}
> > > +  );
> > 
> > nit: This is not a valid style (indentation) for function parameters.
> This would exceed the the 80 limit other way, we have other cases like this,

Leave it as it is. It's not worth it.

> > Doesn't check isDisplayed itself if the element exists?
> No it doesn't if the element hasn't have a node it will fail with undefined.
> http://hg.mozilla.org/qa/mozmill-tests/file/530536be4b4e/lib/utils.js#l385

So instead of workarounds it should be fixed in isDisplayed().

Comment 28

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: 1088638_follow_up.patch

(In reply to Henrik Skupin (:whimboo) from comment #27)
> For manual tests the eye of the tester covers lots of other states in the
> UI, and I'm sure the position is used to verify it. Anyway, if we really
> dont need it, why the retrieval of the result element?
Okey so it doesn't take more than a second for the test to do this check too so I added it.

> 
> > > Doesn't check isDisplayed itself if the element exists?
> > No it doesn't if the element hasn't have a node it will fail with undefined.
> > http://hg.mozilla.org/qa/mozmill-tests/file/530536be4b4e/lib/utils.js#l385
> 
> So instead of workarounds it should be fixed in isDisplayed().
isDisplayed returns true or false but it throws if it doesn't because it tries to get entities from an undefined object, so I just return early if the element doesn't exists. I didn't want to add an assertion inside of isDisplayed because we expect it to return a boolean and we call it in a loop in waitFor functions until it returns true or false, having an assertion would broke that loop.

I hope you like the following changes, thanks.

Comment 29

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

Comment on attachment 8537648 [details] [diff] [review]
1088638_follow_up.patch

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

Yes, that looks great, except two things I mentioned inline.

::: firefox/tests/functional/testAddons/testServiceInstallDisableEnableRemove.js
@@ +98,5 @@
> +    }, "", TIMEOUT_POSITION);
> +  }
> +  catch (e) {
> +    assert.fail("Geolocation position is: " + result.getNode().textContent);
> +  }

Lets just assert via waitFor() if the retrieval of the position fails, and get rid of the try/catch.

But I still wonder how this should work as long as the sidebar.html does not assign the value to the geoLocationResult field.

Comment 30

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: 1088638_follow_up.patch

(In reply to Henrik Skupin (:whimboo) from comment #29)
> But I still wonder how this should work as long as the sidebar.html does not
> assign the value to the geoLocationResult field.
We do assign it:
https://github.com/mozilla/qa-mozmill-tests/blob/6eb2ea5e324f5de86f7252b7664fd013d23fe971/data/services/empty_service/sidebar.html#L10

Comment 31

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

Oh, it's a callback. Now I see. So the getElementById() call is unnecessary then:
https://github.com/mozilla/qa-mozmill-tests/blob/6eb2ea5e324f5de86f7252b7664fd013d23fe971/data/services/empty_service/sidebar.html#L33 But that's tiny..

Comment 32

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #31)
> Oh, it's a callback. Now I see. So the getElementById() call is unnecessary
> then:
> https://github.com/mozilla/qa-mozmill-tests/blob/
> 6eb2ea5e324f5de86f7252b7664fd013d23fe971/data/services/empty_service/sidebar.
> html#L33 But that's tiny..
Why not, we have to set the result somewhere? I added here so it will be retrieved only when geoLocationShare is called, this way the script can live in head section and we will retrieve the element only when the function is called, when the dom is fully loaded and the element exists, otherwise if I retrieve it on top of the script I will have to move the script to bottom of page.

Comment 33

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

Why don't you add all this in handlePosition() directly? Right now this makes it only hard to follow.

Comment 34

[Cosmin Malutan, [:cosmin-malutan]]

True, but adding it in handlePosition will require to retrieve it in handleError too!

Comment 35

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

So lets leave it as it is.

Comment 36

[Cosmin Malutan, [:cosmin-malutan]]

Thanks Henrik for prompts replays.

Comment 37

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8537674 [details] [diff] [review]
1088638_follow_up.patch

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

Tests well. Henrik, you wanna have a look too? It adds the check in utils.js for the element existence.

Comment 38

[Andreea Matei [:AndreeaMatei]]

http://hg.mozilla.org/qa/mozmill-tests/rev/3825b4e57029 (default)

Lets check backporting.

Comment 39

[Cosmin Malutan, [:cosmin-malutan]]

The patch applies cleanly on aurora and beta, it doesn't fail neither fixes a failure, so it can be backported anytime.

Comment 40

[Cosmin Malutan, [:cosmin-malutan]]

Andreea could you uplift the later patch up to beta?

Comment 41

[Andreea Matei [:AndreeaMatei]]

http://hg.mozilla.org/qa/mozmill-tests/rev/cf207521628e (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/b4a96fb6d14f (beta)