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

async/await .not.toThrow idiomatic check #1377

Closed
bestander opened this issue Aug 5, 2016 · 53 comments
Closed

async/await .not.toThrow idiomatic check #1377

bestander opened this issue Aug 5, 2016 · 53 comments

Comments

@bestander
Copy link
Contributor

bestander commented Aug 5, 2016

To check that async functions are not throwing exceptions I do this:

let allCorrect = true;
try {
  await check();
} catch(err) {
  allCorrect = false;
}
expect(allCorrect).toBe(true);

Is it possible to do something like this?

expect(async () => await check()).not.toThrow()

We came across this with @kentaromiura today.

@cpojer
Copy link
Member

cpojer commented Aug 9, 2016

I totally agree we should have this.

@aaronabramov
Copy link
Contributor

aaronabramov commented Aug 9, 2016

i'll make it work!
@cpojer should we add async functions to jest .babelrc?

@cpojer
Copy link
Member

cpojer commented Aug 9, 2016

no, please don't. async/await compiles into a promise. This is literally just like the work you did for pit:

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 async function inside of another async function it should properly make the it callback a promise, right?

@calebmer
Copy link
Contributor

Any status on this?

@louisremi
Copy link

louisremi commented Oct 8, 2016

@cpojer I gave this a try and it appears to be more work than expected, and a consequent evolution of the way expect is used. If expect.toThrow was made compatible with promises, then people would have to use it this 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 syncify util to jest that would allow writing such tests:

it('should throw', async () => {
  const syncFunction = await jest.syncify(asyncFunction);

  expect(syncFunction).toThrow();
});

The syncify implementation is trivial:

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.

@yutin1987
Copy link

yutin1987 commented Oct 14, 2016

a sample to use it this way:

const fn = jest.fn();
fn.mockClear();
fn.mockImplementation(() => Promise.reject(new Error('WoW Error!!')));
await expect(async () => {
  try {
    const result = await fn();
    return () => result;
  } catch (e) {
    return () => { throw e; };
  }
}).toThrowError('WoW Error!!');
const fn = jest.fn();
fn.mockClear();
fn.mockImplementation(() => Promise.reject(new Error('WoW Error!!')));
await expect(async () => await fn()).toThrowError('WoW Error!!');

@calebmer
Copy link
Contributor

Why can't Jest detect if a promise was returned in a toThrow function and add it to a queue that will block the test from completing until it resolves? Or add a new expectation like toResolve or toReject, this would make it explicit that the expectation should be awaited.

@yutin1987
Copy link

Must return a ExpectationResult type, can not is Promise.
https://github.com/facebook/jest/blob/master/packages/jest-matchers/src/index.js#L75

As louisremi said, we need jest.syncify().

@yutin1987
Copy link

yutin1987 commented Oct 16, 2016

let error: Error;
try { await fn(); } catch (e) { error = e; }
expect(error).toEqual(new Error('Error!!'));

@alfonsodev
Copy link

alfonsodev commented Oct 21, 2016

meanwhile we have a solution for this.
here is a example that can be useful for some.
In my case Instead of testing that the function doesn't throw exceptions,
I test the oposite, that exceptions are actually thrown,
because there can be implementations that are actually failing silently
by not throwing exceptions when they should.

So first, I test that the happy path works, like this:

  it('return id if user created correctly', async () => {
    let expectedUserId = 'ed6b4405-e627-4f22-aa8b-6f1aa8a85ae4'
    fetch.setResponse({ status: 201, headers: { location: `/users?id=eq.${expectedUserId}`}})
    let model = new Model(apiHost, { users: 'users' })
    let userId = await model.newUser({username: 'Alice', password: '12345', email: 'alice@darpanet.org' })
    expect(userId).toEqual(expectedUserId)
  })

Then posible situations where I want exceptions to be thrown

    try {
      let err = await m.newUser(invalidUserData)
    } catch(e) {
      expect(e).toBeInstanceOf(Error)
      return   
    }
   expect(true).toBeFalsy() 

The problem here is if you forget expect(true).toBeFalsy() you can get a false positive.
If it would be posible to a test to fail if expect is not called, then it would remove the ned of expect(true).toBeFalsy()

@bvaughn
Copy link
Contributor

bvaughn commented Nov 11, 2016

@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();

@spudly
Copy link

spudly commented Nov 18, 2016

I'd like to cast my vote for this api:

expect(promise).toResolve(optionalResolvedValue);
expect(promise).toReject(optionalRejectionValue);

I like it because it's simple and explicit.

@cowboy
Copy link

cowboy commented Dec 12, 2016

FWIW, I needed to test whether or not async functions threw in jest, so along with the syncify function, I wrote this little helper:

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/)
})

@cpojer
Copy link
Member

cpojer commented Dec 12, 2016

@cowboy that is actually pretty sweet.

@louisremi
Copy link

louisremi commented Dec 12, 2016

@cpojer Should I open a PR to propose adding jest.syncify to the API?

@cpojer
Copy link
Member

cpojer commented Dec 12, 2016

I don't think jest.sincify is a great name. It should live somewhere on expect possibly and be in the jest-matchers package.

@julien-f
Copy link

julien-f commented Dec 14, 2016

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 await-ing for it but it's harder (or verbose) to test that a promise reject.

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 expect() return a promise-like value and to add a toResolve() and toReject() matchers/modifiers:

await expect(promise).toResolve().toBe('foo')
await expect(promise).toReject().toBeInstanceOf(TypeError)

I chose to make toResolve() and toReject() functions (unlike .not) because they can be used by themselves.

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 toThrowError().
I think it would be nice to add a toBeAnError() matcher and make toThrow() chainable:

expect(myFunction).toThrow().toBeAnError(TypeError)
expect(myOtherFunction).toThrow().toMatchObject({ code: 'ENOENT' })

@excitement-engineer
Copy link
Contributor

I really like your proposal for a syncify function @louisremi!

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 sync util could then be added to expect. This can then be used as follows:

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?

@julien-f
Copy link

julien-f commented Feb 6, 2017

Personally, it's not my favorite approach, someone not knowing the expect.sync() function has no way to understand what it does.

I prefer explicit support of promises in expect:

await expect(promise).toReject().toBeInstanceOf(TypeError)

@jgburet
Copy link

jgburet commented Feb 9, 2017

That's kinda strange @excitement-engineer to have an expect in another expect.
I would prefer @julien-f 's solution, but maybe a compromise is possible like Jasmine's done.fail() method:

expect.async(promise).toThrow();
expect.async(async () => await something()).toThrow();

@excitement-engineer
Copy link
Contributor

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!

@mattkrick
Copy link

Maybe I'm taking crazy pills, but this seems to work just fine, no API change needed:

const expectAsyncToThrow = async (promise) => {
  try {
    await promise;
  } catch(e) {
    expect(() => {throw e}).toThrowErrorMatchingSnapshot();
  }
};

test('throws a hello to your mom', async() => {
  await expectAsyncToThrow(Promise.reject(new Error('Hi mom')));
});

It's 1 line, it's expressive, it's useable today. That said, if someone wanted to write a PR...

@excitement-engineer
Copy link
Contributor

@mattkrick

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.

@mattkrick
Copy link

@excitement-engineer good catch! (pun totally intended)

try {
    await promise;
    throw new Error(`Jest: test did not throw. ${Math.random()}`)
  } catch (e) {
    expect(() => { throw e; }).toThrowErrorMatchingSnapshot();
  }

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 -u without looking at their snapshots 😄

@franciscop-invast
Copy link

franciscop-invast commented May 18, 2018

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 sync() since it makes my async code behave more like sync. I removed an extra wrapper since I already called it above. As I know I'm going to be using this only with promises, I could remove a bit by assuming fn is a promise.

It has the added benefit compared to rejects that if you forget either the await or the sync, it'll throw errors and not fail silently. Same as if you try to pass a non-promise.

@piuccio
Copy link

piuccio commented Jun 11, 2018

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

    Expected the function to throw an error matching:
      /error/
    But it didn't throw anything.

Is the change intended?

@jgburet
Copy link

jgburet commented Jun 11, 2018

@piuccio
Try await-ing your expect, eventually.

await expect(
  Promise.reject({ message: 'not an error' })
).rejects.toThrow(/error/);

@adanilev
Copy link

adanilev commented Sep 10, 2018

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();
}

@tomprogers
Copy link

@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 block needs to run the tested code several times to fully test the behavior described in the it. So, you might do:

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.

@julien-f
Copy link

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')
})

@SimenB
Copy link
Member

SimenB commented Dec 13, 2018

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 expect throws. If not, it should be easy enough to create a matcher that takes a function and rethrows if that throws or something like that

@delucca
Copy link

delucca commented Dec 21, 2018

I'm using @adanilev solution too, but I would love a better way to write async/await throw tests :)

@julien-f
Copy link

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 rejectionOf that I posted in this comment:

// 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();

@delucca
Copy link

delucca commented Dec 23, 2018

@julien-f excellent and gracious way of doing it :) thanks for sharing! I'll start using it too

@LukasBombach
Copy link

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();
}

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)

@marcinczenko
Copy link

@LukasBombach isn't expect.assertions(2) used for this very reason?

@LukasBombach
Copy link

Ahhh! I did not see that! Thank you!

@develax
Copy link

develax commented Mar 18, 2019

You can just write:
await expect(asyncFunc()).rejects.toThrowError(TypeOfError);

@Blutude
Copy link

Blutude commented Jan 14, 2020

What about await expect(...).not.toThrow() ? Which is what the original question was about. My async function does not return anything so I cannot use await expect(...).resolves...

@jgburet
Copy link

jgburet commented Jan 15, 2020

@Blutude Your async function does return something: it's a promise resolving to undefined.

await expect(...).resolves.toBeUndefined()

#1377 (comment)
#1377 (comment)
https://jestjs.io/docs/en/expect#rejects

@doublemarked
Copy link

Hello - I created the following matcher to accomplish this in a more idiomatic way,

expect.extend({
  toAsyncThrow(fn: () => Promise<unknown>) {
    const message = () => `expected async function ${this.isNot ? 'not' : ''} to throw`;
    return fn()
      .then(() => ({ pass: false, message }))
      .catch(() => ({ pass: true, message }));
    },
  });

// later 
await expect(async () => {
   await something();
}).not.toAsyncThrow();

Is there some problem with this approach? Why is this not built in, or other people doing it this way?

@jgburet
Copy link

jgburet commented Mar 4, 2021

Because there's already resolves and rejects maybe?
#1377 (comment)

@doublemarked
Copy link

doublemarked commented Mar 4, 2021

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

const handle = await factory.produce();
expect(handle.value).toBeTruthy();

This is because expect() requires a promise handle in order to use .resolves or .rejects, but I need to await that handle to use the result:

// Matcher error: received value must be a promise 
await expect((handle = await factory.produce())).resolves.toBeTruthy();

expect(handle.value).toBeTruthy();

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:

const promiseHandle = factory.produce();

expect(promiseHandle).resolves.toBeTruthy();
const handle = await promiseHandle;

expect(handle.value).toBeTruthy();

Alternatively with the matcher:

let handle;
await expect(async () => {
   handle = await factory.produce();
}).not.toAsyncThrow();

expect(handle.value).toBeTruthy();

@jgburet I am not greatly experienced with this, and welcome any advice to make these types of tests readable and concise.

@jgburet
Copy link

jgburet commented Mar 6, 2021

const pHandle = factory.produce()
await expect(phandle).resolves.toMatchObject({ value: true })

@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests