791634 - Make waitFor() method available in the assertions module (also for soft assertions)

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect, P2)

Description

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

Right now there is a controller.waitFor() call only which will raise an exception and stop the test if the assertion fails. There are a couple of reasons why we would need a function like this also for soft-assertions. In those cases we do not want that the test gets aborted right away.

The best place for this method should be in the assertions module. I wonder if we even should move its core implementation from utils.js to assertions.js.

Comment 1

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

For Mozmill 1.5 we did that move in our own copy of the assertions module on bug 787020.

Comment 2

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

We agreed on in the meeting. So lets get this in for Mozmill 2.0

Comment 3

[Daniela Petrovici]

Attachment: patch v1.0

Created patch for mozmill 2.0. 

I am not aware of the outcome of the discussion regarding moving the waitFor code from utils to assertions, so I left it as it is at the moment. 

I have also changed controller.waitFor with assert.waitFor in mutt tests since we now have this method in assertions.

Comment 4

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

Comment on attachment 736806 [details] [diff] [review]
patch v1.0

Review of attachment 736806 [details] [diff] [review]:
-----------------------------------------------------------------

Could you add some mutt tests for expect.waitFor? I also wonder if we should make controller.waitFor private as I suspect we will only want tests to use assert.waitFor or expect.waitFor.

Comment 5

[Daniela Petrovici]

Attachment: patch v1.1

Also added mutt tests for expect.waitFor and made controller.waitFor private.

Comment 6

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

Comment on attachment 738400 [details] [diff] [review]
patch v1.1

Review of attachment 738400 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozmill/mozmill/extension/resource/driver/controller.js
@@ +422,4 @@
>    return windowMap.contains(id) && windowMap.getValue(id, "loaded");
>  };
>  
> +MozMillController.prototype._waitFor = function (callback, message, timeout,

Why are you renaming this method? To keep backward compatibility we have to leave it in the MozMillController scope and simply forward the request to the assertion module instance.

::: mozmill/mozmill/extension/resource/modules/assertions.js
@@ +10,1 @@
>  var stack = require('stack');

Please fix the separation between Cu.import and require. Also declare it the same way as for mozmillFrame.

@@ +525,5 @@
> +    let condition = true;
> +    let message = aMessage;
> +
> +    try {
> +      utils.waitFor(aCallback, aMessage, aTimeout, aInterval, aThisObject);

Hm, we should get rid of utils.waitFor and implement its logic here. But the problem is that our base class is not Assert but Expect. It should have been made the other way around so Expect could catch possible assertions. I would say we tackle that on a new bug. But before doing so we should make the change in our mozmill-tests repository. Please file those 2 bugs.

::: mutt/mutt/tests/js/assertions/expect_waitfor_negative_tests.js
@@ +1,4 @@
> +var setupModule = function () {
> +  controller = mozmill.getBrowserController();
> +}
> +

Please make use of the tests we already have in our mozmill-tests repository for the waitFor calls.

::: mutt/mutt/tests/js/frame/httpd/testHTTPd2.js
@@ +30,4 @@
>    controller.open(testPages[0]);
>    controller.waitForPageLoad();
>  
> +  assert.waitFor(function () {

Beside those two changes in that file and waitfor.js I see way more `controller.waitFor()` calls in Mutt tests. Also I strongly believe that some Mozmill core code will have to be changed too.

Comment 7

[Daniela Petrovici]

(In reply to Henrik Skupin (:whimboo) from comment #6)
> 
> ::: mozmill/mozmill/extension/resource/driver/controller.js
> @@ +422,4 @@
> >    return windowMap.contains(id) && windowMap.getValue(id, "loaded");
> >  };
> >  
> > +MozMillController.prototype._waitFor = function (callback, message, timeout,
> 
> Why are you renaming this method? To keep backward compatibility we have to
> leave it in the MozMillController scope and simply forward the request to
> the assertion module instance.

Based on the discussion we had, controller is not loaded through the module loader, so assertions.js module has to be changed to look like the other older modules (e.g. utils.js). Due to this I have logged bug no. 868384

> > +      utils.waitFor(aCallback, aMessage, aTimeout, aInterval, aThisObject);
> 
> Hm, we should get rid of utils.waitFor and implement its logic here. But the
> problem is that our base class is not Assert but Expect. It should have been
> made the other way around so Expect could catch possible assertions. I would
> say we tackle that on a new bug. But before doing so we should make the
> change in our mozmill-tests repository. Please file those 2 bugs.

Logged bug 868371 for mozmill-tests and bug 868375 for mozmill 2.0

Comment 8

[Daniela Petrovici]

Attachment: patch v2.0

Made the changes per the last review. There is only one issue left on which I am not sure the current implementation is the one we want.

1) The mutt test verifies if an error of type AssertionError is thrown in line: https://github.com/mozilla/mozmill/blob/master/mutt/mutt/tests/js/assertions/expect_assert.js#L122
2) Due to the fact that Assert is not yet the base class in mozmill 2.0 (bug 868375), we are calling utils.waitFor inside assert.waitFor method
3) utils.waitFor throws an error of type TimeoutError
4) So, assert.waitFor is the only method in Assert class that will throw TimeoutError instead of AssertionError
5) Due to the above, the mutt test will fail because the expected error is of type AssertionError and the actual error is of type TimeoutError.

So, I have created a try-catch block were we catch the TimeoutError and throw an AssertionError instead until bug 868375 is fixed.

If this is not the correct approach, please tell me if I should mark this bug as dependent on bug 868375 or if we are ok with the mutt test failing at the moment. Another possibility would be to change the mutt test to handle only this case until bug 868375.

Comment 9

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

Comment on attachment 749242 [details] [diff] [review]
patch v2.0

Review of attachment 749242 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozmill/mozmill/extension/resource/driver/controller.js
@@ +23,5 @@
>  var hwindow = Cc["@mozilla.org/appshell/appShellService;1"]
>                .getService(Ci.nsIAppShellService).hiddenDOMWindow;
>  
> +var assert = new assertions.Assert();
> +var waitFor = assert.waitFor;

Please move this back to the original position. Those are still utils functions.

::: mozmill/mozmill/extension/resource/modules/assertions.js
@@ +524,5 @@
> +    let condition = true;
> +    let message = aMessage;
> +
> +    try {
> +      utils.waitFor(aCallback, aMessage, aTimeout, aInterval, aThisObject);

I would move all the code from the utils.waitFor method in here and mark it as deprecated. Utils could then call assert.waitFor() too.

::: mozmill/mozmill/extension/resource/modules/driver.js
@@ +88,4 @@
>   * @memberOf driver
>   */
> +var assert = new assertions.Assert();
> +var waitFor = assert.waitFor;

This should be removed in favor of assert/expect. We should propagate it as hard assertion only in the driver module.

::: mutt/mutt/tests/js/utils/waitfor.js
@@ +12,3 @@
>        return true;
>      });
>    }, "TypeError", "Return type 'boolean' in callback is supported.");

I think I mentioned that before, but there is no need anymore to encapsulate the waitFor() call into doesNotThrow when using expect.waitFor().

Comment 10

[Daniela Petrovici]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> Comment on attachment 749242 [details] [diff] [review]
> > +var assert = new assertions.Assert();
> > +var waitFor = assert.waitFor;
> 
> This should be removed in favor of assert/expect. We should propagate it as
> hard assertion only in the driver module.

I am not sure if I understand this correctly. Should I change var waitFor = assert.waitFor with expect.waitFor since I am already in driver.js?
Also, since we do not want to propagate it as hard assertions, should frame.js and controller.js also use expect.waitFor instead of a hard assertion?

> ::: mutt/mutt/tests/js/utils/waitfor.js
> @@ +12,3 @@
> >        return true;
> >      });
> >    }, "TypeError", "Return type 'boolean' in callback is supported.");
> 
> I think I mentioned that before, but there is no need anymore to encapsulate
> the waitFor() call into doesNotThrow when using expect.waitFor().

I am using assert.waitFor here. Should I use expect.waitFor instead?

Comment 11

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

(In reply to Daniela Petrovici from comment #10)
> (In reply to Henrik Skupin (:whimboo) from comment #9)
> > Comment on attachment 749242 [details] [diff] [review]
> > > +var assert = new assertions.Assert();
> > > +var waitFor = assert.waitFor;
> > 
> > This should be removed in favor of assert/expect. We should propagate it as
> > hard assertion only in the driver module.
> 
> I am not sure if I understand this correctly. Should I change var waitFor =
> assert.waitFor with expect.waitFor since I am already in driver.js?

No, as similar to the other modules driver.js should not introduce a waitFor variable and also should not export it.

> Also, since we do not want to propagate it as hard assertions, should
> frame.js and controller.js also use expect.waitFor instead of a hard
> assertion?

Huh, why that? I assume you mis-read my last comment.  We do not want to use expect as replacement for controller.waitFor() calls.

> > ::: mutt/mutt/tests/js/utils/waitfor.js
> > @@ +12,3 @@
> > >        return true;
> > >      });
> > >    }, "TypeError", "Return type 'boolean' in callback is supported.");
> > 
> > I think I mentioned that before, but there is no need anymore to encapsulate
> > the waitFor() call into doesNotThrow when using expect.waitFor().
> 
> I am using assert.waitFor here. Should I use expect.waitFor instead?

You asked a question which already got the anwer in my last comment. So not sure what else i should add here.

Comment 12

[Daniela Petrovici]

Attachment: patch v3.0

Made changes based on review

Comment 13

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

Comment on attachment 753730 [details] [diff] [review]
patch v3.0

Review of attachment 753730 [details] [diff] [review]:
-----------------------------------------------------------------

Some things are remaining to investigate and to get updated. In general it looks close now.

::: mozmill/mozmill/extension/resource/modules/assertions.js
@@ +525,5 @@
> +   * @param {Object} aThisObject this object
> +   */
> +  waitFor: function Expect_waitFor(aCallback, aMessage, aTimeout, aInterval, aThisObject) {
> +    broker.log({'function': 'waitFor() - DEPRECATED',
> +              'message': 'waitFor() is deprecated. '});

nit: indentation fix necessary.

@@ +547,5 @@
> +           self.timeIsUp = Date.now() > deadline;
> +         }
> +       }
> +
> +       var timeoutInterval = hwindow.setInterval(wait, aInterval);

I wonder if we could get rid of the hwindow code here and make use of utils.sleep instead.

@@ +555,5 @@
> +       while (self.result !== true && !self.timeIsUp) {
> +         thread.processNextEvent(true);
> +
> +         let type = typeof(self.result);
> +         if (type !== 'boolean')

please always add brackets around code blocks even if it is a single line.

::: mozmill/mozmill/extension/resource/stdlib/utils.js
@@ +16,5 @@
>  
>  Cu.import("resource://gre/modules/NetUtil.jsm");
>  
> +var assertions = {}; Cu.import('resource://mozmill/modules/assertions.js', assertions);
> +var assert = new assertions.Assert();

nit: missing emptly line between module inclusions and declaration of new variables.

@@ +260,4 @@
>   * Waits for the callback evaluates to true
>   */
>  function waitFor(callback, message, timeout, interval, thisObject) {
> +  assert.waitFor(callback, message, timeout, interval, thisObject);

Please mark this method as obsolete. See some of the MozmillController methods.

Comment 14

[Daniela Petrovici]

Attachment: patch v3.1

(In reply to Henrik Skupin (:whimboo) from comment #13)
> Comment on attachment 753730 [details] [diff] [review]
> patch v3.0
> I wonder if we could get rid of the hwindow code here and make use of
> utils.sleep instead.

I tried but it does not work because then assertions.js will import utils.js which will import assertions.js again.

Comment 15

[cmtalbert]

Comment on attachment 754780 [details] [diff] [review]
patch v3.1

Review of attachment 754780 [details] [diff] [review]:
-----------------------------------------------------------------

this looks good to me. r+

Comment 16

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

Comment on attachment 754780 [details] [diff] [review]
patch v3.1

Review of attachment 754780 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good now. Thanks for the final update.

Landed on master:
https://github.com/mozilla/mozmill/commit/54ab122f6d328eaa201fe226d22517df5aad1cb0

Comment 17

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

The landing of this patch introduced a major regression with waitForPageLoad because waitFor method as ported from utils.js has been modified internally. I never expected that this could happen. 

Daniela, has this patch ever been tested deeply across platforms? I never have seen those results. So in the future we will absolutely depend on those before we land patches.

Backed out:
https://github.com/mozilla/mozmill/commit/300232fcf34df8aba77cba1307d975756876874d

Comment 18

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

Comment on attachment 754780 [details] [diff] [review]
patch v3.1

Review of attachment 754780 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good now. Thanks for the final update.

Landed on master:
https://github.com/mozilla/mozmill/commit/54ab122f6d328eaa201fe226d22517df5aad1cb0

::: mozmill/mozmill/extension/resource/modules/assertions.js
@@ +542,5 @@
> +       var deadline = Date.now() + aTimeout;
> +
> +       function wait() {
> +         if (self.result !== true) {
> +           self.result = aCallback.call(aTimeout);

Why has this been changed from aThisObject to aTimeout?

Not sure what else changed in this method, so please analyze that and tell us about every change you have made. I never expected a change like this inside this method, given that it was a simply copy and paste.

Comment 19

[Daniela Petrovici]

(In reply to Henrik Skupin (:whimboo) from comment #18)
> Why has this been changed from aThisObject to aTimeout?
> 
> Not sure what else changed in this method, so please analyze that and tell
> us about every change you have made. I never expected a change like this
> inside this method, given that it was a simply copy and paste.
I have changed the parameters to have "a" before the name - e.g. aThisObject instead of thisObject. Also I had to add everything in a try-catch block since Expect.waitFor should not throw an error.

Except for the above issue, there was one other. In case of timeout error, the code threw an TimeoutError which is defined in the utils.js file, but not assertions.js. My understanding is that all the functions in assertions module should throw an AssertionError. So, we should just throw an Error instead of TimeoutError and remove lines 237-254 from utils.js (https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/stdlib/utils.js#L237). In case we want to throw a TimeoutError at assert.waitFor we will need to also move the above lines from utils.js to assertions.js file. Please tell me if removing TimeoutError from utils.js and throwing Error in assertions is the correct approach.

Also, related to running the tests, I have run the mutt ones, but not mozmill-tests. For the new patch I will also add mozmill-test reports for all platforms.

Comment 20

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

Why would you remove the current code which throws the TimeoutError? I don't see why we should do that. As what the code in waitFor() does is to wait for a specific condition and if we timeout we have to raise a TimeoutError. ONLY in other cases e.g. syntax errors we should throw something different.

As mentioned in the meeting the real underlying issue here is that you have replaced the parameter for the callback with aTimeout. So we do no longer push the thisObject in. Please add a test for this particular issue.

Otherwise regarding the try/catch block, please lets get bug 868375 done first, so we can avoid unnecessarily patch this code.

Comment 21

[Daniela Petrovici]

(In reply to Henrik Skupin (:whimboo) from comment #20)
> Otherwise regarding the try/catch block, please lets get bug 868375 done
> first, so we can avoid unnecessarily patch this code.
I will start working then on making the Assert the base class. Due to the dependencies between bugs, these are the steps I will follow:
1) work on bug 868371 and fix it - make Assert base class in mozmill-tests library for 1.5 (due to comment #1 in bug 868375)
2) work on bug 868375 and fix it - make Assert base class in mozmill 2.0
3) then finish the remaining work on this bug.

Comment 22

[Daniela Petrovici]

Attachment: patch v3.2

Reports:
MAC: http://mozmill-crowd.blargon7.com/#/functional/report/91511d5536e962e0de7f57ed1ee6e2a4

Linux:
http://mozmill-crowd.blargon7.com/#/functional/report/91511d5536e962e0de7f57ed1ee4f572 - clean
http://mozmill-crowd.blargon7.com/#/functional/report/91511d5536e962e0de7f57ed1ee49837 - with changes from this patch

Comment 23

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

Comment on attachment 768247 [details] [diff] [review]
patch v3.2

Review of attachment 768247 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozmill/mozmill/extension/resource/driver/controller.js
@@ +67,2 @@
>          return this.node.firedEvents[e] == true;
>        }, "Timeout happened before event '" + ex +"' was fired.", timeout, interval);

Please update the message so it reflects what we are expecting.

@@ +249,2 @@
>      return window != null && this.isLoaded();
>    }, "controller(): Window could not be initialized.", undefined, undefined, this);

Same here.

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +23,4 @@
>  // A list of all the subclasses available.  Shared modules can push their own subclasses onto this list
>  var subclasses = [MozMillCheckBox, MozMillRadio, MozMillDropList, MozMillTextBox];
>  
> +var assert = new assertions.Assert();

Can you move this up before the other var declarations?

@@ +492,2 @@
>      return elem.exists();
>    }, "Timeout exceeded for waitForElement " + this.getInfo(), timeout, interval);

We should correct the message. It should reflect what we are waiting for.

@@ +502,2 @@
>      return !elem.exists();
>    }, "Timeout exceeded for waitForElementNotPresent " + this.getInfo(), timeout, interval);

Same here.

@@ +567,2 @@
>            return element.checked == state;
>          }, "Checkbox " + this.getInfo() + " could not be checked/unchecked", 500);

Same here.

@@ +628,3 @@
>          // If we have a XUL element, unwrap its XPCNativeWrapper
>          if (element.namespaceURI == NAMESPACE_XUL) {
>            element = utils.unwrapNode(element);

Same here.

::: mozmill/mozmill/extension/resource/modules/assertions.js
@@ +11,5 @@
>  var broker = {}; Cu.import('resource://mozmill/driver/msgbroker.js', broker);
>  var stack = {}; Cu.import('resource://mozmill/modules/stack.js', stack);
>  
> +var hwindow = Cc["@mozilla.org/appshell/appShellService;1"].
> +              getService(Ci["nsIAppShellService"]).hiddenDOMWindow;

Please use Ci.nsIAppShallService here.

@@ +45,5 @@
> + * TimeoutError
> + *
> + * Error object used for timeouts
> + */
> +function TimeoutError(message, fileName, lineNumber) {

Errors are now located in a separate module. So you don't have to add this anymore. Please update to latest master.

@@ +77,5 @@
>  
> +  /**
> +   * Error object thrown by the Assert class in case of a timeout
> +   */
> +  TimeoutError: TimeoutError,

Not necessary anymore.

@@ +619,5 @@
> +            this object
> +   */
> +  waitFor: function Assert_waitFor(aCallback, aMessage, aTimeout, aInterval, aThisObject) {
> +    timeout = aTimeout || 5000;
> +    interval = aInterval || 100;

Both lines miss the var declaration.

::: mozmill/mozmill/extension/resource/modules/driver.js
@@ -85,5 @@
> - *
> - * @type utils.waitFor
> - * @memberOf driver
> - */
> -var waitFor = utils.waitFor;

Lets keep this in and just forward to assertions.Assert.waitFor().

::: mozmill/mozmill/extension/resource/modules/frame.js
@@ +439,2 @@
>      return shutdown;
>    }, "Local HTTP server has been stopped", TIMEOUT_SHUTDOWN_HTTPD);

This most likely needs an update due to its bitrotted.

::: mozmill/mozmill/extension/resource/stdlib/utils.js
@@ +428,2 @@
>      return ready;
>    }, "Saving dataURL to '" + file.path + "' has been timed out.");

Please update the message here too.

::: mutt/mutt/tests/js/assertions/expect_assert.js
@@ +59,5 @@
>    { fun: "doesNotThrow", params: [function () { throw new Error(); }, undefined, "Throws error"],
> +    result: false, throws: Error},
> +
> +  { fun: "waitFor", params: [function () { return true; }, "Evaluation pass"], result: true},
> +  { fun: "waitFor", params: [function () { return false; }, "Evaluation fail", undefined, undefined, 0],

Please add a timeout of 100ms and a delay of 10ms. There is no need for us to wait 5s here.

@@ +60,5 @@
> +    result: false, throws: Error},
> +
> +  { fun: "waitFor", params: [function () { return true; }, "Evaluation pass"], result: true},
> +  { fun: "waitFor", params: [function () { return false; }, "Evaluation fail", undefined, undefined, 0],
> +    result: false, throws: assert.TimeoutError},

The timeout error is in another module now and accessible via errors.TimeoutError.

@@ +62,5 @@
> +  { fun: "waitFor", params: [function () { return true; }, "Evaluation pass"], result: true},
> +  { fun: "waitFor", params: [function () { return false; }, "Evaluation fail", undefined, undefined, 0],
> +    result: false, throws: assert.TimeoutError},
> +  { fun: "waitFor", params: [function () { return false; }, "Evaluation fail"],
> +    result: false, throws: assert.TimeoutError} 

I don't think we need this twice. So this last test can be removed.

@@ +121,4 @@
>      if (test.result === true) {
>        expect.doesNotThrow(function() {
>          assert[test.fun].apply(assert, test.params);
> +      }, assert.AssertionError || assert.TimeoutError, message);

Same as above. Those errors are under errors.* now.

::: mutt/mutt/tests/js/frame/httpd/testHTTPd2.js
@@ +30,2 @@
>      return persisted.requestSucceeded;
>    }, "HTTPd.js does not respond with a Bad Request");

The file has been relocated.

::: mutt/mutt/tests/js/utils/waitfor.js
@@ -8,2 @@
>  
>  var testWaitForCallback = function () {

I think the whole file should now be moved from utils to assertions.

Comment 24

[Daniela Petrovici]

Attachment: patch v3.3

Made changes based on review. 

Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc94722cb40
http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc947170cef

Comment 25

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

Comment on attachment 773983 [details] [diff] [review]
patch v3.3

Review of attachment 773983 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozmill/mozmill/extension/resource/driver/controller.js
@@ +27,2 @@
>  var sleep = utils.sleep;
> +var waitFor = assert.waitFor;

Would you mind to flip the lines with sleep, so that all variables related to assertions are grouped together and are not separated by other functions?

@@ +248,2 @@
>      return window != null && this.isLoaded();
> +  }, "controller(): Window has been initialized.", undefined, undefined, this);

I think we should get rid of 'undefined, undefined, this' just by making use of self. That would reduce possible failures in refactoring in the future too.

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +25,1 @@
>  var subclasses = [MozMillCheckBox, MozMillRadio, MozMillDropList, MozMillTextBox];

Can you see that this new line has been inserted in the wrong line? I completely separates the comment from the real declaration.

@@ +491,2 @@
>      return elem.exists();
> +  }, "Element '" + this.getInfo() + "' has been found", timeout, interval);

Please mention the function name in the message. Similar to all of our functions in controller.js. That will make it much easier to identify possible failures. This applies to all instances.

::: mozmill/mozmill/extension/resource/modules/assertions.js
@@ +11,5 @@
>  var broker = {}; Cu.import('resource://mozmill/driver/msgbroker.js', broker);
>  var errors = {}; Cu.import('resource://mozmill/modules/errors.js', errors);
>  var stack = {}; Cu.import('resource://mozmill/modules/stack.js', stack);
>  
> +var hwindow = Services.appShell.hiddenDOMWindow;

As already mentioned in my last review please move this into the waitFor method.

@@ +653,5 @@
> +  let condition = true;
> +  let message = aMessage;
> +
> +  try {
> +    Assert.prototype.waitFor.call(this, aCallback, aMessage, aTimeout, aInterval, aThisObject);

You can do 'Assert.prototype.waitFor.apply(this, arguments)' here, which will automatically forward all the arguments to the called method.

@@ +655,5 @@
> +
> +  try {
> +    Assert.prototype.waitFor.call(this, aCallback, aMessage, aTimeout, aInterval, aThisObject);
> +  }
> +  catch (ex) {

You only want to catch errors of type AssertionError here. Otherwise we will lose important exceptions from the inner code.

::: mutt/mutt/tests/js/assertions/expect_assert.js
@@ +59,5 @@
>    { fun: "doesNotThrow", params: [function () { throw new Error(); }, undefined, "Throws error"],
> +    result: false, throws: Error},
> +
> +  { fun: "waitFor", params: [function () { return true; }, "Evaluation pass"], result: true},
> +  { fun: "waitFor", params: [function () { return false; }, "Evaluation fail", 100, 10, 0],

Sorry, but what is '0' at the end of the parameter list? I would expect the thisObject here but not a number. I think we can remove that, right?

@@ +119,4 @@
>      if (test.result === true) {
>        expect.doesNotThrow(function() {
>          assert[test.fun].apply(assert, test.params);
> +      }, errors.AssertionError || errors.TimeoutError, message);

Why have you added TimeoutError here?

::: mutt/mutt/tests/js/assertions/waitfor.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var setupModule = function () {
> +  controller = mozmill.getBrowserController();

While we are at it please also add aModule here.

Comment 26

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

Comment on attachment 773983 [details] [diff] [review]
patch v3.3

Review of attachment 773983 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozmill/mozmill/extension/resource/driver/controller.js
@@ +27,2 @@
>  var sleep = utils.sleep;
> +var waitFor = assert.waitFor;

Would you mind to flip the lines with sleep, so that all variables related to assertions are grouped together and are not separated by other functions?

@@ +248,2 @@
>      return window != null && this.isLoaded();
> +  }, "controller(): Window has been initialized.", undefined, undefined, this);

I think we should get rid of 'undefined, undefined, this' just by making use of self. That would reduce possible failures in refactoring in the future too.

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +25,1 @@
>  var subclasses = [MozMillCheckBox, MozMillRadio, MozMillDropList, MozMillTextBox];

Can you see that this new line has been inserted in the wrong line? I completely separates the comment from the real declaration.

@@ +491,2 @@
>      return elem.exists();
> +  }, "Element '" + this.getInfo() + "' has been found", timeout, interval);

Please mention the function name in the message. Similar to all of our functions in controller.js. That will make it much easier to identify possible failures. This applies to all instances.

::: mozmill/mozmill/extension/resource/modules/assertions.js
@@ +11,5 @@
>  var broker = {}; Cu.import('resource://mozmill/driver/msgbroker.js', broker);
>  var errors = {}; Cu.import('resource://mozmill/modules/errors.js', errors);
>  var stack = {}; Cu.import('resource://mozmill/modules/stack.js', stack);
>  
> +var hwindow = Services.appShell.hiddenDOMWindow;

As already mentioned in my last review please move this into the waitFor method.

@@ +653,5 @@
> +  let condition = true;
> +  let message = aMessage;
> +
> +  try {
> +    Assert.prototype.waitFor.call(this, aCallback, aMessage, aTimeout, aInterval, aThisObject);

You can do 'Assert.prototype.waitFor.apply(this, arguments)' here, which will automatically forward all the arguments to the called method.

@@ +655,5 @@
> +
> +  try {
> +    Assert.prototype.waitFor.call(this, aCallback, aMessage, aTimeout, aInterval, aThisObject);
> +  }
> +  catch (ex) {

You only want to catch errors of type AssertionError here. Otherwise we will lose important exceptions from the inner code.

::: mutt/mutt/tests/js/assertions/expect_assert.js
@@ +59,5 @@
>    { fun: "doesNotThrow", params: [function () { throw new Error(); }, undefined, "Throws error"],
> +    result: false, throws: Error},
> +
> +  { fun: "waitFor", params: [function () { return true; }, "Evaluation pass"], result: true},
> +  { fun: "waitFor", params: [function () { return false; }, "Evaluation fail", 100, 10, 0],

Sorry, but what is '0' at the end of the parameter list? I would expect the thisObject here but not a number. I think we can remove that, right?

@@ +119,4 @@
>      if (test.result === true) {
>        expect.doesNotThrow(function() {
>          assert[test.fun].apply(assert, test.params);
> +      }, errors.AssertionError || errors.TimeoutError, message);

Why have you added TimeoutError here?

::: mutt/mutt/tests/js/assertions/waitfor.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var setupModule = function () {
> +  controller = mozmill.getBrowserController();

While we are at it please also add aModule here.

Comment 27

[Daniela Petrovici]

(In reply to Henrik Skupin (:whimboo) from comment #26)
> @@ +655,5 @@
> > +  catch (ex) {
> 
> You only want to catch errors of type AssertionError here. Otherwise we will
> lose important exceptions from the inner code.
assert.waitFor throws TimeoutError, not AssertionError. Shouldn't we catch only TimeoutError?
 
> ::: mutt/mutt/tests/js/assertions/expect_assert.js
> @@ +59,5 @@
> > +  { fun: "waitFor", params: [function () { return false; }, "Evaluation fail", 100, 10, 0],
> 
> Sorry, but what is '0' at the end of the parameter list? I would expect the
> thisObject here but not a number. I think we can remove that, right?
I modified this based on comment #20 where a mutt test was requested for pushing aTimeout instead of aThisObject to the callback method. 
But I can change the test to be:
{ fun: "waitFor", params: [function () { return !(this instance of Number); }, "Evaluation fail", 100, 10, 0]
To make it more suggestive. Would that be ok?

> @@ +119,4 @@
> >      if (test.result === true) {
> >        expect.doesNotThrow(function() {
> >          assert[test.fun].apply(assert, test.params);
> > +      }, errors.AssertionError || errors.TimeoutError, message);
> 
> Why have you added TimeoutError here?
I did this because the assert/expect.waitFor throw an error of type TimeoutError. Shouldn't we also check that this error was not thrown?

Comment 28

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

(In reply to Daniela Petrovici from comment #27)

FYI, if you are replying to code review comments please use splinter review in the future and not a comment on the bug. It's very hard that way to figure out which code is referred to here...

> (In reply to Henrik Skupin (:whimboo) from comment #26)
> > @@ +655,5 @@
> > > +  catch (ex) {
> > 
> > You only want to catch errors of type AssertionError here. Otherwise we will
> > lose important exceptions from the inner code.
> assert.waitFor throws TimeoutError, not AssertionError. Shouldn't we catch
> only TimeoutError?

I would propose we get rid of TimeoutError here. Otherwise it will be the one and only function here in this module which does not throw an AssertionError. Also make sure to update the jsdoc header of the method to include the @throws line.

> > ::: mutt/mutt/tests/js/assertions/expect_assert.js
> > @@ +59,5 @@
> > > +  { fun: "waitFor", params: [function () { return false; }, "Evaluation fail", 100, 10, 0],
> > 
> > Sorry, but what is '0' at the end of the parameter list? I would expect the
> > thisObject here but not a number. I think we can remove that, right?
> I modified this based on comment #20 where a mutt test was requested for
> pushing aTimeout instead of aThisObject to the callback method. 
> But I can change the test to be:
> { fun: "waitFor", params: [function () { return !(this instance of Number);
> }, "Evaluation fail", 100, 10, 0]
> To make it more suggestive. Would that be ok?

Oh, I see. So I would suggest we pass in a string like 'no_this_object' to make it clearer given that no comment is placed around. Also make use of 'instanceof'. I wouldn't expect that 'instance of' works.

> > @@ +119,4 @@
> > >      if (test.result === true) {
> > >        expect.doesNotThrow(function() {
> > >          assert[test.fun].apply(assert, test.params);
> > > +      }, errors.AssertionError || errors.TimeoutError, message);
> > 
> > Why have you added TimeoutError here?
> I did this because the assert/expect.waitFor throw an error of type
> TimeoutError. Shouldn't we also check that this error was not thrown?

See above how to get this solved.

Comment 29

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: 0001-Bug-791634-Make-waitFor-method-available-in-the-asse.patch

Comment 30

[Cosmin Malutan, [:cosmin-malutan]]

Comment on attachment 773983 [details] [diff] [review]
patch v3.3

I changed what was required in previous comment. 
Reports
Without the patch:
http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc947c8b624

With patch:
http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc947c4841a
http://mozmill-crowd.blargon7.com/#/functional/report/5aa1ca5e7015e3740d269dc947bf188a

Comment 31

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

Comment on attachment 777788 [details] [diff] [review]
0001-Bug-791634-Make-waitFor-method-available-in-the-asse.patch

Review of attachment 777788 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. There are only some nits to get addressed. I hope we can get it landed today. Thanks Cosmin!

::: mozmill/mozmill/extension/resource/driver/controller.js
@@ +28,1 @@
>  var sleep = utils.sleep;

nit: We should add a blank line before to separate the sleep variable. Otherwise it would also conflict with the alphabetical order.

::: mozmill/mozmill/extension/resource/modules/assertions.js
@@ +657,5 @@
> +
> +  try {
> +    Assert.prototype.waitFor.apply(this, arguments);
> +  }
> +  catch (ex if ex instanceof errors.TimeoutError) {

We do no longer throw a TimeoutError. So this has to be an AssertionError. Also only that one we want to catch here, all the others should pass this check and bubble up to the test.

::: mutt/mutt/tests/js/assertions/expect_assert.js
@@ +60,5 @@
> +    result: false, throws: Error},
> +
> +  { fun: "waitFor", params: [function () { return true; }, "Evaluation pass"], result: true},
> +  { fun: "waitFor", params: [function () { return !(this instanceof String); }, "Evaluation fail", 100, 10, "no_this_object"],
> +    result: false, throws: errors.AssertionError}

Thinking more about it I would make it a positive test and check that we have a string here. Only if that doesn't apply we should raise the exception. That was the problem with Daniela's formerly patch when we had a Number there.

::: mutt/mutt/tests/js/utils/waitfor.js
@@ -41,5 @@
> -      onlyTheFirst = false;
> -
> -      return res;
> -    }, undefined, 200);
> -  }, errors.TimeoutError, "waitFor() has to pass after the first true is returned.");

Would you mind to check if there are other TimeoutError usages around which might need an update? If not we probably also want to remove this error class.

Comment 32

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v3.0

(In reply to Henrik Skupin (:whimboo) from comment #31)
> ::: mozmill/mozmill/extension/resource/modules/assertions.js
> @@ +657,5 @@
> > +
> > +  try {
> > +    Assert.prototype.waitFor.apply(this, arguments);
> > +  }
> > +  catch (ex if ex instanceof errors.TimeoutError) {
> 
> We do no longer throw a TimeoutError. So this has to be an AssertionError.
> Also only that one we want to catch here, all the others should pass this
> check and bubble up to the test.
Is the only error thrown from here an you were right is AssertionError.
> Would you mind to check if there are other TimeoutError usages around which
> might need an update? If not we probably also want to remove this error
> class.
No is not used anywhere else. So I removed it.

Comment 33

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

Comment on attachment 779792 [details] [diff] [review]
patch v3.0

Review of attachment 779792 [details] [diff] [review]:
-----------------------------------------------------------------

Beside the inline comment I do not see that testMultipleLoads.js has been updated to use the assertions module instead of controller for the waitFor call. The same applies to a lot of other instances in tests which have been recently added.

Please make this bug your priority for today so we can really get it landed.

::: mozmill/mozmill/extension/resource/modules/errors.js
@@ -77,5 @@
>  }
>  
> -AssertionError.prototype = Object.create(BaseError.prototype, {
> -  constructor : { value : AssertionError }
> -});

Why is the prototype of AssertionError removed here?

Comment 34

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v4.0

(In reply to Henrik Skupin (:whimboo) from comment #33)
> ::: mozmill/mozmill/extension/resource/modules/errors.js
> @@ -77,5 @@
> >  }
> >  
> > -AssertionError.prototype = Object.create(BaseError.prototype, {
> > -  constructor : { value : AssertionError }
> > -});
> 
> Why is the prototype of AssertionError removed here?
I'm sorry for that, it was a mistake.

Comment 35

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

Comment on attachment 780343 [details] [diff] [review]
patch v4.0

Review of attachment 780343 [details] [diff] [review]:
-----------------------------------------------------------------

There are still instances of controller.waitFor() existent. Please check that carefully.

./mozmill/mozmill/extension/resource/driver/controller.js:      this._controller.waitFor(function () {
./mozmill/mozmill/extension/resource/driver/controller.js:    this._controller.waitFor(function () {

Comment 36

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v5.0

Comment 37

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

Comment on attachment 780394 [details] [diff] [review]
patch v5.0

Review of attachment 780394 [details] [diff] [review]:
-----------------------------------------------------------------

I think that looks fine now. But please make sure to keep the right versioning of your patches. This patch is version 5.0 and not 3.0. Otherwise please remove '(also for soft assertions)' from the commit message. Soft assertions are  part of the assertions module and don't have to be specified separately.

Comment 38

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v6.0

Comment 39

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

Comment on attachment 780413 [details] [diff] [review]
patch v6.0

Versions are still not correct...

Comment 40

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

Comment on attachment 780413 [details] [diff] [review]
patch v6.0

Review of attachment 780413 [details] [diff] [review]:
-----------------------------------------------------------------

The commit message is still broken: 'module -r=hskupin'. Please fix this.

Comment 41

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v5.0

Comment 42

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v6.1

Comment 43

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

Landed as:
https://github.com/mozilla/mozmill/commit/0b8ffe04caf5f36f512aadab164b35aa9398ea64