Element failure in testPasswordNotificationBar

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: ashughes, Assigned: ashughes)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozmill-branch-fail][mozmill-test-failure][will be fixed by 610849])

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
MODULE: testPasswordManager/testPasswordNotSaved.js
TEST: testPasswordNotificationBar
ERROR: Unexpectedly found element Lookup: /id("main-window")/id("browser")/id("appcontent")/id("content")/anon({"anonid":"tabbox"})/anon({"anonid":"panelcontainer"})/{"id":"panel12885412035241"}/{"value":"password-save"}/anon({"type":"info"})/{"class":"messageCloseButton tabbable"}
BRANCH: mozilla1.9.1, mozilla1.9.2
PLATFORM: All
(Assignee)

Updated

7 years ago
Blocks: 604718
Whiteboard: [mozmill-branch-fail]
(Assignee)

Updated

7 years ago
Whiteboard: [mozmill-branch-fail] → [mozmill-branch-fail][mozmill-test-failure]
(Assignee)

Updated

7 years ago
Assignee: nobody → anthony.s.hughes
Status: NEW → ASSIGNED
(Assignee)

Comment 1

7 years ago
Created attachment 488082 [details] [diff] [review]
WIP Patch

Here is a work in progress patch.  The cause of the failure was found to be that the close button was not being clicked.  This exposes a case where controller.click offset/2 is not 100% effective.

In the case of the close button, the width and height are 16 and 18 respectively.  This results in a default offset of 8,9 which fails to click the button.  Passing in an offset of 8, 10 fixes the test.

I've also done some minor refactoring which should make the test more reliable.  This may have implications for other notification bars.

Henrik, I'd appreciate feedback on all of this.  Thanks.
Attachment #488082 - Flags: feedback?(hskupin)
Comment on attachment 488082 [details] [diff] [review]
WIP Patch

>-  // After logging in, close the notification bar
>-  var button = tabBrowser.getTabPanelElement(tabBrowser.selectedIndex,
>-                                             '/{"value":"password-save"}/anon({"type":"info"})' +
>-                                             '/{"class":"messageCloseButton tabbable"}');
>-
>-  // The notification bar should not be visible before submitting the credentials
>-  controller.assertNodeNotExist(button);

We should leave this check to make sure that the panel doesn't appear before hitting submit.

>+  // Verify the password notification bar has appeared
>+  var tabPanel = tabBrowser.getTabPanelElement(tabBrowser.selectedIndex);
[..]
>+  controller.waitFor(function() {
>+    return tabPanel.getNode().currentNotification.value == 'password-save';
>+  }, "Save Password notification bar is not found.");

Would have been nice if we could directly work with the '/{"value":"password-save"}' child element and call waitForElement. But it would conflict with the code below when we are waiting until the notification has been disappeared.

Also shouldn't the message read "... has not been opened"?

>+  // Click the close button
>+  // XXX: Default offset of (8, 9) does not work
>+  var closeButton = tabBrowser.getTabPanelElement(tabBrowser.selectedIndex,
>+                                                  '/{"value":"password-save"}/anon({"type":"info"})' +
>+                                                  '/{"class":"messageCloseButton tabbable"}');
>+  controller.click(closeButton, 8, 10);                                                                                                  

Why it clicks at (8, 9)? The button is quite longer for me. It's about 78x16px. That means the center is (39, 8). 

>+  // Verify the password notification bar has disappeared
>+  controller.waitFor(function() {
>+    return tabPanel.getNode().currentNotification == null;
>+  }, "Unexpectedly found the Save Password notification bar.");

In this case I really miss a waitForNotElement method for the controller. :/
Attachment #488082 - Flags: feedback?(hskupin) → feedback+
(Assignee)

Comment 3

7 years ago
>-  // The notification bar should not be visible before submitting the credentials
>-  controller.assertNodeNotExist(button);
> 
> We should leave this check to make sure that the panel doesn't appear before
> hitting submit.

I see no value in keeping this check, unless you can point me to a regression bug where this behaviour has happened.  
 
>+  controller.waitFor(function() {
>+    return tabPanel.getNode().currentNotification.value == 'password-save';
>+  }, "Save Password notification bar is not found.");
>  
> shouldn't the message read "... has not been opened"?

Yeah, this message was poorly drafted.  I think "Save Password notification bar failed to display" would be best.  

>+  // Click the close button
>+  // XXX: Default offset of (8, 9) does not work
>+  var closeButton = tabBrowser.getTabPanelElement(tabBrowser.selectedIndex,
>+                                                  '/{"value":"password-save"}/anon({"type":"info"})' +
>+                                                  '/{"class":"messageCloseButton tabbable"}');
>+  controller.click(closeButton, 8, 10);                      
> 
> Why it clicks at (8, 9)? The button is quite longer for me. It's about 78x16px.
> That means the center is (39, 8). 

This was a real hard one to debug.  I was able to come up with 8, 9 as the values being used by default by debugging the values being used within controller.click() (done using alerts).

Besides, 78x16 does not seem like the right dimensions for the close (X) button, as this button is basically a square.  You may have got the dimensions of another button on the notification bar (maybe "remember" or "never").

But all of that is besides the point, as using 8,10 in the test fixes the issue.



>+  // Verify the password notification bar has disappeared
>+  controller.waitFor(function() {
>+    return tabPanel.getNode().currentNotification == null;
>+  }, "Unexpectedly found the Save Password notification bar.");
> 
> In this case I really miss a waitForNotElement method for the controller. :/

Is waitFor() not more reliable? What is your argument for using waitForElement for waitFor()?  I'm all for putting it back in provided you can make a good case for it and I can make the test reliably pass using it.

Thanks for the feedback so far.
(In reply to comment #3)
> > We should leave this check to make sure that the panel doesn't appear before
> > hitting submit.
> 
> I see no value in keeping this check, unless you can point me to a regression
> bug where this behaviour has happened.  

Not off-hand. Checks like that are cheap and give us extra safety. Given that no regression exists so far, doesn't mean a regression can start to happen in the future. Having a test for exactly this situation can be helpful. 

> > shouldn't the message read "... has not been opened"?
> 
> Yeah, this message was poorly drafted.  I think "Save Password notification bar
> failed to display" would be best.  

We have to talk about next week how to describe the text for the message and how close we want to stay with the way mochitests are doing it. It will give us much more information in the reports.

> This was a real hard one to debug.  I was able to come up with 8, 9 as the
> values being used by default by debugging the values being used within
> controller.click() (done using alerts).

You are always workin on OS X, right? You can use dump statements like the following to not be distracted by alert dialogs:

> dump("** pos: " + x + ", " + y + "\n");

> Besides, 78x16 does not seem like the right dimensions for the close (X)
> button, as this button is basically a square.  You may have got the dimensions
> of another button on the notification bar (maybe "remember" or "never").

You are right, my fault. I checked the remember button. :/ Which platform and branch gives you the failure? I can't reproduce it locally on OS X 10.6 with Namoroka.

> But all of that is besides the point, as using 8,10 in the test fixes the
> issue.

Which will be 1px down from the center. We should check why it fails when clicking at the center of the button. A workaround will not fix the problem in general.

> >+  // Verify the password notification bar has disappeared
> >+  controller.waitFor(function() {
> >+    return tabPanel.getNode().currentNotification == null;
> >+  }, "Unexpectedly found the Save Password notification bar.");
> > 
> > In this case I really miss a waitForNotElement method for the controller. :/
> 
> Is waitFor() not more reliable? What is your argument for using waitForElement
> for waitFor()?  I'm all for putting it back in provided you can make a good
> case for it and I can make the test reliably pass using it.

I mentioned 'waitForNotElement'. It's a method which doesn't exist yet beside assertNodeNotExist(). Sounds like we could already make use of it in a couple of places. Something we should request to get added to the controller class.
(Assignee)

Comment 5

7 years ago
Resolving WONTFIX on bug 610849 (waitForNotification function).  This bug should be resolved with that bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WONTFIX
Whiteboard: [mozmill-branch-fail][mozmill-test-failure] → [mozmill-branch-fail][mozmill-test-failure][fixed-by-610849]
We can leave it open by adding the dependency to bug 610849.
Status: RESOLVED → REOPENED
Depends on: 610849
Resolution: WONTFIX → ---
Whiteboard: [mozmill-branch-fail][mozmill-test-failure][fixed-by-610849] → [mozmill-branch-fail][mozmill-test-failure][will be fixed by 610849]
Move of Mozmill Test related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.
Component: Mozmill Tests → Mozmill Tests
Product: Testing → Mozilla QA
(Assignee)

Comment 8

7 years ago
This failure has been fixed by the changes made by bug 610849.  It has uncovered a new Linux-only 3.5-only failure though.  Resolving this bug and filing a new one for that failure.

http://mozmill-release.brasstacks.mozilla.com/#/general/report/39f8121dd6f501c3df2b92dc66640d40
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.