Closed Bug 636700 Opened 9 years ago Closed 9 years ago

Add installed add-on details to persisted object

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: davehunt, Assigned: davehunt)

References

Details

Attachments

(1 file, 4 obsolete files)

> 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.
Assignee: nobody → dave.hunt
Status: NEW → ASSIGNED
Attachment #515106 - Flags: review?(hskupin)
(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 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.
Attachment #515106 - Flags: review?(hskupin) → review+
Work in progress (currently failing) - moving method to get installed add-ons to addons.js
Attachment #515106 - Attachment is obsolete: true
Attachment #515761 - Flags: feedback?(hskupin)
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.
Attachment #515761 - Flags: feedback?(hskupin) → feedback-
(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.
Attachment #515761 - Attachment is obsolete: true
Attachment #516091 - Flags: review?(hskupin)
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;
}
Well it'll work but it looks suspiciously like spinning the event loop which is as a rule a bad thing.
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.
Attachment #516091 - Flags: review?(hskupin)
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.
Attachment #516091 - Flags: review?(hskupin) → review-
(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.
Attachment #516091 - Attachment is obsolete: true
Attachment #516442 - Flags: review?(hskupin)
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!
Attachment #516442 - Flags: review?(hskupin) → review-
(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.
Attachment #516442 - Attachment is obsolete: true
Attachment #516577 - Flags: review?(hskupin)
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!
Attachment #516577 - Flags: review?(hskupin) → review+
Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/03e69b15b9a4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Mossop: Is there a way to get details of installed add-ons in older branches?
(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.
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.
As talked on IRC we do not want to backport this feature.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.