Simplify t.throws() #1047

novemberborn · 2016-09-23T13:53:07Z

This is my proposal for resolving #661. In short I want to stop deferring to https://nodejs.org/api/assert.html#assert_assert_throws_block_error_message and remove various parameter overloads that are currently supported by the assertion.

Here's what I'd like to support:

t.throws(fn) // Throws an Error (or subclass thereof)
t.throws(fn, SyntaxError) // Throws a SyntaxError (or subclass thereof)
t.throws(fn, 'string') // Throws an Error (or subclass thereof), whose .message === 'string'
t.throws(fn, /regexp/) // Throws an Error (or subclass thereof), whose /regexp/.test(err.message) === true

If you need to test the errors class and message you should use the t.throws return value:

const err = t.throws(fn, SyntaxError)
t.true(err.message === 'expecting integer')

Passing anything other than undefined, functions, strings or regular expressions as the second argument results in an TypeError from t.throws() itself.

The first argument is allowed to be a promise or observable, as is any return value from the fn call. Of course this makes t.throws asynchronous, so users may need to do:

const err = await t.throws(fn)

Questions:

Does t.throws(fn, Constructor) require the thrown value to be an Error? For example:
```
t.throws(() => { throw 'foo' }, String) // Is this allowed?
```
Should t.throws(fn, 'string') and t.throws(fn, /regexp/) be supported at all, or would it be preferable to dereference the error and then use another assertion?
```
const err = t.throws(fn)
t.true(err.message === 'expecting integer')
```
Is there a need for t.throws(fn, SyntaxError, 'string') and t.throws(fn, SyntaxError, /regexp/)
Does anybody have experience with / examples of asserting Error instances across contexts?

The text was updated successfully, but these errors were encountered:

vadimdemedes · 2016-09-29T08:13:22Z

I absolutely support your proposal!

Answers to your questions:

I don't think it should be allowed. ESLint's docs on no-throw-literal rule explain it best:

It is considered good practice to only throw the Error object itself or an object using the Error object as base objects for user-defined exceptions. The fundamental benefit of Error objects is that they automatically keep track of where they were built and originated.

Services like BugSnag, Sentry, Rollbar, etc will provide zero information, if code throws strings, instead of Error objects.

I think they should be supported, with behavior exactly like you outlined in your first code example:

t.throws(fn, 'string') // Throws an Error (or subclass thereof), whose .message === 'string'
t.throws(fn, /regexp/) // Throws an Error (or subclass thereof), whose /regexp/.test(err.message) === true

I use this way constantly and find it very convenient.

I think this would also be a nice thing to have, but not insisting on this one. Let's hear from others whether they use this way or not.
Didn't completely understand the question, mind providing a quick example?

fearphage · 2016-10-02T14:13:42Z

The current implementation seems to almost match your proposal. t.throws(fn, type, message) doesn't work (0.16.0). I can't tell if this is a regression or a mix up in the documentation. I went back 10 or so commits and never saw where this worked. There are no tests for this format either.

I was coming here to file that bug report and submit the PR, but I won't if you're planning to remove it altogether.

novemberborn · 2016-10-07T13:58:55Z

Conclusions so far:

Does t.throws(fn, Constructor) require the thrown value to be an Error?

Yes.

Should t.throws(fn, 'string') and t.throws(fn, /regexp/) be supported at all, or would it be preferable to dereference the error and then use another assertion?

We'll support this interface.

Is there a need for t.throws(fn, SyntaxError, 'string') and t.throws(fn, SyntaxError, /regexp/)

Leaning towards not supporting this.

@vdemedes

Does anybody have experience with / examples of asserting Error instances across contexts?

Didn't completely understand the question, mind providing a quick example?

const err1 = new Error()
const err2 = vm.runInNewContext('new Error()')
err1 instanceof Error // true
err2 instanceof Error // false

@fearphage yea AVA just uses Node.js' API, and it gets messy fast. Hence this proposal.

fearphage · 2016-10-07T14:11:00Z

Is there a need for t.throws(fn, SyntaxError, 'string') and t.throws(fn, SyntaxError, /regexp/)

Leaning towards not supporting this.

Is it difficult to code or reason about? Just a preference? I'm just curious.

If we're target an arity of 2, what about this as an option?

t.throws(fn, error => {
  // insert error assertions
});

I like the encapsulation of the assertions related to the thrown error.

novemberborn · 2016-10-07T14:14:50Z

Is it difficult to code or reason about? Just a preference? I'm just curious.

The assertion becomes overly complex again, which is what we're trying to move away from.

If we're target an arity of 2, what about this as an option?
t.throws(fn, error => {
  // insert error assertions
});
I like the encapsulation of the assertions related to the thrown error.

That'll be hard to distinguish from t.throws(fn, Constructor).

jfmengels · 2016-10-13T09:01:00Z

I like the proposal :)

Does t.throws(fn, Constructor) require the thrown value to be an Error?

Yes. That is a best practice, or rather, using a string is a bad practice, and we should help the user remove that bad practice from their code IMO.
We should also (and I don't remember whether this is already something that we're doing) fail and show an error when the thrown error is a string and not an Error.
It's a bit different if an underlying library throws a string, but even then it should be simple for the user to get around that and do a normal try/catch instead.

Should t.throws(fn, 'string') and t.throws(fn, /regexp/) be supported at all, or would it be preferable to dereference the error and then use another assertion?

That is common enough to keep IMO.

Is there a need for t.throws(fn, SyntaxError, 'string') and t.throws(fn, SyntaxError, /regexp/)

I don't custom Error types that much. I think it's rare enough and simple enough to get around it

const err = t.throws(fn, 'string');
t.true(err instanceof SyntaxError);

Does anybody have experience with / examples of asserting Error instances across contexts?

No :/

From what I see, you're proposing the removal of the message too? That would make it inconsistent with the other APIs. Not sure I agree here.

novemberborn · 2016-10-14T14:18:32Z

We should also (and I don't remember whether this is already something that we're doing) fail and show an error when the thrown error is a string and not an Error.

Yes that's implied.

From what I see, you're proposing the removal of the message too? That would make it inconsistent with the other APIs. Not sure I agree here.

Oh no! Forgot to add that…

I think that rules out syntax like t.throws(fn, SyntaxError, 'string'), since that collides with t.throws(fn, SyntaxError, 'message'). Padding it is just annoying (t.throws(fn, SyntaxError, undefined, 'message'). Though you would need to pad t.throws(fn, undefined, 'message'). I think that's OK though.

sindresorhus · 2016-11-18T09:11:07Z

Alternative proposal.

Make it t.throws(fn, matchers, [message]);, where matchers is an object of different matchers that all has to match. Matchers can be string or regex.

This could easily be an addition to supporting string/regex/constructor in the second argument, for backwards compatibility.

For example:

t.throws(() => foo(), {
    constructor: RangeError,
    message: /unicorn pasta is/
});

Compared to the current way:

const err = t.throws(() => foo(), /unicorn pasta is/);
t.true(err instanceof RangeError);

Possible matchers constructor, message. Could also consider supporting name for when you don't have easy access to the constructor function. Maybe also stack if you want to ensure something is part of it, though I don't think I've ever needed that.

Benefits of this is that it makes everything explicit. No more wondering for users what t.throws(() => foo(), 'Unicorn') matches against - Is it the name, type, or message. There's also benefit in being able to specify all the constraints directly instead of in separate calls later, like potential for a better diff.

We should also have much better validation logic to prevent mistakes. For example, using is-error-constructor on the constructor matcher.

novemberborn · 2016-11-18T10:46:52Z

I like it!

sholladay · 2017-02-11T17:32:12Z

I like Sindre's proposal. It would still return the error for extra assertions, right? Let's say I also want to ensure that multi-line error messages never exceed a certain width, for friendly terminal display. Would it look like this?

const err = t.throws(() => foo(), {
    constructor: RangeError,
    message: /unicorn pasta is/
});
t.is(longestLine(err.message), 80);

I will note that there is a much simpler approach to solve the call site ambiguity problem. Just support only constructors in the second argument, since that's the only type that is non-ambiguous for what it is matching. Delegate everything else to t.is(), t.regex(), etc.

sindresorhus · 2017-02-11T17:36:37Z

It would still return the error for extra assertions, right?

Yes

Just support only constructors in the second argument, since that's the only type that is non-ambiguous for what it is matching. Delegate everything else to t.is(), t.regex(), etc.

It creates verbose code for common cases though. The most common case is wanting to ensure the error is the correct type and has the correct message. I think we should optimize for that.

alextes · 2017-03-21T08:57:41Z

Love this proposal. Eliminating the constructor vs. validator function ambiguity would've saved me a lot of time and confusion (example).

Is there still something to be done on the 'question' side of things? It's hard to give a helpful push without a clear direction 😄.

novemberborn · 2017-03-21T10:06:30Z

@alextes we can go with @sindresorhus' proposal: #1047 (comment)

Match constructor by reference, name by equality, and message either by testing a regular expression or string equality.

sindresorhus · 2017-03-21T10:14:48Z

I had the need the other day to match against a custom error property. Should we support that too?

novemberborn · 2017-03-21T11:35:47Z

I had the need the other day to match against a custom error property. Should we support that too?

I think that's better served by assigning the thrown error to a variable and asserting on that.

alextes · 2017-03-21T11:54:12Z

@sindresorhus I feel the consensus was around supporting common use-cases for error comparison. In other words, offer shortcuts to common comparisons but keep the API surface small as one can already do any comparison with the returned error. 'The other day' only tells us about recency, how about frequency? I think this decision is a tricky balance between frequency, the cost of doing without, and diff potential. Personally I'm leaning towards leaving it out, like novemberborn.

@novemberborn I'd actually lean against Sindre's suggestion of possibly adding a name matcher. Can I ask, when would a name be preferred over a constructor? I can't think of anything. That for me makes this an infrequent help at best, which for simplicity's sake I would then leave out.

One more detail I'd like to bring to the attention of the jury 😄. Sindre Proposal also adds backwards compatibility for the second argument, when not a matcher object, of type string / regex / constructor.

Regex
Match on string representation of actual error.
Constructor
Match constructor function by reference.
String
Currently unsupported by Node. Is this one to be an exact match of the string representation of the error? Or just the message of the error?
Validation Function
Currently supported by Node. We drop it. The reasoning being this is again custom and not commonly useful checking. Thus up to the user to do with the returned error.

My fingers are starting to itch to write the implementation on this one 😛.

novemberborn · 2017-03-21T13:56:53Z

@alextes

Can I ask, when would a name be preferred over a constructor?

It's usually better to compare errors using their .name. if (error instanceof MyError) doesn't necessarily work across VM contexts, for example. Libraries might also not bother exporting their error constructors.

String
Currently unsupported by Node. Is this one to be an exact match of the string representation of the error? Or just the message of the error

It's currently implemented as comparing the message. We should do the same with the regular expression argument, IMO.

Historically the problem with the throws assertion is that it tries to do too many things, making it hard to understand what you're actually testing. Running a regular expression against the string representation of an error is an example of that. I didn't even know that's how it worked!

Personally I don't think we should maintain backwards compatibility, given how confusing throws is.

sholladay · 2017-03-21T14:25:33Z

@alextes

I'd actually lean against Sindre's suggestion of possibly adding a name matcher. Can I ask, when would a name be preferred over a constructor?

I don't personally need this feature much, but for sake of argument...

Say myApp() uses a database under the hood and potentially rejects with various DatabaseErrors. You probably want to make assertions about failure cases for myApp() working with the database, but the relevant error constructor does not live in your app and very well may not be exposed by the database driver library (it is uncommon to export all such classes).

You still need to distinguish between different types of errors and, without the constructor, name is the preferred way to ID an error (should be more stable than message). Changes to these are arguably semver major, so you should probably test them. This is the best you can do without the constructor.

test(async (t) => {
    const option = {
        databaseName : 'somethingThatDoesNotExist'
    };
    t.throws(myApp(option), {
        name : 'DatabaseNotFoundError'
    });
});

alextes · 2017-03-21T14:33:47Z

Thanks @novemberborn and @sholladay! I understand the need and even preference for name now.

I didn't know Ava's assert has seen some big change. Congrats on the big PR! I read through ava's assert module two weeks back, since then #1302 landed. Forgive me for missing Ava no longer almost directly relies on Node's assert.

Comparing just the message makes a lot of sense to me if we already support the name separately. It would break with Node's behavior. I have no idea how to weigh the two. (I feel your argument to make it simpler is more compelling.)

I didn't even know that's how it worked!

Happy to be able to contribute something small 😄.

novemberborn · 2017-03-21T15:05:42Z

@alextes thanks! Be sure to check out #1314 which contains a few more changes.

without the constructor, name is the preferred way to ID an error (should be more stable than message).

@sholladay, I think Node.js is moving to standardizing error codes, though I couldn't find a definitive proposal on that. File system errors have code and errno properties. We should support whatever practice Node.js settles on, though we can add that later.

StoneCypher · 2017-07-05T22:09:24Z

I really don't think we should be excluding valid throws just because a linter rule claims it's bad practice.

I lost nearly an entire day because I threw a string, which is fully valid, and .throws broke instead of catching it or telling me what it didn't like

StoneCypher · 2017-07-05T22:10:30Z

I don't really think my commentary belongs on this issue, which isn't about throwing strings in any meaningful way, but a maintainer closed the valid and appropriate issue about that explicitly in favor of this one

I apologize for what seems to me to be unnecessarily changing the subject

novemberborn · 2017-07-06T07:24:38Z

@StoneCypher When we implement t.throws() in AVA directly we'll make sure it explains why a thrown value still causes the assertion to fail.

novemberborn · 2017-07-06T07:26:21Z

Oh, I see that's #1440.

StoneCypher · 2017-07-06T16:27:21Z

Thanks. That seems ideal to me.

Remove `core-assert` dependency. When passed a function as the second argument, `t.throws()` now assumes its a constructor. This removes support for a validation function, which may or may not have worked. Refs #1047. `t.throws()` now fails if the exception is not an error. Fixes #1440. Regular expressions are now matched against the error message, not the result of casting the error to a string. Fixes #1445. Validate second argument to `t.throws()`. Refs #1676.

Remove `core-assert` dependency. When passed a function as the second argument, `t.throws()` now assumes its a constructor. This removes support for a validation function, which may or may not have worked. Refs #1047. `t.throws()` now fails if the exception is not an error. Fixes #1440. Regular expressions are now matched against the error message, not the result of casting the error to a string. Fixes #1445. Validate second argument to `t.throws()`. Refs #1676. Assertion failures now display how AVA arrived at the exception. Constructors are printed when the error is not a correct instance. Fixes #1471.

Fixes #1047. Fixes #1676. A combination of the following expectations is supported: ```js t.throws(fn, {of: SyntaxError}) // err instanceof SyntaxError t.throws(fn, {name: 'SyntaxError'}) // err.name === 'SyntaxError' t.throws(fn, {is: expectedErrorInstance}) // err === expectedErrorInstance t.throws(fn, {message: 'expected error message'}) // err.message === 'expected error message' t.throws(fn, {message: /expected error message/}) // /expected error message/.test(err.message) ```

Remove `core-assert` dependency. When passed a function as the second argument, `t.throws()` now assumes its a constructor. This removes support for a validation function, which may or may not have worked. Refs #1047. `t.throws()` now fails if the exception is not an error. Fixes #1440. Regular expressions are now matched against the error message, not the result of casting the error to a string. Fixes #1445. Validate second argument to `t.throws()`. Refs #1676. Assertion failures now display how AVA arrived at the exception. Constructors are printed when the error is not a correct instance. Fixes #1471.

Fixes #1047. Fixes #1676. A combination of the following expectations is supported: ```js t.throws(fn, {of: SyntaxError}) // err instanceof SyntaxError t.throws(fn, {name: 'SyntaxError'}) // err.name === 'SyntaxError' t.throws(fn, {is: expectedErrorInstance}) // err === expectedErrorInstance t.throws(fn, {message: 'expected error message'}) // err.message === 'expected error message' t.throws(fn, {message: /expected error message/}) // /expected error message/.test(err.message) ```

novemberborn added enhancement new functionality question labels Sep 23, 2016

This was referenced Sep 23, 2016

t.throws shows undefined undefined undefined when a string is thrown #661

Closed

t.throws not that helpful when the message assertion does not match #779

Closed

sindresorhus mentioned this issue Nov 4, 2016

Suggestion: Support t.throws(..., ..., message?: string | RegExp | undefined) #1102

Closed

sindresorhus mentioned this issue Nov 20, 2016

t.throws() returns promise instead of promise's rejection reason #1120

Closed

sindresorhus mentioned this issue Dec 9, 2016

t.notThrows() gives no info on thrown error in case an error is thrown #1150

Closed

novemberborn added help wanted and removed question labels Mar 21, 2017

sindresorhus mentioned this issue Mar 21, 2017

Overhaul test implementation 👷🏗 #1314

Merged

sindresorhus mentioned this issue Apr 19, 2017

Provide a convenience API for testing error constructor and message #1362

Closed

novemberborn mentioned this issue Apr 20, 2017

Let's make .throws failed test reporting more consistent with others #1363

Closed

kevva mentioned this issue Jul 5, 2017

Unexpected throw in .throws() #1439

Closed

sindresorhus mentioned this issue Jul 10, 2017

t.throws() with regex not working as expected #1445

Closed

novemberborn mentioned this issue Jul 30, 2017

Improve t.throws output for mismatched constructor #1471

Closed

novemberborn mentioned this issue Aug 27, 2017

t.throws is not matching errors properly by Prototype #1493

Closed

novemberborn added this to the 1.0 milestone Aug 28, 2017

novemberborn added the scope:assertions label Oct 26, 2017

sindresorhus mentioned this issue Jan 14, 2018

Follow Semantic Versioning, version 1 (RFC) #1637

Closed

novemberborn mentioned this issue Jan 24, 2018

Make t.throws() accept async function as parameter #1650

Merged

novemberborn mentioned this issue Jan 31, 2018

t.throws passes with false, undefined, and null #1676

Closed

novemberborn mentioned this issue Feb 12, 2018

Reimplement t.throws() and t.notThrows() #1704

Merged

novemberborn closed this as completed in 3c13c33 Feb 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify t.throws() #1047

Simplify t.throws() #1047

novemberborn commented Sep 23, 2016

vadimdemedes commented Sep 29, 2016 •

edited by sindresorhus

Loading

fearphage commented Oct 2, 2016

novemberborn commented Oct 7, 2016

fearphage commented Oct 7, 2016

novemberborn commented Oct 7, 2016

jfmengels commented Oct 13, 2016 •

edited

Loading

novemberborn commented Oct 14, 2016

sindresorhus commented Nov 18, 2016 •

edited

Loading

novemberborn commented Nov 18, 2016

sholladay commented Feb 11, 2017

sindresorhus commented Feb 11, 2017

alextes commented Mar 21, 2017 •

edited

Loading

novemberborn commented Mar 21, 2017

sindresorhus commented Mar 21, 2017

novemberborn commented Mar 21, 2017

alextes commented Mar 21, 2017

novemberborn commented Mar 21, 2017

sholladay commented Mar 21, 2017 •

edited

Loading

alextes commented Mar 21, 2017 •

edited

Loading

novemberborn commented Mar 21, 2017

StoneCypher commented Jul 5, 2017

StoneCypher commented Jul 5, 2017

novemberborn commented Jul 6, 2017

novemberborn commented Jul 6, 2017

StoneCypher commented Jul 6, 2017

Simplify t.throws() #1047

Simplify t.throws() #1047

Comments

novemberborn commented Sep 23, 2016

vadimdemedes commented Sep 29, 2016 • edited by sindresorhus Loading

fearphage commented Oct 2, 2016

novemberborn commented Oct 7, 2016

fearphage commented Oct 7, 2016

novemberborn commented Oct 7, 2016

jfmengels commented Oct 13, 2016 • edited Loading

novemberborn commented Oct 14, 2016

sindresorhus commented Nov 18, 2016 • edited Loading

novemberborn commented Nov 18, 2016

sholladay commented Feb 11, 2017

sindresorhus commented Feb 11, 2017

alextes commented Mar 21, 2017 • edited Loading

novemberborn commented Mar 21, 2017

sindresorhus commented Mar 21, 2017

novemberborn commented Mar 21, 2017

alextes commented Mar 21, 2017

novemberborn commented Mar 21, 2017

sholladay commented Mar 21, 2017 • edited Loading

alextes commented Mar 21, 2017 • edited Loading

novemberborn commented Mar 21, 2017

StoneCypher commented Jul 5, 2017

StoneCypher commented Jul 5, 2017

novemberborn commented Jul 6, 2017

novemberborn commented Jul 6, 2017

StoneCypher commented Jul 6, 2017

vadimdemedes commented Sep 29, 2016 •

edited by sindresorhus

Loading

jfmengels commented Oct 13, 2016 •

edited

Loading

sindresorhus commented Nov 18, 2016 •

edited

Loading

alextes commented Mar 21, 2017 •

edited

Loading

sholladay commented Mar 21, 2017 •

edited

Loading

alextes commented Mar 21, 2017 •

edited

Loading