Add doesThrow and doesNotThrow to the assertions module

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [module-refactor], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
As given on bug 637880 it would be kinda helpful to have methods like the following included:

expect/assert.throws(callback, [classname], message);
expect/assert.doesNotThrow(callback, [classname], message);

I will work on that.
(Assignee)

Updated

7 years ago
Whiteboard: [module-refactor]
(Assignee)

Updated

7 years ago
Blocks: 639544
No longer blocks: 639545
(Assignee)

Comment 1

7 years ago
Created attachment 519704 [details] [diff] [review]
Patch v1
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Attachment #519704 - Flags: review?(gmealer)
Attachment #519704 - Flags: feedback?(fayearthur+bugs)
(Assignee)

Comment 2

7 years ago
Created attachment 519705 [details] [diff] [review]
Patch v1.1

Now with the dump output removed.
Attachment #519704 - Attachment is obsolete: true
Attachment #519704 - Flags: review?(gmealer)
Attachment #519704 - Flags: feedback?(fayearthur+bugs)
Attachment #519705 - Flags: review?(gmealer)
Attachment #519705 - Flags: feedback?(fayearthur+bugs)
(Assignee)

Comment 3

7 years ago
I used a different naming schema so it applies to the method we already have for the Expect and Assert class.
Summary: Add throws and notThrows to the assertions module → Add doesThrow and doesNotThrow to the assertions module
Comment on attachment 519705 [details] [diff] [review]
Patch v1.1

Few things:

If we're going to have aErrorClass (the middle param) be optional, you can't just check for aErrorClass being undefined. As it is (callback, message) won't work, because it'll think the message is the error class. 

Instead you have to check the number of parameters; if it's 2, you haven't defined aErrorClass, if it's 3, you have. Then you assign off to internal vars and go from there.

Another problem is these never rethrow exceptions that aren't of the type you're looking for; instead you seem to try to account for them in the diagnosis, which is incorrect behavior for this type of test. 

If aClassName is given, -only- exceptions of the class name should be trapped and diagnosed; anything else should be immediately rethrown (or preferably not caught at all via a catch condition, though I know that's been problematic.)

Reason for doing all this is the same as why you never do unconditional exception handling and only trap things you're specifically looking for. If the callback itself has an unanticipated error in it, you still want the exception.

But if putting this logic together is too complicated, just eliminate the aErrorClass parameter and go with (callback, message). It'll make the methods considerably simpler.

Some nits:

Re: doesThrow()

      if (aErrorClass)
        diagnosis = "expected '" + aErrorClass + "'";

This is essentially repeated in both try/catch clauses. Instead of doing it this way, initialize diagnosis to "" instead of undefined. Then in the catch clause do:

diagnosis = "got '" + ex.name + "', "; // note extra ", " at end

...then after the catch clause do:

      if (aErrorClass)
        diagnosis += "expected '" + aErrorClass + "'"; // note +=

That way you don't repeat it and it'll work for all cases.

Re:

condition = aErrorClass === undefined || aErrorClass === ex.name;

...not syntactically required, but this'll be much clearer if you put parens around the rval (same goes for any statement w/ both = and == or ===):

condition = (aErrorClass === undefined || aErrorClass === ex.name);

Re: doesNotThrow,

      if (aErrorClass)
        diagnosis = aErrorClass ? "not expected '" + aErrorClass + "'" : undefined;

...ternary operators are hard to read. I recommend only using them in the case you have one single value to return on both sides of the colon, otherwise use if/else. But if you're going to return an expression like this put parens around it for clarity:

      if (aErrorClass)
        diagnosis = aErrorClass ? ("not expected '" + aErrorClass + "'") : undefined;

Also the catch message:

      if (aErrorClass)
        diagnosis += ", expected '" + aErrorClass + "'";

...is incorrect. You -didn't- expect aErrorClass there. And as I said above you don't want to print out a message on any other type of error, you want to rethrow.

Generally, I question why the logic for building the diagnosis looks different between the two functions, but I think the logic probably needs a complete overhaul anyway to correctly rethrow unexpected errors.
Attachment #519705 - Flags: review?(gmealer) → review-
Comment on attachment 519705 [details] [diff] [review]
Patch v1.1

I'd take a look at the node implementation here:

https://github.com/joyent/node/blob/master/lib/assert.js#L259

I pushed a halfway there node-based throws() here:
https://github.com/harthur/mozmill/blob/assert/mozmill/mozmill/extension/resource/modules/assertions.js#L318

I think we should stick with throws() and doesNotThrow() to stay close with some kind of standard.

I think the errorClass parameter should be the actual error class instead of a string for correctness.
Attachment #519705 - Flags: feedback?(fayearthur+bugs) → feedback-
(In reply to comment #5)

> I think the errorClass parameter should be the actual error class instead of a
> string for correctness.

We seemed to have some previous problems passing around the error classes between modules and getting comparisons to work, hence us going to string comparisons. Not sure if it was user error or what. If we didn't have that issue, I'd agree.
(Assignee)

Comment 7

7 years ago
As talked on IRC Heather will update the patch and attach a complete version on bug 637880. Once it has been checked in, I will backport it into our repository.
No longer blocks: 637880
Depends on: 637880
(Assignee)

Comment 8

7 years ago
Created attachment 528972 [details] [diff] [review]
Patch v2

Nearly straight backport of the throws/doNotThrow additions in the Mozmill's assertion module, except for necessary changes in handling the Error (we can't use instanceOf) and updated JsDoc which is still missing in the original file. Heather, would be great if you could update those parts later on.
Attachment #519705 - Attachment is obsolete: true
Attachment #528972 - Flags: review?(gmealer)
Comment on attachment 528972 [details] [diff] [review]
Patch v2

Looks fine. r+
Attachment #528972 - Flags: review?(gmealer) → review+
(Assignee)

Comment 10

7 years ago
Landed as:
https://github.com/geoelectric/mozmill-api-refactor/commit/165a7b67ea37fb645ccee59c5e68508755057fe1
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.