477135 - Implement automatic waiting for elements before accessing them

Status: RESOLVED WONTFIX

(Testing Graveyard :: Mozmill, defect, P3)

Description

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

In my test for checking the top site Amazon.com I had to create two helper functions for now, which let me assert and click on elements *when* they really exists. With the functions given by MozMill itself I run into different failures due to slow loading of web pages.

+/**
+ *  Waits until element exists before calling assertNode
+ */
+function delayedAssertNode(aNode, aTimeout) {
+  controller.waitForElement(aNode, aTimeout);
+  controller.assertNode(aNode);
+}
+
+/**
+ *  Waits until element exists before clicking on it
+ */
+function delayedClick(aNode, aTimeout) {
+  controller.waitForElement(aNode, aTimeout);
+  controller.click(aNode);
+}


(In reply to bug 476236 comment #2)
> * Shouldn't the delayed* functions go into a shared module?  They seem like
> they would be just the right thing for a shared module.

The question is should this behavior not better integrated into MozMill itself?
All tests will run into this problem when handling lazily loaded content. This
doesn't only apply for content but also chrome elements - see the
bookmarkEditPanel. Mikeal and Adam, what do you both think?

Adam and Mikeal, what do you both think about? Shall we enhance the existing function to also accept a timeout value and which will check for the existence or should it be placed in a shared module?

Comment 1

[Mikeal Rogers [:mikeal] (mrogers@)]

Ah, here's the main problem.

waitForElement should throw an exception if the timeout expires. This would remove the need for your assertNode call completely.

As for the waitForElement and then click call, I think this has some utility and we should think about merging a function like this in to that main controller code.

Comment 2

[Mikeal Rogers [:mikeal] (mrogers@)]

controller.waitFor calls all throw exceptions if their timeout is exceeded as of r372.

Adam, I can implemented this waitThenClick call at any time but I'm wondering how we should name it?

Comment 3

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

(In reply to comment #1)
> waitForElement should throw an exception if the timeout expires. This would
> remove the need for your assertNode call completely.

Oh, that's true. Havent't thought on that. Thanks!

> As for the waitForElement and then click call, I think this has some utility
> and we should think about merging a function like this in to that main
> controller code.

Can't this be done in one step when the click function is called? Means the first action which is done is a waitForElement? Going that way we make sure that the element exists when the event is fired. Otherwise click will also throw the exception. That would be completely transparent for the user.

Comment 4

[Mikeal Rogers [:mikeal] (mrogers@)]

> Can't this be done in one step when the click function is called? Means the
> first action which is done is a waitForElement? Going that way we make sure
> that the element exists when the event is fired. Otherwise click will also
> throw the exception. That would be completely transparent for the user.

I'm opposed to magic like this that implicitly adds testing to your controller calls. What if you _want_ to attempt a click right away, and if the node isn't there you want it to fail.

Comment 5

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

We could introduce a new timeout parameter for such user action functions. If you wanna click directly just set it to 0. So waitForElement will return immediately or throw an exception if the node was not found.

Comment 6

[Mikeal Rogers [:mikeal] (mrogers@)]

Committed revision 394.

Comment 7

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

Functionality looks good so far. Why you have called it waitThenClick. IMO delayedClick sounds like a better name. Can we do the same for other functions like type?

Comment 8

[Adam Christian]

waitSomethingSomething is the naming convention we are using for waitForElement etc. I knew there was going to be a request for waitThenDoOtherStuff which is why I didn't comment on this before but it seems like this whole thing should be made a bit more dynamic.. something like, waitThen which takes a node, timeout, then a function and its params.

waitThen(waitNode, 1000, controller.click( elem));
waitThen(waitNode, 1000,controller.type(typeElem, 'Mozilla'));

Seems to me that having another waitThenFunction for everything else in the controller is a bad idea. I still don't understand why this even needs to happen, what's so hard about:

 controller.waitForElement(new elementslib.Elem( controller.window.content.document.body ));
 controller.type(new elementslib.Name(controller.window.content.document, 'q'), 'Mozilla');

Comment 9

[Mikeal Rogers [:mikeal] (mrogers@)]

I agree that this is a little annoying from an implementation perspective, but from a public API perspective it's actually quite nice.

First off, your API suggestion simply won't work since that type call is going to happen BEFORE the wait call.

We could do a dynamic API use a custom gettr that translates waitThenSomething to a wait call, then a controller.something call. The problem with this API is that calls other than click, like your type call, have differing priorities for their arguments and simple making all the wait arguments go before the arguments to the next action call will be really annoying for some calls, like waitThenType.

I know it feels a little icky adding these kinds of calls in somewhat arbitrarily. My thinking is that once people write a test libraries that do common tasks, and we see them using these a lot, that we merge that functionality into mozmill proper. waitThenClick followed this pattern.

In the future I won't consider additions like this to the controller unless they follow the same pattern of common use in other test libraries, so I think we can mitigate the flood of feature requests you are concerned about for waitThen API calls.

Comment 10

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

Mikeal, we could also remove this function for MozMill 1.1 and I can setup a shared module for now. As you said this would be cleaner. Otherwise questions will eventually arise why only the click function works in such a way. I see that you are not quiet happy but I want to see you so!

Comment 11

[Mikeal Rogers [:mikeal] (mrogers@)]

Adam is concerned that adding a lot of these will eventually bloat and confuse the code base and after a long discussion I have to agree.

Adam and I discussed this and we came up with a general API that could work best for these use cases.

controller.waitThen(elem).click()
controller.waitThen(elem).type('asdf')

This is kind of a jquery style chaining API. This is the nicest looking and easiest to use general API that we could think of.

One of us will implement this on Monday and have it in for 1.1 so that we have a sustainable to unchanging API moving forward for this very common use case.

Comment 12

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

Wow, that looks fantastic! Clean, easy to read and really helpful for all commands which depends on the existence of an element.

(In reply to comment #11)
> controller.waitThen(elem).click()
> controller.waitThen(elem).type('asdf')

I think you shortened it a bit here, so you normally have to use the following code?

controller.waitThen(elem).click(elem)
controller.waitThen(elem).type(elem, 'asdf')

I don't think that the element is somehow magically transfered to the second function call.

Comment 13

[Mikeal Rogers [:mikeal] (mrogers@)]

Nope, you don't need to pass it to the second function.

waitThen returns an object with a reference to the controller it's attached to and the elem you passed it. That object has a custom getter that returns a anonymous function that calls the method you're calling on the controller passing elem as the first argument.

Gotta love dynamic languages :)

Comment 14

[Adam Christian]

I can't get this to work as defined, the implementation henrik pointed out is obviously much easier to do as you simply have waitThen return an instance of the MozMillController which allows you to chain the methods. How much time/research to we want to put into this before 1.1

Comment 15

[Mikeal Rogers [:mikeal] (mrogers@)]

Let's just punt it, i want to get this release out and this missing isn't going to block any testability.

Comment 16

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

Something really helpful but we can live without it for now.

Eventually we could create a shared module for now and move the functions I've written in my Amazon test there?

http://hg.mozilla.org/qa/mozmill-tests/file/fd0cd7b00bdd/firefox/content/test_checkAmazon.js#l55

Comment 17

[Mikeal Rogers [:mikeal] (mrogers@)]

I like this feature, if we have time I'd love to get it in :)

Comment 18

[Mikeal Rogers [:mikeal] (mrogers@)]

After months of discussion and mediation in the windmill community we've decided that in windmill 1.3 we'll be implementing automated waiting for any element you try to simulate an event on.

I think this is also the direction we should take the mozmill API in during the next release so I'm going to suggest that we re-target this bug for the next release and just implement automatic waiting for all elements before interaction.

Comment 19

[Mikeal Rogers [:mikeal] (mrogers@)]

This is happening in 1.3

Comment 20

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

Mozmill will reach its end of life soon. We are currently working on getting all the tests for Firefox ported to Marionette. For status updates please see bug 1080766.