Test failures in /testAddons/testEnableDisableUndoUninstall.js

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mihaelav, Assigned: cosmin)

Tracking

Version 2

Firefox Tracking Flags

(firefox36 fixed)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
Test:      testEnableDisableUndoUninstall.js 
Failure:   
/testAddons/testEnableDisableUndoUninstall.js - testUndoUninstall - Addon is no longer marked for uninstall - 'uninstall' 
/testAddons/testEnableDisableUndoUninstall.js - testAddonNotUninstalled - AddonsManager_isAddonInstalled: Add-on has been specified. - got 'undefined' 
Branches:  Nightly
Platforms: Ubuntu, Windows

This fails only on Nightly

Report:
http://mozmill-release.blargon7.com/#/remote/failure?app=Firefox&branch=All&platform=All&from=2014-10-23&to=&test=%2FtestSecurity%2FtestSSLStatusAfterRestart.js&func=testDisplayCertificateStatusAfterRestart
(Assignee)

Comment 1

3 years ago
I added a loop to stress the addon removal and undo, and I reproduced this locally:
http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/tests/functional/testAddons/testEnableDisableUndoUninstall.js#l125


My first guess is that there is a ViewChanged event we should wait for when enabling and disabling the addon , otherwise subelements like undo link or enable button wont be available, but I'll have to check.
(Assignee)

Comment 2

3 years ago
Created attachment 8518152 [details] [diff] [review]
patch v1.0

I checked for ViewChanged/transitioend/animationend events but we don't have any of this. We just have to plainly wait for nodes to exists and to be displayed, I ran the loop for 50 times and it passed every time.
Assignee: nobody → cosmin.malutan
Status: NEW → ASSIGNED
Attachment #8518152 - Flags: review?(andrei.eftimie)
Attachment #8518152 - Flags: review?(andreea.matei)
(In reply to Cosmin Malutan from comment #2)
> I checked for ViewChanged/transitioend/animationend events but we don't have

Those wont work. There should be specific events or observer notifications when add-on are getting uninstalled or undo'ed.
(Assignee)

Comment 4

3 years ago
I checked for the events above, I also explored the extensions code, there is nothing, perhaps we might ask a developer. 
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.xml#1191
You will have to setup an add-on listener and wait for the 'onOperationCancelled' event:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4725

Comment 6

3 years ago
Comment on attachment 8518152 [details] [diff] [review]
patch v1.0

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

I've actually been seeing the same failure for the Addon Install button lately locally.

+1 for getting all of this fixed.
I see all of them are using `getAddonChildElement` internally, can we wait there for the requested element to be present or ready?
Attachment #8518152 - Flags: review?(andrei.eftimie)
Attachment #8518152 - Flags: review?(andreea.matei)
Attachment #8518152 - Flags: feedback+
(Assignee)

Comment 7

3 years ago
(In reply to Henrik Skupin (:whimboo) from comment #5)
> You will have to setup an add-on listener and wait for the
> 'onOperationCancelled' event:
I tried that, it doesn't work, after around the 5th loop it fails. Those are the event's that triggers the UI changes too, so they don't notify when the UI is changed.
I would like to go with Andrei's suggestion and wait for element in getAddonChildElement, that way we might fix other failures like the one Andrei suggested. 


http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.xml#1637
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4637
Flags: needinfo?(hskupin)
(In reply to Andrei Eftimie from comment #6)
> I see all of them are using `getAddonChildElement` internally, can we wait
> there for the requested element to be present or ready?

We cannot implicitly wait for those elements. Then we would do it for any case. Maybe better to pass in a wait parameter, which indicates that for this case we want to wait for the element? Then we still have the chance in other cases to return directly.
Flags: needinfo?(hskupin)

Comment 9

3 years ago
(In reply to Henrik Skupin (:whimboo) from comment #8)
> (In reply to Andrei Eftimie from comment #6)
> > I see all of them are using `getAddonChildElement` internally, can we wait
> > there for the requested element to be present or ready?
> 
> We cannot implicitly wait for those elements. Then we would do it for any
> case. Maybe better to pass in a wait parameter, which indicates that for
> this case we want to wait for the element? Then we still have the chance in
> other cases to return directly.

Why not wait for every case if we're seeing this fail intermittently for most elements?
In cases you want to check that an element does not exist, we don't want to wait 5s. So we could default this parameter to true, and in cases we don't want to wait for the element, we can pass-in false. So we won't have to specify the parameter in all the current cases.
(Assignee)

Comment 11

3 years ago
I would rather wait by default so we might fix other issues, and add a flag to don't wait for those specific cases, otherwise we would have to add dozens of flags in tests to fix all possibles issues.

Comment 12

3 years ago
(In reply to Cosmin Malutan from comment #11)
> I would rather wait by default so we might fix other issues, and add a flag
> to don't wait for those specific cases, otherwise we would have to add
> dozens of flags in tests to fix all possibles issues.

I'm fine either way. Please check in both cases how many instance you'd have to update. I'm inclined to think that in most cases we want to use these elements, so it would make sense the default be to wait.
(Assignee)

Comment 13

3 years ago
I added the waitfor, so at the moment there is no instance where we don't expect the node! So defaulting the wait to true would make the more sense.
Flags: needinfo?(hskupin)
I do not see why needinfo for me has been set here. Anyway, what you are stating I already have proposed in comment 10. So not sure what other information you want from me.
Flags: needinfo?(hskupin)
(Assignee)

Comment 15

3 years ago
Created attachment 8518890 [details] [diff] [review]
patch v2.0

Done as suggested. Thanks
http://mozmill-crowd.blargon7.com/#/remote/report/43b0604030afed1afa48590287003716
http://mozmill-crowd.blargon7.com/#/functional/report/43b0604030afed1afa48590287002ae7
Attachment #8518152 - Attachment is obsolete: true
Attachment #8518890 - Flags: review?(andrei.eftimie)
Attachment #8518890 - Flags: review?(andreea.matei)

Comment 16

3 years ago
Comment on attachment 8518890 [details] [diff] [review]
patch v2.0

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

I like this solution.

::: lib/addons.js
@@ +585,5 @@
>      var addon = spec.addon;
>      var attribute = spec.attribute;
>      var value = spec.value;
> +    var viewPrefix = (this.selectedView.getNode().id == "detail-view") ?
> +                     "detailView_" : "listView_";

Please rearrange this.
nit: use strict equality

Also `detail-view` didn't previously set `parent: addon`, please check what it does, and remove it if needed.

@@ +607,5 @@
> +      value: value,
> +      parent: addon
> +    };
> +
> +    if(!wait) {

nit: missing space
Attachment #8518890 - Flags: review?(andrei.eftimie)
Attachment #8518890 - Flags: review?(andreea.matei)
Attachment #8518890 - Flags: review-
(Assignee)

Comment 17

3 years ago
Created attachment 8519738 [details] [diff] [review]
patch v2.1

(In reply to Andrei Eftimie from comment #16)
> > +    var viewPrefix = (this.selectedView.getNode().id == "detail-view") ?
> > +                     "detailView_" : "listView_";
> 
> Please rearrange this.
> nit: use strict equality
We had a getter for this: isDetailViewActive

> Also `detail-view` didn't previously set `parent: addon`, please check what
> it does, and remove it if needed.
The addon is not compulsory, we need it only for list-view, but it doesn't harm if we don't have it for details view and neither if we do.

Report
http://mozmill-crowd.blargon7.com/#/remote/report/43b0604030afed1afa485902876c153e
Attachment #8518890 - Attachment is obsolete: true
Attachment #8519738 - Flags: review?(andrei.eftimie)

Comment 18

3 years ago
Comment on attachment 8519738 [details] [diff] [review]
patch v2.1

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

lgtm
Attachment #8519738 - Flags: review?(hskupin)
Attachment #8519738 - Flags: review?(andrei.eftimie)
Attachment #8519738 - Flags: review+
Comment on attachment 8519738 [details] [diff] [review]
patch v2.1

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

r=me with the nits fixed.

::: lib/addons.js
@@ +585,5 @@
>      var addon = spec.addon;
>      var attribute = spec.attribute;
>      var value = spec.value;
>      var type = spec.type;
> +    var wait = (typeof spec.wait !== "undefined") ? spec.wait : true;

I don't see the jsdoc updated.

@@ +598,5 @@
>        assert.ok(attribute, arguments.callee.name + ": DOM attribute has been specified.");
>        assert.ok(value, arguments.callee.name + ": Value has been specified.");
>      }
>  
> +    var viewPrefix = this.isDetailViewActive ? "detailView_" : "listView_";

nit: There is only one prefix we handle here. So I don't see a need for 'view'.

@@ +612,3 @@
>      }
>      else {
> +      let element;

Be consistent. `var` or `let` but not mixed. Generally we should prefer let those days for typed variables.
Attachment #8519738 - Flags: review?(hskupin) → review+
Please get this patch updated so we can land it ASAP. It fails a lot, and for me I can reproduce it each time I run this test locally on a Windows 7 machine.
(Assignee)

Comment 21

3 years ago
Created attachment 8522012 [details] [diff] [review]
patch v2.2

Fixed those nits. Thanks
Attachment #8519738 - Attachment is obsolete: true
Attachment #8522012 - Flags: review?(andrei.eftimie)
Attachment #8522012 - Flags: review?(andreea.matei)

Comment 22

3 years ago
Comment on attachment 8522012 [details] [diff] [review]
patch v2.2

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

https://hg.mozilla.org/qa/mozmill-tests/rev/05fd4195ab14 (default)
Attachment #8522012 - Flags: review?(andrei.eftimie)
Attachment #8522012 - Flags: review?(andreea.matei)
Attachment #8522012 - Flags: review+
Attachment #8522012 - Flags: checkin+

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox36: --- → fixed
Resolution: --- → FIXED
(Assignee)

Comment 23

3 years ago
Andreea, could you backport this fix down to beta and release, in order to fix bug 1112517?
You need to log in before you can comment on or make changes to this bug.