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

[SamVerschueren]

Description

When using t.throws() with a promise that rejects with a string instead of an Error object, the assertion fails.

import test from 'ava';

async function fn(): Promise<any> {
    return Promise.reject('foo');
}

test(async t => {
    t.throws(fn(), 'foo');
});

When using Promise.reject(new Error('foo'));, it works as expected.

It's due to these lines. I have no idea why it overrides the err reference with a function. I looked into core-assert and it doesn't seem like it works with passing in a function as the expected parameter. But might be missing something here.

Environment

Node.js v4.2.1
darwin 15.3.0

[novemberborn]

@SamVerschueren that is expected. With 0.13.0 t.throws() only tests against errors, before it would try and cast other types to errors, which was wrong.

Note that your examples are different. In the first you reject with a string, in the second with an error.

[SamVerschueren]

Oh ok, I thought that it was still working with 0.13.x. My mistake then. Good to know :).

[sindresorhus]

I think our output could be better than just undefined undefined undefined though.

[SamVerschueren]

You're right!

[SamVerschueren]

Just to clear things up a bit regarding this issue. Should AVA handle the case where a dev throws something else then an Error (string, boolean, number, ...)? Or should it show an error that he should throw an error instead? Like implemented in my 2 PRs?

[jamestalmage]

@SamVerschueren
Sorry I haven't gotten to review your PR's yet. There are two closed issues that discuss the expected behaviors of t.throws.

#468 (comment)

It should return the thrown Error (if it's synchronous). Or a Promise for the rejection reason if an async object is returned (be it Observable or Promise). In other words, t.throws should turn a rejected promise into a resolved promise for the rejection reason. This allows you to await the rejection reason for additional assertions (if you are attaching additional properties).

#493 (comment)

The assertion should fail if non-Errors are used. This includes both the sync and async versions. The message should be a very helpful message that explains "You threw a non-Error, which is bad form". In other words, we should minimize confusion for developers who intentionally threw a non-Error and are trying to make assertions against it, that that is not supported behavior. If users want to assert against non-Errors, they need to catch them themselves.

---

This is complicated enough that we should pretty thoroughly document it. Perhaps it's enough to justify a recipe: "Properly Handling Errors in AVA" that includes detailed explanations on the pitfalls of non-Errors.

---

There is a limited value to grabbing non-Error's, and that is specifically when you are writing a framework or tool that executes user supplied callbacks. In that case it's often beneficial to write a few assertions to define how your code responds when your users code throws non-Errors. Since that's the rarer corner case, you should be responsible for jumping through the extra hoops to assert that behavior (by doing the more verbose thing and wrapping your tests with try/catch, etc). Again - there is probably room for a recipe here.

[novemberborn]

Node's assert.throws is rather overloaded isn't it? Maybe we could simplify the current implementation, especially given the changes from #493:

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

We remove support for validation functions, preferring you use the t.throws() return value instead:

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

I'd still be in favor of the string and regular expression expectation shorthand if you're expecting an error but are not too fussed about its inheritance. Not opposed to removing that too and requiring users to run their assertion on the return value.

(Also I haven't verified whether Node's assert.throws() can deal with the subclassing)

While there's a strong convention for throwing proper errors we should still provide an assertion that can capture non-error exceptions. E.g.:

const excp = t.throwsAny(fn)
t.true(excp instanceof Foo)

[jamestalmage]

Is throwsAny really that useful? When would you ever use it?

[iamstarkov]

while upgrading ava from 0.12 to 0.13 messages like

  1. fs › should reject on empty input
  undefined undefined undefined

are not really helpful

[iamstarkov]

if i'm in favor of pure string rejection reasons, what should i do?

[SamVerschueren]

@iamstarkov Why do you prefer rejecting with strings?

[iamstarkov]

because it is a String reason which is promoted on mdn page. but its fine i can change my code.

at the same time a lot of devs will still use strings as rejection reasons though, and it a bit uncomfortable if testing framework will dictate how promises should be rejected

[kasperlewau]

because it is a String reason which is promoted on mdn page.

It doesn't explicitly promote the usage of a string rejection though, it's just another example - just like the Error rejection.

Some (old) food for thought; http://www.devthought.com/2011/12/22/a-string-is-not-an-error/

[iamstarkov]

you are right, but i still think that im not the only one and not the last one who will reject using just strings

[iamstarkov]

i would suggest:

• more descriptive ava error
• allow to reject strings

[novemberborn]

While the convention is for throwing errors (or subclasses) I don't think we should preclude users from asserting other values. E.g. I can easily imagine writing code that throws/rejects with object values that don't extend from Error, perhaps because they don't require an expensive stack trace computation.

I'm in favor of t.throws(fn) requiring errors (regardless of the subsequent expectation parameter). This will help with accidental undefined throws (when throwing a variable reference). But we'd still need a way to capture any throw.

[kasperlewau]

But we'd still need a way to capture any throw.

How about adding a 4th 'loose' parameter to t.throws to accept non-errors, as opposed to introducing yet another assertion method? Default would be false.

// loose
t.throws(Promise.reject('foo'), null, 'foo', true);
t.throws(() => throw 'foo', null, 'foo', true);

// strict
t.throws(Promise.reject(new Error('foo'), Error, 'foo');
t.throws(() => throw new Error('foo'), Error, 'foo', false);

Too much?

---

I don't suppose we could hijack the msg parameter of throws and say;
if typeof msg === 'boolean' && !!msg - accept non-errors to be thrown / rejected.

// loose
t.throws(Promise.reject('foo'), null, true);

// strict
t.throws(Promise.reject(new Error('foo'), Error, 'foo');

No need for a fourth parameter - running assertions on the message property of non-errors doesn't yield anything anyways (afaik).

---

All of this would change the semantics of throws for the worse in that it won't return the non-Error (whereas it will return actual Errors, #576 & #651), which could prove confusing. Should throws always return the result regardless of what was thrown/rejected?

[spudly]

I think that whether or not throwing a string is allowed should be up to the linter and not the test runner. Given that the language allows you to throw or reject with whatever you want, I would think that t.throws should allow you to do so. I would expect all of the following to be valid:

t.throws(Promise.reject('foo'), String);
t.throws(Promise.reject('foo'), 'some string');
t.throws(Promise.reject('foo'), /somePattern/);

That said, I would never use it, because I use a good linter and only throw Errors.

[jamestalmage]

Given that the language allows you to throw or reject with whatever you want, I would think that t.throws should allow you to do so.

We want to promote best practices wherever possible. If you have control of the code, you should be throwing errors.

because it is a String reason which is promoted on mdn page

They show an Error example, and mention that it is probably a good idea to throw Errors. I say it is a code smell.

---

I guess I can live with t.throwsAny to make life simpler for people who are converting tests for legacy code that throws bad values. Ideally we wouldn't even do that. I don't think it's a big need.

[sindresorhus]

I guess I can live with t.throwsAny

I can't.

[novemberborn]

The language lets you throw any value. AVA should (easily!) let you assert the correct value was thrown, whether it's an Error or not.

Perhaps t.throws(fn, 'message string') and t.throws(fn, /message regexp/) require the thrown value to be an error? t.throws(fn) wouldn't care and t.throws(fn, Constructor) would verify the prototype chain. If you want an Error but don't care about its message you'd use t.throws(fn, Error).

I don't think we should have yet another argument, t.throws() is plenty overloaded as it is. And I'd like to remove support for validator functions.

[SamVerschueren]

From the first post of @novemberborn in this thread, I assumed that AVA stopped asserting literals being thrown in the code

That is expected. With 0.13.0 t.throws() only tests against errors, before it would try and cast other types to errors, which was wrong.

But when reading further down, it looks like somehow that behaviour broke or was removed without thinking about the consequences.

---

I have a double feeling about the solution of throwing an error when a literal is thrown and force the user to throw/reject with an error. Yes in a perfect world that would be very nice. But this might not be the task of a test runner to force a best practice. It might be up to a linter to force the developer to not throw literals.

---

If we would accept errors and literals, please don't add an extra method like throwsAny. I think it can be easily avoided.

t.throws(fn(), Error)

This is quite clear, we expect an Error object.

t.throws(fn(), 'Foo Bar') or t.throws(fn(), /Foo Bar/)

2 options here

1. fn() throws an Error

If fn() throws an Error (see is-error or something alike), assert that err.message is Foo Bar.

2. fn() throws a string literal

If fn() throws a string literal, assert that err === Foo Bar.

What's wrong with something like this? Maybe I'm missing something but this would work for everyone, no?

[novemberborn]

I assumed that AVA stopped asserting literals being thrown in the code

Not really, see #582. AVA used to convert rejection reasons that weren't instanceof Error to errors by new Error(reason). That was crazy wrong.

The undefined undefined undefined issue you raised is due to Node's assert.throws() behaving differently than expected. That's related to throwing literals but I think it could also occur when using validation functions that reject errors. This is why I'm saying we should rewrite t.throws() to not use Node's assert.throws() at all, so we can get these things right.

Whether t.throws() should accept non-errors is yet another discussion 😄

---

If fn() throws a string literal, assert that err === Foo Bar.

I don't think t.throws() should start comparing literals. Users should use the t.throws() error return value and perform their own secondary assertion.

[sindresorhus]

Whether t.throws() should accept non-errors is yet another discussion

It shouldn't. If you want to do that you can easily try/catch yourself.

[SamVerschueren]

It shouldn't. If you want to do that you can easily try/catch yourself.

I'm ok with that. Willing to write a small recipe for asserting errors to explain when to use try-catch and when to use t.throws.

[jamestalmage]

Willing to write a small recipe for asserting errors to explain when to use try-catch and when to use t.throws.

I'm not sure how that would read. The best practice we want to promote is "Always throw Errors".

It's one thing if you are testing code outside your control, but that kind of begs the question: "Why are you testing code outside your control?". (TBH, I have written tests for some closed source API's to verify behavior, but that's really rare).

[SamVerschueren]

@jamestalmage I know, but people will create issues for that as to why it throws an error regarding literals. The recipe could clarify the behaviour and could encourage throwing errors is best practice. A lot of developers don't know that and throw strings for instance.

[sindresorhus]

but people will create issues for that as to why it throws an error regarding literals

Sounds like we just need a good error message.

[novemberborn]

It'd be good if after this long discussion somebody could summarize the expected API and behavior of t.throws(). There's still a lot of implicit complexity in the current implementation.

[novemberborn]

Please see #1047 for my proposal on simplifying t.throws().

[novemberborn]

Whilst we haven't simplified the assertion interface yet, the error reporting has improved a lot. Closing this.

[StoneCypher]

It's not clear why, but I was redirected to this ticket to discuss that I think it's probably counterproductive for .throws to reject fully valid thrown exceptions merely because a string rather than an Error object is being thrown

This is contrary to the language standard, is not mentioned in the documentation, and is not mentioned in the resulting error text