634227 - Implementation of a driver module

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

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

We need a shared module for different waitFor calls, which can be used in our tests to wait for certain situations. I will come up with a proposal of a first set of methods and how failures will be turned into error messages.

Comment 1

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

Selenium 2 has an 'implicit wait' feature, which means that if an element doesn't exist or is not visible at the time you're attempting to locate it, it will retry every second until a timeout. If it's not found/visible in the specified timeout then an ElementNotFound/ElementNotVisible exception is thrown. This would be an awesome thing to have in Mozmill.

See the tests for usage:
http://code.google.com/p/selenium/source/browse/trunk/java/client/test/org/openqa/selenium/ImplicitWaitTest.java

Comment 2

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

Thanks Dave. That sounds even that something like that would be a great addition for Mozmill itself. CC'ing Andrew who works on the refactoring of the controller module.

Comment 3

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

I wonder how much of that module should already be covered in Mozmill itself. I made some thoughts but didn't came up with a good list of possible waitFor methods.

If you want to wait for an element to appear or to be loaded to operate on, it should be part of Mozmill. Because waitFor calls will always be bound to the controller the test is operating on, it should still be available on that object in Mozmill 2.0. 

Currently Mozmill has the following wait methods:

void sleep(in int timeout);
void waitFor(in func callback, in string message, in int timeout, in interval, in object this);
void waitForElement(in Elem element, in int timeout, in int interval);
void waitForElementNotPresent(in Elem element, in int timeout, in int interval);
void waitForImage(in Elem element, in int timeout, in int interval);
void waitThenClick(in Elem element, in int timeout, in int interval);
void waitForPageLoad(in document, in int timeout, in int interval);

and waitForEval which should be removed in 2.0:

void waitForEval(in string, expression, in int timeout, in int interval, in object subject);


I don't think we need anything else which should be moved out to a separate waitfor module.

Geo, what do you think? Why we brought such a module on our list of items to implement?

Comment 4

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

Henrik,

Because we were talking about adopting the Selenium IDE model, where every condition has assert, expect, and waitFor.

IOW, I didn't anticipate waitFors like you're doing, I anticipated waitForIsTrue(), etc., with only a very small handful of domain-specific waitForPageLoad() type things where it makes sense.

I'm not 100% fond of having to use callbacks for simple synchronization. It's overly complicated. I'd like to have more specific and clearer waitFors for common cases.

Please consider making waitFors that follow your assert/expect modules, then let's talk about whether that's a good idea.

Comment 5

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

For what will you wait for in such cases? It will not work. You can't pass in lets say a property 'disabled' and wait until it is set. In cases like those the value is transferred as value and not as reference. The waitFor will wait forever. Once you pass in objects, which are transferred as reference, the waitFor function has to know what to do with the object. How do you want to specify that? A callback is the only solution we have.

IMO there is no value having waitFor functions similar to our assert/expect methods.

Comment 6

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

Yeah, you're right. I thought about it over lunch and realized there was no getting away from it.

There might still be value in establishing a waitFor module for synchronizations specific to our testing (wrapper functions that encapsulate the waitFor([callback]) bit, like waitForPageLoad does) but let's table it for now.

FWIW, lots of things on your list come down to "look at it and figure out what we need to do." Doesn't mean we need to do anything, just need it analyzed.

Comment 7

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

Yeah, as discussed on IRC something interesting could be waitForProperty but we will wait for the implementation once we see a real benefit. For now lets close it as wontfix. If needed we can reopen later.

Comment 8

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

Reopening because with our refactoring the controller object will not be accessible directly. We need some wrappers to augment those general waitfor functions into the global scope.

Comment 9

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

Ok, so we only want to have the waitFor method of the Mozmill controller wrapped in that module for now. We have to figure out later how to work with the element related waitfor actions. The implicit waitfor methods of webdriver are still on our list to investigate.

A patch for that wrapper should be up later today.

Comment 10

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

Ok, as discussed on IRC it would make sense to move that waitFor function and other methods which are not directly bound to elements into a driver module, which will be available as driver in the global scope.

Comment 11

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

Attachment: WIP v1

First working WIP. Geo can you please check the code changes only? Please don't care about the docs which I haven't been updated yet. I only want to know if those changes will met our expectation.

As we talked on IRC I had problems with instanceof for the catch clause if the throw happens in another securable module. For now we distinguish the types of errors via the name property. Further I had problems with the Error class itself because it's a native class. Given that I have changed it to use the inheritance class too, and created our own generic error class as base class.

Comment 12

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

Comment on attachment 516416 [details] [diff] [review]
WIP v1

This one's a little borderline for me, and I'm giving you a -. But as we discussed online, we can still go with it and improve it later. So it's f-, but still may end up an r+. Just consider this me voicing my objections to the strategy in a non-binding way.

My main concern is the GenericError bit. It's dangerous to try to recreate a system-provided class as a project-side class (even in a dynamic language) because you have to match the functionality exactly or face unknown problems later when you try to use it with other code.

There are two ways I can see right now that this won't match functionality:

1) It can't take a string as the constructor value.
2) It has no special toString implementation, which I think Error does.

I don't really think it's a good move, given that it's driven by two concerns that are more design problems than technical problems:

First, there's the special stack-handling. I think A-Team already voiced some concerns about us doing this, because it gets you off in some tricky territory. Well, here's one of the tricky territories; it's problematic to just do what we -should- be able to do: 

function TimeoutError(...) { 
  Error.call(...); 
} 
TimeoutError.prototype = Error;

I'm starting to think that rather than us going cowboy on the stack handling, we should be more patient and ask that Mozmill 2.0 clip the stack to not include system-provided files (this is likely doable with a top-level exception handler from whatever code calls our tests). That will give us what we want without us having to do a bunch of workarounds, at the cost of having messy errors until then.

But prototype objects also gets us into the second: apparently Error doesn't work with inheritance.js. More and more, I think inheritance.js was a bad idea. Everything we do with it could have been done without it and with roughly the same syntax. We adopted a neat solution without really identifying what it solved.

It's given us nearly no benefit, but we've identified plenty of problems: we can't control what's an instance and what's a prototype method; it doesn't handle properties correctly; it apparently doesn't work with native classes. Who knows what else?

We're going to be talking to the A-Team about whether to continue with it, and I suspect that will be no, not unless we can make a serious pitch for why we should use it. I don't have any good arguments other than "it makes me more comfy as a classic OOP guy" but that's a crappy reason. For many reasons, one should program natively in the language they're in, not try to make it look like something else.

So final thoughts:

Like I said on IRC, you have limited time, so if this is what you have let's get it packaged up so we can merge it in (though I would like to have, as a minimum improvement, construct-from-string so that we can create errors in the typical way.)

However, I don't think this approach is really a good idea, and if we go with this it'll likely be something we revise in the near future after we figure out our OOP strategy.

Comment 13

[cmtalbert]

Comment on attachment 516416 [details] [diff] [review]
WIP v1

A couple of quick drive-by review comments.
1. This entire patch seems to have been written because you don't like the error that waitFor throws.  It would have been much, much simpler (and required far less of your time) to simply patch mozmill directly in this case, and I question whether such a fix really should be in a shared module at all.
2. I reiterate everything Geo says about stack frame munging.  Really really hate the idea that's in a shared module.
3. I don't have any opinion about the inheritance stuff, but it does sound like (from merely reading the comments on this bug and others) that the API you're using really is causing more harm than good.  When these things are upstreamed, we will likely be converting them to use normal Javascript prototype inheritance where ever  inheritance is really needed.  We're looking at traits.js but, more and more it just feels less fragile to write it in the paradigm the language uses.  But that's just FYI in case that affects your decision about inheritance.js.
4. How are you going to deal with name collisions in your global scope?  I.e. if your tests have a "driver" object in their global scope and (as we discussed) the mozmill "controller" object becomes called "driver" then how are we going to handle that?  Are you intending that at this point you'll simply stop using the shared module "driver" object?

If this entire exercise is to change the error returned from waitfor, then I urge you to just fix bug 637941 and be done with it.

Comment 14

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

Believe me I don't like the inheritance stuff, but as I have said, I had problems with Object.create or the prototypal inheritance. Error has been implemented as native code and the snippet as given doesn't work:

function TimeoutError(...) { 
  Error.call(this, message, file, line); 
} 
TimeoutError.prototype = Error;


Whatever I have tried the parameters specified in the constructor of Error will not be set. Also properties like .message will be undefined. To have something working for now from the test side I changed it to the inheritance class. Even it is not perfect it will work for the moment, and I hope that Heather can chime in here and help us to transform it to the prototypal solution. That's something which is required for Mozmill.

As discussed yesterday I will just make sure to offer the same interface for the Error classes, so we can change their implementation later without affecting the tests.

(In reply to comment #13)
> 1. This entire patch seems to have been written because you don't like the
> error that waitFor throws.  It would have been much, much simpler (and required
> far less of your time) to simply patch mozmill directly in this case, and I
> question whether such a fix really should be in a shared module at all.

If we don't get it for Mozmill 1.5.x, I don't see another chance for us. We have to be able to run it with 1.5.x. 

> 2. I reiterate everything Geo says about stack frame munging.  Really really
> hate the idea that's in a shared module.

Clint, as already talked this is for the time being. Once Mozmill can handle it correctly all that will go away. Not sure how often we want to bring this topic up again.

> 4. How are you going to deal with name collisions in your global scope?  I.e.
> if your tests have a "driver" object in their global scope and (as we
> discussed) the mozmill "controller" object becomes called "driver" then how are
> we going to handle that?  Are you intending that at this point you'll simply
> stop using the shared module "driver" object?

In future versions we can check if driver is available in the global scope and simply don't import this module. That's all to say I think.

> If this entire exercise is to change the error returned from waitfor, then I
> urge you to just fix bug 637941 and be done with it.

Again, only if we can get another hotfix release.

Comment 15

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

Attachment: WIP v2

More foo magic for the custom error classes. It completely removes the dependency on the inheritance module. I have also pushed that commit to my branch on your repository:

https://github.com/geoelectric/mozmill-api-refactor/commit/9d6488394f77a7a00d5177777262ab86f4dfbd70

Comment 16

[cmtalbert]

(In reply to comment #14)
> Again, only if we can get another hotfix release.
That was exactly my point.  You could have patched 1.5.x, fixed it on master, and released a hotfix in the time it took to write this patch and comment on this bug.  It just seems like make-work to me.  Let's add this to the list of things to chat about on Tuesday.

Comment 17

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

Henrik, looks basically OK. The error class generator is tricky, but I'll trust you if you say that's what it takes to get this to work cleanly. I appreciate the work you did to refactor it.

But WIP v2 uploaded here is identical to WIP v1. (though the github commit is your new stuff).

Can you reupload WIP v2 just so I can see a side-by-side diff and properly f+ it?

Clint, re: the driver module, it should go away when 2.0 comes out. We're putting it in place temporarily just to keep breakage/refactoring at a minimum. That's one of the main reasons I don't want a huge number of supporting code for it. 

Re: the errors, I think TimeoutError and AssertionError are all we'll define, and ultimately I'd like those upstreamed (which means we should inject them into the module w/o the errors. prefix using init, if we haven't done that already).

Also, I've really come to agree re: using native object paradigms. Any variance from that, IMO, should be on a very widely used framework. If Prototype.js wasn't exclusively content, I'd suggest that, but I'd want something on that scale of ubiquity.

Comment 18

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

Attachment: WIP v2 (for real)

Wow, sorry for that. Looks like I missed to export the patch before the upload. Here is the patch.

Comment 19

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

(In reply to comment #16)
> That was exactly my point.  You could have patched 1.5.x, fixed it on master,
> and released a hotfix in the time it took to write this patch and comment on
> this bug.  It just seems like make-work to me.  Let's add this to the list of
> things to chat about on Tuesday.

Well, that surprises me a bit. In the meeting 2 weeks ago it sounded that we do not want to release another hotfix except for critical issues. But if that is possible I would be up for it, but I will not turn the wheel now. I assume other things will come up so we can have a combined hotfix release.

Comment 20

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

Comment on attachment 516700 [details] [diff] [review]
WIP v2 (for real)

Looks fine, thanks for the re-upload. 

On further thought, I think the class factory way you did this is pretty elegant, if straight inheritance doesn't work right, so kudos. At some point, if only for my own edification, I want to figure out why we can't inherit new Errors.

Like I said above, I think we want to inject the error classes in the init function since they'll likely be globals once upstreamed. But I'm going to go ahead and merge this in, and we'll take care of it later.

Comment 21

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

Attachment: Patch v1

Now with documentation and small fixes. I have also removed the default services import.

Comment 22

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

Comment on attachment 516745 [details] [diff] [review]
Patch v1

Looks fine. Go ahead and land it when you get a chance so you can define the commit comment. :)

Comment 23

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

Landed as:
https://github.com/geoelectric/mozmill-api-refactor/commit/b40d80c6dcf10a0713e5fa690c33742f19f10687