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

Seems t.throws is not matching errors properly by Prototype for ES6 classes that extend Error #15055

Closed
maxcnunes opened this issue Aug 28, 2017 · 4 comments
Labels
assert Issues and PRs related to the assert subsystem. question Issues that look for answers.

Comments

@maxcnunes
Copy link

Using ava which has a copy of node assert module I realized few tests trying to match a custom error with t.throws were failing.

  assert.throws(
    () => deleteItem(initialState, operation),
    NotFoundObject
  );

Although this custom error is extending from Error class:

import ExtendableError from 'es6-error';

class CustomError extends ExtendableError {
  constructor(message, httpStatusCode, data) {
    super(message);
    this.httpStatusCode = httpStatusCode;
    this.data = data;
  }

  toString() {
    const message = super.toString();
    return (
      this.data
      ? `${message}. Error data: ${JSON.stringify(this.data, null, 2)}`
      : message
    );
  }
}

export class NotFoundObject extends CustomError {
  constructor(objectId, objectType) {
    super(`${objectType || 'Object'} with id ${objectId} does not exist`, 400);
  }
}

Digging into the code I found the assert is returning false if the expected error is a prototype of Error. Which doesn't make sense. It should return true since it is a prototype of Error:

  if (Error.isPrototypeOf(expected)) {
    return false;
  }

https://github.com/nodejs/node/blob/master/lib/assert.js#L584

So, am I reading that code wrong and it should really return false?


Reference initial issue found on ava assert module: sindresorhus/core-assert#2

@BridgeAR
Copy link
Member

BridgeAR commented Aug 28, 2017

Can you please provide a test case that does not contain any code besides core code?

The check as it is right now was intentional. Without a failing test case it is difficult though to see what your issue is because my test case works just fine.

class ES6Error extends Error {}

const functionThatThrows = () => {
  throw new ES6Error('foo');
};

assert.throws(functionThatThrows, ES6Error);

@mscdex
Copy link
Contributor

mscdex commented Aug 28, 2017

FWIW according to @sindresorhus, ava is moving away from using core-assert anyway, so the behavior in ava may change again sometime in the future.

@mscdex mscdex added assert Issues and PRs related to the assert subsystem. question Issues that look for answers. labels Aug 28, 2017
@refack
Copy link
Contributor

refack commented Aug 28, 2017

@maxcnunes I've compiled a gist by inlining your es6-error dependency.

It works with node's assert, which leads me to think that the bug might be in core-assert and not is node.

Could you try and reproduce with this snippet as is vs. with it replacing assert with core-assert?

@maxcnunes
Copy link
Author

Thanks everybody for taking the time dealing with this issue. As already expected, the problem was in my side 😢 On preparing a test to reproduce the error I realized the code I was working on was loading the error objects from lib directory instead of src directory. For some reason the errors would always come with Error prototype instead of the custom error type.
Anyway, I will close this issue since the assert is working properly. Again thanks for everybody's time :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. question Issues that look for answers.
Projects
None yet
Development

No branches or pull requests

4 participants