Closed
Bug 636700
Opened 14 years ago
Closed 14 years ago
Add installed add-on details to persisted object
Categories
(Mozilla QA Graveyard :: Mozmill Tests, defect)
Mozilla QA Graveyard
Mozmill Tests
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: davehunt, Assigned: davehunt)
References
Details
Attachments
(1 file, 4 obsolete files)
4.46 KB,
patch
|
whimboo
:
review+
|
Details | Diff | Splinter Review |
Forked from bug 634567
Assignee | ||
Comment 1•14 years ago
|
||
> 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•14 years ago
|
||
(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•14 years ago
|
||
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+
Assignee | ||
Comment 4•14 years ago
|
||
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 5•14 years ago
|
||
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-
Assignee | ||
Comment 6•14 years ago
|
||
(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)
Assignee | ||
Comment 7•14 years ago
|
||
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•14 years ago
|
||
Well it'll work but it looks suspiciously like spinning the event loop which is as a rule a bad thing.
Comment 9•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #516091 -
Flags: review?(hskupin)
Comment 10•14 years ago
|
||
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-
Assignee | ||
Comment 11•14 years ago
|
||
(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 12•14 years ago
|
||
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-
Assignee | ||
Comment 13•14 years ago
|
||
(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 14•14 years ago
|
||
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+
Comment 15•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 16•14 years ago
|
||
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 → ---
Assignee | ||
Comment 17•14 years ago
|
||
Mossop: Is there a way to get details of installed add-ons in older branches?
Comment 18•14 years ago
|
||
(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•14 years ago
|
||
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•14 years ago
|
||
As talked on IRC we do not want to backport this feature.
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•