Closed Bug 931704 Opened 9 years ago Closed 8 years ago

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

Categories

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

x86
All
defect

Tracking

(firefox32 wontfix, firefox33 fixed, firefox34 fixed, firefox35 fixed, firefox36 fixed, firefox-esr24 wontfix, firefox-esr31 fixed)

RESOLVED FIXED
Tracking Status
firefox32 --- wontfix
firefox33 --- fixed
firefox34 --- fixed
firefox35 --- fixed
firefox36 --- fixed
firefox-esr24 --- wontfix
firefox-esr31 --- fixed

People

(Reporter: mario.garbi, Assigned: cosmin-malutan)

References

()

Details

(Whiteboard: [mozmill-test-failure])

Attachments

(2 files, 7 obsolete files)

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
I reproduced this failure locally. It reproduces every time, I will have a look and return with more info.
Assignee: nobody → cosmin.malutan
Status: NEW → ASSIGNED
Priority: -- → P1
If this is a regression we want a regression range.
(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.
Priority: P1 → P4
Happened again today on Windows NT 6.2.9200 (x86) with Firefox 27.0a2 de:
http://mozmill-daily.blargon7.com/#/functional/report/8a42e4b8f277f9c0268b061eb8a5c7eb
I ran the test for about 40 times on the affected machine and it did't reproduced.
Happened again on Windows NT 6.2.9200 (x86_64) with Firefox 27.0a2 en-US:
http://mozmill-daily.blargon7.com/#/functional/report/8a42e4b8f277f9c0268b061eb8eeda6b
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.
Priority: P4 → P2
This is by far not a P2. It's intermittent and happening very rarely.
Priority: P2 → P3
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.
Attached patch skip_patch (obsolete) — Splinter Review
Hi Andreea, here is the skip patch.
Attachment #831526 - Flags: review?(andreea.matei)
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.
Attachment #831526 - Flags: review?(andreea.matei) → review-
Attached patch skip_patch (obsolete) — Splinter Review
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
Attachment #831526 - Attachment is obsolete: true
Attachment #832119 - Flags: review?(andreea.matei)
Attached patch skip_patch (obsolete) — Splinter Review
I missed to skip the tests too.
Attachment #832119 - Attachment is obsolete: true
Attachment #832119 - Flags: review?(andreea.matei)
Attachment #832134 - Flags: review?(andreea.matei)
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)
Attachment #832134 - Flags: review?(andreea.matei) → review+
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][mozmill-test-skipped]
(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.
Priority: P3 → P2
Attached patch skip_patch (obsolete) — Splinter Review
I removed the name of the test from the skip message.
Attachment #832134 - Attachment is obsolete: true
Attachment #8388405 - Flags: review?(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?
Attachment #8388405 - Flags: review?(andrei.eftimie) → review-
Comment on attachment 8388405 [details] [diff] [review]
skip_patch

Yep, to many tabs opened.
Attachment #8388405 - Attachment is obsolete: true
This fails about twice at every 100 testruns. It didn't failed when I ran the test via manifest file but in complete testrun!
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.
Sounds good. We should try it out on default for about a week.
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.
Attachment #8405428 - Flags: review?(andrei.eftimie)
Attachment #8405428 - Flags: review?(andreea.matei)
Attachment #8405428 - Flags: review+
Attachment #8405428 - Flags: checkin+
Patch applies cleanly on all branches, on esr24 the test is not disabled.
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)
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [mozmill-test-failure][mozmill-test-skipped] → [mozmill-test-failure]
Seems this is not fixed yet. Only 1 occurrence in quite a long while:
http://mozmill-daily.blargon7.com/#/functional/report/ca725ca957252135fc72c3c08f305dae
Status: RESOLVED → REOPENED
Priority: P2 → P3
Resolution: FIXED → ---
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
(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
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>
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.
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...
Failed 12 times today with Beta, Windows mainly, several locales.
Attached patch 931704.patch (obsolete) — Splinter Review
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.
Attachment #8515013 - Flags: review?(andrei.eftimie)
Attachment #8515013 - Flags: review?(andreea.matei)
(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.
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.
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.
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.
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
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.
(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
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 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.
Attachment #8515013 - Flags: review?(andrei.eftimie)
Attachment #8515013 - Flags: review?(andreea.matei)
Attachment #8515013 - Flags: review-
Attached patch patch v2.0 (obsolete) — Splinter Review
(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!
Attachment #8517427 - Flags: feedback?(hskupin)
Attachment #8517427 - Flags: feedback?(andrei.eftimie)
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 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?
Attachment #8517427 - Flags: feedback?(hskupin) → feedback-
Attached patch patch v2.1 (obsolete) — Splinter Review
Thanks Henrik.
Attachment #8515013 - Attachment is obsolete: true
Attachment #8517427 - Attachment is obsolete: true
Attachment #8517427 - Flags: feedback?(andrei.eftimie)
Attachment #8517456 - Flags: review?(andrei.eftimie)
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...'?
Attached patch patch v3.0Splinter Review
Done
Attachment #8517456 - Attachment is obsolete: true
Attachment #8517456 - Flags: review?(andrei.eftimie)
Attachment #8517460 - Flags: review?(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)
Attachment #8517460 - Flags: review?(andrei.eftimie)
Attachment #8517460 - Flags: review+
Attachment #8517460 - Flags: checkin+
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.
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!
Status: REOPENED → RESOLVED
Closed: 9 years ago8 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.