882068 - Test failure "linkId.getNode(...) is null" in testSessionStore/testUndoTabFromContextMenu.js with Mozmill 2.0

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Andreea Matei [:AndreeaMatei]]

This is failing with the current state of Mozmill 2.0, I've tested it on Linux and OS X, default branch. I will check windows as well.

* http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbecae4d7
* http://mozmill-crowd.blargon7.com/#/functional/report/8aa237803fd4b0e7348adc8bbecaabde

Comment 1

[Andreea Matei [:AndreeaMatei]]

It's reproducing on Windows as well. It might be a regression somehow, it works  with builds previous to 8th June.

Good build: ftp://ftp.mozilla.org/pub/firefox/nightly/2013-06-07-03-10-55-mozilla-central
Bad build: ftp://ftp.mozilla.org/pub/firefox/nightly/2013-06-08-03-12-12-mozilla-central

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dc8e78ed8c44&tochange=6006abb39d8e

Comment 2

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

Do we have a more close pushlog with tinderbox builds? Also can this be a regression in Mozmill with the latest landings? Might be worth to check that too.

Comment 3

[Andreea Matei [:AndreeaMatei]]

I will provide soon a more close pushlog.

I don't think it's a regression in mozmill, I tested with an older version of Mozmill (which has the latest push in 22 May) and it's still failing with the latest nightly, while with the build from 7th June it passes.

Comment 4

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

But why it's not failing with Mozmill 1.5? That's the question here.

Comment 5

[Andreea Matei [:AndreeaMatei]]

So I've found this responsible changeset with hg bisect:
http://hg.mozilla.org/mozilla-central/rev/73483f4906a7

And the related tinderbox build for linux 32bit:
ftp://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-linux/1370617546

I figure is the event used now, they've replaced "pageshow" with "load" there. I know we're using this in windows.js in 2.0, but I'm not sure why only that test is failing.

Comment 6

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

Thank you Andreea. That's good to know. Could you provide a simplified mozmill test which showcases this issue?

Comment 7

[Andreea Matei [:AndreeaMatei]]

Attachment: testcase

Here's a simplified testcase. I have removed the code that it's not relevant, found out that selecting a tab and closing it does not interfere in reproducing the issue, neither the empty tab that remains opened from the for loop.
This means that even if we close that empty tab and no longer need to select the second one cause we're automatically there, we also fails to get the ID.

Here we open 2 tabs, select the second one and check the ID there.

Comment 8

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v1

This resulted to be a timing issue, using a small sleep it passed, in order to finish the animation.
So I've found the difference between bad and good behavior is in the innerHTML code. When we're not waiting enough, the label property from innerHTML of the tab will say "Connecting..". When it's done the label will show the index number.

Reports 1.5:
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/206c36485a655b168e7426423731dbdb
OS X: http://mozmill-crowd.blargon7.com/#/functional/report/206c36485a655b168e7426423731f0f2
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/206c36485a655b168e7426423734b128

Reports 2.0:
Linux: I wasn't able to finish it due to that dialog, but I ran the tests that use selectedIndex individually.
OS X: http://mozmill-crowd.blargon7.com/#/functional/report/206c36485a655b168e7426423731d3b4
Windows: I wasn't able to finish it due to the IO completion port error

Comment 9

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

Comment on attachment 773181 [details] [diff] [review]
patch v1

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

::: lib/tabs.js
@@ +129,5 @@
>    set selectedIndex(index) {
>      this._controller.click(this.getTab(index));
>      assert.waitFor(function () {
> +      return this.selectedIndex === index &&
> +             this._tabs.getNode().innerHTML.indexOf('label="Connecting…"') === -1;

Sorry, but here I have to say ouch. Why do we have to do that? I really don't like that.

If we need something special for sessionstore why we can't call waitForPageLoad() in the test? It's exactly what seems to be necessary here.

Comment 10

[Andreea Matei [:AndreeaMatei]]

Attachment: testcase

Hm, it seems I've overcomplicated the id problem, a simple waitForElement() fixes that. The real issue is that after closing a tab, we don't get the count up in the "Recently closed tabs" and neither the closed tab data from nsISessionStore.

It's not related to selectedIndex but it's due to the switching to another tab. As explained before, we load the page but somehow the innerHTML shows the transition from "Connecting" to the right label isn't over. Also I see a "busy" property when that happens.

Strange that 1.5 doesn't seem to have this problem.

Comment 11

[Andreea Matei [:AndreeaMatei]]

Attachment: dumps

Comment 12

[Andreea Matei [:AndreeaMatei]]

Actually, it's not reproducing anymore with the waitForPageLoad() fix from bug 865641.

http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc94714f763

Comment 13

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

This is not a wontfix here. It actually got fixed by my patch on bug 865641.