Failure in testIdentityPopupOpenClose | Identity popup has been closed

VERIFIED FIXED

Status

VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: remus.pop, Assigned: remus.pop)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozmill-test-failure], URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

7 years ago
Created attachment 608339 [details]
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.
This bug report misses information like the version of Firefox. Also please add the link to the failing tests.
(Assignee)

Comment 2

7 years ago
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.
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.
(Assignee)

Comment 4

7 years ago
Created attachment 608651 [details] [diff] [review]
patch v1 (all branches)

Two words: works great; so here is a patch.
Assignee: nobody → remus.pop
Status: NEW → ASSIGNED
Attachment #608651 - Flags: review?(vlad.mozbugs)
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.
Attachment #608651 - Flags: review?(vlad.mozbugs) → review-
(Assignee)

Comment 6

7 years ago
Created attachment 608669 [details] [diff] [review]
fix v2 [landed]

Using the popup now.
Attachment #608651 - Attachment is obsolete: true
Attachment #608669 - Flags: review?(vlad.mozbugs)
Attachment #608669 - Flags: feedback?(hskupin)
Comment on attachment 608669 [details] [diff] [review]
fix v2 [landed]

++
Attachment #608669 - Flags: review?(vlad.mozbugs) → review+
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.
Attachment #608669 - Flags: feedback?(hskupin) → feedback+
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.
> 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.
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?
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Sure. Go ahead
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.