931704 - Test failure "plainTheme is undefined" in /restartTests/testAddons_changeTheme/test1.js

Status: RESOLVED FIXED

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

Description

[mario garbi]

Started happening over the weekend on Linux and Windows with 'it', 'de' and 'en-US' locales of Nightly 27:

http://mozmill-daily.blargon7.com/#/functional/report/95c9c51e8c2d9f14917fc749dd845b11

Comment 1

[Cosmin Malutan, [:cosmin-malutan]]

I reproduced this failure locally. It reproduces every time, I will have a look and return with more info.

Comment 2

[Andrei Eftimie]

If this is a regression we want a regression range.

Comment 3

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Andrei Eftimie from comment #2)
> If this is a regression we want a regression range.
It doesn't reproduce, I was running the tests with "mozmill -t" instead of "mozmill-restart -t" and it wasn't running the test in the right order. 
Anyway, I will keep trying to reproduce it.

Comment 4

[mario garbi]

Happened again today on Windows NT 6.2.9200 (x86) with Firefox 27.0a2 de:
http://mozmill-daily.blargon7.com/#/functional/report/8a42e4b8f277f9c0268b061eb8a5c7eb

Comment 5

[Cosmin Malutan, [:cosmin-malutan]]

I ran the test for about 40 times on the affected machine and it did't reproduced.

Comment 6

[mario garbi]

Happened again on Windows NT 6.2.9200 (x86_64) with Firefox 27.0a2 en-US:
http://mozmill-daily.blargon7.com/#/functional/report/8a42e4b8f277f9c0268b061eb8eeda6b

Comment 7

[mario garbi]

And again on Windows NT 6.0.6002 Service Pack 2 (x86) with Firefox 27.0a2 fr:
http://mozmill-daily.blargon7.com/#/functional/report/59e7437f35dec9e49daab388d31019c5

Raising the priority due to increasing number of failures.

Comment 8

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

This is by far not a P2. It's intermittent and happening very rarely.

Comment 9

[Andreea Matei [:AndreeaMatei]]

Cosmin, please prepare a skip patch. This failed nearly each day since October 28th. Would also be good to check on a remote Windows machine if you cannot reproduce locally.

Comment 10

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: skip_patch

Hi Andreea, here is the skip patch.

Comment 11

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 831526 [details] [diff] [review]
skip_patch

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

::: tests/functional/restartTests/testAddons_changeTheme/manifest.ini
@@ +5,3 @@
>  [test3.js]
> +disabled = Bug 931704 - plainTheme is undefined.
> +

We use to change the manifest for the main restartTests folder, not this one.

::: tests/functional/restartTests/testAddons_changeTheme/test1.js
@@ +95,5 @@
>    assert.equal(plainTheme.getNode().getAttribute("pending"), "enable");
>  
>    // We need access to this addon in teardownModule
>    installedAddon = plainTheme;
>  }

Please add a blank line before the skip, across all files.

Comment 12

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: skip_patch

Hi Andreea, sorry for that, I think I didn't made a skip for a restart before.
http://mozmill-crowd.blargon7.com/#/functional/report/d7f12eb72275b30155ba68b8bc50a422

Comment 13

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: skip_patch

I missed to skip the tests too.

Comment 14

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 832134 [details] [diff] [review]
skip_patch

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

Disabled:
http://hg.mozilla.org/qa/mozmill-tests/rev/56f1bbcf9330 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/822c9a522159 (aurora)

Comment 15

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

(In reply to Andreea Matei [:AndreeaMatei] from comment #9)
> Cosmin, please prepare a skip patch. This failed nearly each day since
> October 28th. Would also be good to check on a remote Windows machine if you
> cannot reproduce locally.

Then it is not a P3 anymore.

Comment 16

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: skip_patch

I removed the name of the test from the skip message.

Comment 17

[Andrei Eftimie]

Comment on attachment 8388405 [details] [diff] [review]
skip_patch

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

I think you got the wrong bug?

Comment 18

[Cosmin Malutan, [:cosmin-malutan]]

Comment on attachment 8388405 [details] [diff] [review]
skip_patch

Yep, to many tabs opened.

Comment 19

[Cosmin Malutan, [:cosmin-malutan]]

This fails about twice at every 100 testruns. It didn't failed when I ran the test via manifest file but in complete testrun!

Comment 20

[Cosmin Malutan, [:cosmin-malutan]]

I ran the tests here with the latest environment for the past three days and I couldn't reproduce it. If it fails so rarely we might want to reenable it.

Comment 21

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

Sounds good. We should try it out on default for about a week.

Comment 22

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: re-patch_v1.0

Here is the re-enable patch for default. 
Reports: 
http://mozmill-crowd.blargon7.com/#/functional/report/7ab760e27012969ae02e3b9e41f0e7e3
http://mozmill-crowd.blargon7.com/#/functional/report/7ab760e27012969ae02e3b9e41f13b8c
http://mozmill-crowd.blargon7.com/#/functional/report/7ab760e27012969ae02e3b9e41f1038e

Comment 23

[Andrei Eftimie]

Comment on attachment 8405428 [details] [diff] [review]
re-patch_v1.0

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

Reenabled the tests:
http://hg.mozilla.org/qa/mozmill-tests/rev/22f4c75e9663 (default)

Lets see if there's any failures.

Comment 24

[Cosmin Malutan, [:cosmin-malutan]]

Patch applies cleanly on all branches, on esr24 the test is not disabled.

Comment 25

[Andrei Eftimie]

I've seen no failures related to this test.
There have been 3 faeilures: http://mozmill-daily.blargon7.com/#/functional/failure?app=All&branch=All&platform=All&from=2014-04-14&to=2014-04-23&test=%2FrestartTests%2FtestAddons_changeTheme%2Ftest3.js&func=testChangedThemeToDefault
but all of them were becuase test2 was not run (bug 972912).

Transplanted:
http://hg.mozilla.org/qa/mozmill-tests/rev/183f0700b5fb (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/c6f8d3e2d3de (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/faeccde149be (mozilla-release)

Comment 26

[Andrei Eftimie]

Seems this is not fixed yet. Only 1 occurrence in quite a long while:
http://mozmill-daily.blargon7.com/#/functional/report/ca725ca957252135fc72c3c08f305dae

Comment 27

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

There was a failure today here, on OS X 10.7.5, locale: el

/restartTests/testAddons_changeTheme/test3.js,	testChangedThemeToDefault, 'false' should equal 'true'

http://mozmill-release.blargon7.com/#/functional/report/3ed251269efbb25c65b0fc5d9d376eb3

Comment 28

[Andrei Eftimie]

(In reply to Liz Henry :lizzard from comment #27)
> There was a failure today here, on OS X 10.7.5, locale: el
> 
> /restartTests/testAddons_changeTheme/test3.js,	testChangedThemeToDefault,
> 'false' should equal 'true'
> 
> http://mozmill-release.blargon7.com/#/functional/report/
> 3ed251269efbb25c65b0fc5d9d376eb3

That is actually bug 972912

Comment 29

[Daniel Gherasim]

Failed twice today with this on latest Beta 33.0b1.
One on Windows and one on OSX.

http://mozmill-release.blargon7.com/#/functional/failure?app=All&branch=All&platform=All&from=2014-05-08&test=%2FrestartTests%2FtestAddons_changeTheme%2Ftest1.js&func=testInstallTheme

Comment 30

[Andreea Matei [:AndreeaMatei]]

Once again with release es-CL, OSX 10.7:
http://mozmill-release.blargon7.com/#/functional/report/eefb0f8a26a2a6196c62c274480eac95

Comment 31

[Andrei Eftimie]

Failed again: http://mozmill-release.blargon7.com/#/functional/report/bfd52839a01858de2f33d25f7683bce2


Interesing part from the console log:
> 03:53:44 TEST-START | restartTests\testAddons_changeTheme\test1.js | setupModule
> 03:53:44 TEST-START | restartTests\testAddons_changeTheme\test1.js | testInstallTheme
> 03:53:47 ERROR | Test Failure | {
> 03:53:47   "exception": {
> 03:53:47     "message": "plainTheme is undefined", 
> 03:53:47     "lineNumber": 80, 
> 03:53:47     "name": "TypeError", 
> 03:53:47     "fileName": "resource://mozmill/modules/frame.js -> file:///c:/jenkins/workspace/release-mozilla-beta_functional/data/mozmill-tests/firefox/tests/functional/restartTests/testAddons_changeTheme/test1.js"
> 03:53:47   }
> 03:53:47 }
> 03:53:47 TEST-UNEXPECTED-FAIL | restartTests\testAddons_changeTheme\test1.js | testInstallTheme
> 03:53:47 TEST-START | restartTests\testAddons_changeTheme\test1.js | teardownModule
> 03:53:47 TEST-END | restartTests\testAddons_changeTheme\test1.js | finished in 3209ms
> 03:53:47 1411556188953  addons.xpi  WARN  Failed to remove temporary file C:\DOCUME~1\mozilla\LOCALS~1\Temp\tmp-d3b.xpi for addon http://localhost:43336/addons/themes/plain.jar: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.remove]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm :: AI_removeTemporaryFile :: line 4948"  data: no] Stack trace: AI_removeTemporaryFile()@resource://gre/modules/addons/XPIProvider.jsm:4948 < AI_startInstall/<()@resource://gre/modules/addons/XPIProvider.jsm:5701 < Handler.prototype.process()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866 < this.PromiseWalker.walkerLoop()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745 < Assert_waitFor()@resource://mozmill/modules/assertions.js:595 < Httpd.prototype.stop()@resource://mozmill/modules/frame.js:653 < AppQuitObserver.prototype.observe()@resource://mozmill/modules/frame.js:440 < shutdownApplication()@resource://mozmill/modules/frame.js:66 < events.startShutdown()@resource://mozmill/modules/frame.js:170 < sendMessage()@resource://mozmill/driver/msgbroker.js:44 < MozMillController.prototype.restartApplication()@resource://mozmill/driver/controller.js:444 < teardownModule()@resource://mozmill/modules/frame.js -> file:///c:/jenkins/workspace/release-mozilla-beta_functional/data/mozmill-tests/firefox/tests/functional/restartTests/testAddons_changeTheme/test1.js:50 < Runner.prototype.execFunction()@resource://mozmill/modules/frame.js:755 < Runner.prototype.runTestModule()@resource://mozmill/modules/frame.js:723 < Runner.prototype.runTestFile()@resource://mozmill/modules/frame.js:693 < runTestFile()@resource://mozmill/modules/frame.js:778 < Bridge.prototype._execFunction()@resource://jsbridge/modules/Bridge.jsm:140 < Bridge.prototype.execFunction()@resource://jsbridge/modules/Bridge.jsm:147 < resource://jsbridge/modules/Server.jsm:32 < Server.Session/<()@resource://jsbridge/modules/Server.jsm:32 < Sockets.Client.prototype.onMessage/event.notify()@resource://jsbridge/modules/Sockets.jsm:40 < <file:unknown>

Comment 32

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

This also looks like a bug in the addons manager. Not sure how this bubbles up. Which line is that exactly in our test? I mean what do we exactly do when this failure occurs.

Comment 33

[Andrei Eftimie]

Test (failure is in the next line when we reference painTheme):
> var plainTheme = addonsManager.getAddons({attribute: "value", value: persisted.theme[0].id})[0];

addons.js:
> getAddons : function AddonsManager_addons(aSpec) {
>   var spec = aSpec || {};
>   return this.getElements({
>     type: "addons",
>     subtype: spec.attribute,
>     value: spec.value,
>     parent: this.selectedView
>   });
> },

and:
> getElements : function AddonsManager_getElements(aSpec) {
>   var spec = aSpec || { };
>   var type = spec.type;
>   var subtype = spec.subtype;
>   var value = spec.value;
>   var parent = spec.parent;
>   var root = parent ? parent.getNode() : this._controller.tabs.activeTab;
>   var nodeCollector = new domUtils.nodeCollector(root);
>   switch (type) {
>     // Add-ons
>     case "addons":
>       nodeCollector.queryNodes(".addon").filterByDOMProperty(subtype, value);
>       break;
>     [..]
>   }
>   return nodeCollector.elements;
> }

Is it possible that the addon itself, once installed has no read permissions... thus doesn't get listed in about:addons? That might explain the failure (and the log message when trying to remove the file).

I don't know enough of how addons are used internally...

Comment 34

[Andreea Matei [:AndreeaMatei]]

Failed 12 times today with Beta, Windows mainly, several locales.

Comment 35

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: 931704.patch

The node is not ready at the moment we try to use it, so we have to wait for it. Addons Manager uses nodecolector, whose elements don't have waitForElement methods so we have to use findElement.
This is a small fix that doesn't touch the library.

Comment 36

[Andrei Eftimie]

(In reply to Cosmin Malutan from comment #35)
> Created attachment 8515013 [details] [diff] [review]
> 931704.patch
> 
> The node is not ready at the moment we try to use it, so we have to wait for
> it. Addons Manager uses nodecolector, whose elements don't have
> waitForElement methods so we have to use findElement.
> This is a small fix that doesn't touch the library.

If this is indeed the problem, the solution would be to:
1) fix it in addons.js (by not using nodecollector) or
2) enhance nodecollector so it can wait for required elements to exist

If none of these work, let's fallback into fixing the test directly.

Comment 37

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

So why are entries not listed when the category has been changed? This sounds more like a problem in the correct event listener, or that it is getting fired too early while the list view is still loading.

Comment 38

[Cosmin Malutan, [:cosmin-malutan]]

Because the entry has nothing no do with the view state, it's appended by javascript.
Manualy:
1 Open AMO
2 Set Theme category
3 In a new tab open www.mozqa.com/data/firefox/addons/install.html?addon=themes/plain.jar
4 Install the theme
5 Switch the tab back to AMO, the new plugin got appended to the list.

Comment 39

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

Also here an event gets fired, which in this case seem to happen too soon. When you switch back to themes, it should already be part of the list.

Btw. AMO means addons.mozilla.org, while AOM meens add-on manager.

Comment 40

[Cosmin Malutan, [:cosmin-malutan]]

Unfortunately the element should be visible and usually is but when it fails it's not there.
It will be much more easier to wait for element that to add listeners for the ViewChanged events and to remove them, and in case element is added before we open the AOM it might not trigger at all.
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#752
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#2671
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#2616

Comment 41

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

What I want to say here is that I do not like workarounds around broken code which needs to be improved! So before we do what your patch is showing ensure that this behavior is wanted.

Comment 42

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #41)
> What I want to say here is that I do not like workarounds around broken code
> which needs to be improved! So before we do what your patch is showing
> ensure that this behavior is wanted.
The insertion addition is done an an dispatched event "onInstallEnded" so it's clearly asynchronously and it can happen right away after we open AOM, or before, either way there is nothing broken. 
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#417

(In reply to Andrei Eftimie from comment #36)
> If this is indeed the problem, the solution would be to:
> 1) fix it in addons.js (by not using nodecollector) or
> 2) enhance nodecollector so it can wait for required elements to exist
> 
> If none of these work, let's fallback into fixing the test directly.

2) Not feasible, nodeCollector creates elements with nodes not with selectors so it's basically impossible to fallback to waitForElement. Also we can't assert for elements to be found because this is how the api for nodecollector is expected, to return elements only if they are found and we might expect to don't find any, so that couldn't be touch.
1) Also modifying the addons, getElements will modify the api which is called in lots of places, some addons might be filter by different proprieties than attribute, also sometimes we expect more than one element so using findElement directly in the library will change the functionality that is expected in other tests.
http://hg.mozilla.org/qa/mozmill-tests/file/4da795d4e2e3/firefox/lib/addons.js#l1136  

I'd rather have this fixed in test, is simple is readable it won't affect other tests

Comment 43

[Andrei Eftimie]

We should think about this a bit more and weigh in pros/cons. I don't particularly like this spot-fix since the issue is probably manifesting itself in other places as well and we might be able to properly fix it (if we find a good overall solution)

Comment 44

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

Comment on attachment 8515013 [details] [diff] [review]
931704.patch

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

As Andrei mentioned, if we do changes it should happen by a addons module change, but not inside the test itself.

Comment 45

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.0

(In reply to Henrik Skupin (:whimboo) from comment #44)
> As Andrei mentioned, if we do changes it should happen by a addons module
> change, but not inside the test itself.
I see no easy way to change this in library without changing the tests too because as I mentioned in comment 42. 
Here I added an extra argument in API so it wont change old functionality, and a waitFor which will pass on the first loop in case that flag is not set!

Comment 46

[Cosmin Malutan, [:cosmin-malutan]]

Personally I would rather go with the first fix, using findelement in test. The only reason we have wrappers for nodes is so we can get instances even if the node doesn't exists and to have the ability to wait for it, this is the case. Changing addons module will change the expected behavior that we have for three methods, and there is no way we can use findElement to get elements in addon module because there are cases where we expect more than one element to be retrieved. 
* getAddons
* getElement
* getElements

Comment 47

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

Comment on attachment 8517427 [details] [diff] [review]
patch v2.0

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

::: firefox/tests/functional/restartTests/testAddons_changeTheme/test1.js
@@ +74,5 @@
>    });
>  
>    var plainTheme = addonsManager.getAddons({attribute: "value",
> +                                            value: persisted.theme[0].id,
> +                                            required: true})[0];

Why not simply call waitFor() here and wait until you have an existent element?

Comment 48

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.1

Thanks Henrik.

Comment 49

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

Comment on attachment 8517456 [details] [diff] [review]
patch v2.1

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

::: firefox/tests/functional/restartTests/testAddons_changeTheme/test1.js
@@ +77,5 @@
> +  assert.waitFor(() => {
> +    plainTheme = addonsManager.getAddons({attribute: "value",
> +                                          value: persisted.theme[0].id})[0];
> +    return !!plainTheme;
> +  }, "Addon has been found.");

Add-on is too general. Why not 'New installed theme...'?

Comment 50

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.0

Done

Comment 51

[Andrei Eftimie]

Comment on attachment 8517460 [details] [diff] [review]
patch v3.0

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

Great then, lets get this in:
https://hg.mozilla.org/qa/mozmill-tests/rev/c9c0b42bf8ec (default)

Comment 52

[Andrei Eftimie]

And transplanted on Aurora:
https://hg.mozilla.org/qa/mozmill-tests/rev/9c79066b0eec (mozilla-aurora)

Lets leave it at least until tomorrow before backporting it further.

Comment 53

[Andrei Eftimie]

All seems fine, lets transplant:
remote:   https://hg.mozilla.org/qa/mozmill-tests/rev/91500e181dbf (mozilla-beta)
remote:   https://hg.mozilla.org/qa/mozmill-tests/rev/b77eaeb32514 (mozilla-release)
remote:   https://hg.mozilla.org/qa/mozmill-tests/rev/d3f617ebaf0d (mozilla-esr31)

Thanks Cosmin!