show error message if literal is thrown - fixes #661

[SamVerschueren]

This PR fixes #661 which prints Expected an object to be thrown.. It's the same error message that ESLint shows when trying to throw a literal. Feel free to provide feedback of any kind.

[mention-bot]

By analyzing the blame information on this pull request, we identified @kasperlewau, @ariporad and @sotojuan to be potential reviewers

[sindresorhus]

Can you also add what was thrown to the message, the type and value. Might be helpful.

[SamVerschueren]

Also added the types to actual and expected. Not sure if necessary as it isn't being printed in the console?

[sindresorhus]

Not sure if necessary as it isn't being printed in the console?

Not really necessary as they're only useful in an AssertionError.

[SamVerschueren]

Not really necessary as they're only useful in an AssertionError.

But it's ok to keep it like this?

[SamVerschueren]

Not sure if this check is good enough in every case though. Should I use something like is-error instead? Because now someone can still throw {foo: 'bar'}.

[jamestalmage]

👍 for is-error.

[novemberborn]

Hi @SamVerschueren. I don't think this PR is the correct solution to #661.

First some background.

t.throws() essentially wraps assert.throws(). That method supports constructor/instance checks, regular expression matches, and validation functions. We support those checks as well as an error message comparison validation.

These lines implement that error message comparison validation. err here is not the error thrown by fn(), it's how that error should be checked.

In this example the error message does not compare ('foo'.message !== 'foo'):

import test from 'ava'

async function fn() {
    return Promise.reject('foo');
}

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

The reason you get undefined undefined undefined is that we're assuming a validation error like that causes an AssertionError to be thrown. However Node throws the actual error which is the 'foo' string.

In other words we need to check the error coming out of assert.throws() and call test() with the right arguments.

[SamVerschueren]

Hi @novemberborn. Thanks for looking into this. Can you explain why it's not a solution for #661? I read your post multiple times but can't really find an answer :). After looking into it again, I might have found a solution that's "cleaner" and might fit your approach more. I will make another PR with the new solution so we can compare and make sure I understand you correctly.

These lines implement that error message comparison validation. err here is not the error thrown by fn(), it's how that error should be checked.

Yes I know, I had it wrong in the original issue. I'm not touching those lines. I thought that was the issue but after diving deeper I noticed it's a custom compare function for assert.throws.

[novemberborn]

Can you explain why it's not a solution for #661?

t.throws() shouldn't reject literals. It should ensure something was thrown. If that something didn't match expectations it should correctly report that. Right now it's the reporting that's broken, not the validation.

[jamestalmage]

t.throws() shouldn't reject literals. It should ensure something was thrown.

I disagree. #468 (comment)

[SamVerschueren]

I agree with @jamestalmage although you might argue if AVA should enforce this best practice...

[jamestalmage]

you might argue if AVA should enforce this best practice

I think it protects you from careless mistakes.

[novemberborn]

@SamVerschueren hope it's OK if I close this for now. I reckon resolving #661 will require some more changes.