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

Promise rejections are not handled #8

Closed
ifiokjr opened this issue Nov 12, 2018 · 11 comments
Closed

Promise rejections are not handled #8

ifiokjr opened this issue Nov 12, 2018 · 11 comments

Comments

@ifiokjr
Copy link

ifiokjr commented Nov 12, 2018

Bug

  • package version: 1.0.2
  • node version: 10.13.0
  • yarn version: 1.12.3

Relevant code or config

See https://jestjs.io/docs/en/expect#rejects

test('rejects to octopus', async () => {
  await expect(Promise.reject(new Error('octopus'))).rejects.toThrow('octopus');
});

test('rejects to octopus', () => {
  // make sure to add a return statement
  return expect(Promise.reject(new Error('octopus'))).rejects.toThrow(
    'octopus',
  );
});

What you did:

Neither of the above scenarios works with jest-chain. I haven't looked into how jest currently deals with promises but it seems that jest-chain breaks the expected behavior.

What happened (please provide anything you think will help):

The tests pass regardless of whether or not the promises throw. This seems related to #1

@yatki
Copy link

yatki commented Mar 22, 2019

This is a very annoying problem. It took me a while to realise it was being caused because of jest-chain. Would be great if you could fix it.

@anodynos
Copy link

Any news on this? I'd really like to start using jest-chain, but this issue puts me off :-(

@dobesv
Copy link

dobesv commented Oct 23, 2019

I think there might be a bit of a conflict between jest-chain and asynchronous tests. The async code has to return a promise, but just-chain changes it to return a Matcher. Maybe if jest-chain returned a matcher that was also a promise it would fix it ? e.g. the matcher implements then by pass-thru to whatever value was actually returned by the matching function.

@InsOpDe
Copy link

InsOpDe commented Apr 29, 2020

@dobesv Am I using this correctly?
with

await expect(rbac.can(ERole.user, EOperation.postSave))
			.rejects.toBeInstanceOf(RbacProgrammaticException)
			.rejects.toMatchObject({
				name: "RbacProgrammaticException"
			});

or

expect(Promise.resolve(rbac.can(ERole.user, EOperation.postSave)))
			.rejects.toBeInstanceOf(RbacProgrammaticException)
			.rejects.toMatchObject({
				name: "RbacProgrammaticException"
			});

I get

Property 'rejects' does not exist on type 'Promise<ChainedMatchers<Promise>>'

It works like so:

await (await expect(rbac.can(ERole.user, EOperation.postSave))
			.rejects.toBeInstanceOf(RbacProgrammaticException))
			.rejects.toMatchObject({
				name: "RbacProgrammaticException"
			});

@dobesv
Copy link

dobesv commented Apr 29, 2020

@InsOpDe I'm not sure my change (assuming you are running the code from my PR) allows actually chaining on rejects. It just makes it so that importing jest-chain doesn't completely prevent you from using async matchers. That said, supporting what you wrote would be cool. Maybe there's a way to do that.

@dobesv
Copy link

dobesv commented Apr 29, 2020

@InsOpDe Actually I see from the tests I added that this would have worked for resolves so I'm not sure why rejects wouldn't work as well. Are you sure you're running my (not yet merged) code?

@InsOpDe
Copy link

InsOpDe commented Apr 29, 2020

@dobesv
acutally i got that idea of chaining promises from your tests.
I have your fork like so in my package.json:

"jest-chain": "git://github.com/dobesv/jest-chain#promise-support",

So yes, im certain that I have your codebase.

@dobesv
Copy link

dobesv commented Apr 29, 2020

Hmm, you know, it looks like the issue might be in the typescript types not reflecting the change. I'm not sure how best to address that ...

Maybe if you hand-edit the typescript spec to change ChainedMatchers as follows:

  interface ChainedMatchers<T>
    extends jest.JestMatchersShape<
      Matchers<ChainedMatchers<T>, T>,
      Matchers<Promise<ChainedMatchers<T>> & ChainedMatchers<T>, T>
    > {}

If that works I could push the same change to my branch.

@InsOpDe
Copy link

InsOpDe commented Apr 30, 2020

With this change i would not get any type-error 👍

@dobesv
Copy link

dobesv commented Apr 30, 2020

@InsOpDe Thanks for your help, I've updated the PR. Now it's just up to the project maintainer(s) to take a look at it.

@mattphillips
Copy link
Owner

Sorry for the delay in coming to this! This should be fixed in: https://www.npmjs.com/package/jest-chain/v/1.1.6

@dobesv thanks for all of your help in this issue and your PR!

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

6 participants