Closed Bug 688375 Opened 13 years ago Closed 12 years ago

Test failure "Add-on not specified" in testAddons_enableDisableExtension

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: vladmaniac, Assigned: vladmaniac)

References

()

Details

(Whiteboard: [mozmill-test-failure])

Attachments

(2 files, 5 obsolete files)

MODULE: tests/functional/restartTests/testAddons_enableDisableExtension
TEST: test4.js
FAIL: AddonsManager_isAddonEnabled([object Proxy])@resource://mozmill/stdlib/securable-module.js -> file:///c:/docume~1/mozilla/locals~1/temp/tmpzsqjzy.mozmill-tests/lib/addons.js:322 

BRANCH: mozilla-aurora
Assignee: nobody → vlad.mozbugs
Status: NEW → ASSIGNED
vlad.maniac changed story state to started in Pivotal Tracker
Whiteboard: [mozmill-test-failure]
Attached file results of investigation (obsolete) —
Results of investigation: 

Ran the test several times on the given environment configuration and firefox aurora build. I can't reproduce the error. 

The following line of code produces the failure:

assert.ok(addonsManager.isAddonEnabled({addon: addon}), "The addon is enabled"); 

Inspected addons.js for isAddonEnabled method - this should not cause the failure 

The only reason I can think of is that add-ons manager was loaded lazily in test4.js and causes the error from above. 

I am starting to think that running tests on VMs affects timing performance somehow, because many failures we have been dealing with so far were not reproducible on our side and the only difference is that we are not running tests on VMs. 

Does anything from above make any sense to you?
What happens when you trigger this test using the testrun scripts (ie. not just executing the test itself)?
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #2)
> The following line of code produces the failure:
> 
> assert.ok(addonsManager.isAddonEnabled({addon: addon}), "The addon is
> enabled"); 

This message doesn't correlate to the message you have used in the summary. So why it should be this line which is failing? Can you please add the link to the failure details view on brasstacks to the URL field? Thanks.
(In reply to Henrik Skupin (:whimboo) from comment #4)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #2)
> > The following line of code produces the failure:
> > 
> > assert.ok(addonsManager.isAddonEnabled({addon: addon}), "The addon is
> > enabled"); 
> 
> This message doesn't correlate to the message you have used in the summary.
> So why it should be this line which is failing? Can you please add the link
> to the failure details view on brasstacks to the URL field? Thanks.

'addon' from assert.ok(addonsManager.isAddonEnabled({addon: addon}), "The addon is enabled"); is null and above the code line is addonManager.open(); that is why I tend to think that the add-ons Manager was loaded lazily and had no time to retrieve the desired add-on. 

The line for retrieving the add-on is 'var addon = addonsManager.getAddons({attribute: "value",  value: persisted.addon.id})[0];'

The entire code of the test function in test4.js (the tests which fails) is 

function testEnabledAddon() {
  addonsManager.open();

  // Get the addon by name 
  var addon = addonsManager.getAddons({attribute: "value", 
                                       value: persisted.addon.id})[0];

  // Check if the addon is enabled
  assert.ok(addonsManager.isAddonEnabled({addon: addon}), "The addon is enabled");   
}
                                      '
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #3)
> What happens when you trigger this test using the testrun scripts (ie. not
> just executing the test itself)?

http://mozmill-crowd.brasstacks.mozilla.com/db/df4c8750af577eceac3393a5eb8b4859

http://mozmill-crowd.brasstacks.mozilla.com/#/functional/top?branch=All&platform=All&from=2011-09-23&to=2011-09-23

Doesn't fail on Linux. Need to go further and set up env for windows XP
Well it seems like the failure is reproducible only on win 2000 
We also have a similar situation with a test failing on win 2000 only: bug 685854 

My proposal: 
Skip the patch in setup module if OS == win2000. value of OS can be retrieved using utils.js help. 
Your comments are crucial here, thanks!
This failed once yesterday on 9.0a1 WinXP, so it's not only Win2000.

Can you investigate whether this is reproducible manually? ie. make sure we don't have an AOM or AMO regression.
It's not reproducible manually at all. I don't know why it could fail only on earlier versions of windows. I won't skip it for now, because it's a challenge for me to come up with an answer
This is failing on a daily basis. So for now, let's skip it on all platforms while we investigate the fix.
OS: Linux → All
Hardware: x86 → All
Attached patch skip test (obsolete) — Splinter Review
Skips the test.
Attachment #563343 - Flags: review?(alex.lakatos)
Comment on attachment 563343 [details] [diff] [review]
skip test

I would prefer the skip message to keep the error in some sort of quote marks. use :
>+// Bug 688375 - Test failure "Add-on not specified" in testAddons_enableDisableExtension
>+setupModule.__force_skip__ = "Bug 688375 - Test failure 'Add-on not " +
>+                             "specified' in testAddons_enableDisableExtension";
Attachment #563343 - Flags: review?(alex.lakatos) → review-
Comment on attachment 563343 [details] [diff] [review]
skip test

Also, please change the commit message
Updated as requested.
Attachment #563343 - Attachment is obsolete: true
Attachment #563372 - Flags: review?(alex.lakatos)
Attachment #563372 - Flags: review?(anthony.s.hughes)
Attachment #563372 - Flags: review?(alex.lakatos)
Attachment #563372 - Flags: review+
In some cases we also get: The addon is enabled - got 'false'.

Anthony, please check that test on qa-horus.
Attachment #563372 - Flags: review?(anthony.s.hughes) → review+
Comment on attachment 563372 [details] [diff] [review]
skip test v2 [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/9589c25c8039 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/727d2f3b46ec (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/ab9bdfcce6fb (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/09722bcd0b5a (mozilla-release)
Attachment #563372 - Attachment description: skip test v2 → skip test v2 [checked-in]
Test has now been skipped. I will do some quick debugging early next week for this test on qa-horus.
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][mozmill-test-skipped]
Attached patch new patch (obsolete) — Splinter Review
Can checkin this patch see if it does any good?
It has 2 extra checks - maybe we'll get more specific failures if the test will fail again 

Thanks
Attachment #571940 - Flags: review?(hskupin)
Comment on attachment 571940 [details] [diff] [review]
new patch

We don't check-in patches for testing purposes. You know have access to qa-horus and we could try to reproduce this issue on this box. It's still an outstanding task.
Attachment #571940 - Flags: review?(hskupin)
This could also fail if the add-on does not install properly, by that meaning "Modal dialog has been found and processed" error.
Normally this should fail and the rest of the test files should not be run. I am looking right now at a similar fail. test1 fails with "Modal dialog has been found and processed " and test2 fails with "AddonsManager_isAddonInstalled: Add-on not specified.". I got this from: http://mozmill-release.brasstacks.mozilla.com/#/functional/report/4eb0d6a0badb9e4011dcd54b315b6a80
Remus, please only concentrate on the first failure which occurs. We can't stop Mozmill right now from executing tests in additional restart test modules. It's something which should be fixed already in Mozmill 2 and we can make advantage of, once we can upgrade to that version.
Proposed fix. 

We are now waiting for the add-on list to be completed in the add-ons manager. we are going to stop failing with "Add-on not specified" unless the add-on is really not present in the list. 

The method should be in fact in the Shared modules, specifically in addons.js but I want to see this test passing on qa-set before changing a shared module.
Attachment #575198 - Flags: review?(anthony.s.hughes)
Attachment #571940 - Attachment is obsolete: true
Comment on attachment 575198 [details] [diff] [review]
fix patch v1.0 - initial proposed fix

If the addonsManager open() function which already exists in the Shared Module does not check for this, it should.

Asking for feedback from Henrik.
Attachment #575198 - Flags: review?(anthony.s.hughes) → feedback?(hskupin)
Comment on attachment 575198 [details] [diff] [review]
fix patch v1.0 - initial proposed fix

I'm not sure why we need this. We are waiting for the event sent from the category view, which states that the view has been completed loading. Every add-on should be listed there. Also using last-child is wrong because the installed add-on doesn't have to be the last one in the list.

Can you reproduce this issue all the time on any of your boxes? Whether which platform is in use? I would need steps to be able to reproduce it myself otherwise.
Attachment #575198 - Flags: feedback?(hskupin) → feedback-
(In reply to Henrik Skupin (:whimboo) from comment #25)
> Comment on attachment 575198 [details] [diff] [review] [diff] [details] [review]
> fix patch v1.0 - initial proposed fix
> 
> I'm not sure why we need this. We are waiting for the event sent from the
> category view, which states that the view has been completed loading. Every
> add-on should be listed there

Are we really sure that this is enough? I don't know for sure if waiting for the category implies waiting for the add-on list to be complete

. Also using last-child is wrong because the
> installed add-on doesn't have to be the last one in the list.
> 

It is not wrong because I wait for the entire add-on list to be complete, not only my installed add-on so I don't care about the any add-on position - just want the complete list.
Because the list loads progressively, when the last add-on is there, it is certain that there are all there. 

> Can you reproduce this issue all the time on any of your boxes? Whether
> which platform is in use? I would need steps to be able to reproduce it
> myself otherwise.

The issue is sometimes reproducible, not always, and not it is not platform related - it has been seen on all platforms sporadic. For those reasons, the only fix we can come up with is this one, or just skip the patch 

I vote for trying it. :)
Ah yes, sorry - this patch is already skipped
(In reply to Henrik Skupin (:whimboo) from comment #25)
> I'm not sure why we need this. We are waiting for the event sent from the
> category view, which states that the view has been completed loading. Every
> add-on should be listed there. Also using last-child is wrong because the
> installed add-on doesn't have to be the last one in the list.

Dave, can we please get feedback from you on when the 'ViewChanged' event gets fired? As far as I got when implementing our API for the Mozmill tests, the event will happen when the selected view has been completed loading, which also involves that any add-on in that list is also visible.
(In reply to Henrik Skupin (:whimboo) from comment #28)
> (In reply to Henrik Skupin (:whimboo) from comment #25)
> > I'm not sure why we need this. We are waiting for the event sent from the
> > category view, which states that the view has been completed loading. Every
> > add-on should be listed there. Also using last-child is wrong because the
> > installed add-on doesn't have to be the last one in the list.
> 
> Dave, can we please get feedback from you on when the 'ViewChanged' event
> gets fired? As far as I got when implementing our API for the Mozmill tests,
> the event will happen when the selected view has been completed loading,
> which also involves that any add-on in that list is also visible.

Yep, when ViewChanged fires the UI should be completely updated, if its a view of a list of add-ons then they should all be in the DOM, if it's a details view then it should be fully filled out and if it is the get add-ons page then it should be fully loaded.
Thanks Dave for your feedback. That makes me confident. Vlad, please update the patch and remove this extra waitFor. If it doesn't work upload a new patch and ask for deeper feedback. Probably Geo would have to check this because I'm on vacation starting this Friday.

Otherwise please use dump() calls to debug the test and print the current states to the console. That always helps a lot.
Depends on: 715022
Henrik's patch will also fix this - I say we un-skip this test
Attached patch unskip test patch v1.0 (obsolete) — Splinter Review
i propose to unskip this test, as dependency is fixed. 

feel free to - the patch if otherwise.
Attachment #561680 - Attachment is obsolete: true
Attachment #563372 - Attachment is obsolete: true
Attachment #575198 - Attachment is obsolete: true
Attachment #586039 - Flags: review?(anthony.s.hughes)
Comment on attachment 563372 [details] [diff] [review]
skip test v2 [checked-in]

Unmarking obsolete. Let's not mark patches which have been checked in as obsolete. If the code is checked in it is not obsolete, by definition.
Attachment #563372 - Attachment is obsolete: false
Please don't get sloppy with your commit messages. Uploading a patch with a proper message.
Attachment #586039 - Attachment is obsolete: true
Attachment #586039 - Flags: review?(anthony.s.hughes)
Attachment #587075 - Flags: review+
Comment on attachment 587075 [details] [diff] [review]
re-enable test [checked-in]

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

Will port to other branches assuming default testrun passes tomorrow. Vlad, please check the results and report here.
Attachment #587075 - Attachment description: re-enable test → re-enable test [checked-in: default]
This test is not failing any more - Anthony, can you please check in for the rest of Firefox branches? 

http://mozmill-release.blargon7.com/#/functional

Thanks
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #37)
> This test is not failing any more - Anthony, can you please check in for the
> rest of Firefox branches? 
> 
> http://mozmill-release.blargon7.com/#/functional
> 
> Thanks

This got merged forward with the default->mozilla-aurora merge yesterday. I'll go ahead and land this on beta and release.
Comment on attachment 587075 [details] [diff] [review]
re-enable test [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/7546dedccb26 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/1db64f904a17 (mozilla-release)
Attachment #587075 - Attachment description: re-enable test [checked-in: default] → re-enable test [checked-in]
Going to call this fixed by bug 715022.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [mozmill-test-failure][mozmill-test-skipped] → [mozmill-test-failure]
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: