Skip to content

Why does code need to be a string in the expected parm of t.throws? #1901

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ronen opened this issue Aug 13, 2018 · 7 comments · Fixed by #1902
Closed

Why does code need to be a string in the expected parm of t.throws? #1901

ronen opened this issue Aug 13, 2018 · 7 comments · Fixed by #1902
Labels

Comments

@ronen
Copy link
Contributor

ronen commented Aug 13, 2018

Description

t.throws() and t.throwsAsync() take an optional expected argument, which is documented as

... a matcher object with one or more of the following properties:

  • [...]
  • code: the expected .code value of the thrown error

But the documentation doesn't mention that it requires expected .code value to be a string (https://github.com/avajs/ava/blob/master/lib/assert.js#L109).

In my app, .code is an integer. Is there a reason not to allow a matcher for integers? If so it'd be nice to mention that requirement in the doc. If not, it'd be nice to change it :)

(I tried passing the code as a string, in case it'd test with == but no luck :)

Of course simple workaround is to use err = async t.throwsAsync(...); t.is(err.code, 12345)

Thanks

Environment

$ node -e "var os=require('os');console.log('Node.js ' + process.version + '\n' + os.platform() + ' ' + os.release())"
Node.js v10.8.0
darwin 17.7.0
$ npx ava --version
1.0.0-beta.7
$ npm --version
6.3.0
@novemberborn
Copy link
Member

This was modeled after the .code property in Node.js, which is a string: #1708

But happy to support numbers as well.

@oantoro
Copy link
Contributor

oantoro commented Aug 13, 2018

Hello @novemberborn

Can I work on this?

@novemberborn
Copy link
Member

@okyantoro let's see if @ronen was keen on tackling this soon, given that he raised the issue. Appreciate the enthusiasm though 👍

@oantoro
Copy link
Contributor

oantoro commented Aug 13, 2018

Sure @novemberborn :)

@ronen
Copy link
Contributor Author

ronen commented Aug 13, 2018

@okyantoro go for it!

(I'm happy to complain but not actually fix anything myself :)

@novemberborn
Copy link
Member

All yours @okyantoro!

@oantoro
Copy link
Contributor

oantoro commented Aug 13, 2018

Thank you @novemberborn @ronen

I created a pull request in #1902. Let me know if I do something wrong with the unit test.

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants