Skip to content
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

throws doesn't count against assertion count and doesn't require error param #309

Closed
mattkrick opened this issue Dec 6, 2015 · 11 comments
Closed

Comments

@mattkrick
Copy link
Contributor

Here's my actual code:

test('LocalStrategy:Signup:AlreadyExists  ', async t => {
  t.plan(1)
  await signupDB('test@test', '123123');
  t.throws(signupDB('test@test', 'XXXXXX'));
});

As is, it fails unless the first line is t.plan(0) (Assertion count does not match planned). I would assume a throws would count as an assertion, right?

Second, the docs show error is the second required param, but the assertion is successful no matter what value I put in there. I can even leave it undefined (as above) & it still passes.

As a workaround, I'm using a try catch block that works fine, but it'd be nice to condense the code & just use throws.

@jamestalmage
Copy link
Contributor

How does signupDB throw? Does it do it synchronously? Then you need to wrap the call in a function:

t.throws(() => signupDB(...));

Does it return a promise that gets rejected? In that case, do not wrap it in a function, but do return the result of t.throws:

test(t=> {
  return t.throws(signupDB(...));
});

@mattkrick
Copy link
Contributor Author

Ahhh, it was the return that got me, thanks!
Any word on the error param? Seems like it's optional, but if supplied, it can also take the Error's name (in a regex), as well. If it's functioning correctly let me know & I'll write a little PR for the readme.

@jamestalmage
Copy link
Contributor

If I recall correctly, you must supply a string for an === comparison. I think I noticed that in the source a few days ago and meant to add regexp matching to my todos. Thanks for the reminder.

@mattkrick
Copy link
Contributor Author

That works for me, thanks!
I'll leave this open in case you wanna use it as a reminder, feel free to close.

@jamestalmage
Copy link
Contributor

Nope, I read the source wrong on that. Should support the same values for err that core-assert#throws does, with the added option of passing in a string for an === comparison

@mattkrick
Copy link
Contributor Author

Ah, that explains it then, from core-assert:
assert.throws = function(block, /*optional*/error, /*optional*/message) {...

@sindresorhus
Copy link
Member

So what's the resolution on this? Anything we can do with the docs to make the behavior clearer?

@mattkrick
Copy link
Contributor Author

Thanks for following up on this!

In the throws section on the readme, it'd be nice to show an example explicitly pointing out that the statement must begin with return, that was the big gotcha for me since it's the only one that requires a return.

With regards to the assertion, I'd say for now just add in that error can also be optional. In a future version, it'd be nice to be able to test for Error.name as well as Error.message. Currently, I'm only regex testing on the name since I can't return two throws. As a workaround for now I can do a try/catch block, but that's just not as clean.

@ariporad
Copy link
Contributor

@sindresorhus: If you don't mind, I'm going to give this a go. I've been discussing this with @jamestalmage, and here's what I think I need to do (I've started on this a little):

  • Replace test._assertCount with test._assertPromises. Every assertion pushes a Promise (that can just be Promise.resolve/.reject if needed) onto that array, then test.exit() compares the length of the array with the planned number of assertions.
  • Add some tests. So far, I think I need to test (credit for most of this goes to @jamestalmage):
    • That you simply don't need to return the promise from t.throws
    • test.cb
      • t.end is called with a non-yet rejected promise passed to t.throws
      • t.end is called with a non-yet resolved promise passed to t.throws
    • Test returns a promise that resolves before a t.throws promise.
    • Test returns a promise that rejects before a t.throws promise.
    • Multiple t.throws, with several resolving and rejecting promises.
    • All the ways to mess up t.plan
      • The proper number of assertions are there when the test is ended, but another one is preformed while doing Promise.all(test._assertPromises).
        • I think this can be solved by simply checking the number of assertions after all the promises have settled.
      • The wrong number of assertions are there when the test is ended, but another one is added while doing Promise.all(test._assertPromises).
      • ???
    • ???

If anyone has any other ideas for things to test, or other feedback, that would be awesome!

@jamestalmage
Copy link
Contributor

Just to clarify, there is a bit of a new idea here. That is to track the promises passed to t.throws() and t.doesNotThrow, and avoid the requirement to do explicitly return t.throws

ariporad added a commit to ariporad/ava that referenced this issue Dec 23, 2015
ariporad added a commit to ariporad/ava that referenced this issue Dec 23, 2015
@ariporad
Copy link
Contributor

Ok, This is fixed in #360.

ariporad added a commit to ariporad/ava that referenced this issue Dec 25, 2015
ariporad added a commit to ariporad/ava that referenced this issue Dec 25, 2015
ariporad added a commit to ariporad/ava that referenced this issue Dec 25, 2015
ariporad added a commit to ariporad/ava that referenced this issue Dec 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants