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

expect .to.throw does not work with new Errors #430

Closed
JaKXz opened this issue Apr 22, 2015 · 18 comments
Closed

expect .to.throw does not work with new Errors #430

JaKXz opened this issue Apr 22, 2015 · 18 comments

Comments

@JaKXz
Copy link

JaKXz commented Apr 22, 2015

This is my test:

  it('should throw a RangeError when the end date is before the start date', function () {
    var dateRange = {
      start: '2015-04-01',
      end: '2014-04-29',
    };
    var error = new RangeError('Invalid date range');
    var test = function () {
      throw error;
    };
    expect(test).to.throw(new RangeError('Invalid date range'));
  });

which fails with the message:

    AssertionError: expected [Function] to throw 'RangeError: Invalid date range' but 'RangeError: Invalid date range' was thrown
        at Context.<anonymous> (test/widgets/staff-planning-projection.spec.js:90:1)

I couldn't find any explanations / solutions / workarounds for this, so I think it's a bug with the expect(x).to.throw assertion. As a redundancy check, I tried:

expect(test).to.throw(error);

and that passed. Any thoughts?

@keithamus
Copy link
Member

Looks like the docs explicitly mention that you can pass in new errors and expect them to work - so yes I'd say this is a bug.

The Throw method is quite a big one, its in assertions.js#L1195-1310. Upon some inspection it looks like when you pass in a new *Error() it sets desiredError to that reference (see L1211-1214), and then does an === on desiredError (see L1228-1239).

I think the solution would be to check if desiredError matches both type and message, so String(err) === String(desiredError) would suffice - however this could be a breaking change, so maybe we should play it safe and when you make a Pull Request, put it in the 3.x.x branch instead of against master.

If you need any more help, please let me know and I'll be happy to assist.

@keithamus
Copy link
Member

In fact, looking through the code, it might be worthwhile ditching the whole desiredError check, and in the L1211-1214 if, just set the constructor and message to the error instance's constructor and message. This would significantly reduce the complexity of the assertion too 😄

@keithamus
Copy link
Member

Maybe as well, add some documentation so that if people really want to test the error by value then they can do expect(err).to.throw(new Error('foo')).and.equal(myErrorReference).

@wyvernzora
Copy link
Contributor

@keithamus I was looking into this issue, and I attempted to implement the change you suggested. I checked if constructor and errMsg were null, and if they were, I set them to those of the err. The actual changes I made can be found here in my fork.

However, I am getting this test failure:

  1) assert doesNotThrow:

      AssertionError: expected 'expected [Function] to not throw null but \'Error: foo\' was thrown' to equal 'expected [Function] to not throw an error but \'Error: foo\' was thrown'
      + expected - actual

      -expected [Function] to not throw null but 'Error: foo' was thrown
      +expected [Function] to not throw an error but 'Error: foo' was thrown

      at global.err (test/bootstrap/index.js:17:35)
      at Context.<anonymous> (test/assert.js:568:5)

I speculate that it's because the name is null, but I don't seem to see under what conditions the error message would display an error. The best I could do is to make it display 'Error', which still fails the test. Any idea how can I proceed from here? I'd appreciate any help with this.

@keithamus
Copy link
Member

@jluchiji, this issue predates our efforts to pull out the error checking code into a new module, and so I'd tread carefully when implementing code here. Have a read of chai-as-promised/issues#47 and #470 and #457.

I'd say for now, the best place to move the error assertions forward is in those areas - then we can take a look at behaviours like this.

@keithamus
Copy link
Member

Removing the pull-request-wanted label - because as mentioned, we need to refactor the error check code out into its own lib.

@Sionnach733
Copy link

Any update on this issue? is there a workaround for tests like this?

@niftylettuce
Copy link

Same bug.

@niftylettuce
Copy link

See #551

@sukrosono
Copy link
Contributor

i have a sinon.spy on a function then execute expect(myfn).to.throw it pass.
but the sinon.spy.callCount doesn't increase.
sinon-chai also not work in this case?
Is this the way it designed or this just my fault to write like that?
sorry i just new 😄
@keithamus any advice sir?

@keithamus
Copy link
Member

Hey @brutalcrozt. You would be better off asking in our chat forums, either on Gitter or Slack. Alternatively you could try raising an issue with sinon-chai - but I'd recommend making sure you have a reduced test case before filing an issue.

@lucasfcosta
Copy link
Member

lucasfcosta commented Jun 24, 2016

@keithamus The behavior of checking for both the error and the message when they exist has been fixed, but we took the design decision of doing strict (===) comparisons when an Error instance is passed. So the correct assertion here would be expect(test).to.throw(RangeError, 'Invalid date range');, and not the one with the new operator (since passing an error instance triggers strict comparison).

We did this in order to enable users to choose between both behaviors (strict comparisons and a comparison using the constructor and/or message).

@suissa
Copy link

suissa commented Jul 18, 2016

Same bug here.

My code:

'use strict'

module.exports = (obj) => {
  if(obj instanceof Object)
    if(Object.isFrozen(obj)) return obj
    else return Object.freeze(obj)
  else throw new TypeError('You need to send an OBJECT!')
}

test:

  describe('An Immutable',  () => {
    it(' cannot accept a value different than OBJECT', () => {
      expect(require('./iammutable')(1)).to.throw(TypeError, 'You need to send an OBJECT!')
    })
  })

output:

 An Immutable
      1)  cannot accept a value different than OBJECT


  1 failing

  1) I am Mutable, but not!  An Immutable  cannot accept a value different than OBJECT:
     TypeError: You need to send an OBJECT!
      at module.exports (iammutable.js:7:14)
      at Context.it (iammutable.test.js:30:37)

@meeber
Copy link
Contributor

meeber commented Jul 18, 2016

Hi @suissa. What you're running into is something different, and not a bug. You need to pass a function to expect instead of calling the function and passing its return value to expect. The way you have it now, your function is being called and throwing an error before expect is called, so expect doesn't have a chance to catch the error.

Working example:

  describe('An Immutable',  () => {
    it(' cannot accept a value different than OBJECT', () => {
      expect(() => require('../lib/iammutable')(1)).to.throw(TypeError, 'You need to send an OBJECT!')
    })
  })

@farkhandaAbbas
Copy link

this is my test:
var {
generateMessage
} = require('./message');
describe('generateMessage',()=>{
it('it should create correct obj',()=>{
var from='farkhanda';
var text='some message';
var message=generateMessage(from,text);
expect(message.createdat).toBeA('number');
});
});
fail with this error:
generateMessage
it should create correct obj:
TypeError: expect(...).toBeA is not a function
at Context.it (server\utils\message.test.js:12:31)

@meeber
Copy link
Contributor

meeber commented Nov 12, 2017

@farkhandaAbbas .toBeA isn't part of the Chai API. Try this instead: .to.be.a('number');.

@matthewjablack
Copy link

It seems this is still an issue.

This is the current logic for testing throw

https://github.com/chaijs/chai/blob/main/test/expect.js#L3028

Line 3028

    var specificError = new RangeError('boo');

    var goodFn = function () { 1==1; }
      , badFn = function () { throw new Error('testing'); }
      , refErrFn = function () { throw new ReferenceError('hello'); }
      , ickyErrFn = function () { throw new PoorlyConstructedError(); }
      , specificErrFn = function () { throw specificError; }
      , customErrFn = function() { throw new CustomError('foo'); }
      , emptyErrFn = function () { throw new Error(); }
      , emptyStringErrFn = function () { throw new Error(''); }; 

...
Line 3056

    expect(specificErrFn).to.throw(specificError);

However, if you change the code to use a different instance of the error thrown like so:

    var specificError = new RangeError('boo');
    var specificError2 = new RangeError('boo');

...

    expect(specificErrFn).to.throw(specificError2);

the following error and diff are shown

AssertionError: expected [Function: specificErrFn] to throw 'RangeError: boo' but 'RangeError: boo' was thrown
      + expected - actual

@reymillenium
Copy link

reymillenium commented Mar 16, 2023

This is my test:

  it('should throw a RangeError when the end date is before the start date', function () {
    var dateRange = {
      start: '2015-04-01',
      end: '2014-04-29',
    };
    var error = new RangeError('Invalid date range');
    var test = function () {
      throw error;
    };
    expect(test).to.throw(new RangeError('Invalid date range'));
  });

which fails with the message:

    AssertionError: expected [Function] to throw 'RangeError: Invalid date range' but 'RangeError: Invalid date range' was thrown
        at Context.<anonymous> (test/widgets/staff-planning-projection.spec.js:90:1)

I couldn't find any explanations / solutions / workarounds for this, so I think it's a bug with the expect(x).to.throw assertion. As a redundancy check, I tried:

expect(test).to.throw(error);

and that passed. Any thoughts?

You are comparing two entirely different error instances, which happens to hold the same message. But still, they are different error objects, therefore the tests with chai assertion won't pass.

Why?
Because, according to the official Chais's documentation:

When one argument is provided, and it’s an error instance, .throw invokes the target function and asserts that an error is thrown that’s strictly (===) equal to that error instance.

That's why when you actually compare the same objects, they do pass the test:

  it('should throw a RangeError when the end date is before the start date', function () {
    var dateRange = {
      start: '2015-04-01',
      end: '2014-04-29',
    };
    var error = new RangeError('Invalid date range');
    var test = function () {
      throw error;
    };
    expect(test).to.throw(error);
  });

And also, if you compare only the error messages, they will pass as well:

  it('should throw a RangeError when the end date is before the start date', function () {
    var dateRange = {
      start: '2015-04-01',
      end: '2014-04-29',
    };
    var error = new RangeError('Invalid date range');
    var test = function () {
      throw error;
    };
    expect(test).to.throw(new RangeError('Invalid date range').message);
  });

Which is the same as:

  it('should throw a RangeError when the end date is before the start date', function () {
    var dateRange = {
      start: '2015-04-01',
      end: '2014-04-29',
    };
    var error = new RangeError('Invalid date range');
    var test = function () {
      throw error;
    };
    expect(test).to.throw('Invalid date range');
  });

Why?
Because, according to the official Chais's documentation:

When one argument is provided, and it’s a string, .throw invokes the target function and asserts that an error is thrown with a message that contains that string.

What I do is to simply compare the error's message. As long as it's the same, then it's ok. That's it.

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