1108948 - Test failure 'Addon is no longer marked for uninstall' in /testAddons/testEnableDisableUndoUninstall.js

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

Description

[Cosmin Malutan, [:cosmin-malutan]]

Module:    MODULE
Test:      testUndoUninstall    
Failure:   'Addon is no longer marked for uninstall'
Branches:  mozilla-beta
Platforms: All

Failed for more than 100 times.


http://mozmill-release.blargon7.com/#/functional/failure?app=Firefox&branch=All&platform=All&from=2014-12-02&to=2014-12-09&test=%2FtestAddons%2FtestEnableDisableUndoUninstall.js&func=testUndoUninstall

Comment 1

[Cosmin Malutan, [:cosmin-malutan]]

I couldn't reproduce this failure. I ran the test alone for 20 times and a complete testrun on OSX 10.7 with latest Beta.

Comment 2

[Cosmin Malutan, [:cosmin-malutan]]

I'd say we should skip this, and try to reproduce it on a staging node. 
Teo can you do this?

Comment 3

[Teodor Druta]

Attachment: skipundouninstall.patch

This is the skip patch for mozilla-beta.

Comment 4

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

Skipped on beta:
https://hg.mozilla.org/qa/mozmill-tests/rev/2417e4b357a4

Lets get this figured out! It smells like some kind of regression we see in beta.

Comment 5

[Andreea Matei [:AndreeaMatei]]

Daniela, giving that this is kinda blocking finishing bug 1093598, could you maybe check this out quickly?

Comment 6

[Daniela Domnici]

I tried to reproduce it using Windows 8 production machine (Beta branch) and Ubuntu 14.04 production machine(beta branch). It didn`t fail at all. Here are the results:

Windows: http://mozmill-crowd.blargon7.com/#/functional/reports?app=All&branch=35&platform=Win&from=2014-12-23&to=2014-12-29 
Linux: http://mozmill-crowd.blargon7.com/#/functional/reports?app=All&branch=35&platform=Linux&from=2014-12-23&to=2014-12-29 
And also I`ve tried to do ondemand on the staging machine, but the test is  skipped in the manifest and I`ve stopped the ondemand.

Comment 7

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.0

I reproduced this on a staging node, it reproduced about twice in every 10 runs. 
As we can see in any failing report the addon is actually removed in the upcoming test, that's why test testAddonNotUninstalled fails too. So it was preaty clear that we failed to click on the undo link. If we click on an element that is not displayed, even if it exists it takes no action, so I added an waitFor->isDisplayed and it didn't failed anymore. I ran more than 50 testrun with the fix applied and it didn't failed anymore.
Failure example: 
http://mozmill-crowd.blargon7.com/#/functional/report/04e54d608fc589a28217304fe64f7e78

Comment 8

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

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

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

::: lib/addons.js
@@ +446,4 @@
>  
> +    // Wait for button to be displayed
> +    assert.waitFor(() => {
> +      button = this.getAddonButton(spec);

Doesn't the button exist all the time? I don't see why we have to retrieve it over and over again.

@@ +450,5 @@
> +
> +      return utils.isDisplayed(this._controller, button);
> +    });
> +
> +    button.click();

Maybe we should add another check right after which checks that the addon is marked to be removed. I think we can also enhance the other methods with that. I would suggest doing that in a follow-up bug.

Comment 9

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.1

(In reply to Henrik Skupin (:whimboo) from comment #8)
> > +    // Wait for button to be displayed
> > +    assert.waitFor(() => {
> > +      button = this.getAddonButton(spec);
> 
> Doesn't the button exist all the time? I don't see why we have to retrieve
> it over and over again.
We don't, I just thought it would be safer, but you're right we are better off without catching it.

> @@ +450,5 @@
> > +
> > +      return utils.isDisplayed(this._controller, button);
> > +    });
> > +
> > +    button.click();
> 
> Maybe we should add another check right after which checks that the addon is
> marked to be removed. I think we can also enhance the other methods with
> that. I would suggest doing that in a follow-up bug.
We do that check in test itself, that was the failure.

By the way, the fix is for nightly, if it goes in it has to be backported to beta, and the skip-patch reverted. 

Report:
http://mozmill-crowd.blargon7.com/#/functional/report/04e54d608fc589a28217304fe66bfbce

Comment 10

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

(In reply to Cosmin Malutan from comment #9)
> > Maybe we should add another check right after which checks that the addon is
> > marked to be removed. I think we can also enhance the other methods with
> > that. I would suggest doing that in a follow-up bug.
> We do that check in test itself, that was the failure.

And that's the problem. If a test is calling this method without doing the check itself, we will not notice the problem. When we have helper methods, those should assist as much as possible. Lets get a new bug filed so we can do this check inside the lib and not in each of the tests.

Comment 11

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #10)
> (In reply to Cosmin Malutan from comment #9)
> > > Maybe we should add another check right after which checks that the addon is
> > > marked to be removed. I think we can also enhance the other methods with
> > > that. I would suggest doing that in a follow-up bug.
> > We do that check in test itself, that was the failure.
> 
> And that's the problem. If a test is calling this method without doing the
> check itself, we will not notice the problem. When we have helper methods,
> those should assist as much as possible. Lets get a new bug filed so we can
> do this check inside the lib and not in each of the tests.
Done, bug 1118672. Henrik if you're fresh here could you leave an feedback or an review?

Comment 12

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8545135 [details] [diff] [review]
patch v1.1

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

Looks good. Henrik, are you ok with the fix?

Comment 13

[Andreea Matei [:AndreeaMatei]]

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

Comment 14

[Cosmin Malutan, [:cosmin-malutan]]

The patch applies cleanly on Aurora and Beta and fixes the issue, Andreea, could we back-port it?

Comment 15

[Andreea Matei [:AndreeaMatei]]

Aurora will be done with the merge today. I'll have beta landed tomorrow.

Comment 16

[Andreea Matei [:AndreeaMatei]]

http://hg.mozilla.org/qa/mozmill-tests/rev/4ce16abf4f01 (beta)
Thanks Cosmin!