738268 - Failure in testIdentityPopupOpenClose | Identity popup has been closed

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Remus Pop (:RemusPop)]

Attachment: screenshot of fail

This is a problem that appears only on Mac and it's been around for some time.
I've investigated this and the only case in which the test fails is when the Firefox window does not have focus. I don't know how it loses it, but by manually reproducing it I mean manually move the focus to another window.
When running the tests the whole folder was given as a -t parameter

I also attached a screen capture in which you can clearly see the focus is on terminal.

Comment 1

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

This bug report misses information like the version of Firefox. Also please add the link to the failing tests.

Comment 2

[Remus Pop (:RemusPop)]

This is failing on beta, as we can see in the dashboard. But as I expected, manually moving focus to another window/desktop when testing has the same error on all versions of Firefox.

Comment 3

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

So the click in the content area is probably the reason for. Can we change it to send an ESC key event? That would have to work even if Firefox doesn't have the focus.

Comment 4

[Remus Pop (:RemusPop)]

Attachment: patch v1 (all branches)

Two words: works great; so here is a patch.

Comment 5

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

Comment on attachment 608651 [details] [diff] [review]
patch v1 (all branches)

Don't use null as reference element but the popup you want to close.

Comment 6

[Remus Pop (:RemusPop)]

Attachment: fix v2 [landed]

Using the popup now.

Comment 7

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 608669 [details] [diff] [review]
fix v2 [landed]

++

Comment 8

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

Comment on attachment 608669 [details] [diff] [review]
fix v2 [landed]

Code wise ok, but I totally don't see a reason why we have to add a comment here. It's totally self-explaining.

Comment 9

[u279076]

Comment on attachment 608669 [details] [diff] [review]
fix v2 [landed]

>+  // Press Escape to close the popup
>+  controller.keypress(popup, 'VK_ESCAPE', {});

While the "Press Escape" part is self explanatory, I don't think the "close the popup" part is necessarily self explanatory (especially for complete newcomers). I would suggest rewording the comment to simply:

> // Close the Identity notification

Though it's so trivial, I would not block check-in for this change.

Comment 10

[u279076]

> http://mozmill-release.blargon7.com/#/functional/failure?branch=All&platform=All&from=2012-01-01&to=2012-03-23&test=%2FtestSecurity%2FtestIdentityPopupOpenClose.js&func=testIdentityPopupOpenClose.js%3A%3AtestIdentityPopupOpenClose

This failure has only happened 3 times in the last 3 months (once on 14.0a1, twice on 12.0beta). Given that, I'll check this in on all branches but we wont be able to verify this easily. Best case, we monitor testruns and reopen if this happens again in the future.

Comment 11

[u279076]

Comment on attachment 608669 [details] [diff] [review]
fix v2 [landed]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/ac8432c23f55 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/2f6fcf5f558a (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/42ff47e2bd60 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/8697a2cee272 (mozilla-release)

Comment 12

[u279076]

If there are no failures by Monday, please mark this verified.

Henrik, I think this change is probably safe to push to esr10 -- what do you think?

Comment 13

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

Sure. Go ahead

Comment 14

[u279076]

Comment on attachment 608669 [details] [diff] [review]
fix v2 [landed]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/2f3a11b72707 (mozilla-esr10)