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

Suggest new assertions on async rejection #507

Open
3cp opened this issue Apr 28, 2020 · 11 comments
Open

Suggest new assertions on async rejection #507

3cp opened this issue Apr 28, 2020 · 11 comments

Comments

@3cp
Copy link

3cp commented Apr 28, 2020

Following up #472 (comment)

Suggest to add the two assertions (or something similar) from tape-promise https://github.com/jprichardson/tape-promise#new-assertions

It would not change the existing behaviour, as the two assertions are async.

Example from tape-promise:

test('reject and doesNotReject example', async (t) => {
  await t.rejects(asyncFunction)
  await t.rejects(asyncFunction())
  await t.doesNotReject(Promise.resolve())
})

The await is on the assertion, not on the input of the assertion. It reads "The input promise will eventually reject".

Right now I am using following code to migrate from tape-promise to tape v5.

test('...', async t => {

  try {
    await something();
    t.fail('should not pass');
  } catch (err) {
    t.ok(err instanceof SomeErrorType);
  }
});
@ljharb
Copy link
Collaborator

ljharb commented Apr 28, 2020

This only works if you await it. Additionally, tape works with any promise-returning test function.

node itself has issues with their promise assertion helpers, namely that they need to force an await for work properly, but there's no wait to do that.

What I would suggest is using normal promises:

return something().then(result => { throw result; }, err => err) which will "invert" the promise.

@3cp
Copy link
Author

3cp commented Apr 28, 2020

Oh, I see your point. Thx.

@3cp 3cp changed the title New feature suggestion Suggest new assertions on async rejection Apr 28, 2020
@ljharb
Copy link
Collaborator

ljharb commented Apr 28, 2020

To be clear; node's assert has rejects and doesNotReject, so that's the primary motivation to add it.

The problem (one which node itself also has) is, what do you do if someone does t.rejects(Promise.reject()), and doesn't await that value? Unhandled rejections in the language are perfectly fine, but you do not want those in your tests.

@Raynos
Copy link
Collaborator

Raynos commented Apr 28, 2020

@ljharb

Can you return a thenable from t.rejects that checks to see if someone called then() in nextTick() and if not add a failure like "double end" or "forgot to call end" to the TAP output ?

@ljharb
Copy link
Collaborator

ljharb commented Apr 28, 2020

The user can do that - the problem is that tape can’t force the user to do so.

Calling t.rejects in a test suddenly making the test wait on a promise sounds very strange to me, and we’d have to do some pretty complex acrobatics to combine multiple rejects/doesNotReject promise with the final promise the user may or may not return, including those of subtests.

@ljharb
Copy link
Collaborator

ljharb commented Apr 28, 2020

also, nextTick isn’t soon enough; the test promise might take many ticks to settle.

@Raynos
Copy link
Collaborator

Raynos commented Apr 28, 2020

I guess I am saying what if t.rejects requires await t.rejects as part of the public interface.

Basically make it return a BlockingThenable and tell the user dude you did not await t.rejects

Maybe that's not better, maybe that's worse. But it's an option.

@ljharb
Copy link
Collaborator

ljharb commented Apr 28, 2020

I guess I'm still not clear on what that would mean, a "blocking thenable". Even if we could detect the "fail" case (where the promise does not reject) when the test ends, we couldn't detect the "succeed" case (where the promise does reject).

@3cp
Copy link
Author

3cp commented Apr 28, 2020

I have been hurt by forgetting await before t.rejects.

Also I need to explain to co-worker why t.rejects(await someAsync()) doesn't work.

Appreciate if the tool can point out the direction in error message, which is probably the best we can do here, if it's possible.

@ljharb
Copy link
Collaborator

ljharb commented Apr 28, 2020

If node is incapable of improving the experience here, I'm not sure how we can :-/

@fregante
Copy link
Contributor

Also I need to explain to co-worker why t.rejects(await someAsync()) doesn't work.

That is no different from:

t.rejects(someSync())
console.log(throws())
window.addEventListener('load', onload()) // <-- notice it's called

While they're annoying, they're entry-level errors and there's nothing the engine or other tools can do (except TypeScript in some cases).

As far as Tape or Node is concerned, that line of code isn't any different from

const v = await someAsync();
t.rejects(v)

or just

throw new Error();
t.rejects(v)

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

No branches or pull requests

4 participants