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

How to test an ES7 async function #415

Closed
krnlde opened this Issue Apr 3, 2015 · 41 comments

Comments

Projects
None yet
@krnlde

krnlde commented Apr 3, 2015

Hey guys,
I use ES7 async functions and tried to debug them. Having this method to test:

// ...
const local = new WeakMap();

export default class User {

  // ...

  async password(password) {
    if (!password) return local.get(this).get('hash'); // remove this for security reasons!
    if (password.length < 6) throw new Error('New password must be at least 6 characters long');
    if (!password.match(passwordPattern)) throw new Error(`New password must match ${passwordPattern}`);
    local.get(this).set('hash', await Password.hash(password));
  }

  // ...

}

and this test case:

import chai from 'chai';
import chaiAsPromised from 'chai-as-promised';

import User from '../../../server/User';

chai.use(chaiAsPromised);
const expect = chai.expect;

describe('.password()', () => {

  const testuser = new User({username: 'Testuser', password: '123abc'});

  // FINDME
  it(`should throw when too short`, () => {
    return expect(testuser.password('1a')).to.eventually.throw();
  });

  // ...

});

...the test case does not catch the error thrown - instead the case succeeds first and fails later (asynchronously) with an uncaught error because the method threw in the scope of the it() function not the expect().

Any suggestions or advice?

Thanks in advance!

P.S.: Also I created a stackoverflow issue for this a few days ago, but now answer so far. That's why I am calling you guys. http://stackoverflow.com/questions/29334775/how-to-test-an-es7-async-function-using-mocha-chai-chai-as-promised

@krnlde

This comment has been minimized.

Show comment
Hide comment
@krnlde

krnlde Apr 3, 2015

btw. expect(() => testuser.password('1a')) and expect(async () => await testuser.password('1a')) do not work either.

The only thing that seems to work right now is catching the error via try catch:

it(`should throw when too short`, async (done) => {
  try {
    await testuser.password('1a');
    done('failed');
  } catch (e) {
    done();
  }
});

But this is ugly and doesn't even use the bdd methods or profit from chai's fluent pattern.

krnlde commented Apr 3, 2015

btw. expect(() => testuser.password('1a')) and expect(async () => await testuser.password('1a')) do not work either.

The only thing that seems to work right now is catching the error via try catch:

it(`should throw when too short`, async (done) => {
  try {
    await testuser.password('1a');
    done('failed');
  } catch (e) {
    done();
  }
});

But this is ugly and doesn't even use the bdd methods or profit from chai's fluent pattern.

@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus

keithamus Apr 3, 2015

Member

@krnlde I can't see any reason why that code wouldn't work.

What happens if you change the test to the following?

  it(`should throw when too short`, (done) => {
    testuser.password('1a').then(done, done);
  });

You should see the test fail appropriately with the error, and not timeout. If it times out - its likely that your code isn't working like it should. It it fails with an error, then it is likely something wrong is happening within either chai or chai-as-promised.

Member

keithamus commented Apr 3, 2015

@krnlde I can't see any reason why that code wouldn't work.

What happens if you change the test to the following?

  it(`should throw when too short`, (done) => {
    testuser.password('1a').then(done, done);
  });

You should see the test fail appropriately with the error, and not timeout. If it times out - its likely that your code isn't working like it should. It it fails with an error, then it is likely something wrong is happening within either chai or chai-as-promised.

@krnlde

This comment has been minimized.

Show comment
Hide comment
@krnlde

krnlde Apr 3, 2015

Same issue as mentioned before. The error is not thrown into the Promise, but in the it() scope.



  User
    .password()
      1) should throw when too short


  0 passing (13ms)
  1 failing

  1) User .password() should throw when too short:
     Error: New password must be at least 6 characters long
      at User.callee$1$0$ ...

The 1) at the tail is too late. It shouldn't appear at all. If a normal function is caught by .should.throw or should.eventually.throw no additional Error is shown.

krnlde commented Apr 3, 2015

Same issue as mentioned before. The error is not thrown into the Promise, but in the it() scope.



  User
    .password()
      1) should throw when too short


  0 passing (13ms)
  1 failing

  1) User .password() should throw when too short:
     Error: New password must be at least 6 characters long
      at User.callee$1$0$ ...

The 1) at the tail is too late. It shouldn't appear at all. If a normal function is caught by .should.throw or should.eventually.throw no additional Error is shown.

@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus

keithamus Apr 3, 2015

Member

The test I wrote should error as expected above. The log you pasted looks right to me.

You could try the following:

  it(`should throw when too short`, () => {
    return testuser.password('1a').then(() => throw Error('should error'), () => {})
  });

This should pass the test suite without error. If it does then we know mocha and babel aren't the problem.

Member

keithamus commented Apr 3, 2015

The test I wrote should error as expected above. The log you pasted looks right to me.

You could try the following:

  it(`should throw when too short`, () => {
    return testuser.password('1a').then(() => throw Error('should error'), () => {})
  });

This should pass the test suite without error. If it does then we know mocha and babel aren't the problem.

@krnlde

This comment has been minimized.

Show comment
Hide comment
@krnlde

krnlde Apr 4, 2015

When copy pasting your code I get a syntax error at throw with SyntaxError: ...: Unexpected token.

If I wrap the error in curlies...

  it(`should throw when too short`, () => {
    return testuser.password('1a').then(() => {throw Error('should error')}, () => {})
  });

...the promise never settles and mocha skips after the timeout default of 2000ms.

If I do:

it.only(`should throw when too short`, (done) => {
  testuser.password('1a').then(function () {
    console.log('fullfilled');
    done();
  }, function () {
    console.log('rejected');
    done();
  });
});

The console.log('rejected'); is called properly and the test succeeds.

krnlde commented Apr 4, 2015

When copy pasting your code I get a syntax error at throw with SyntaxError: ...: Unexpected token.

If I wrap the error in curlies...

  it(`should throw when too short`, () => {
    return testuser.password('1a').then(() => {throw Error('should error')}, () => {})
  });

...the promise never settles and mocha skips after the timeout default of 2000ms.

If I do:

it.only(`should throw when too short`, (done) => {
  testuser.password('1a').then(function () {
    console.log('fullfilled');
    done();
  }, function () {
    console.log('rejected');
    done();
  });
});

The console.log('rejected'); is called properly and the test succeeds.

@krnlde

This comment has been minimized.

Show comment
Hide comment
@krnlde

krnlde Apr 4, 2015

More investigation done. Facebook's regenerator runtime definitely returns a new Promise when calling an async function as stated here: https://github.com/facebook/regenerator/blob/master/runtime.js#L106 . I've debugged the whole runtime and can confirm that.
Also the async spec says a Promise should be returned. Here https://github.com/lukehoban/ecmascript-asyncawait#details and here http://wiki.ecmascript.org/doku.php?id=strawman:async_functions#details (same text)

So from my understanding - since we have a Promise that is returned - these assertions both should work:

// expect-style
it.only(`should throw when too short`, () => {
  return expect(testuser.password('1a')).eventually.throw();
});

// or in should-style
it.only(`should throw when too short`, () => {
  return testuser.password('1a').should.eventually.throw();
});

But they don't.

Maybe it has sth to do with this line here https://github.com/facebook/regenerator/blob/master/runtime.js#L122
Where an error is already caught. Afaik errors, once caught, are not available to the promise chain anymore. So what probably happens here is that the callThrow method (which equals Generator.prototype.throw) throws the error in the it() scope. That would explain why a try catch works but an expect(...) doesn't.

Any thoughts?

krnlde commented Apr 4, 2015

More investigation done. Facebook's regenerator runtime definitely returns a new Promise when calling an async function as stated here: https://github.com/facebook/regenerator/blob/master/runtime.js#L106 . I've debugged the whole runtime and can confirm that.
Also the async spec says a Promise should be returned. Here https://github.com/lukehoban/ecmascript-asyncawait#details and here http://wiki.ecmascript.org/doku.php?id=strawman:async_functions#details (same text)

So from my understanding - since we have a Promise that is returned - these assertions both should work:

// expect-style
it.only(`should throw when too short`, () => {
  return expect(testuser.password('1a')).eventually.throw();
});

// or in should-style
it.only(`should throw when too short`, () => {
  return testuser.password('1a').should.eventually.throw();
});

But they don't.

Maybe it has sth to do with this line here https://github.com/facebook/regenerator/blob/master/runtime.js#L122
Where an error is already caught. Afaik errors, once caught, are not available to the promise chain anymore. So what probably happens here is that the callThrow method (which equals Generator.prototype.throw) throws the error in the it() scope. That would explain why a try catch works but an expect(...) doesn't.

Any thoughts?

@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus

keithamus Apr 4, 2015

Member

What happens if you try the following:

  it(`should throw when too short`, (done) => {
    testuser.password('1a').should.eventually.throw().notify(done);
  });

Also what versions of all software are you using? babel, chai, mocha, chai-as-promised, everything.

Member

keithamus commented Apr 4, 2015

What happens if you try the following:

  it(`should throw when too short`, (done) => {
    testuser.password('1a').should.eventually.throw().notify(done);
  });

Also what versions of all software are you using? babel, chai, mocha, chai-as-promised, everything.

@krnlde

This comment has been minimized.

Show comment
Hide comment
@krnlde

krnlde Apr 4, 2015

I get

   .password()
     1) should throw when too short


 0 passing (0ms)
 1 failing

 1) User .password() should throw when too short:
    TypeError: Cannot read property 'eventually' of undefined

Your code transformed in the expect-style (expect(testuser.password('1a')).eventually.throw().notify(done);) returns

    .password()
      1) should throw when too short


  0 passing (0ms)
  1 failing

  1) User .password() should throw when too short:
     Error: New password must be at least 6 characters long

as always.

What I'm curious about is that

  testuser.password('1a')
    .then(() => console.log("Shouldn't appear"))
    .catch(() => console.log("All good"))

resolves to All good as expected.

An extract of my package.json (npm installd daily) is:

  "devDependencies": {
    "babel": "latest",
    "chai": "latest",
    "chai-as-promised": "latest",
    "gulp": "latest",
    "gulp-eslint": "latest",
    "gulp-less": "latest",
    "gulp-open": "latest",
    "gulp-plumber": "latest",
    "gulp-run": "latest",
    "gulp-sourcemaps": "latest",
    "mocha": "latest",
    "run-sequence": "latest"
  }

Which resolves to: babel@5.0.8, chai@2.2.0, chai-as-promised@4.3.0 and mocha@2.2.1 at the time of writing this.

I'm running io.js v1.6.3 on a Win8.1 Pro x64.

krnlde commented Apr 4, 2015

I get

   .password()
     1) should throw when too short


 0 passing (0ms)
 1 failing

 1) User .password() should throw when too short:
    TypeError: Cannot read property 'eventually' of undefined

Your code transformed in the expect-style (expect(testuser.password('1a')).eventually.throw().notify(done);) returns

    .password()
      1) should throw when too short


  0 passing (0ms)
  1 failing

  1) User .password() should throw when too short:
     Error: New password must be at least 6 characters long

as always.

What I'm curious about is that

  testuser.password('1a')
    .then(() => console.log("Shouldn't appear"))
    .catch(() => console.log("All good"))

resolves to All good as expected.

An extract of my package.json (npm installd daily) is:

  "devDependencies": {
    "babel": "latest",
    "chai": "latest",
    "chai-as-promised": "latest",
    "gulp": "latest",
    "gulp-eslint": "latest",
    "gulp-less": "latest",
    "gulp-open": "latest",
    "gulp-plumber": "latest",
    "gulp-run": "latest",
    "gulp-sourcemaps": "latest",
    "mocha": "latest",
    "run-sequence": "latest"
  }

Which resolves to: babel@5.0.8, chai@2.2.0, chai-as-promised@4.3.0 and mocha@2.2.1 at the time of writing this.

I'm running io.js v1.6.3 on a Win8.1 Pro x64.

@krnlde

This comment has been minimized.

Show comment
Hide comment
@krnlde

krnlde Apr 4, 2015

I built a runnable.com instance with the test case so you can see what I see http://runnable.com/VSAUjLYcbqoxDfiY

krnlde commented Apr 4, 2015

I built a runnable.com instance with the test case so you can see what I see http://runnable.com/VSAUjLYcbqoxDfiY

@krnlde

This comment has been minimized.

Show comment
Hide comment
@krnlde

krnlde Apr 4, 2015

I got the solution! Expecting async function to actually "throw" is wrong, since it returns a promise. You'll have to expect().to.be.rejected instead.

Here's the modified test case:

it.only(`should be rejected when too short`, () => {
  return expect(testuser.password('1a')).to.be.rejectedWith(Error);
});

// should-style
it.only(`should be rejected when too short`, () => {
  return testuser.password('1a').should.be.rejectedWith(Error);
});

The question is, does an .eventually.throw() make sense? Is it different to .rejectedWith(Error)?

krnlde commented Apr 4, 2015

I got the solution! Expecting async function to actually "throw" is wrong, since it returns a promise. You'll have to expect().to.be.rejected instead.

Here's the modified test case:

it.only(`should be rejected when too short`, () => {
  return expect(testuser.password('1a')).to.be.rejectedWith(Error);
});

// should-style
it.only(`should be rejected when too short`, () => {
  return testuser.password('1a').should.be.rejectedWith(Error);
});

The question is, does an .eventually.throw() make sense? Is it different to .rejectedWith(Error)?

@krnlde krnlde closed this Apr 4, 2015

@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus

keithamus Apr 5, 2015

Member

Oh, duh. Of course! Glad you found the issue @krnlde

.eventually.throw may make sense, especially as an async function throw is a rejection. You should raise this in chai-as-promised as that would be the correct place to file it.

Member

keithamus commented Apr 5, 2015

Oh, duh. Of course! Glad you found the issue @krnlde

.eventually.throw may make sense, especially as an async function throw is a rejection. You should raise this in chai-as-promised as that would be the correct place to file it.

@krnlde

This comment has been minimized.

Show comment
Hide comment
@krnlde

krnlde Apr 5, 2015

Done. Thanks for your help @keithamus !

krnlde commented Apr 5, 2015

Done. Thanks for your help @keithamus !

@fritx

This comment has been minimized.

Show comment
Hide comment
@fritx

fritx Feb 25, 2016

Expecting async function to actually "throw" is wrong, since it returns a promise.

Cool. I was wrong too.

Thanks for tips.
I use this code in my project nixe with should as dep.

  it('electron-prebuilt should be installed', async () => {
    await access('node_modules/electron-prebuilt')
  })

  it('no `electron` should be installed', async () => {
    // access('node_modules/electron').should.throw() // wrong
    access('node_modules/electron').should.be.rejected()
  })

fritx commented Feb 25, 2016

Expecting async function to actually "throw" is wrong, since it returns a promise.

Cool. I was wrong too.

Thanks for tips.
I use this code in my project nixe with should as dep.

  it('electron-prebuilt should be installed', async () => {
    await access('node_modules/electron-prebuilt')
  })

  it('no `electron` should be installed', async () => {
    // access('node_modules/electron').should.throw() // wrong
    access('node_modules/electron').should.be.rejected()
  })

@fritx fritx referenced this issue Feb 25, 2016

Open

记录6 #10

@jonahx

This comment has been minimized.

Show comment
Hide comment
@jonahx

jonahx Oct 22, 2016

@krnlde This solution is incorrect imo. The whole point of using async/await in es7 is so that you can write code without "thinking in promises." That is, you write:

async function() {
  try {
    await something();
  catch (e) {
    //handle
  }
}

It makes async programming "just like normal again". An assertion library that supports es7 should allow for this style of programming as well.

jonahx commented Oct 22, 2016

@krnlde This solution is incorrect imo. The whole point of using async/await in es7 is so that you can write code without "thinking in promises." That is, you write:

async function() {
  try {
    await something();
  catch (e) {
    //handle
  }
}

It makes async programming "just like normal again". An assertion library that supports es7 should allow for this style of programming as well.

@lucasfcosta

This comment has been minimized.

Show comment
Hide comment
@lucasfcosta

lucasfcosta Oct 22, 2016

Member

Hi @jonahx, thanks for sharing your thoughts, but actually Chai does support the ES7 spec, but your example doesn't really reflect how the spec works.
When you await for a an async function to complete you will have synchronous code because you will wait for that promise to be resolved and you will be able to test it, as you've said it yourself.

But let's get to the ES7 spec:

So that's how the whole thing happens, you can't really use the example you have just described because something() will never throw an error, it will reject the promise returned by it instead.

In a nutshell: due to how the spec works your example does not match how the feature works.

Please let me know if you think I'm wrong or if you need further explanation. Thanks again for sharing your thoughts 😄

Member

lucasfcosta commented Oct 22, 2016

Hi @jonahx, thanks for sharing your thoughts, but actually Chai does support the ES7 spec, but your example doesn't really reflect how the spec works.
When you await for a an async function to complete you will have synchronous code because you will wait for that promise to be resolved and you will be able to test it, as you've said it yourself.

But let's get to the ES7 spec:

So that's how the whole thing happens, you can't really use the example you have just described because something() will never throw an error, it will reject the promise returned by it instead.

In a nutshell: due to how the spec works your example does not match how the feature works.

Please let me know if you think I'm wrong or if you need further explanation. Thanks again for sharing your thoughts 😄

@jonahx

This comment has been minimized.

Show comment
Hide comment
@jonahx

jonahx Oct 22, 2016

Hi @lucasfcosta,

Thanks for your reply. At this point I see a few possibilities:

  1. We're miscommunicating somehow
  2. I'm misunderstanding something
  3. You're misunderstanding something, and my original claim was correct.

To clarify, please try this example out in the babel playground:

async function test() {
  return Promise.reject('boom!');
}

async function main() {
  try {
    const result = await test();
    console.log(result);
  }
  catch (e) {
    console.log(e);
  }
}

main();

This logs "boom!" as expected given the mental model I explained in my previous post. Please let me know how I'm wrong, if you still think I am. Thanks.

jonahx commented Oct 22, 2016

Hi @lucasfcosta,

Thanks for your reply. At this point I see a few possibilities:

  1. We're miscommunicating somehow
  2. I'm misunderstanding something
  3. You're misunderstanding something, and my original claim was correct.

To clarify, please try this example out in the babel playground:

async function test() {
  return Promise.reject('boom!');
}

async function main() {
  try {
    const result = await test();
    console.log(result);
  }
  catch (e) {
    console.log(e);
  }
}

main();

This logs "boom!" as expected given the mental model I explained in my previous post. Please let me know how I'm wrong, if you still think I am. Thanks.

@jonahx

This comment has been minimized.

Show comment
Hide comment
@jonahx

jonahx Oct 22, 2016

Also, just for full clarification, here's how I'm currently working around the issue in my own code:

async function doesItThrow(fn) {
  var threwError = true;
  try {
    await fn();
    threwError = false;
  } catch (e) { }
  return threwError;
}

global.assert.throwsAsync = async (fn, msg) => assert(await doesItThrow(fn), msg);

// and in the test themselves....

await assert.throwsAsync( 
    async () => await doSomething(missingParams),
    'Should error when missing params'
);

I thought it was strange that I had to write that myself.

jonahx commented Oct 22, 2016

Also, just for full clarification, here's how I'm currently working around the issue in my own code:

async function doesItThrow(fn) {
  var threwError = true;
  try {
    await fn();
    threwError = false;
  } catch (e) { }
  return threwError;
}

global.assert.throwsAsync = async (fn, msg) => assert(await doesItThrow(fn), msg);

// and in the test themselves....

await assert.throwsAsync( 
    async () => await doSomething(missingParams),
    'Should error when missing params'
);

I thought it was strange that I had to write that myself.

@meeber

This comment has been minimized.

Show comment
Hide comment
@meeber

meeber Oct 22, 2016

Contributor

@jonahx I do stuff like this frequently:

it("someTest", async () => {
  let err = "_PRETEST_";

  try {
    await someFn();
  } catch (e) {
    err = e;
  }

  expect(err.message).to.equal("some error message");
});

What is it you're asking for? A builtin Chai assertion to handle the try/catch part under the hood, so all you need to do is provide the function? If so, what is it about the previous solution of using chai-as-promised you didn't like?:

it("some test", () => {
  return expect(someFn()).to.be.rejectedWith(Error);
});
Contributor

meeber commented Oct 22, 2016

@jonahx I do stuff like this frequently:

it("someTest", async () => {
  let err = "_PRETEST_";

  try {
    await someFn();
  } catch (e) {
    err = e;
  }

  expect(err.message).to.equal("some error message");
});

What is it you're asking for? A builtin Chai assertion to handle the try/catch part under the hood, so all you need to do is provide the function? If so, what is it about the previous solution of using chai-as-promised you didn't like?:

it("some test", () => {
  return expect(someFn()).to.be.rejectedWith(Error);
});
@jonahx

This comment has been minimized.

Show comment
Hide comment
@jonahx

jonahx Oct 22, 2016

@meeber, Yes, that's what I'm suggesting.

What I dislike about return expect(someFn()).to.be.rejectedWith(Error); is that it forces you to explicitly be thinking about promises again. The "domain language," as it were, has reverted to that of promises. As I see it, the whole point of async/await is that it abstracts promises away. Client code dealing with promises just has to await something, but beyond that, it works exactly as does non-Promise code. Depending on your view, this may strike you as a persnickety or academic point, but imo it's not. The language is important -- it reflects and influences your mental model.

jonahx commented Oct 22, 2016

@meeber, Yes, that's what I'm suggesting.

What I dislike about return expect(someFn()).to.be.rejectedWith(Error); is that it forces you to explicitly be thinking about promises again. The "domain language," as it were, has reverted to that of promises. As I see it, the whole point of async/await is that it abstracts promises away. Client code dealing with promises just has to await something, but beyond that, it works exactly as does non-Promise code. Depending on your view, this may strike you as a persnickety or academic point, but imo it's not. The language is important -- it reflects and influences your mental model.

@lucasfcosta

This comment has been minimized.

Show comment
Hide comment
@lucasfcosta

lucasfcosta Oct 22, 2016

Member

@jonahx I understand your point now.

Well, since we're dealing with promises behind the curtains I think it wouldn't be much of a problem to handle that using chai-as-promised. However your argument that it changes the mental model of the code being written is also true, but maybe doing changes on the core to meet those expectations would override some of chai-as-promised features.

We could indeed add that to our core since the only modification we need to handle async functions would be done into the throws assertion (after all, this is the only assertion which handles function invocation) and by doing this chai-as-promised would still be useful.

I'm not sure about whether this would be a good choice or not, so let me ping @keithamus, @vieiralucas and @shvaikalesh so we can get some extra input on this.

Thanks again for your response, let's keep on talking about it so we can figure out which is the optimal decision for this issue. 😄

Member

lucasfcosta commented Oct 22, 2016

@jonahx I understand your point now.

Well, since we're dealing with promises behind the curtains I think it wouldn't be much of a problem to handle that using chai-as-promised. However your argument that it changes the mental model of the code being written is also true, but maybe doing changes on the core to meet those expectations would override some of chai-as-promised features.

We could indeed add that to our core since the only modification we need to handle async functions would be done into the throws assertion (after all, this is the only assertion which handles function invocation) and by doing this chai-as-promised would still be useful.

I'm not sure about whether this would be a good choice or not, so let me ping @keithamus, @vieiralucas and @shvaikalesh so we can get some extra input on this.

Thanks again for your response, let's keep on talking about it so we can figure out which is the optimal decision for this issue. 😄

@meeber

This comment has been minimized.

Show comment
Hide comment
@meeber

meeber Oct 22, 2016

Contributor

@jonahx Agreed on all points. As @lucasfcosta alluded to, we're in a tricky spot currently in terms of adding promise-related features, as Chai currently doesn't have any promise support builtin due to the robustness of chai-as-promised.

For now, I recommend creating a new issue for this. Would be useful to see exact examples of how you envision the syntax being if it was Chai-supported.

Contributor

meeber commented Oct 22, 2016

@jonahx Agreed on all points. As @lucasfcosta alluded to, we're in a tricky spot currently in terms of adding promise-related features, as Chai currently doesn't have any promise support builtin due to the robustness of chai-as-promised.

For now, I recommend creating a new issue for this. Would be useful to see exact examples of how you envision the syntax being if it was Chai-supported.

@vieiralucas

This comment has been minimized.

Show comment
Hide comment
@vieiralucas

vieiralucas Oct 22, 2016

Member

Another option that I see is to add an alias to the rejected and rejectedWith assertion on chai-as-promised. They could be called something like throwAsync and throwAsyncWith

Member

vieiralucas commented Oct 22, 2016

Another option that I see is to add an alias to the rejected and rejectedWith assertion on chai-as-promised. They could be called something like throwAsync and throwAsyncWith

@vieiralucas

This comment has been minimized.

Show comment
Hide comment
@vieiralucas

vieiralucas Oct 22, 2016

Member

I also agree that Chai should eventually support promises more "natively".

So, as @meeber suggested, can you create a new issue for this explaining how do you think Chai should support it?

Member

vieiralucas commented Oct 22, 2016

I also agree that Chai should eventually support promises more "natively".

So, as @meeber suggested, can you create a new issue for this explaining how do you think Chai should support it?

@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus

keithamus Oct 22, 2016

Member

I also agree that Chai should eventually support promises more "natively".

FYI I did propose folding chai-as-promised into the chai org a while back (domenic/chai-as-promised#105 (comment)). Having said that given the direction of async functions - I wonder if perhaps we even need to have any code to support them, or if it becomes supported implicitly by JS features?

Perhaps if we want to continue this discussion in earnest, we should move it to a new issue.

Member

keithamus commented Oct 22, 2016

I also agree that Chai should eventually support promises more "natively".

FYI I did propose folding chai-as-promised into the chai org a while back (domenic/chai-as-promised#105 (comment)). Having said that given the direction of async functions - I wonder if perhaps we even need to have any code to support them, or if it becomes supported implicitly by JS features?

Perhaps if we want to continue this discussion in earnest, we should move it to a new issue.

@joshuarh

This comment has been minimized.

Show comment
Hide comment
@joshuarh

joshuarh Apr 23, 2017

I like @lucasfcosta's idea of adding support for async functions in throws to core rather than delegating out to chai-as-promised.

Given the following function:

function throwsAsync() {
  return new Promise((resolve, reject) => {
    reject(new Error())
  })
}

I personally find the following syntax:

expect(async () => await throwsAsync()).to.throw()

much more expressive of intent in an async/await world than the chai-as-promised method:

expect(throwsAsync()).to.be.rejected

joshuarh commented Apr 23, 2017

I like @lucasfcosta's idea of adding support for async functions in throws to core rather than delegating out to chai-as-promised.

Given the following function:

function throwsAsync() {
  return new Promise((resolve, reject) => {
    reject(new Error())
  })
}

I personally find the following syntax:

expect(async () => await throwsAsync()).to.throw()

much more expressive of intent in an async/await world than the chai-as-promised method:

expect(throwsAsync()).to.be.rejected
@jeff3yan

This comment has been minimized.

Show comment
Hide comment
@jeff3yan

jeff3yan May 4, 2017

I've been trying this method for testing ES7 async functions that are expected to throw an error:

it('should throw an error', async () => {
  try {
    await shouldThrowAnError();
  } catch (err) { return; }
  throw new Error('Should have thrown an error');
})

Is this a reasonable usage of chai for expectation of error? Or should I be using a more explicit and readable method?

jeff3yan commented May 4, 2017

I've been trying this method for testing ES7 async functions that are expected to throw an error:

it('should throw an error', async () => {
  try {
    await shouldThrowAnError();
  } catch (err) { return; }
  throw new Error('Should have thrown an error');
})

Is this a reasonable usage of chai for expectation of error? Or should I be using a more explicit and readable method?

@meeber

This comment has been minimized.

Show comment
Hide comment
@meeber

meeber May 5, 2017

Contributor

@jeff3yan I think that's a reasonable approach.

Contributor

meeber commented May 5, 2017

@jeff3yan I think that's a reasonable approach.

@workflow

This comment has been minimized.

Show comment
Hide comment
@workflow

workflow May 13, 2017

@jeff3yan How would you assert on the type/message of your error using that approach?

workflow commented May 13, 2017

@jeff3yan How would you assert on the type/message of your error using that approach?

@jeff3yan

This comment has been minimized.

Show comment
Hide comment
@jeff3yan

jeff3yan May 25, 2017

@workflow You could do the error assertion in the catch block?

jeff3yan commented May 25, 2017

@workflow You could do the error assertion in the catch block?

@workflow

This comment has been minimized.

Show comment
Hide comment
@workflow

workflow May 26, 2017

@jeff3yan You are right of course, I must've been tired. Thank you!

workflow commented May 26, 2017

@jeff3yan You are right of course, I must've been tired. Thank you!

@MadDeveloper

This comment has been minimized.

Show comment
Hide comment
@MadDeveloper

MadDeveloper Jan 3, 2018

A sample solution:

it('should throw an error', async () => {
    await expect(shouldThrowAnError()).to.be.rejected;
});

MadDeveloper commented Jan 3, 2018

A sample solution:

it('should throw an error', async () => {
    await expect(shouldThrowAnError()).to.be.rejected;
});
@krnlde

This comment has been minimized.

Show comment
Hide comment
@krnlde

krnlde Jan 3, 2018

yes, as stated in the original answer here.
Instead of await you could just return. Chai will wait for promises out of the box.

krnlde commented Jan 3, 2018

yes, as stated in the original answer here.
Instead of await you could just return. Chai will wait for promises out of the box.

@jtlapp

This comment has been minimized.

Show comment
Hide comment
@jtlapp

jtlapp Jan 30, 2018

There doesn't appear to be a solution analogous to rejectedWith() in the assert library.

But the assert-rejected package provides a pretty simple solution:

import * as assertRejected from 'assert-rejected';

//...
    it("reports failure resulting in exception", () => {
        return assertRejected(promiseToTry.then(err => {
            assert.instanceOf(err, KindOfError);
        });
    });

jtlapp commented Jan 30, 2018

There doesn't appear to be a solution analogous to rejectedWith() in the assert library.

But the assert-rejected package provides a pretty simple solution:

import * as assertRejected from 'assert-rejected';

//...
    it("reports failure resulting in exception", () => {
        return assertRejected(promiseToTry.then(err => {
            assert.instanceOf(err, KindOfError);
        });
    });
@machineghost

This comment has been minimized.

Show comment
Hide comment
@machineghost

machineghost Feb 7, 2018

This discussion has been going on awhile, and yet after reading it I don't understand: why can't this be really, really simple? If Chai just added one new assertion type, throwsAsync, wouldn't every async-using Chai lover across the planet join arms and rejoice? They wouldn't need a separate library, they could test asynchronous code (almost) identically to how they test synchronous code, and every other Chai use would remain unaffected.

// How to Use
const { expect } = require('chai');
const throwsAsync = () => { return Promise.reject('fake error') };
expect(async () => { await throwsAsync() }).to.throwAsync();
// Synchronous equivalent has (almost) the same signature:
// expect(() => { throw new Error('fake error'); } ).to.throw();

// How it Works
const throwsAsync = async functionToTest => {
    try {
        await functionToTest();
    } catch (err) {
        // do what Chai normally does with a sync error
        expect(() => { throw err }).to.throw();
        return;
    }
    // do what Chai normally does when no error is thrown
    expect(() => {}).to.throw();
 }

// How it Works (ES 2015)
const throwsAsync = functionToTest => {
    return functionToTest()
         .then(() => 
             // do what Chai normally does when no error is thrown
             expect(() => {}).to.throw();
         )
         .catch(err =>
            // do what Chai normally does with a sync error
            expect(() => { throw err }).to.throw();
        )
    }

 }

I would be happy to submit a PR of the above, except with real code for the actual assertion definition.

machineghost commented Feb 7, 2018

This discussion has been going on awhile, and yet after reading it I don't understand: why can't this be really, really simple? If Chai just added one new assertion type, throwsAsync, wouldn't every async-using Chai lover across the planet join arms and rejoice? They wouldn't need a separate library, they could test asynchronous code (almost) identically to how they test synchronous code, and every other Chai use would remain unaffected.

// How to Use
const { expect } = require('chai');
const throwsAsync = () => { return Promise.reject('fake error') };
expect(async () => { await throwsAsync() }).to.throwAsync();
// Synchronous equivalent has (almost) the same signature:
// expect(() => { throw new Error('fake error'); } ).to.throw();

// How it Works
const throwsAsync = async functionToTest => {
    try {
        await functionToTest();
    } catch (err) {
        // do what Chai normally does with a sync error
        expect(() => { throw err }).to.throw();
        return;
    }
    // do what Chai normally does when no error is thrown
    expect(() => {}).to.throw();
 }

// How it Works (ES 2015)
const throwsAsync = functionToTest => {
    return functionToTest()
         .then(() => 
             // do what Chai normally does when no error is thrown
             expect(() => {}).to.throw();
         )
         .catch(err =>
            // do what Chai normally does with a sync error
            expect(() => { throw err }).to.throw();
        )
    }

 }

I would be happy to submit a PR of the above, except with real code for the actual assertion definition.

@meeber

This comment has been minimized.

Show comment
Hide comment
@meeber

meeber Feb 10, 2018

Contributor

@machineghost In the example below, wouldn't Chai need Promise-handling logic in every step of the chain after throwAsync? (This is essentially what chai-as-promised provides).

expect(myAsyncFn).to.throwAsync().and.have.property('code', 42);
Contributor

meeber commented Feb 10, 2018

@machineghost In the example below, wouldn't Chai need Promise-handling logic in every step of the chain after throwAsync? (This is essentially what chai-as-promised provides).

expect(myAsyncFn).to.throwAsync().and.have.property('code', 42);
@machineghost

This comment has been minimized.

Show comment
Hide comment
@machineghost

machineghost Feb 12, 2018

I must confess I'm ignorant to the inner workings of Chai, so to support "and.have.property(whatever)" you might well need chai-as-promised. And again, because I don't understand how Chai works, I don't understand why it's such a big deal to simply incorporate that library into Chai proper (in much the same way that previously "experimental" Babel plugins have now become part of core presets thanks to the evolution of the language).

But if for some reason it is difficult or impossible to make Chai proper fully support a basic part of the language (I think it's safe to call promises "a basic part of the language" at this point, wouldn't you?), I would counter that an:

expect(myAsyncFn).to.throwAsync()

in Chai proper, which has the limitation of not being chain-off-able, is still infinitely better than not having support for promises/async/await at all in the best/most popular Javascript assertion library.

machineghost commented Feb 12, 2018

I must confess I'm ignorant to the inner workings of Chai, so to support "and.have.property(whatever)" you might well need chai-as-promised. And again, because I don't understand how Chai works, I don't understand why it's such a big deal to simply incorporate that library into Chai proper (in much the same way that previously "experimental" Babel plugins have now become part of core presets thanks to the evolution of the language).

But if for some reason it is difficult or impossible to make Chai proper fully support a basic part of the language (I think it's safe to call promises "a basic part of the language" at this point, wouldn't you?), I would counter that an:

expect(myAsyncFn).to.throwAsync()

in Chai proper, which has the limitation of not being chain-off-able, is still infinitely better than not having support for promises/async/await at all in the best/most popular Javascript assertion library.

@Eggham

This comment has been minimized.

Show comment
Hide comment
@Eggham

Eggham Mar 31, 2018

I used .rejected, as @krnlde suggested. But my code complains that function is not thenable . Would appreciate it if anyone could point out what is wrong. A simplified version of code is as follows:

Code:

// index.js
async function a() {
    throw new Error('I am always unhappy!');
};
module.exports.a = a;

Test:

// index_test.js
const chai = require('chai');
const chaiAsPromised = require('chai-as-promised');
chai.use(chaiAsPromised);
const expect = chai.expect;
const index = require('./index.js');

it('should catch thrown error', () => {
    expect(() => index.a()).to.be.rejected;
});

Test output:

1) should catch thrown error

  0 passing (5ms)
  1 failing

  1) should catch thrown error:
     TypeError: [Function] is not a thenable.
      at assertIsAboutPromise (node_modules/chai-as-promised/lib/chai-as-promised.js:31:19)
      at Assertion.<anonymous> (node_modules/chai-as-promised/lib/chai-as-promised.js:53:13)
      at Assertion.propertyGetter (node_modules/chai/lib/chai/utils/addProperty.js:62:29)
      at Object.get (<anonymous>)
      at Object.proxyGetter [as get] (node_modules/chai/lib/chai/utils/proxify.js:86:22)
      at Context.it (index_test.js:8:34)

Eggham commented Mar 31, 2018

I used .rejected, as @krnlde suggested. But my code complains that function is not thenable . Would appreciate it if anyone could point out what is wrong. A simplified version of code is as follows:

Code:

// index.js
async function a() {
    throw new Error('I am always unhappy!');
};
module.exports.a = a;

Test:

// index_test.js
const chai = require('chai');
const chaiAsPromised = require('chai-as-promised');
chai.use(chaiAsPromised);
const expect = chai.expect;
const index = require('./index.js');

it('should catch thrown error', () => {
    expect(() => index.a()).to.be.rejected;
});

Test output:

1) should catch thrown error

  0 passing (5ms)
  1 failing

  1) should catch thrown error:
     TypeError: [Function] is not a thenable.
      at assertIsAboutPromise (node_modules/chai-as-promised/lib/chai-as-promised.js:31:19)
      at Assertion.<anonymous> (node_modules/chai-as-promised/lib/chai-as-promised.js:53:13)
      at Assertion.propertyGetter (node_modules/chai/lib/chai/utils/addProperty.js:62:29)
      at Object.get (<anonymous>)
      at Object.proxyGetter [as get] (node_modules/chai/lib/chai/utils/proxify.js:86:22)
      at Context.it (index_test.js:8:34)
@leggsimon

This comment has been minimized.

Show comment
Hide comment
@leggsimon

leggsimon Mar 31, 2018

@Eggham Would this be because you're passing a function to expect rather than the promise itself. I think it would only work on a promise. Which is the result of actually calling the function which returns you a promise (in your case index.a()).

Maybe try changing

it('should catch thrown error', () => {
    expect(() => index.a()).to.be.rejected;
});

to

it('should catch thrown error', () => {
    expect(index.a()).to.be.rejected;
});

I believe the syntax you're using (passing a function that calls a function) is used for asserting thrown errors in synchronous functions. Since this isn't synchronous it won't ever "throw" it'll only reject.

leggsimon commented Mar 31, 2018

@Eggham Would this be because you're passing a function to expect rather than the promise itself. I think it would only work on a promise. Which is the result of actually calling the function which returns you a promise (in your case index.a()).

Maybe try changing

it('should catch thrown error', () => {
    expect(() => index.a()).to.be.rejected;
});

to

it('should catch thrown error', () => {
    expect(index.a()).to.be.rejected;
});

I believe the syntax you're using (passing a function that calls a function) is used for asserting thrown errors in synchronous functions. Since this isn't synchronous it won't ever "throw" it'll only reject.

@Eggham

This comment has been minimized.

Show comment
Hide comment
@Eggham

Eggham Mar 31, 2018

@leggsimon Yeah! You are right. I apparently barked on a wrong tree. Thank you very much!

Eggham commented Mar 31, 2018

@leggsimon Yeah! You are right. I apparently barked on a wrong tree. Thank you very much!

@AbdelrahmanHafez

This comment has been minimized.

Show comment
Hide comment
@AbdelrahmanHafez

AbdelrahmanHafez Jul 31, 2018

So, I found behavior that I can not understand.

The code below should fail, yet it passes.

  async function throwAsyncError () {
    throw new Error('Bad stuff happened.')
  }

  it.only('throws an error', async () => {
    expect(throwAsyncError()).to.not.be.rejected;
  });

When I add a return to the test like below:

  it.only('throws an error', async () => {
    return expect(throwAsyncError()).to.not.be.rejected;
  });

It fails like it should. Any explanations?

AbdelrahmanHafez commented Jul 31, 2018

So, I found behavior that I can not understand.

The code below should fail, yet it passes.

  async function throwAsyncError () {
    throw new Error('Bad stuff happened.')
  }

  it.only('throws an error', async () => {
    expect(throwAsyncError()).to.not.be.rejected;
  });

When I add a return to the test like below:

  it.only('throws an error', async () => {
    return expect(throwAsyncError()).to.not.be.rejected;
  });

It fails like it should. Any explanations?

@meeber

This comment has been minimized.

Show comment
Hide comment
@meeber

meeber Aug 1, 2018

Contributor

@AbdelrahmanHafez This is just how promises work. Consider this example:

async function throwAsyncError () {
  throw new Error('Bad stuff happened.')
}

it.only('throws an error', async function myTestFn () {
  Promise.reject(Error("This rejected promise is swallowed."));
  Promise.reject(Error("This rejected promise is also swallowed."));
  throwAsyncError(); // This rejected promise is also swallowed
  expect(throwAsyncError()).to.not.be.rejected; // This rejected promise is also swallowed
});

The above test passes even though there are four rejected promises inside of it. You can reject as many promises as you want inside of an async function; it's not going to halt execution, or throw an error, or cause the promise that's implicitly returned by myTestFn to be rejected. Your test runner (e.g., Mocha) has no way of knowing that there were any promises rejected inside of myTestFn during its execution.

To fix the problem, you can either: a) add await before one of the rejected promises; or, b) return one of the rejected promises. In either case, this will cause the promise that's returned by myTestFn to be rejected, and your test runner will know that the test failed.

Contributor

meeber commented Aug 1, 2018

@AbdelrahmanHafez This is just how promises work. Consider this example:

async function throwAsyncError () {
  throw new Error('Bad stuff happened.')
}

it.only('throws an error', async function myTestFn () {
  Promise.reject(Error("This rejected promise is swallowed."));
  Promise.reject(Error("This rejected promise is also swallowed."));
  throwAsyncError(); // This rejected promise is also swallowed
  expect(throwAsyncError()).to.not.be.rejected; // This rejected promise is also swallowed
});

The above test passes even though there are four rejected promises inside of it. You can reject as many promises as you want inside of an async function; it's not going to halt execution, or throw an error, or cause the promise that's implicitly returned by myTestFn to be rejected. Your test runner (e.g., Mocha) has no way of knowing that there were any promises rejected inside of myTestFn during its execution.

To fix the problem, you can either: a) add await before one of the rejected promises; or, b) return one of the rejected promises. In either case, this will cause the promise that's returned by myTestFn to be rejected, and your test runner will know that the test failed.

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