Change utils.waitFor method to throw distinct TimeoutErrors

VERIFIED FIXED

Status

Testing Graveyard
Mozmill
VERIFIED FIXED
7 years ago
2 years ago

People

(Reporter: whimboo, Assigned: ahal)

Tracking

Details

(Whiteboard: [mozmill-2.0+][mozmill-1.5.4+])

Attachments

(2 attachments)

(Reporter)

Description

7 years ago
Right now the waitFor method throws a plain Error which makes it hard for any code to distinguish between timeouts and real failures.

I implement a workaround for that now in our api refactor repo on bug 634227.

Updated

7 years ago
Whiteboard: [mozmill-2.0?] → [mozmill-2.0+]
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Created attachment 516724 [details] [diff] [review]
Patch 1.0 - Throw a custom error message in waitFor

Create a new TimeoutError object which inherits from Error.prototype.
Attachment #516724 - Flags: review?(ctalbert)
Attachment #516724 - Flags: review?(ctalbert)
Created attachment 516743 [details] [diff] [review]
Patch 1.1 - Keep the err.stack this time

The last patch loses the stack property derived from the Error object.  I would have thought the proper way to do this was something along the lines of 'Error.apply(this, arguments)' but that didn't work for some reason.  This works around that issue and preserves all the good info found in the Error object.
Attachment #516743 - Flags: review?(ctalbert)
(Reporter)

Comment 3

7 years ago
Comment on attachment 516743 [details] [diff] [review]
Patch 1.1 - Keep the err.stack this time

>     message = message || arguments.callee.name + ": Timeout exceeded for '" + callback + "'";

Please also fix that line. Right now it's pretty useless if a message has been specified.

Comment 4

7 years ago
Comment on attachment 516743 [details] [diff] [review]
Patch 1.1 - Keep the err.stack this time

This looks good.  Henrik, I believe the "message = message ||" line should stay as is.  The thinking here is that if the test supplies a specific message, then we use that for the error.  If the test does not supply a message, we use our default message.  That's exactly how an API should work in my opinion -- assuming that the user provided values always trump default values.
Attachment #516743 - Flags: review?(ctalbert) → review+
I tend to agree with Clint. That line is only as useless as the message you send it.  Otherwise you always have the option of not sending a message at all.
(Reporter)

Comment 6

7 years ago
Comment on attachment 516743 [details] [diff] [review]
Patch 1.1 - Keep the err.stack this time

>+  var err = new Error();
>+  if (err.stack) {
>+    this.stack = err.stack;
>+  }
>+  this.message = message === undefined ? err.message : message;
>+  this.fileName = fileName === undefined ? err.fileName : fileName;
>+  this.lineNumber = lineNumber === undefined ? err.lineNumber : lineNumber;
>+};
>+TimeoutError.prototype = new Error();
>+TimeoutError.prototype.constructor = TimeoutError;
>+TimeoutError.prototype.name = 'TimeoutError';

Please also check bug 634227 how I did that for our shared modules. I wonder if we find a consensus in the middle. At least a separate module would be nice to not clutter utils.js with more and more methods.

Also TimeoutError would have to be added to the EXPORTED_SYMBOLS list, otherwise it's pretty useless for us.

Re: 'message' you are right. With the given new error class we can identify errors more easily.
Attachment #516743 - Flags: feedback-
> Please also check bug 634227 how I did that for our shared modules.
I did look at that patch, and it seems to me that it is even bigger than this one (i.e adding the findCallerFrame method).  I want to do this properly, which means inheriting from Error.prototype. It may not make a huge difference to us, but it will definitely change the way Firefox or other people handle this error.

> At least a separate module would be nice to
> not clutter utils.js with more and more methods.
I'm not opposed to moving it somewhere else, but I don't think we want an entire new module just for one custom Error. I also don't think we'll really find a need to make any other custom Errors either.  In most cases we should just be able to throw one of the six default errors: http://www.javascriptkit.com/javatutors/trycatch2.shtml

> Also TimeoutError would have to be added to the EXPORTED_SYMBOLS list,
> otherwise it's pretty useless for us.
Good catch.

Comment 8

7 years ago
(In reply to comment #6)
> Comment on attachment 516743 [details] [diff] [review]
> Patch 1.1 - Keep the err.stack this time
> 
> >+  var err = new Error();
> >+  if (err.stack) {
> >+    this.stack = err.stack;
> >+  }
> >+  this.message = message === undefined ? err.message : message;
> >+  this.fileName = fileName === undefined ? err.fileName : fileName;
> >+  this.lineNumber = lineNumber === undefined ? err.lineNumber : lineNumber;
> >+};
> >+TimeoutError.prototype = new Error();
> >+TimeoutError.prototype.constructor = TimeoutError;
> >+TimeoutError.prototype.name = 'TimeoutError';
> 
> Please also check bug 634227 how I did that for our shared modules. I wonder if
> we find a consensus in the middle. At least a separate module would be nice to
> not clutter utils.js with more and more methods.
> 
> Also TimeoutError would have to be added to the EXPORTED_SYMBOLS list,
> otherwise it's pretty useless for us.
> 
> Re: 'message' you are right. With the given new error class we can identify
> errors more easily.

With that patch you did Henrik, you're doing a lot of munging the stack which we're not going to do at this level.  That will be factored out elsewhere.  You also added a "function" to the error message which is nice, but the error message already has a caller on its prototype [1] which it inherits, so that seems unneeded.  A lot of the other stuff you're doing with the error class must be a result of the inheritance model you're using because you're doing all the work of moving objects from the prototype to another object prototype, which Andrew does quite simply by inheriting directly from the "native" error object.

So I don't really see this as a huge departure from what you're doing.

Also, I concur that we don't need a separate errors module at the moment.  I also don't anticipate needing to define a bunch of specific error handlers.  If that becomes the case in the future then we'll refactor at that time.

[1]: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Error
(Reporter)

Comment 9

7 years ago
Clint, I talk about the updated and checked-in patch which doesn't use the inheritance module anymore but has a generator for custom errors. Also there is nothing about stack munging.

https://github.com/geoelectric/mozmill-api-refactor/blob/master/modules/errors.js#L50
master: https://github.com/mozautomation/mozmill/commit/e55c0cee250e00ac991499a2eda8b12a9face548

I added the TimeoutError to EXPORTED_SYMBOLS though I realized that you could use e.name == "TimeoutError" instead of instanceof(e) == TimeoutError.  Either way should work.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Reporter)

Comment 11

7 years ago
The risk for backporting is kinda minimal and it would help us in the meantime to improve our own modules for the rewrite. Could we target it for an upcoming 1.5.x release?
Whiteboard: [mozmill-2.0+] → [mozmill-2.0+][mozmill-1.5.x?]

Comment 12

7 years ago
Reopening; we're backporting to 1.5.4
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [mozmill-2.0+][mozmill-1.5.x?] → [mozmill-2.0+][mozmill-1.5.4+]
We're looking at upgrading Thunderbird MozMill to 1.5.4, and need this to cleanly fix bug 662960.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 14

7 years ago
pulled to hotfix-1.5: https://github.com/mozautomation/mozmill/commit/11a105dc53eaa5e406ee1e34e74de432e138aabe
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
(Reporter)

Comment 15

7 years ago
Looks good with a Mozmill 1.5.4 build. Verified fixed.
Status: RESOLVED → VERIFIED
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.