610757 - Need a waitForElementNotPresent() in controller module

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[u279076]

This comes from Henrik's last point in https://bugzilla.mozilla.org/show_bug.cgi?id=609071#c2

>+  // 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. :/

I'll let Henrik elaborate here.

Comment 1

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

Right now the controller API doesn't allow us to wait until an element has been disappeared. We will have to code it on our own with code given by Anthony above. We should have a function which waits until an element disappears to fix this leak.

waitForElement(elem) vs. waitForNotElement(elem)

Shouldn't be that hard to implement and will help us on various places.

Comment 2

[Aaron Train [:aaronmt]]

Not a fan of the name, too ambiguous. Does an element really become 'not an element', or rather are we really waiting for a node to have a null value.

Comment 3

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

waitForElementNotFound() perhaps? The other name is a little awkward.

Comment 4

[Aaron Train [:aaronmt]]

Sounds good. I think we should always consider using adjectives or verbs to describe our functions off the bat.

Comment 5

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

It's a deal. But to be consistent with assertNodeNotExists we should use a name like waitForElementNotExists. Not sure why we are using Node vs. Element but it's not something we should change for Mozmill 1.5.x releases.

Comment 6

[cmtalbert]

(In reply to comment #1)
> Right now the controller API doesn't allow us to wait until an element has been
> disappeared. We will have to code it on our own with code given by Anthony
> above. We should have a function which waits until an element disappears to fix
> this leak.
Leak?  What does this leak?  I don't understand.

I still think that waitForElementNotExists is a really awkward name.  Why not waitForElementToDisappear()?  It's clear, it's concise, it's explicit.  It doesn't map to our other apis but I don't really think that naming the API awkwardly is a desired outcome.  waitForElementToNotExist() would be another suggestion.

What is the impact of this API on tests?  How many times have you needed it?  If you've needed it a dozen times, then it's something we should probably take.  However, I doubt it as this has never come up in the year you've been working on it.  The waitForAPIs are very hard to get correct, so I need to judge importance versus schedule impact in order to decide on whether this is in 1.5.2 or out of it.

Comment 7

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

I still like my suggestion of waitForElementNotFound(). But I also agree with Henrik's point that the assert and waitFor that correspond should match, and would take it even further that they should match exactly.

Any chance we could change the assert name while establishing the waitFor, and use the better syntax? We could leave the assertNodeNotExists() as a deprecated synonym, if there is any test code hanging on it.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

How about waitForElementNotPresent, which is what Selenium 1.x uses?

Comment 9

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Sure, I like aligning on Selenium.

Comment 10

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

(In reply to comment #6)
> Leak?  What does this leak?  I don't understand.

It leaks the functionality to easily wait until an element is not present anymore. We have a couple of situations for that and are using an ugly workaround at the moment. 

Otherwise I can second the last comment from Dave.

Comment 11

[Heather Arthur [:harth]]

Attachment: controller.waitForElementNotPresent()

patch, tested.

Comment 12

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

Comment on attachment 503370 [details] [diff] [review]
controller.waitForElementNotPresent()

>+MozMillController.prototype.waitForElementNotPresent = function(elem, timeout, interval) {
>+  this.waitFor(function() {

Usually there should be a space between function and '()' because it's an anonymous one. 

>+    return !elem.exists();
>+  }, "Timeout exceeded for waitForElementNotPresent " + elem.getInfo(), timeout, interval);
>+
>+  frame.events.pass({'function':'Controller.waitForElementNotPresent()'});

For Mozmill 2.0 I would really like to see that we also pass the getInfo() value to frame.events.pass. We really miss that information when multiple calls to the same controller method happen.

It's great to see progress! Thanks Heather

Comment 13

[cmtalbert]

(In reply to comment #12)
Comments on the review, I'd always prefer anonymous functions not be anonymous.  Even if they have simple names.  But I don't really care that much on the 1.5.2 codebase.  It's up to you if you want to change it.

> >+    return !elem.exists();
> >+  }, "Timeout exceeded for waitForElementNotPresent " + elem.getInfo(), timeout, interval);
> >+
> >+  frame.events.pass({'function':'Controller.waitForElementNotPresent()'});
> 
> For Mozmill 2.0 I would really like to see that we also pass the getInfo()
> value to frame.events.pass. We really miss that information when multiple calls
> to the same controller method happen.
I think that should be figured out in a new bug, not this one.  Please file if you have the time.

Let's go with what we've got here.

R+ thanks Heather!

Comment 14

[Heather Arthur [:harth]]

1.5.2:
https://github.com/mozautomation/mozmill/commit/dd9543ee60ed6139016c59ec55b9c9ac73bb28c3

master:
https://github.com/mozautomation/mozmill/commit/91cde87515ce12762859f79fc3b64d33179df398

I didn't mess with the anonymous function because all the other waitFor methods use that syntax, we can change all at once if we want to.

Comment 15

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

Anthony, could you please verify that it works with the password notification bar test?

Comment 16

[u279076]

(In reply to comment #15)
> Anthony, could you please verify that it works with the password notification
> bar test?

Seems to work fine locally.  I'd like to do a spotcheck on qa-horus though, since that's where the testPasswordNotSaved failure was happening.  I'll take care of that on bug 614973 though.

Comment 17

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

Does it mean we can mark it as verified fixed?

Comment 18

[u279076]

(In reply to comment #17)
> Does it mean we can mark it as verified fixed?

Yes. Done.

Comment 19

[u279076]

I found a flaw with this method in testPasswordNotSaved:

If an element has not been instantiated yet, this method times out.  In essences it's looking for !element.exists() for something which does not exist.  Would it be possible to add an OR conditional for NULL to handle cases where an element has not been created yet?

Comment 20

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

Lets reopen for further investigation.

Comment 21

[Heather Arthur [:harth]]

Are we talking about doing this:

controller.waitForElementNotPresent(null) ?

or:

var notElement = new elementslib.ID(controller.window.document, 'adjicoeo289');
controller.waitForElementNotPresent(notElement);

or something else?

Comment 22

[u279076]

Let me give you some context via testPasswordNotSaved, as it might help to explain.  The following is the basic steps our test performs:

1. Load a login page
2. Verify the notification bar is not visible by checking for the (X) button
3. Enter the username and password into the form
4. Click the Login button
5. Verify the notification bar is visible by checking for the (X) button
6. Click the (X) button
7. Verify the notification bar is not visible by checking for the (X) button

waitForElementNotPresent() works for #7 because the element has been created previously.

waitForElementNotPresent() times out for #2 because the element has not been created yet.

Comment 23

[Heather Arthur [:harth]]

for me doing waitForElementNotPresent() will immediately return on an element that doesn't exist.

when I replace the assertNodeNotExist() in testPasswordNotSaved with waitForElementNotPresent() it still passes, is there a testcase you can attach?

Comment 24

[u279076]

(In reply to comment #23)
> for me doing waitForElementNotPresent() will immediately return on an element
> that doesn't exist.
> 
> when I replace the assertNodeNotExist() in testPasswordNotSaved with
> waitForElementNotPresent() it still passes, is there a testcase you can attach?

If you are trying to reproduce the testPasswordNotSaved case, you will need to run the entire suite using testrun_general.py to reproduce it.

Comment 25

[u279076]

(In reply to comment #24)
> (In reply to comment #23)
> > for me doing waitForElementNotPresent() will immediately return on an element
> > that doesn't exist.
> > 
> > when I replace the assertNodeNotExist() in testPasswordNotSaved with
> > waitForElementNotPresent() it still passes, is there a testcase you can attach?
> 
> If you are trying to reproduce the testPasswordNotSaved case, you will need to
> run the entire suite using testrun_general.py to reproduce it.

Also, Linux-only.

Comment 26

[Heather Arthur [:harth]]

(In reply to comment #24)
> (In reply to comment #23)
> > for me doing waitForElementNotPresent() will immediately return on an element
> > that doesn't exist.
> > 
> > when I replace the assertNodeNotExist() in testPasswordNotSaved with
> > waitForElementNotPresent() it still passes, is there a testcase you can attach?
> 
> If you are trying to reproduce the testPasswordNotSaved case, you will need to
> run the entire suite using testrun_general.py to reproduce it.

I can't see any waitForElementNotPresent's in mozmill-tests/firefox/testPasswordManager/testPasswordNotSaved.js, where is this file?

Comment 27

[u279076]

That's because we don't have a patch landed to implement waitForElementNotPresent() yet (I'm working on this on bug 614973).  Basically, anywhere you see an assertNodeNotExist() is where I am trying to use it.

Comment 28

[cmtalbert]

(In reply to comment #22)
> Let me give you some context via testPasswordNotSaved, as it might help to
> explain.  The following is the basic steps our test performs:
> 
> 1. Load a login page
> 2. Verify the notification bar is not visible by checking for the (X) button
> 3. Enter the username and password into the form
> 4. Click the Login button
> 5. Verify the notification bar is visible by checking for the (X) button
> 6. Click the (X) button
> 7. Verify the notification bar is not visible by checking for the (X) button
> 
> waitForElementNotPresent() works for #7 because the element has been created
> previously.
Cool, that's what I believe waitForElementNotPresent is supposed to do.
> 
> waitForElementNotPresent() times out for #2 because the element has not been
> created yet.
Correct, that's because in step 2 you really only want an assertNodeNotExist to ensure that you don't have a notification bar.

Wait for element not exist is specifically supposed to wait for an element that does to exist to stop existing.  By definition, it SHOULD time out if the element doesn't exist at first.

Comment 29

[u279076]

Thanks for clearing that up, Clint.  I was not 100% sure that was the case.  With that, I think we can re-mark VERIFIED FIXED.

Comment 30

[Heather Arthur [:harth]]

(In reply to comment #27)
> That's because we don't have a patch landed to implement
> waitForElementNotPresent() yet (I'm working on this on bug 614973).  Basically,
> anywhere you see an assertNodeNotExist() is where I am trying to use it.

Anthony, I only see one assertNodeNotExist() on line 84 (correlating to step 2 I believe), and when I replace it it works fine, can you attach the file you're using?

Comment 31

[u279076]

One other detail I might have left out:

Please test using the 1.9.1 branch, as the failure is most reproducible there.

Comment 32

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

So we can mark it verified fixed?

Comment 33

[Aaron Train [:aaronmt]]

Verified fixed on 1.5.2

Comment 34

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

This needs an update of the MDN documentation.