Maniac Vlad Florin (:vladmaniac) Reporter
	Description • 12 years ago

Firefox versions: All branches
OS: all
Mozmill version: 1.5.17 
Error report on ci: http://mozmill-ci.blargon7.com/#/remote/report/29fc09ba0c8360d637617903a01672f1
Error log: 
DiscoveryPane_getInstallSource@resource://mozmill/stdlib/securable-module.js -> file:///c:/docume~1/mozilla/locals~1/temp/tmp99_jp9.mozmill-tests/lib/addons.js:1251 testInstallUpAndComingAddon@resource://mozmill/modules/frame.js -> file:///c:/docume~1/mozilla/locals~1/temp/tmp99_jp9.mozmill-tests/tests/remote/restartTests/testDiscoveryPane_UpAndComingModule/test1.js:49 @resource://mozmill/modules/frame.js:591 @resource://mozmill/modules/frame.js:661 @resource://mozmill/modules/frame.js:707 @resource://mozmill/modules/frame.js:540 @resource://mozmill/modules/frame.js:719 @resource://jsbridge/modules/server.js:179 @resource://jsbridge/modules/server.js:183 @resource://jsbridge/modules/server.js:283 
Error: 'aElement is undefined'

	Maniac Vlad Florin (:vladmaniac) Reporter
	Comment 1 • 12 years ago

Assigning myself since Andreea is already actively working on several bugs and I would like this investigated asap. 

We want to re-use this test and unskip as many remote tests as we can.

	Maniac Vlad Florin (:vladmaniac) Reporter
	Comment 2 • 12 years ago

This test installs an add-on randomly from the up-and coming list so the first thing I did was to check if all the extensions from the list are installing correctly - no errors were found. 
Currently I am running this particular test on a heavy load environment, but since the error is "aElement" I would really want to enable extension logging for this test and monitor jenkins master see what we get. 

If passing, we should have a report log which would look like this: 

*** LOG addons.xpi: Download started for https://addons.mozilla.org/firefox/downloads/latest/376972/addon-376972-latest.xpi?src=discovery-upandcoming to file /tmp/tmp-y7m.xpi
*** LOG addons.xpi: Download of https://addons.mozilla.org/firefox/downloads/latest/376972/addon-376972-latest.xpi?src=discovery-upandcoming completed.
*** LOG addons.repository: Requesting https://services.addons.mozilla.org/en-US/firefox/api/1.5/search/guid:%7Bd4e0dc9c-c356-438e-afbe-dca439f4399d%7D?src=firefox&appOS=Linux&appVersion=17.0a1
*** LOG addons.xpi: Starting install of https://addons.mozilla.org/firefox/downloads/latest/376972/addon-376972-latest.xpi?src=discovery-upandcoming
*** LOG addons.xpi: Addon {d4e0dc9c-c356-438e-afbe-dca439f4399d} will be installed as an unpacked directory
*** LOG addons.xpi: Install of https://addons.mozilla.org/firefox/downloads/latest/376972/addon-376972-latest.xpi?src=discovery-upandcoming completed.

	Maniac Vlad Florin (:vladmaniac) Reporter
	Comment 3 • 12 years ago

* enables extension logging which would be very useful for debugging

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 4 • 12 years ago

Comment on attachment 651332 [details] [diff] [review]
enable extension logging patch v.10

This will not help. If you check the stack of the failure you will see that we fail here:

    47   // Install the addon

    48   var addToFirefox = discovery.getElement({type: "addon_installButton"});

>   49   var currentInstallSource = discovery.getInstallSource(addToFirefox);

	Maniac Vlad Florin (:vladmaniac) Reporter
	Comment 5 • 12 years ago

Yes - the extension page loads lazily in the discovery pane. Its easy to see this manually - just perform the manual steps of the test. 

I am going to check if waiting for the element helps in any way

	Maniac Vlad Florin (:vladmaniac) Reporter
	Comment 6 • 12 years ago

* added waitForElement(addToFirefox); 
* removed existent trailing whitespaces in the patch

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 7 • 12 years ago

Comment on attachment 651654 [details] [diff] [review]
patch v1.1 [backed-out]

Sounds reasonable. Lets try this out.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 8 • 12 years ago

http://hg.mozilla.org/qa/mozmill-tests/rev/4acb7f79b5f6

Lets see how it behaves. Please check the reports in the next couple of days. If we are green, we should backport the patch.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 9 • 12 years ago

Vlad, please monitor your own fixes. This one is not going to work yet. It's failing again just slightly different:

http://mozmill-ci.blargon7.com/#/remote/report/d87d47fd1034f072b9bece6ee635e911

    elem is undefined

	Maniac Vlad Florin (:vladmaniac) Reporter
	Comment 10 • 12 years ago

(In reply to Henrik Skupin (:whimboo) from comment #9)
> Vlad, please monitor your own fixes. 

I am - please keep in mind that all of us had a national day on the 15th

	Maniac Vlad Florin (:vladmaniac) Reporter
	Comment 11 • 12 years ago

(In reply to Henrik Skupin (:whimboo) from comment #9)
> Vlad, please monitor your own fixes. This one is not going to work yet. It's
> failing again just slightly different:
> 
> http://mozmill-ci.blargon7.com/#/remote/report/
> d87d47fd1034f072b9bece6ee635e911
> 
>     elem is undefined

Normally I would think the default timeout should be enough here, but since this is intermittent and I cannot reproduce this locally, I can only suggest that we set a bigger timeout.

	Maniac Vlad Florin (:vladmaniac) Reporter
	Comment 12 • 12 years ago

* Adding extra timeout

	Dave Hunt [:davehunt] [he/him] ⌚BST
	Comment 13 • 12 years ago

If this was a timeout I would expect the failure to be "Timeout exceeded for waitForElement..."

I suspect perhaps that discovery.getElement({type: "addon_installButton"}) is returning undefined.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 14 • 12 years ago

(In reply to Dave Hunt (:davehunt) from comment #13)
> I suspect perhaps that discovery.getElement({type: "addon_installButton"})
> is returning undefined.

Most likely. Especially nodeCollector doesn't work with dynamically created pages which dynamically inject nodes. This could be the reason here. Sorry that I missed that.

At some point we need that feature in nodeCollector so I would propose we add that. Vlad, mind filing a bug on it?

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 15 • 12 years ago

Comment on attachment 651654 [details] [diff] [review]
patch v1.1 [backed-out]

Backed out this patch due to it's a noop:
http://hg.mozilla.org/qa/mozmill-tests/rev/fad47411458c

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 16 • 12 years ago

Comment on attachment 652337 [details] [diff] [review]
increase timeout v1.0

This is not going to work. Questions beside. Is the install button visible when the pane has been loaded or do you have to hover over an item?

	Maniac Vlad Florin (:vladmaniac) Reporter
	Comment 17 • 12 years ago

Added bug 783490 and marked as blocking for this one. 
Henrik, can you please add the required whiteboard entries as I'm not sure about which would be needed there.

	Maniac Vlad Florin (:vladmaniac) Reporter
	Comment 18 • 12 years ago

(In reply to Henrik Skupin (:whimboo) from comment #16)
> Comment on attachment 652337 [details] [diff] [review]
> increase timeout v1.0
> 
> This is not going to work. Questions beside. Is the install button visible
> when the pane has been loaded or do you have to hover over an item?

No hovering, but you have to wait for the element. Its clearly visible that its loaded lazily in the page.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 19 • 12 years ago

We cold go with a sleep, but I would say lets disable this test for now until the nodeCollector bug has been fixed.

	Maniac Vlad Florin (:vladmaniac) Reporter
	Comment 20 • 12 years ago

* disabling the test as to Henrik's suggestion

	Maniac Vlad Florin (:vladmaniac) Reporter
	Comment 21 • 12 years ago

* esr branch is affected looking at the flags, so adding a patch for esr also

	Dave Hunt [:davehunt] [he/him] ⌚BST
	Comment 22 • 12 years ago

Comment on attachment 652705 [details] [diff] [review]
[mozilla-esr10][skip patch] disable test v1.0

>Bug 780556 - Disable the test due to 'elem is undefined' error. r=hskupin, r=davehunt

This should be dhunt. Other than that I don't see any difference between this patch and attachment 652704 [details] [diff] [review]. Is a patch needed for ESR?

	Dave Hunt [:davehunt] [he/him] ⌚BST
	Comment 23 • 12 years ago

Comment on attachment 652704 [details] [diff] [review]
[default][skip patch] disable test v1.0

Please also disable the test in the manifest file.

	Maniac Vlad Florin (:vladmaniac) Reporter
	Comment 24 • 12 years ago

* fixed to disable the test in the manifest also

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 26 • 12 years ago

Comment on attachment 653257 [details] [diff] [review]
[default][skip patch] disable test v1.1

>+// XXX: Bug 780556
>+//      Skip because of 'elem is undefined' error

Why such a comment? All the information we need is in the skip message already.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 27 • 12 years ago

Comment on attachment 653258 [details] [diff] [review]
[mozilla-esr10][skip patch] disable test v1.1

Same for the esr10 patch.

	Maniac Vlad Florin (:vladmaniac) Reporter
	Comment 28 • 12 years ago

* removed comment 

I agree its redundant, but used it for consistency. We used to have a practice in adding an //XXX: with the bug details before skipping the test.

	Maniac Vlad Florin (:vladmaniac) Reporter
	Comment 29 • 12 years ago

* killed redundant comment

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 30 • 12 years ago

Comment on attachment 653677 [details] [diff] [review]
disable test v1.2 [checked-in]

Just a note, please be more specific with the commit messages in the future. Means what the patch is doing. Using the bug summary is not always a good idea.

http://hg.mozilla.org/qa/mozmill-tests/rev/6b74e72c1080 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/ed13a0dafd1b (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/2f4760268961 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/19d7a25e2e09 (release)

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 31 • 12 years ago

Comment on attachment 653678 [details] [diff] [review]
disable test v1.2 [mozilla-esr10][checked-in]

http://hg.mozilla.org/qa/mozmill-tests/rev/a7b933926e9c (esr10)

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 32 • 10 years ago

Cosmin, will this be fixed with the patch on bug 732353?

	Cosmin Malutan, [:cosmin-malutan]
	Comment 33 • 10 years ago

Yes, it will, I'll mark this as dependent.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 34 • 10 years ago

No need to add this as dependency. We can dupe against bug 732353.