783484 - Test failure 'Shutdown expected but none detected before end of test ' in /restartTests/testAddons_uninstallExtension/test4.js

Status: RESOLVED FIXED

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

Description

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

The test has been started to fail recently. Can someone please check if this is a regression?

Failure message: "Shutdown expected but none detected before end of test"

Comment 1

[Maniac Vlad Florin (:vladmaniac)]

Yes - seen this yesterday evening happening. I'll take it. Thanks for submitting the bug

Comment 2

[Maniac Vlad Florin (:vladmaniac)]

Looks like I had no luck running into this on my testing box. 

http://mozmill-crowd.blargon7.com/#/functional/reports?branch=All&platform=All&from=2012-08-17&to=2012-08-17

I was trying more with the fr locale which is likely more to fail, but still no luck. 
Judging by the error it seems like we never get the shutdown command, which in our case would be clicking on the restart link in add-ons manager

Comment 3

[Maniac Vlad Florin (:vladmaniac)]

So this happens no more. If we check the URL section in the bug, it has aprox one month of clean results. Therefore WFM until further notice

Comment 4

[u279076]

I saw this failure today with the 17rc build on Mac OSX 10.8.2:
http://mozmill-ci.blargon7.com/#/functional/report/674977957b923f4905160d1b9a3a43ef

Comment 5

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

Putting back into the pool for watching those results.

Comment 6

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

Given that it fails for Firefox 17 we have to take care about it because we will get a lot of esr17 releases. Lets work on this next week.

Comment 7

[Daniela Petrovici]

Happened yesterday on ESR17 and Windows 7 SP1:
http://mozmill-ci.blargon7.com/#/functional/report/352218d7e3125c857fc1d371670887cb
Happened on FF 17 and Mac OS X 10.8.2 (x86_64):
http://mozmill-ci.blargon7.com/#/functional/report/352218d7e3125c857fc1d3716716fa4b

Comment 8

[Daniela Petrovici]

Reproduced on Mac OS X 10.8.2 (x86_64) and Aurora branch:
http://mozmill-ci.blargon7.com/#/functional/report/352218d7e3125c857fc1d371671e455d

Comment 9

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

Can someone try if it is reproducible?

Comment 10

[u279076]

FWIW, reproduced with the 17.0.1 candidate builds:
http://mozmill-ci.blargon7.com/#/functional/report/352218d7e3125c857fc1d3716716fa4b

Comment 11

[Daniela Petrovici]

Happened in 11/29 on MAC OS 10.8.2 with Firefox 17.0.1:
http://mozmill-ondemand.blargon7.com/#/functional/report/352218d7e3125c857fc1d37167308bf6
Happened in 12/1 on Mac OS X 10.7.5 (x86_64) with Aurora FR:
http://mozmill-ci.blargon7.com/#/functional/report/352218d7e3125c857fc1d371675ab136

Comment 12

[Andreea Matei [:AndreeaMatei]]

Has happened again on Aurora, with Windows Vista:

http://mozmill-ci.blargon7.com/#/functional/report/9e41582ed5e806fa373351d9d339c5d2

I'll try today to reproduce it.

Comment 13

[Andreea Matei [:AndreeaMatei]]

Again on Windows 7, with Aurora - fr locale:
http://mozmill-ci.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb5115d7cc

Comment 14

[Daniela Petrovici]

Started reproducing on Nightly FR with Windows 7 SP1:
http://mozmill-ci.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb513567cd

Comment 15

[Andrei Eftimie]

Have not been able to reproduce it locally yet.

Happened again (Vista, 21.0a1, fr):
http://mozmill-ci.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51cbc4ea

Comment 16

[Andrei Eftimie]

Same issue as bug 780957
Unable to click on an element if outside the visible viewport.

Comment 17

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

Thanks Andrei. So lets get bug 780957 fixed and rechecked this one afterward.

Comment 18

[Andrei Eftimie]

Has not happened in almost 3 weeks (last recorded failure was in the 29th January)

We might mark it WFM, but:

since it is related to bug 780957 which has been fixed (fix only applied to 17, 18 and 19)
we should keep an eye out if it resurfaces (especially see if it is on a branch with the previously mentioned fix applied, or on a newer branch).

Depending on the case bug 780957 fix might also fix this (if it only shows up on unpatched branches) or it might require a slightly different approach.

Comment 19

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

Marking as fixed as the dependant bug was fixed. We can reopen if it turns out this did not in fact fix this issue.

Comment 20

[mario garbi]

Happened again today on Mac 10.8.2 with FF 17.0.4:
http://mozmill-ci.blargon7.com/#/functional/report/172b2945b09ec259eb6e92f6441f9459

Comment 21

[Andreea Matei [:AndreeaMatei]]

Has happened again this weekend, with nightly fr locale, Windows Vista:
http://mozmill-ci.blargon7.com/#/functional/report/2a6536e9db9f5f44ed48c58510c222ff

Comment 22

[Daniela Petrovici]

Happened again on Nightly with Mac OS X 10.6.8 (x86_64): http://mozmill-ci.blargon7.com/#/functional/report/2a6536e9db9f5f44ed48c58510fd1f85

Comment 23

[Daniela Petrovici]

Happened again on Nightly IT with MAC OS 10.6.8: http://mozmill-ci.blargon7.com/#/functional/report/db924715258381629c26ddbad649e967

Comment 24

[Andrei Eftimie]

Also on Nightly, en-US, Win7:
http://mozmill-ci.blargon7.com/#/functional/report/db924715258381629c26ddbad698c9f7

Lots of failures in the last weeks, I will give this higher priority on my todo list to get it fixed.

*Still not sure if we fail to click on the restart link (simplest explanation, have not been able to make it fail this way manually). Or something else entirely which hangs the restart.

Comment 25

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

This is failing a lot these days. Andrei, please try to get it reproduced so we can fix it.

Comment 26

[Andrei Eftimie]

Attachment: restartLink properties diff (pass vs fail)

I've managed to reproduce this locally.
Fails about 1 in 10 runs.

I dumped all properties of the restartLink, attached is a diff of those properties. Based on the dimensions and positions (0) I might assert that we try to click on the link before it is actually displayed.

If this is the case, waiting for the element before clicking it should solve the problem. Running tests now to verify (will probably leave them overnight running to make sure no failures).

But I am a bit concerned about the difference on the markup, mainly:

Pass
> command="cmd_restartApp"

Fail
> oncommand="document.getBindingParent(this).restart();"

This might not mean anything, and it might change as the element loads into view.

Comment 27

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

(In reply to Andrei Eftimie from comment #26)
> But I am a bit concerned about the difference on the markup, mainly:
> 
> Pass
> > command="cmd_restartApp"
> 
> Fail
> > oncommand="document.getBindingParent(this).restart();"

Blair, do you have an explanation for this behavior of the restart button in the extension pane?

Comment 28

[Andrei Eftimie]

Attachment: Patch v1

Comment 29

[Andrei Eftimie]

Missing comment for the previously attached patch.

Ran tests overnight. More then 1000 runs without incident.
It should fix the problem.

Comment 30

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 731081 [details] [diff] [review]
Patch v1

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

I see we are using this restartLink element in several tests, I wonder why only this one failed. 
Anyway, it would be good as a precaution to have the fix on all of them.

::: tests/functional/restartTests/testAddons_uninstallExtension/test4.js
@@ +35,5 @@
>  
>    // Restart the browser using restart prompt
>    var restartLink = addonsManager.getElement({type: "listView_restartLink",
>                                                parent: enabledExtension});
> +  controller.waitForElement(restartLink);

Won't waitThenClick() work, instead of this and click()?

Comment 31

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Henrik Skupin (:whimboo) from comment #27)
> Blair, do you have an explanation for this behavior of the restart button in
> the extension pane?

No idea, sorry :\

Comment 32

[Andrei Eftimie]

Attachment: Patch v2

We are now waiting for every instance of the restartLink.

> Won't waitThenClick() work, instead of this and click()?
Not really, since we want to wait for the element to be available, and only then do startUserShutdown()

Comment 33

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 731788 [details] [diff] [review]
Patch v2

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

::: tests/functional/restartTests/testAddons_changeTheme/test1.js
@@ +78,5 @@
>    // Restart the browser using restart prompt
>    var restartLink = addonsManager.getElement({type: "listView_restartLink",
>                                                parent: plainTheme});
> +  controller.waitForElement(restartLink);
> +  

Trailing whitespaces.

::: tests/functional/restartTests/testAddons_changeTheme/test2.js
@@ +45,5 @@
>    // Restart the browser using restart prompt
>    var restartLink = addonsManager.getElement({type: "listView_restartLink",
>                                                parent: defaultTheme});
> +  controller.waitForElement(restartLink);
> +  

Same here

::: tests/functional/restartTests/testAddons_enableDisableExtension/test2.js
@@ +41,5 @@
>    // Click on the list view restart link
>    var restartLink = addonsManager.getElement({type: "listView_restartLink",
>                                                parent: addon});
> +  controller.waitForElement(restartLink);
> +  

Whitespaces

::: tests/functional/restartTests/testAddons_uninstallExtension/test2.js
@@ +53,5 @@
>    // Restart the browser using restart prompt
>    var restartLink = addonsManager.getElement({type: "listView_restartLink",
>                                                parent: toDisableExtension});
> +  controller.waitForElement(restartLink);
> +  

Trailing whitespaces

Comment 34

[Andrei Eftimie]

Attachment: Patch v2.1

Sorry about that.
Fix trailing whitespaces.

Comment 35

[mario garbi]

Happened again today with Firefox 23.0a1 on Mac OS X 10.7.5 (x86_64):
http://mozmill-ci.blargon7.com/#/functional/report/25ad365ca7bcf4905e9b700b4fbb635b

Comment 36

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 732224 [details] [diff] [review]
Patch v2.1

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/c866a17f277c (default)

Comment 37

[Andreea Matei [:AndreeaMatei]]

Unfortunately this has not fixed the issue:
http://mozmill-ci.blargon7.com/#/functional/report/740ab23e062758ae8a240b263d27da26

Henrik, should we leave this change as an improvement though?

Comment 38

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

I think we should backout the patch. Given that we don't get a failure that the restartLink element doesn't exist. So it exists even without the extra waitForElement() call. We simply don't click on it.

Most likely it has something to do with the visibility state, and we click on it before it has been made visible.

Comment 39

[Andreea Matei [:AndreeaMatei]]

Backed out:
http://hg.mozilla.org/qa/mozmill-tests/rev/833b3cde6a7e

Comment 40

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

Can we please get this test disabled given that we can't find a solution that quickly? This failure is bouncing us for days now.

Comment 41

[Andrei Eftimie]

Attachment: skip patch

Patch to skip the test.

*also needed to skip test5 because it relies on test4

Comment 42

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 734532 [details] [diff] [review]
skip patch

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

Landed skip:
http://hg.mozilla.org/qa/mozmill-tests/rev/89cdb2dc35db (default)

Comment 43

[Andreea Matei [:AndreeaMatei]]

Transplanted as:
http://hg.mozilla.org/qa/mozmill-tests/rev/cc4bbbe9477a (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/680465288494 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/7353505d07dd (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/243579a3a0ff (esr17)

Comment 44

[Andrei Eftimie]

I might have had the right idea to "wait" for the restartLink based on previous evidence that when it failed it wasn't properly initialised.

But the implementation was not good. I used controller.waitFor() which, as I've now looked more deeply into mozmill's code, only checks that the element is available in the DOM. And the link was there, it only seemed not to be initialised.

The correct approach might be to wait for the element not to be disabled.
But alas, I am unable to reproduce it anymore. (Not locally, and not on our CI VM's).

Will try further to reproduce it.

Comment 45

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

There is a CSS transformation going on for the restart button link and the width is changing from auto to a specific value. The link is not disabled in any way. That might help you but better would be to have a look at the source what's happening for real when clicking on enable or disable.

Comment 46

[u279076]

I'm not sure if this is related but testAddons_UninstallExtension test1 through test3 failed today for Firefox 23.0b1 pl on Windows 8 64-bit:
http://mozmill-ondemand.blargon7.com/#/functional/report/91511d5536e962e0de7f57ed1ed83fec

Comment 47

[Andreea Matei [:AndreeaMatei]]

Has happened 3 times today on Aurora, windows 8:

http://mozmill-daily.blargon7.com/#/functional/failure?branch=All&platform=All&from=2013-09-18&to=&test=%2FrestartTests%2FtestAddons_uninstallExtension%2Ftest4.js&func=test4.js%3A%3AteardownModule

Test4 and test5 were unskipped as part of bug 915193 and need to be disabled.

Comment 48

[Andreea Matei [:AndreeaMatei]]

Attachment: skipTests.patch

Disabling tests, manifest file wasn't changed.

Comment 49

[Andrei Eftimie]

Comment on attachment 806603 [details] [diff] [review]
skipTests.patch

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

Yes, these 2 tests shouldn't have been unskipped.
The manifest.ini skip is still there.

Disabled:
http://hg.mozilla.org/qa/mozmill-tests/rev/678d05fab339 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/d56aac198f68 (mozilla-aurora)

Comment 50

[Andrei Eftimie]

Attachment: unskip_1.patch

These have been skipped for a long time now.
We've had numerous changes, including the move to mozmill 2.0

I've ran this 50 times in a loop. No failures.
It _was_ intermittent before. So this is not failproof.

I suggest we reenable tests 4 & 5 and monitor the outcome.
Attached is an unskip patch for default

Comment 51

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8388514 [details] [diff] [review]
unskip_1.patch

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

Re-enabled:
http://hg.mozilla.org/qa/mozmill-tests/rev/8fa4cef860a5 (default)

Comment 52

[Andreea Matei [:AndreeaMatei]]

Enabled on aurora as well:
http://hg.mozilla.org/qa/mozmill-tests/rev/0b8dd0ef98b1 (aurora)

Comment 53

[Andreea Matei [:AndreeaMatei]]

Andrei, could you please check if everything is fine on the rest of the branches to unskip the test?

Comment 54

[Andrei Eftimie]

Attachment: unskip_release.patch

Unskip for release.

Comment 55

[Andrei Eftimie]

Attachment: unskip_esr24.patch

Unskip for ESR24

Comment 56

[Andrei Eftimie]

And testruns:
http://mozmill-crowd.blargon7.com/#/functional/report/16dc29d7a411d565992bbd7dc84803fa
http://mozmill-crowd.blargon7.com/#/functional/report/16dc29d7a411d565992bbd7dc847fbaa

Comment 57

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8394651 [details] [diff] [review]
unskip_release.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/1a452afb4700 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/4bb20a4ae08f (esr24)

Thanks!