ava
ava copied to clipboard
Allow throws / throwsAsync to work with any value, not just errors
t.throws() and t.throwsAsync() require the resulting exception to be a proper error:
https://github.com/avajs/ava/blob/952a0177758c5601a72c2b050fc0308e7fa774c7/lib/assert.js#L147-L154
I propose we add an any: boolean option to the expectations object used for these assertions. If true it won't cause a failed assertion when the exception is not an error.
We need to update the validation logic here to allow this property:
https://github.com/avajs/ava/blob/952a0177758c5601a72c2b050fc0308e7fa774c7/lib/assert.js#L74
Don't forget to update the type definition:
https://github.com/avajs/ava/blob/952a0177758c5601a72c2b050fc0308e7fa774c7/index.d.ts#L11
If any is true, we should try and change the typing of the return value to be unknown:
https://github.com/avajs/ava/blob/952a0177758c5601a72c2b050fc0308e7fa774c7/index.d.ts#L252
https://github.com/avajs/ava/blob/952a0177758c5601a72c2b050fc0308e7fa774c7/index.d.ts#L263
And update our own tests:
https://github.com/avajs/ava/blob/952a0177758c5601a72c2b050fc0308e7fa774c7/test-tap/assert.js#L786
https://github.com/avajs/ava/blob/952a0177758c5601a72c2b050fc0308e7fa774c7/test-tap/assert.js#L976
See also the discussion in https://github.com/avajs/ava/issues/1841.
I think we should add a short note to the docs that if the user is in control of the error, they should prefer to make it a proper Error.
Hello @novemberborn & @sindresorhus, I would like to tackle this issue if possible.
Thank you,
@randomicon00 that's great go for it 😄
@novemberborn Will do thanks :)
@sindresorhus @novemberborn
There are very valid use cases where throwing a non-error value not only makes sense but is almost the only viable solution.
My particular case is a sort of asynchronous calculation graph implementation, where any node can either have a value, or be in an 'error' or 'stale' state, so when a node is trying to calculate its value and one of its dependencies is either an error or is stale, I would prefer to propagate this state by re-throwing it.
However using a 'magic' string for the stale state instead of an Error class allows for very subtle logic since I can write something like if (error === STALE) { ... } which would be impossible with an instance of a class.
Comparing values using === is exactly how most reactivity systems implement their logic.
Currently in each and every test I have to wrap my functions that can throw STALE in order to test them. This is quite ridiculous.
IMO, if one specifies { is } in the Expectation that should be perfectly OK to allow values of any type. Even better design would be to pass a function (err) => expectation(err) instead of Expectation. Then I would be able to use any asserts already in t, e.g. t.throws(() => myCode(), err => t.is(err, STALE)) or t.throws(() => myCode(), err => t.true(err instanceof Whatever))
In any case I find it quite weird that a testing framework should be opinionated about which values the code being tested might throw :)
@novemberborn I'd like to take a crack at this.
It assigned to me, I still didn't complete it but you can try I guess. I am still working on it.
@randomicon00 , it's cool if you are working on it. I though that it was inactive, so asked.
any updates on this? it's very annoying that there is no escape hatch...
@novemberborn I'd still like to take a crack at this.
By all means @adiSuper94! I'm low on time but I do want this in for AVA 6. Hopefully you get to it before I do 😀
I have added a PR that tries to fix this. @novemberborn your comments outlining the places for code change was very helpful. Thank you.
If any is true, we should try and change the typing of the return value to be unknown
I am a bit confused about this. The signature of the throws/throwsAsync function itself doesn't seem change.
The return type of of the function passed to throws/throwsAsync is any.
And even if it throws a non error value, wouldn't the return type still remain the same?
If I am missing anything could you explain how this can be done ?