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

Too aggressive in declaring the test "finished" #1327

Closed
jamestalmage opened this issue Mar 30, 2017 · 6 comments · Fixed by singapore/gl-got#20
Closed

Too aggressive in declaring the test "finished" #1327

jamestalmage opened this issue Mar 30, 2017 · 6 comments · Fixed by singapore/gl-got#20

Comments

@jamestalmage
Copy link
Contributor

The following works in 0.18.2:

test(t => {
	t.notThrows(new Promise(resolve => {
		setTimeout(() => {
			t.pass();
			resolve();
		}, 100);
	}));
});

On master, it fails with:

Assertion passed, but test has already ended

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):

test(t => {
   t.notThrows(new Promise(resolve => {
       t.plan(2);
       setTimeout(() => {
         t.pass();
         resolve();
       }, 100);
   }));
});

The above fails with:

Planned for 2 assertions, but got 1.

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.

@novemberborn
Copy link
Member

That's an interesting use case. I don't think it's a great use of t.notThrows() but it seems easier to make this work then to properly communicate why it doesn't.

novemberborn added a commit that referenced this issue Apr 1, 2017
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.
@sindresorhus
Copy link
Member

I'm tempted to say we require t.throws() to be awaited. I (and many users I've talked to) find it surprising that t.throws() works with a Promise in a "sync" test. Would be better to have a clear separation and only allow t.throws() with a promise in an async test (test(async t =>{})).

@sindresorhus
Copy link
Member

sindresorhus commented Apr 3, 2017

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.

@novemberborn
Copy link
Member

@sindresorhus I like that.

sindresorhus pushed a commit that referenced this issue Apr 3, 2017
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
@novemberborn novemberborn reopened this Apr 3, 2017
novemberborn added a commit that referenced this issue Apr 3, 2017
`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.
@jamestalmage
Copy link
Contributor Author

How would we enforce that? Can you tell a function is async before calling it?

@novemberborn
Copy link
Member

@jamestalmage see #1335.

sindresorhus pushed a commit that referenced this issue Apr 5, 2017
`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.
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

Successfully merging a pull request may close this issue.

3 participants