-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
async/await .not.toThrow idiomatic check #1377
Comments
I totally agree we should have this. |
i'll make it work! |
no, please don't. expect(() => {
return new Promise(…);
}).toThrow(); when the function is called and returns a promise and it doesn't throw, we should wait for the promise to resolve and then make sure it throws. Since it creates an |
Any status on this? |
@cpojer I gave this a try and it appears to be more work than expected, and a consequent evolution of the way return expect(asyncFunction).toThrow();
// or better
await expect(asyncFunction).toThrow(); But once you make expect.toThrow compatible with promises, you'll have a hard time justifying not making all matchers compatible with promises. And that's a lot of work and added complexity when our initial problem was this try+await+catch block. It might be better to add a it('should throw', async () => {
const syncFunction = await jest.syncify(asyncFunction);
expect(syncFunction).toThrow();
}); The syncify = async (fn) => {
try {
const result = await fn();
return () => { return result; };
} catch (e) {
return () => { throw e; };
}
}; Let me know if this idea makes sense to you. |
a sample to use it this way:
|
Why can't Jest detect if a promise was returned in a |
Must return a ExpectationResult type, can not is Promise. As louisremi said, we need |
|
meanwhile we have a solution for this. So first, I test that the happy path works, like this:
Then posible situations where I want exceptions to be thrown
The problem here is if you forget |
@alfonsodev Maybe a slightly safer temporary solution would be: let message;
try {
await thingYouExpectToFail();
} catch (error) {
message = error.message;
}
expect(message).toBe('some string'); // Or expect(message).toBeTruthy(); |
I'd like to cast my vote for this api:
I like it because it's simple and explicit. |
FWIW, I needed to test whether or not const getAsyncThrowable = fn => async (...args) =>
await syncify(async () => await fn(...args)) So that I can write tests like: it('should throw if the parameter is invalid', async () => {
const myAsyncFunctionThrowable = getAsyncThrowable(myAsyncFunction)
expect(await myAsyncFunctionThrowable('good')).not.toThrow()
expect(await myAsyncFunctionThrowable('bad')).toThrow(/invalid/)
}) |
@cowboy that is actually pretty sweet. |
@cpojer Should I open a PR to propose adding jest.syncify to the API? |
I don't think |
This proposal is indeed quite nice to test an async function but it requires some boilerplate when all you need to test is a promise. Currently it's easy to test whether a promise resolve by simply This could be mitigated by using a helper function which would swap the resolution/rejection of the promise: expect(await rejectionOf(promise)).toBeInstanceOf(TypeError)
const returnArg = value => value
const throwArg = value => { throw value }
const rejectionOf = promise => promise.then(throwArg, returnArg) But it may be better to make await expect(promise).toResolve().toBe('foo')
await expect(promise).toReject().toBeInstanceOf(TypeError)
Small digression: this example also leads me to think that some matchers are missing, e.g. there are currently no ways to test an error without using expect(myFunction).toThrow().toBeAnError(TypeError)
expect(myOtherFunction).toThrow().toMatchObject({ code: 'ENOENT' }) |
I really like your proposal for a I have simplified it a bit (together with your helper function @cowboy) and came up with: sync = fn => async (...args) => {
try {
const result = await fn(...args);
return () => { return result; };
} catch (e) {
return () => { throw e; };
}
}; A it('should throw', async () => {
const syncFunc = expect.sync(asyncFunc);
expect(await syncFunc('bad')).toThrow();
}); Or in shortened syntax: it('should throw', async () => {
expect(await expect.sync(asyncFunc)('bad')).toThrow();
}); What do you guys think? |
Personally, it's not my favorite approach, someone not knowing the I prefer explicit support of promises in await expect(promise).toReject().toBeInstanceOf(TypeError) |
That's kinda strange @excitement-engineer to have an expect.async(promise).toThrow();
expect.async(async () => await something()).toThrow(); |
Would you like to weigh in @cpojer on the proposals made thus far to see if one could be implemented? This is an issue that I keep stumbling on when testing async code, would be great if a solution gets added to jest in the short run! |
Maybe I'm taking crazy pills, but this seems to work just fine, no API change needed:
It's 1 line, it's expressive, it's useable today. That said, if someone wanted to write a PR... |
const expectAsyncToThrow = async (promise) => {
try {
await promise;
} catch(e) {
expect(() => {throw e}).toThrowErrorMatchingSnapshot();
}
};
test('throws a hello to your mom', async() => {
await expectAsyncToThrow(Promise.resolve('Hi mom'));
}); The code snippet above also passes! The problem is that if the promise resolves then the catch statement is never called. This causes the test to pass when it should fail. |
@excitement-engineer good catch! (pun totally intended)
throwing inside a try block is usually an eslint faux pas, but i think we can make an exception here. Put a nice random string in there so folks can't accidentally run |
I've changed it a bit. Since my original method returns a promise, I have no problem in just calling it instead of returning a function so the calling code becomes a bit simpler: // My final utility function, "synchronizer":
const sync = fn => fn.then(res => () => res).catch(err => () => { throw err });
// Example function for testing purposes
const check = async arg => {
if (arg > 5) throw new Error('Too large!');
if (arg < 0) throw new Error('Too small!');
return 'Good to go';
};
// Write the expect() in quite a clear way
it('checks the boundaries properly', async () => {
expect(await sync(check(-1))).toThrow();
expect(await sync(check(0))).not.toThrow();
expect(await sync(check(5))).not.toThrow();
expect(await sync(check(10))).toThrow();
}); I decided calling it It has the added benefit compared to |
Up until Jest 22 this used to work expect(Promise.reject({ message: 'not an error' })).rejects.toThrow(/error/); As of Jest 23+ it now says
Is the change intended? |
@piuccio await expect(
Promise.reject({ message: 'not an error' })
).rejects.toThrow(/error/); |
None of the suggestions above were working for me so I ended up doing: expect.assertions(2);
try {
await mySpecialAsyncFunction();
} catch (e) {
expect(e.message).toEqual("some message");
expect(e instanceof MySpecialError).toBeTruthy();
} |
@adanilev 's pattern works (and it's what I've been using with jest 23), but it's clunky. jest should do better than this. Often, a single it(`rejects invalid passwords`, () => {
expect( getIsPasswordValid('') ).toBe( false )
expect( getIsPasswordValid(' ') ).toBe( false )
expect( getIsPasswordValid('\n') ).toBe( false )
}) That becomes way clunkier if the code being tested is supposed to throw under several different scenarios: it(`throws when the argument is bad`, () => {
expect.assertions(6)
try {
await testTheArgument('')
} catch(error) {
expect(error instanceof SpecialError).toBe(true)
expect(error.message).toBe('SOME_ERROR_CODE')
}
try {
await testTheArgument(' ')
} catch(error) {
expect(error instanceof SpecialError).toBe(true)
expect(error.message).toBe('SOME_ERROR_CODE')
}
try {
await testTheArgument('\n')
} catch(error) {
expect(error instanceof SpecialError).toBe(true)
expect(error.message).toBe('SOME_ERROR_CODE')
}
}) We can't go on like this. |
That's exactly what I wanted to address in my API proposal: await expect(testTheArgument('')).toReject(error => {
expect(error intanceof SpecialError).toBe(true)
expect(error.message).toBe('special error')
}) |
You should be able to add your own predicate function for that, right? E.g. https://github.com/jest-community/jest-extended/blob/master/README.md#tosatisfypredicate await expect(testTheArgument('')).rejects.toSatisfy(error => {
expect(error instanceof SpecialError).toBe(true)
expect(error.message).toBe('special error')
return true
}) Should work as |
I'm using @adanilev solution too, but I would love a better way to write async/await throw tests :) |
Thanks @SimenB, I didn't know about this lib, I'm keeping it close in case I need it. @odelucca For me the best way for now is // invert fulfillment and rejection: fulfill with with rejected value, reject fulfillment value
const rejectionOf = promise => promise.then(
value => { throw value },
reason => reason
);
const e = await rejectionOf(mySpecialAsyncFunction());
expect(e.message).toEqual("some message");
expect(e instanceof MySpecialError).toBeTruthy(); |
@julien-f excellent and gracious way of doing it :) thanks for sharing! I'll start using it too |
That does not work because your tests will be green even when no error is thrown (because then the catch won't be triggered and no expectations will be run. No expectations are also green) |
@LukasBombach isn't |
Ahhh! I did not see that! Thank you! |
You can just write: |
What about |
@Blutude Your async function does return something: it's a promise resolving to await expect(...).resolves.toBeUndefined() #1377 (comment) |
Hello - I created the following matcher to accomplish this in a more idiomatic way,
Is there some problem with this approach? Why is this not built in, or other people doing it this way? |
Because there's already |
@jgburet The issue I was encountering with that approach is that it forces me to work directly with promises when receiving results from functions that shouldn't throw. For example code like the following:
This is because
So my test code started to look like the following, which I feel raises the cognitive load a good bit, not to mention the intermixing of promise/async paradigms:
Alternatively with the matcher:
@jgburet I am not greatly experienced with this, and welcome any advice to make these types of tests readable and concise. |
const pHandle = factory.produce()
await expect(phandle).resolves.toMatchObject({ value: true }) |
This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
To check that async functions are not throwing exceptions I do this:
Is it possible to do something like this?
We came across this with @kentaromiura today.
The text was updated successfully, but these errors were encountered: