-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Too aggressive in declaring the test "finished" #1327
Comments
That's an interesting use case. I don't think it's a great use of |
Whilst not a great way to write tests, users may end up adding more assertions from inside a promise chain that's passed to the `t.throws()` or `t.notThrows()` assertions. These should be counted for the plan, and not fail the test as being extraneous. Fixes #1327.
I'm tempted to say we require |
I already always do: test(async t => {
await t.throws(…);
}); Instead of: test(t => {
t.throws(…);
}); The former is clearer by being explicitly async. It's easier to add stuff after the throws assertion and get expected order of execution. You can get the error without having to change things around. Simpler to document. |
@sindresorhus I like that. |
Whilst not a great way to write tests, users may end up adding more assertions from inside a promise chain that's passed to the `t.throws()` or `t.notThrows()` assertions. These should be counted for the plan, and not fail the test as being extraneous. Fixes #1327
`t.throws()` and `t.notThrows()` can be called with an observable or promise. This commit forces users to wait for the assertion to complete before finishing the test. Usually this means the test has to be written like: ```js test(async t => { await t.throws(Promise.reject(new Error())) }) ``` Or for callback tests: ```js test.cb(t => { t.throws(Promise.reject(new Error())).then(t.end) }) ``` This simplifies internal logic and helps discourage code like in #1327. Anecdotally users are surprised by the previous behavior where a synchronous test worked with an asynchronous assertion (#1327 (comment)). Fixes #1327.
How would we enforce that? Can you tell a function is |
@jamestalmage see #1335. |
`t.throws()` and `t.notThrows()` can be called with an observable or promise. This commit forces users to wait for the assertion to complete before finishing the test. Usually this means the test has to be written like: ```js test(async t => { await t.throws(Promise.reject(new Error())) }) ``` Or for callback tests: ```js test.cb(t => { t.throws(Promise.reject(new Error())).then(t.end) }) ``` This simplifies internal logic and helps discourage code like in #1327. Anecdotally users are surprised by the previous behavior where a synchronous test worked with an asynchronous assertion (#1327 (comment)). Fixes #1327.
The following works in
0.18.2
:On master, it fails with:
We should wait until all pending assertions have resolved before declaring a test "finished" and throwing when new assertions show up.
Adding a call to
t.plan()
causes it to behave oddly going all the way back (we even added a test solidifying this odd behavior in #360):The above fails with:
Similar results for as far back as I tested.
I think we should allow for added assertions, as long as we still have pending assertions. When
t.plan
is used, I don't think we should check for too few assertions until we've drained all pending assertions.The text was updated successfully, but these errors were encountered: