636700 - Add installed add-on details to persisted object

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Forked from bug 634567

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Add installed add-on details to persisted object. v1.1

> Add installed extension details to the persisted object. v1
> 
> >+    persisted.addons = [];
> 
> Do it the same way as for the results and define that empty array already in
> the Endurance class of the testrun module.

I wasn't able to do this as the callback couldn't reference the variable. I'm not really strong enough in with my JavaScript so would like to see how this could be done.

> >+  Components.utils.import("resource://gre/modules/AddonManager.jsm");
> 
> Put this at the top directly below the license.

Done.

> >+  if (persisted.addons) {
> >+    AddonManager.getAllAddons(function(aAddons) {
> 
> nit: space between function and (

Done.

> >+      aAddons.forEach(function (addon) {
> >+        if (addon.type === "extension") {
> >+          persisted.addons.push({
> >+            id : addon.id,
> >+            name : addon.name,
> >+            version : addon.version
> 
> Please also add isCompatible to that list. That's a kinda important information
> for us.

Done.

> In general we should move the retrieval of addons into the addons.js module,
> which then simply returns the array. I would also like to use it for the update
> tests.

I also tried to do this but had the same issue with variable scope. I should be able to do this once I understand this better.

Comment 2

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

(In reply to comment #1)
> > >+    persisted.addons = [];
> > 
> > Do it the same way as for the results and define that empty array already in
> > the Endurance class of the testrun module.
> 
> I wasn't able to do this as the callback couldn't reference the variable. I'm
> not really strong enough in with my JavaScript so would like to see how this
> could be done.

So you have placed the above mentioned line in the TestRun class of the automation scripts? If it gets set correctly the property on the persisted object should exist. You can also do 'if ("addons" in persisted)' to check if the property exists.

> > In general we should move the retrieval of addons into the addons.js module,
> > which then simply returns the array. I would also like to use it for the update
> > tests.
> 
> I also tried to do this but had the same issue with variable scope. I should be
> able to do this once I understand this better.

Do you have some code to show? Have you tried to implement it as a global function? Have you exported the function?

Comment 3

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

Comment on attachment 515106 [details] [diff] [review]
Add installed add-on details to persisted object. v1.1

Only feedback+, because the move to addons.js is missing.

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: WIP: Add installed add-on details to persisted object. v2

Work in progress (currently failing) - moving method to get installed add-ons to addons.js

Comment 5

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

Comment on attachment 515761 [details] [diff] [review]
WIP: Add installed add-on details to persisted object. v2

>+function getInstalledAddons() {
>+  var addonInfo = [ ];
>+
>+  AddonManager.getAllAddons(function (aAddons) {
>+    aAddons.forEach(function (addon) {
>+      addonInfo.push({
>+        id : addon.id,
>+        type : addon.type,
>+        name : addon.name,
>+        version : addon.version,
>+        isCompatible : addon.isCompatible
>+      });
>+      dump("addonInfo.length: " + addonInfo.length + "\n")
>+    });
>+  });

I see this problem now. It's really because it's getting executed asynchronously. As a workaround for now you could do:

------------
  let addonInfo = null;

  AddonManager.getAllAddons(function (aAddons) {
    addonInfo = [addon for each (addon in aAddons)]
  });

  mozmill.utils.waitFor(function () {
    return !!addonInfo;
  })

  return addonInfo;
------------

But I would suggest to ask Dave or Blair if there is a better way in doing that.


> // Export of functions
>+exports.getInstalledAddons = getInstalledAddons;
> exports.addToWhiteList = addToWhiteList;

alphabetical order please.

>   if (persisted.endurance) {

You should use '"endurance" in persisted'. Missed that the last time.

>+  if (persisted.addons) {
>+    persisted.addons = addons.getInstalledAddons();
>+  }

Here you do not need a if clause. Directly assign it.

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Add installed add-on details to persisted object. v2.1

(In reply to comment #5)
> Comment on attachment 515761 [details] [diff] [review]
> WIP: Add installed add-on details to persisted object. v2
> 
> >+function getInstalledAddons() {
> >+  var addonInfo = [ ];
> >+
> >+  AddonManager.getAllAddons(function (aAddons) {
> >+    aAddons.forEach(function (addon) {
> >+      addonInfo.push({
> >+        id : addon.id,
> >+        type : addon.type,
> >+        name : addon.name,
> >+        version : addon.version,
> >+        isCompatible : addon.isCompatible
> >+      });
> >+      dump("addonInfo.length: " + addonInfo.length + "\n")
> >+    });
> >+  });
> 
> I see this problem now. It's really because it's getting executed
> asynchronously. As a workaround for now you could do:
> 
> ------------
>   let addonInfo = null;
> 
>   AddonManager.getAllAddons(function (aAddons) {
>     addonInfo = [addon for each (addon in aAddons)]
>   });
> 
>   mozmill.utils.waitFor(function () {
>     return !!addonInfo;
>   })
> 
>   return addonInfo;
> ------------
> 
> But I would suggest to ask Dave or Blair if there is a better way in doing
> that.

Done. Will CC Dave and Blair for their feedback.

> > // Export of functions
> >+exports.getInstalledAddons = getInstalledAddons;
> > exports.addToWhiteList = addToWhiteList;
> 
> alphabetical order please.

Done.

> >   if (persisted.endurance) {
> 
> You should use '"endurance" in persisted'. Missed that the last time.

Done.

> >+  if (persisted.addons) {
> >+    persisted.addons = addons.getInstalledAddons();
> >+  }
> 
> Here you do not need a if clause. Directly assign it.

Done.

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Adding Dave and Blair to the CC list for feedback on Henrik's solution to the asynchronous method to get all installed addons:

 /**
 * Gets all installed add-ons
 *
 * @returns Installed add-ons
 * @type {array}
 */
function getInstalledAddons() {
  let addonInfo = null;

  AddonManager.getAllAddons(function (aAddons) {
    addonInfo = aAddons;
  });

 mozmill.utils.waitFor(function () {
   return !!addonInfo;
 });
 
 return addonInfo;
}

Comment 8

[Dave Townsend [:mossop]]

Well it'll work but it looks suspiciously like spinning the event loop which is as a rule a bad thing.

Comment 9

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

Comment on attachment 516091 [details] [diff] [review]
Add installed add-on details to persisted object. v2.1

Resetting review request due to still open questions.

Comment 10

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

Comment on attachment 516091 [details] [diff] [review]
Add installed add-on details to persisted object. v2.1

>+  persisted.addons = addons.getInstalledAddons();

getInstalledAddons() returns the complete information for all add-ons. In our case we only want to have some of those properties. So you should filter out the properties we do not want to have in our report.

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Add installed add-on details to persisted object. v3

(In reply to comment #10)
> Comment on attachment 516091 [details] [diff] [review]
> Add installed add-on details to persisted object. v2.1
> 
> >+  persisted.addons = addons.getInstalledAddons();
> 
> getInstalledAddons() returns the complete information for all add-ons. In our
> case we only want to have some of those properties. So you should filter out
> the properties we do not want to have in our report.

Added a callback filter.

Comment 12

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

Comment on attachment 516442 [details] [diff] [review]
Add installed add-on details to persisted object. v3

>+ * @param {function} callback
>+ *        Callback function to call for filtering results

We should name the parameter filterCallback which makes it easier to discover. Also please give a bit more information what's required for the callback, i.e. the parameter and the return value. Also make it an optional parameter, which will then fallback to a direct copy of the add-on information.

>+  persisted.addons = addons.getInstalledAddons(function(addon) {

nit: space between function and opening bracket.

>+    return {
>+      id : addon.id,
>+      type : addon.type,
>+      name : addon.name,
>+      version : addon.version,
>+      isCompatible : addon.isCompatible

After checking the list of properties of an add-on I think we should also include isActive.

With those changes we are good to go. Thanks for the updates Dave!

Comment 13

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Add installed add-on details to persisted object. v3.1

(In reply to comment #12)
> Comment on attachment 516442 [details] [diff] [review]
> Add installed add-on details to persisted object. v3
> 
> >+ * @param {function} callback
> >+ *        Callback function to call for filtering results
> 
> We should name the parameter filterCallback which makes it easier to discover.
> Also please give a bit more information what's required for the callback, i.e.
> the parameter and the return value. Also make it an optional parameter, which
> will then fallback to a direct copy of the add-on information.

Done.

> >+  persisted.addons = addons.getInstalledAddons(function(addon) {
> 
> nit: space between function and opening bracket.

Done.

> >+    return {
> >+      id : addon.id,
> >+      type : addon.type,
> >+      name : addon.name,
> >+      version : addon.version,
> >+      isCompatible : addon.isCompatible
> 
> After checking the list of properties of an add-on I think we should also
> include isActive.

Done.

Comment 14

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

Comment on attachment 516577 [details] [diff] [review]
Add installed add-on details to persisted object. v3.1

Looks great and gives the expected output. Let get it checked-in so we can use it during the tomorrows testday. Thanks Dave!

Comment 15

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/03e69b15b9a4

Comment 16

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

Ups, we would also need a backport for the older branches. Once our normal tests will make use of it. It's not something we need immediately.

Comment 17

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Mossop: Is there a way to get details of installed add-ons in older branches?

Comment 18

[Dave Townsend [:mossop]]

(In reply to comment #17)
> Mossop: Is there a way to get details of installed add-ons in older branches?

On older versions the nsIExtensionManager interface allows access to certain information, for a lot of it though you need to access it through the RDF datasource directly.

Comment 19

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

Dave, while thinking about it, I don't think it's worth porting that back to older branches. None of them will be used that excessively as mozilla2.0 and later. I think we can close it.

Comment 20

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

As talked on IRC we do not want to backport this feature.