Right now a controller.waitFor() will always raise an exception and cause a hard stop of the test module. To be able to have soft-assertions we should add a waitFor() wrapper to the Expect/Assert classes in assertions.js.

Just some questions before we start uploading some code here: 1. I suppose this needs to refactor all existent tests and shared modules to make usage of the waitFor wrapper from assertions.js ? 2. Do we have a rule which ones would be soft-assertions and which hard-assertions? (replace controller.waitFor with assert.waitFor or expect.waitFor ? ) I'm thinking we will have lots to argue here because its really subjective in some situations. 3. If question 1's answer is positive and we need the refactoring in this patch, do we refactor only active tests or both enabled and disabled tests? Thanks in advance!

We clarified those questions on IRC today.

* initial patch * having the hard assertion as a waitFor wrapper * the wait for in the soft assertion is a modification of the mozmill.utils.waitFor to adapt it to 'soft failing'

Comment on attachment 658448 [details] [diff] [review] patch v1.0 Review of attachment 658448 [details] [diff] [review]: ----------------------------------------------------------------- Why can we not just call mozmill.utils.waitFor and catch the exception?

(In reply to Dave Hunt (:davehunt) from comment #4) > Comment on attachment 658448 [details] [diff] [review] > patch v1.0 > > Review of attachment 658448 [details] [diff] [review]: > ----------------------------------------------------------------- > > Why can we not just call mozmill.utils.waitFor and catch the exception? Good idea. This will be more elegant and we will not make this ugly code redundancy.

Comment on attachment 658448 [details] [diff] [review] patch v1.0 Review of attachment 658448 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/assertions.js @@ +420,5 @@ > + * @param {Number} aInterval Interval between evaluation attempts > + * @param {Object} thisObject this object > + */ > +Expect.prototype.waitFor = function Expect_waitFor(aCallback, aMessage, aTimeout, > + aInterval, thisObject) { I don't see a reason why we have to reimplement this method in the assertions module for Mozmill 1.5. We should call the mozmill.utils.waitFor() instance as for Assert and simply catch the exception to make it a soft assertion. @@ +498,5 @@ > + * @param {Object} thisObject this object > + */ > +Assert.prototype.waitFor = function Assert_waitFor(aCallback, aMessage, aTimeout, > + aInterval, thisObject) { > + var controller = mozmill.getBrowserController(); No, that's not what we want. Don't create a controller here. This module should have no references to the controller at all.

Ups, sorry for overriding Dave's review.

(In reply to Henrik Skupin (:whimboo) from comment #7) > Ups, sorry for overriding Dave's review. No problem, I was already working on it. Dave's simple comment caught exactly the essence

* fixed to address the elegant solution and catch the exception in the Expect part * removed creation of the controller in Assert part as Henrik suggested. We call mozmill.utils.waitFor

Comment on attachment 658843 [details] [diff] [review] patch v1.1 Review of attachment 658843 [details] [diff] [review]: ----------------------------------------------------------------- Before submitting the next patch please ensure that you have this tested. We have lib/tests/test_assertions.js therefore. ::: lib/assertions.js @@ +409,5 @@ > > /** > + * Waits for the callback evaluates to true > + * > + * @param {Callback} aCallback Callback for evaluation `Callback` is not a valid type. What you want here is Function. @@ +413,5 @@ > + * @param {Callback} aCallback Callback for evaluation > + * @param {String} aMessage Message to show for result > + * @param {Number} aTimeout Timeout in waiting for evaluation > + * @param {Number} aInterval Interval between evaluation attempts > + * @param {Object} thisObject this object `aThisObject` please @@ +420,5 @@ > + aInterval, thisObject) { > + try { > + mozmill.utils.waitFor(aCallback, aMessage, aTimeout, aInterval, thisObject); > + } catch (ex) { > + let diagnosis = arguments.callee.name + ": Timeout exceeded for '" + aCallback + "'"; Why do we need `arguments.callee.name`? That's not necessary here as what I can see. @@ +422,5 @@ > + mozmill.utils.waitFor(aCallback, aMessage, aTimeout, aInterval, thisObject); > + } catch (ex) { > + let diagnosis = arguments.callee.name + ": Timeout exceeded for '" + aCallback + "'"; > + > + return this._test(false, aMessage, diagnosis); Why you call `_test()` always with false? It would mean that an expect.waitFor() call will always fail. @@ +460,5 @@ > > +/** > + * Waits for the callback evaluates to true > + * > + * @param {Callback} aCallback Callback for evaluation Same as for Expect. @@ +464,5 @@ > + * @param {Callback} aCallback Callback for evaluation > + * @param {String} aMessage Message to show for result > + * @param {Number} aTimeout Timeout in waiting for evaluation > + * @param {Number} aInterval Interval between evaluation attempts > + * @param {Object} thisObject this object `aThisObject`

@@ +422,5 @@ > + mozmill.utils.waitFor(aCallback, aMessage, aTimeout, aInterval, thisObject); > + } catch (ex) { > + let diagnosis = arguments.callee.name + ": Timeout exceeded for '" + aCallback + "'"; > + > + return this._test(false, aMessage, diagnosis); Why you call `_test()` always with false? It would mean that an expect.waitFor() call will always fail. Why do you say that expect.waitFor() call will always fail? I am calling _test in the catch block which is the right behavior. we want to catch the timeout failure from mozmill.utils.waitFor in a catch block so that we have the desired soft assertion. Please test with any test you like, or with test_assertions.js, the expect.waitFor works as expected.

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #11) > Why do you say that expect.waitFor() call will always fail? I am calling > _test in the catch block which is the right behavior. we want to catch the > timeout failure from mozmill.utils.waitFor in a catch block so that we have > the desired soft assertion. You will have to call _test() not in the catch block but after the try/catch or in finalize. Otherwise we will not see a pass step. > Please test with any test you like, or with test_assertions.js, the > expect.waitFor works as expected. As said before you have not added tests for the new waitFor() method yet, which you have to do.

(In reply to Henrik Skupin (:whimboo) from comment #12) > (In reply to Maniac Vlad Florin (:vladmaniac) from comment #11) > > Why do you say that expect.waitFor() call will always fail? I am calling > > _test in the catch block which is the right behavior. we want to catch the > > timeout failure from mozmill.utils.waitFor in a catch block so that we have > > the desired soft assertion. > > You will have to call _test() not in the catch block but after the try/catch > or in finalize. Otherwise we will not see a pass step. > Agreed. Though I'm beginning to think we will have some show stoppers here but I'll try this today. > > Please test with any test you like, or with test_assertions.js, the > > expect.waitFor works as expected. > > As said before you have not added tests for the new waitFor() method yet, > which you have to do. Makes sense

As I suspected in comment 13 its not possible to not use the _test() call in the catch block. This is beacuse if we are not using it there, we need to re-define the failing condition from mozmill.utils.waitFor() and this will cause us to let go the elegant method Dave proposed in comment 4. What we can do is something like Expect.prototype.waitFor = function Expect_waitFor(aCallback, aMessage, aTimeout, aInterval, aThisObject) { try { mozmill.utils.waitFor(aCallback, aMessage, aTimeout, aInterval, aThisObject); } catch (ex) { let diagnosis = "Timeout exceeded for '" + aCallback + "'"; return this._test(false, aMessage, diagnosis); } return this._test(true, aMessage, undefined); }; this means pass the test if no errors occur in the try block and there is nothing to be caught in the catch block. This way we will see a pass step. As for using test_assertions.js, it will always fail for assert.waitFor because at the moment our wrapper does not return anything, it just throws the error from mozmill.utils.waitFor(). If we were to use test_assertions.js as a unit test we need to also add a try catch block to the assert.waitFor and re-throw the mozmill.utils.waitFor error. If this does not make sense to you guys, I'll try to provide more clarifications.

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #14) > As I suspected in comment 13 its not possible to not use the _test() call in > the catch block. This is beacuse if we are not using it there, we need to > re-define the failing condition from mozmill.utils.waitFor() and this will > cause us to let go the elegant method Dave proposed in comment 4. Really not sure what you mean here. We absolutely have to call _test() within this waitFor() method because we want to send a pass or fail frame. What result we send simply depends on the first parameter. > Expect.prototype.waitFor = function Expect_waitFor(aCallback, aMessage, > aTimeout, > aInterval, aThisObject) { > try { > mozmill.utils.waitFor(aCallback, aMessage, aTimeout, aInterval, > aThisObject); > } catch (ex) { > let diagnosis = "Timeout exceeded for '" + aCallback + "'"; > > return this._test(false, aMessage, diagnosis); > } > > return this._test(true, aMessage, undefined); > }; > > this means pass the test if no errors occur in the try block and there is > nothing to be caught in the catch block. This way we will see a pass step. That's exactly how it should look like. Just initialize the condition and diagnosis string before the try/catch block and overwrite their value as necessary. > As for using test_assertions.js, it will always fail for assert.waitFor > because at the moment our wrapper does not return anything, it just throws > the error from mozmill.utils.waitFor(). No, we catch those exceptions by using expect.throws() or expect.doesNotThrow(). So this assumption is not valid. > If we were to use test_assertions.js as a unit test we need to also add a > try catch block to the assert.waitFor and re-throw the mozmill.utils.waitFor > error. In Assert.waitFor() we do not catch the thrown exception from mozmill.utils.waitFor(). So why do you propose to re-throw an exception?

Considering Henrik's comments and internal reviews being passed, this is the follow up patch

Comment on attachment 660342 [details] [diff] [review] patch v1.2 Review of attachment 660342 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/assertions.js @@ +413,5 @@ > + * @param {Function} aCallback Callback for evaluation > + * @param {String} aMessage Message to show for result > + * @param {Number} aTimeout Timeout in waiting for evaluation > + * @param {Number} aInterval Interval between evaluation attempts > + * @param {Object} thisObject this object aThisObject @@ +419,5 @@ > +Expect.prototype.waitFor = function Expect_waitFor(aCallback, aMessage, aTimeout, > + aInterval, aThisObject) { > + let diagnosis = undefined; > + let condition = true; > + nit: trailing whitespace

* Sorry for the nits. Fixed

Comment on attachment 660394 [details] [diff] [review] patch v1.3 Review of attachment 660394 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/assertions.js @@ +417,5 @@ > + * @param {Object} aThisObject this object > + */ > +Expect.prototype.waitFor = function Expect_waitFor(aCallback, aMessage, aTimeout, > + aInterval, aThisObject) { > + let diagnosis = undefined; When you declare a variable you should initialize it with `null`. Undefined is not appropriate here. @@ +423,5 @@ > + > + try { > + mozmill.utils.waitFor(aCallback, aMessage, aTimeout, aInterval, aThisObject); > + } catch (ex) { > + diagnosis = "Timeout exceeded for '" + aCallback + "'"; Not sure if we really want to include the callback definition here. We probably should leave diagnosis empty and take ex.message for the message.

* fixed as requested * passed internal review with the new changes

Comment on attachment 660414 [details] [diff] [review] patch v1.4 Review of attachment 660414 [details] [diff] [review]: ----------------------------------------------------------------- Some minor nits I would like to see fixed. ::: lib/assertions.js @@ +418,5 @@ > + */ > +Expect.prototype.waitFor = function Expect_waitFor(aCallback, aMessage, aTimeout, > + aInterval, aThisObject) { > + let condition = true; > + var message = aMessage; Stick with var or let but don't mix both. Keep the one we make use of in this file. @@ +427,5 @@ > + message = ex.message; > + condition = false; > + } > + > + return this._test(condition, message, undefined); If no other parameters are following you can remove `undefined` from the list. ::: lib/tests/test_assertions.js @@ +45,5 @@ > { fun: "notMatch", params: ["Mozilla", /firefox/, "regex does not match string"], result: true}, > { fun: "notMatch", params: ["Mozilla", /Mozilla/, "regex matches string"], result: false}, > + > + { fun: "waitFor", params: [function () {return true;}, "Evaluation pass"], result: true}, > + { fun: "waitFor", params: [function () {return false;}, "Evaluation fail"], result: false} nit: missing blanks around the function statements.

* nits are gone

Pushed to default: http://hg.mozilla.org/qa/mozmill-tests/rev/0f65259bc23b Vlad, please check if the patch can safely be back-ported to other branches. I would like to see it landed across all of them.

(In reply to Henrik Skupin (:whimboo) from comment #23) > Pushed to default: > http://hg.mozilla.org/qa/mozmill-tests/rev/0f65259bc23b > > Vlad, please check if the patch can safely be back-ported to other branches. > I would like to see it landed across all of them. You can safely backport, just checked.

http://hg.mozilla.org/qa/mozmill-tests/rev/bf2bb4a6d4f1 (aurora) http://hg.mozilla.org/qa/mozmill-tests/rev/c9ea13826486 (beta) http://hg.mozilla.org/qa/mozmill-tests/rev/007bbaa24209 (release) http://hg.mozilla.org/qa/mozmill-tests/rev/967a5d18ce57 (esr10) Vlad, can you please file a new bug for the Mozmill product which will also need this implemented? Thanks.

When investigating an issue with disconnects in bug 711360 I discovered that these methods may suffer from a similar issue. They call the mozmill.utils.waitFor rather than controller.waitFor, the latter sends a message via the broker that keeps the connection alive and prevents a disconnect due to a timeout.

Oh, right. That sounds like the issue. So please get a bug filed against Mozmill. We will have to move the line from the controller variant to utils.js.