For a long time we have problems that users do not understand how the waitForEval function works. Also I read a couple of js stuff over this weekend and everyone mentions that eval is evil. It can open holes when we eval stuff from a website. To make execution of tests more secure and easier to write/understand I would propose to add a general waitFor function. Existing functions will be wrappers and call the new function so we can stay compatible with existing tests. I will attach a patch in a couple of minutes and I hope that we can fold it into 1.4.2.

This patch adds the waitFor function which uses a closure to check for the condition. Also it replaces all internal calls for waitForEval. I have run all of our Firefox tests against the patched version of Mozmill and everything passes except it makes bug 503277 more visible. But we should fix our tests instead until we have a solution for bug 503277.

My only nit with this is that I really don't like the method name. /** * Waits for the callback evaluates to true */ function waitFor(callback, timeout, interval) { The method name should be a lot more indicative for what is being waited. IMO, method names should never end in a preposition.

Generally, I'd agree. In this specific case, I've typically called this kind of function waitFor as well, as the first parameter gives you the "what." I've found any sort of waitForCondition/waitForClosure/waitForFunction type naming doesn't make the code clearer at all, just longer. That said, I've also usually tried to wrap it whenever possible to give specific wait conditions with more concrete naming (e.g. waitForElementEnabled, which calls waitFor internally--test code calls the more specific functions whenever possible.)

Comment on attachment 458380 [details] [diff] [review] Patch to introduce waitFor >diff --git a/mozmill/mozmill/extension/resource/modules/controller.js b/mozmill/mozmill/extension/resource/modules/controller.js >index cb6ca70..b0f436a 100644 >--- a/mozmill/mozmill/extension/resource/modules/controller.js >+++ b/mozmill/mozmill/extension/resource/modules/controller.js >@@ -53,99 +53,52 @@ var hwindow = Components.classes["@mozilla.org/appshell/appShellService;1"] > var aConsoleService = Components.classes["@mozilla.org/consoleservice;1"]. > getService(Components.interfaces.nsIConsoleService); > >-function waitForEval (expression, timeout, interval, subject) { >- if (interval == undefined) { >- interval = 100; >- } >- if (timeout == undefined) { >- timeout = 30000; >- } >- >- var self = {}; >- self.counter = 0; >- self.result = eval(expression); >- >- function wait(){ >- self.result = eval(expression); >- self.counter += interval; >- } >- >- var timeoutInterval = hwindow.setInterval(wait, interval); >- >- var thread = Components.classes["@mozilla.org/thread-manager;1"] >- .getService() >- .currentThread; >- >- while((self.result != true) && (self.counter < timeout)) { >- thread.processNextEvent(true); >- } >- if (self.counter < timeout) { var r = true; } >- else { var r = false; } >- >- hwindow.clearInterval(timeoutInterval); >- >- return r; >-} >+var sleep = utils.sleep; >+var waitFor = utils.waitFor; >+var waitForEval = utils.waitForEval; I'd prefer not to have these convenience functions. Your call, though. > waitForEvents = function() {} >+ > waitForEvents.prototype = { > /** > * Initialize list of events for given node > */ >- init : function waitForEvents_init(node, events) >-{ >- if (node.getNode != undefined) >- node = node.getNode(); >- >- this.events = events; >- this.node = node; >- node.firedEvents = {}; >- this.registry = {}; >- >- for each(e in events) { >- var listener = function(event) { >- this.firedEvents[event.type] = true; >+ init : function waitForEvents_init(node, events) { >+ if (node.getNode != undefined) >+ node = node.getNode(); >+ >+ this.events = events; >+ this.node = node; >+ node.firedEvents = {}; >+ this.registry = {}; >+ >+ for each(e in events) { >+ var listener = function(event) { >+ this.firedEvents[event.type] = true; >+ } >+ this.registry[e] = listener; >+ this.registry[e].result = false; >+ this.node.addEventListener(e, this.registry[e], true); > } >- this.registry[e] = listener; >- this.registry[e].result = false; >- this.node.addEventListener(e, this.registry[e], true); >- } > }, > > /** > * Wait until all assigned events have been fired > */ > wait : function waitForEvents_wait(timeout, interval) >-{ >- for (e in this.registry) { >- var r = waitForEval("subject['" + e + "'] == true", >- timeout, interval, this.node.firedEvents) >- if (!r) >+ { >+ for (e in this.registry) { >+ try { >+ utils.waitFor(function() { >+ return this.node.firedEvents[e] == true; >+ }, timeout, interval); >+ } catch (e) { > throw new Error("Timeout happened before event '" + e +"' was fired."); >- >- this.node.removeEventListener(e, this.registry[e], true); >- } >-} >-} >- >-function waitForImage(elem, timeout, interval) { >- if (interval == undefined) { >- interval = 100; >- } >- if (timeout == undefined) { >- timeout = 30000; >- } >- return waitForEval('subject.complete == true', timeout, interval, elem.getNode()); >-} >- >-function waitForElement(elem, timeout, interval) { >- if (interval == undefined) { >- interval = 100; >- } >- if (timeout == undefined) { >- timeout = 30000; >+ } >+ >+ this.node.removeEventListener(e, this.registry[e], true); >+ } > } >- return waitForEval('subject.exists()', timeout, interval, elem); > } > > var Menu = function (elements, doc, window) { >@@ -189,14 +142,18 @@ Menu.prototype.reload = function () { > } > > var MozMillController = function (window) { >- // TODO: Check if window is loaded and block until it has if it hasn't. > this.window = window; > > this.mozmillModule = {}; > Components.utils.import('resource://mozmill/modules/mozmill.js', this.mozmillModule); > >- waitForEval("try { subject != null; } catch(err){}", 5000, undefined, window) >- waitForEval("try { subject.documentLoaded != undefined; } catch(err){}", 5000, undefined, window) >+ try { >+ utils.waitFor(function() { >+ return window.documentLoaded != undefined; >+ }, 5000, 100); >+ } catch (e) { >+ throw new Error(arguments.callee.name + ": Window could not be initialized."); >+ } This removes the 'subject != null' check. Is this needed? > if ( controllerAdditions[window.document.documentElement.getAttribute('windowtype')] != undefined ) { > this.prototype = new utils.Copy(this.prototype); >@@ -205,6 +162,8 @@ var MozMillController = function (window) { > } > } > >+MozMillController.prototype.sleep = utils.sleep; >+ Again, not sure we need the convenience method. > MozMillController.prototype.keypress = function(el, aKey, modifiers) { > var element = (el == null) ? this.window : el.getNode(); > if (!element) { >@@ -461,7 +420,10 @@ MozMillController.prototype.check = function(el, state) { > state = (typeof(state) == "boolean") ? state : false; > if (state != element.checked) { > this.click(el); >- this.waitForEval("subject.checked == " + state, 500, 100, element); >+ this.waitFor(function() { >+ return element.checked == state; >+ }, timeout, interval); >+ > result = true; > } > >@@ -481,30 +443,33 @@ MozMillController.prototype.radio = function(el) > } > > this.click(el); >- this.waitForEval("subject.selected == true", 500, 100, element); >+ this.waitFor(function() { >+ return element.selected == true; >+ }, timeout, interval); > > frame.events.pass({'function':'Controller.radio(' + el.getInfo() + ')'}); > return true; > } > >-// The global sleep function has been moved to utils.js. Leave this symbol >-// for compatibility reasons >-var sleep = utils.sleep; >+MozMillController.prototype.waitFor = function(callback, timeout, interval) { >+ utils.waitFor(callback, timeout, interval); >-MozMillController.prototype.sleep = utils.sleep; >+ frame.events.pass({'function':'controller.waitFor()'}); >+} > >-MozMillController.prototype.waitForEval = function (expression, timeout, interval, subject) { >- var r = waitForEval(expression, timeout, interval, subject); >- if (!r) { >- throw new Error("timeout exceeded for waitForEval('"+expression+"')"); >- } >+MozMillController.prototype.waitForEval = function(expression, timeout, interval, subject) { >+ waitFor(function() { >+ return eval(expression); >+ }, timeout, interval); >+ >+ frame.events.pass({'function':'controller.waitForEval()'}); > } Any reason not to use utils.waitForEval? >-MozMillController.prototype.waitForElement = function (elem, timeout, interval) { >- var r = waitForElement(elem, timeout, interval); >- if (!r) { >- throw new Error("timeout exceeded for waitForElement "+elem.getInfo()); >- } >+MozMillController.prototype.waitForElement = function(elem, timeout, interval) { >+ this.waitFor(function() { >+ return elem.exists(); >+ }, timeout, interval); >+ > frame.events.pass({'function':'Controller.waitForElement()'}); > } > >@@ -520,10 +485,14 @@ MozMillController.prototype.__defineGetter__("menus", function() { > }); > > MozMillController.prototype.waitForImage = function (elem, timeout, interval) { >- var r = waitForImage(elem, timeout, interval); >- if (!r) { >- throw new Error("timeout exceeded for waitForImage "+elem.getInfo()); >+ try { >+ this.waitFor(function() { >+ return elem.getNode().complete == true; >+ }, timeout, interval); >+ } catch (e) { >+ throw new Error("timeout exceeded for waitForImage " + elem.getInfo()); > } >+ > frame.events.pass({'function':'Controller.waitForImage()'}); > } > >@@ -972,7 +941,7 @@ function browserAdditions( controller ) { > //if a user tries to do waitForPageLoad(2000), this will assign the interval the first arg > //which is most likely what they were expecting > if (typeof(_document) == "number"){ >- var timeout = _document; >+ timeout = _document; Is this global? > } > //incase they pass null > if (_document == null){ >@@ -980,22 +949,16 @@ function browserAdditions( controller ) { > } > //if _document isn't a document object > if (typeof(_document) != "object") { >- var _document = controller.tabs.activeTab; >- } >- >- if (interval == undefined) { >- var interval = 100; >- } >- if (timeout == undefined) { >- var timeout = 30000; >+ _document = controller.tabs.activeTab; > } > >- var win = _document.defaultView; >+ waitFor(function() { >+ return _document.defaultView.documentLoaded == true; >+ }, timeout, interval); > >- waitForEval("subject.documentLoaded == true", timeout, interval, win); > //Once the object is available it's somewhere between 1 and 3 seconds before the DOM > //Actually becomes available to us >- utils.sleep(1); >+ sleep(1); > frame.events.pass({'function':'Controller.waitForPageLoad()'}); > } > } >@@ -1024,13 +987,17 @@ MozMillAsyncTest.prototype.run = function () { > } > } > >- var r = waitForEval("subject._done == true", this.timeout, undefined, this); >- if (r == true) { >- return true; >- } else { >- throw new Error("MozMillAsyncTest did not finish properly: timed out. Done is " + this._done); >+ try { >+ utils.waitFor(function() { >+ return this._done == true; >+ }, timeout, interval); >+ } catch (e) { >+ throw new Error("MozMillAsyncTest timed out. Done is " + this._done); > } >+ >+ return true; > } >+ > MozMillAsyncTest.prototype.finish = function () { > this._done = true; > } >diff --git a/mozmill/mozmill/extension/resource/modules/utils.js b/mozmill/mozmill/extension/resource/modules/utils.js >index ca3d709..f3f3ce8 100644 >--- a/mozmill/mozmill/extension/resource/modules/utils.js >+++ b/mozmill/mozmill/extension/resource/modules/utils.js >@@ -41,7 +41,7 @@ var EXPORTED_SYMBOLS = ["openFile", "saveFile", "saveAsFile", "genBoiler", > "getFile", "Copy", "getChromeWindow", "getWindows", "runEditor", > "runFile", "getWindowByTitle", "getWindowByType", "tempfile", > "getMethodInWindows", "getPreference", "setPreference", >- "sleep"]; >+ "sleep", "waitFor", "waitForEval"]; > > var hwindow = Components.classes["@mozilla.org/appshell/appShellService;1"] > .getService(Components.interfaces.nsIAppShellService) >@@ -391,27 +391,59 @@ function setPreference(aName, aValue) { > > /** > * Sleep for the given amount of milliseconds >- **/ >-function sleep (milliseconds) { >- var self = {}; >+ */ >+function sleep(milliseconds) { >+ var self = {init: false}; >+ >+ waitFor(function() { >+ var init = self.init; >+ self.init = !init; >+ return init; >+ }, undefined, milliseconds); >+} >+ >+/** >+ * Waits for the callback evaluates to true >+ */ >+function waitFor(callback, timeout, interval) { >+ timeout = timeout || 30000; >+ interval = interval || 100; >+ >+ self = {counter: 0, result: callback()}; > >- // We basically just call this once after the specified number of milliseconds > function wait() { >- self.timeup = true; >+ self.result = callback(); >+ self.counter += interval; > } > >- // Calls repeatedly every X milliseconds until clearInterval is called >- var interval = hwindow.setInterval(wait, milliseconds); >+ var timeoutInterval = hwindow.setInterval(wait, interval); >+ var thread = Components.classes["@mozilla.org/thread-manager;1"]. >+ getService().currentThread; > >- var thread = Components.classes["@mozilla.org/thread-manager;1"] >- .getService() >- .currentThread; >- // This blocks execution until our while loop condition is invalidated. Note >- // that you must use a simple boolean expression for the loop, a function call >- // will not work. >- while(!self.timeup) >+ while((self.result != true) && (self.counter < timeout)) { > thread.processNextEvent(true); >- hwindow.clearInterval(interval); >+ } >+ >+ hwindow.clearInterval(timeoutInterval); >+ >+ if (self.counter >= timeout) { >+ throw new Error(arguments.callee.name + ": Timeout exceeded for '" + callback + "'"); >+ } >+ >+ return true; >+} >+ >+/** >+ * Waits until the expression evaluates to true >+ */ >+function waitForEval(expression, timeout, interval, subject) { >+ try { >+ waitFor(function() { >+ return eval(expression); >+ }, timeout, interval); >+ } catch (e) { >+ throw new Error(arguments.callee.name + ": Timeout exceeded for '" + expression + "'"); >+ } > > return true; > } If this works and is tested its a big improvement.

(In reply to comment #4) > >+var sleep = utils.sleep; > >+var waitFor = utils.waitFor; > >+var waitForEval = utils.waitForEval; > > I'd prefer not to have these convenience functions. Your call, though. We need thoseh at least for compatibility. I only have moved those to the top and it will allow us to execute those functions without having to include the utils.js module in tests. We can consider the removal/move for Mozmill 2.0. Now we can't remove those declarations. > >+ utils.waitFor(function() { > >+ return window.documentLoaded != undefined; > >+ }, 5000, 100); > > This removes the 'subject != null' check. Is this needed? Ouch, good catch. No that wasn't expected. I will fix it. > >+MozMillController.prototype.waitForEval = function(expression, timeout, interval, subject) { > >+ waitFor(function() { > >+ return eval(expression); > >+ }, timeout, interval); > >+ > >+ frame.events.pass({'function':'controller.waitForEval()'}); > > } > > Any reason not to use utils.waitForEval? utils.waitForEval doesn't send a pass event and it's something we want to have for the controller. So I think it's appropriate to have it here. > >@@ -972,7 +941,7 @@ function browserAdditions( controller ) { > > //if a user tries to do waitForPageLoad(2000), this will assign the interval the first arg > > //which is most likely what they were expecting > > if (typeof(_document) == "number"){ > >- var timeout = _document; > >+ timeout = _document; > > Is this global? Blame git here. timeout is a parameter of the function. Sadly it lists the object instead the member function. > If this works and is tested its a big improvement. It is and it works very stable. But I would like to get as much as possible testing for it. So if we wanna have it for 1.4.2, I would target the beta2.

Taking over r+ from previous review. I would like to have feedback from Clint on it.

Comment on attachment 458420 [details] [diff] [review] Patch v1.1 [checked-in] >diff --git a/mozmill/mozmill/extension/resource/modules/controller.js b/mozmill/mozmill/extension/resource/modules/controller.js >index cb6ca70..e0b1e56 100644 >--- a/mozmill/mozmill/extension/resource/modules/controller.js >+++ b/mozmill/mozmill/extension/resource/modules/controller.js >-function waitForEval (expression, timeout, interval, subject) { > /** > * Wait until all assigned events have been fired > */ > wait : function waitForEvents_wait(timeout, interval) >-{ >- for (e in this.registry) { >- var r = waitForEval("subject['" + e + "'] == true", >- timeout, interval, this.node.firedEvents) >- if (!r) >+ { >+ for (e in this.registry) { >+ try { >+ utils.waitFor(function() { >+ return this.node.firedEvents[e] == true; >+ }, timeout, interval); >+ } catch (e) { > throw new Error("Timeout happened before event '" + e +"' was fired."); You're using e as the incrementor variable in the for loop. And you're using e as the exception variable in the try/catch. Nothing technically wrong with that, but it makes the code appear quite confusing. Use ex for the exception variable in the try/catch please. >+MozMillController.prototype.waitFor = function(callback, timeout, interval) { >+ utils.waitFor(callback, timeout, interval); This is really an awful name for the function. I'd much rather prefer that it be called "waitForCallback" as that is what it actually does. Can we search/replace this patch and do that? I think "waitFor" in a test will make about as much sense to non-programers as "waitForEval" did, i.e. none. Please consider changing it to waitForCallback. > Other than those two nits, this looks good.

I don't think waitForCallback will be any clearer--just longer, which potentially affects clarity since it'll be used a lot. It has the same problem as waitForEval: it's highlighting the plumbing used to pass in the exit condition, not the semantic intent of the function in context of the code that uses it. The code you quoted is actually much clearer to me without "callback" in it as it reads, in English, "wait for node's firedEvents[e] to equal true." The fact that we use a closure/callback to pass the condition is just an implementation detail. IME, the real problem with waitFor naming is that nothing adequately gets across "I'm going to repeatedly call this, sleeping in between call, until it returns true." waitFor has become the standard idiom for that (Selenium, etc.) but doesn't really communicate it directly. If I were going to nitpick, it'd be that all the various places that use something like: waitFor(function() { return foo == true; }, timeout, interval) ...should just be: waitFor(function() { return foo; }... as == true is syntactic garbage in a boolean comparison. Also would mention that the way exceptions are trapped in both waitFor() and waitForEval() will eat errors thrown by the callback/eval itself, and rethrow them as if they were timeouts due to the unconditional exception trapping. Unfortunately, I don't know enough JS exception handling at this point to know if you can specifically throw/check for a timeout error and let all other kinds of errors bubble up. If so, we should.

(In reply to comment #8) > I don't think waitForCallback will be any clearer--just longer, which > potentially affects clarity since it'll be used a lot. > > It has the same problem as waitForEval: it's highlighting the plumbing used to > pass in the exit condition, not the semantic intent of the function in context > of the code that uses it. Well, not exactly waitForEval also had that squirrely "subject" thingy, didn't it? ;) > > The code you quoted is actually much clearer to me without "callback" in it as > it reads, in English, "wait for node's firedEvents[e] to equal true." The fact > that we use a closure/callback to pass the condition is just an implementation > detail. > True, but I wonder about people who are not seasoned JS programmers working with this API. If Se is using waitFor, then I'm much more inclined to leave it as waitFor. That's a good argument too. > > If I were going to nitpick, it'd be that all the various places that use > something like: > > waitFor(function() { return foo == true; }, timeout, interval) > > ...should just be: > > waitFor(function() { return foo; }... > > as == true is syntactic garbage in a boolean comparison. true, good point. The only benefit is that since we have no control over the passed in function, we have to force it to return a boolean, I believe. I'd have to go back through and test the patch to see if I could break it without doing that. > Also would mention that the way exceptions are trapped in both waitFor() and > waitForEval() will eat errors thrown by the callback/eval itself, and rethrow > them as if they were timeouts due to the unconditional exception trapping. > > Unfortunately, I don't know enough JS exception handling at this point to know > if you can specifically throw/check for a timeout error and let all other kinds > of errors bubble up. If so, we should. You can do this, but I think it'd be a separate bug.

(In reply to comment #9) > True, but I wonder about people who are not seasoned JS programmers working > with this API. If Se is using waitFor, then I'm much more inclined to leave it > as waitFor. That's a good argument too. Well, looking at Selenium, looks like their generic is waitForCondition(JS_snippet, timeout), executing the snippet as an eval. They simply don't have the closure version, but if we're going for verbose, I like wording like Condition better because it's more generic. OTOH, applying that to the closure version may confuse Se vets who expect an eval there. I guess at the end of the day, it doesn't really matter as long as we document well, so I'm fine with whatever everyone else finds clear. > You can [throw typed exceptions], but I think it'd be a separate bug. OK. I'll wait until after this patch is done, then review that code and refile if I still see that issue.

(In reply to comment #7) > You're using e as the incrementor variable in the for loop. And you're using e > as the exception variable in the try/catch. Nothing technically wrong with > that, but it makes the code appear quite confusing. Use ex for the exception > variable in the try/catch please. My bad! I will change that. > I think "waitFor" in a test will make about as much sense to non-programers as > "waitForEval" did, i.e. none. Please consider changing it to waitForCallback. > > > Other than those two nits, this looks good. My intention was to have a base function, all other API function can based on. The easiest, safest, and probably only way to do that is with a callback. That way the caller has the complete responsibility about the code, which is used to form the condition. I would be open to change the name, i.e. waitForCondition, but I wouldn't call it waitForCallback. The first parameter makes it already more then clear, what this function is about.

Landed on master and 1.4.2: http://github.com/mozautomation/mozmill/commit/aedb2b2137d6d42f53fec3226b00a5445508e0c1 http://github.com/mozautomation/mozmill/commit/45213c70e89c05d8a728aa9ca293ffe1fad7b660

This needs a follow-up because of a really strange behavior which causes us to end up in an infinite loop for waitForPageLoaded().

The waitForPageLoad function is still fragile and I don't wanna touch it here a lot. It's really part of bug 503277. This patch fixes the problem I have discovered for blank or error pages. We somehow have to cache the defaultView for the current tab. Otherwise we end up in an infinite loop, where we execute the callback but never return.

Comment on attachment 460884 [details] [diff] [review] Follow-up > MozMillController.prototype.waitForEval = function(expression, timeout, interval, subject) { >- waitFor(function() { >- return eval(expression); >- }, timeout, interval); >+ try { >+ waitFor(function() { >+ return eval(expression); >+ }, timeout, interval); >+ } catch (ex) { >+ throw new Error(arguments.callee.name + ": Timeout exceeded for '" + expression + "'"); It would be nice if the timeout was also printed. Other than that, looks fine

(In reply to comment #16) > It would be nice if the timeout was also printed. Other than that, looks fine We don't print the timeout at all for all waitFor functions. I would like to see that in a follow up bug, if you don't mind. Pushed to master and 1.4.2: http://github.com/mozautomation/mozmill/commit/03c6956b22eb738daab24031bec49fdf0f43923f http://github.com/mozautomation/mozmill/commit/1a192597f022be45fb0c0d012a8b5d835f957fde Clint or Jeff, I would need help to fix the sleep call. It's a function which doesn't work well with the new waitFor function. Has one of you time to check that?

(In reply to comment #17) > Clint or Jeff, I would need help to fix the sleep call. It's a function which > doesn't work well with the new waitFor function. Has one of you time to check > that? Jeff and I decided on IRC to move this part to its own bug, because it looks like a regression in recent Minefield builds. We only have to make sure to find a solution for that for 1.4.2. Marking this bug as resolved fixed.

Verified via successful test run

Documentation added: https://developer.mozilla.org/en/Mozmill/Mozmill_Controller_Object#waitFor()