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

fix(inherited properties): Allow for status / statusCode to exist on the prototype #219

Merged
merged 1 commit into from Aug 1, 2018

Conversation

austince
Copy link
Contributor

@austince austince commented Jul 27, 2018

In reference to: #218 and @keithamus's suggestion.

This is now possible:

const createError = require('http-errors');

  try {
   // create an error from a custom prototype w/ status properties
    throw createError(400, "Bad request");
  } catch (err) {
    expect(err).to.have.property('message');
    expect(err).to.have.status(400);
    return;
  }

@austince
Copy link
Contributor Author

Looks like a test just timed out and needs to be restarted. I can't do it on my end without pushing, but let me know if that's what I should do.

Copy link
Member

@keithamus keithamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM! Someone else from @chaijs/chai-http should merge.

@JamesMessinger
Copy link
Member

JamesMessinger commented Jul 30, 2018

Looks good, but one nitpick.... I don't think the http-errors dependency is necessary, for two reasons:

1. It seems like overkill to bring in a new dependency just for a single test

2. The test relies on the specific, current implementation of http-errors. If that implementation ever changes in the future it could break this test. Instead, I would recommend just testing the actual thing that we're testing, which is whether the assertion works with prototype properties. That could easily be tested by creating an object with a prototype property in the test, rather than relying on an external library.

Just my $0.02. Otherwise, I like the fix 👍

@austince
Copy link
Contributor Author

Sure, I can do the prototype approach. My choice to include http-errors was because it is a popular library in the ecosystem that relies on chai-http, so might as well test against it as a devDependency. That said, happy to do the no-dep approach later today!

@austince
Copy link
Contributor Author

austince commented Jul 31, 2018

@BigstickCarpet is that more in line with what you were thinking? Also, should I increase the timeouts on those tests? 2 seconds seems to be regularly exceeded.

Copy link
Member

@keithamus keithamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still 👍 from me

@JamesMessinger
Copy link
Member

@austince - Perfect! That's exactly what I was thinking. 👍

As for the tests... I think those timeout errors were just sporadic failures. Those 2 tests usually take < 200ms. I just re-ran the build and everything passed this time 👍

So I say we're good to merge this one.

@JamesMessinger JamesMessinger merged commit d515691 into chaijs:master Aug 1, 2018
@austince
Copy link
Contributor Author

austince commented Aug 1, 2018

Awesome, thanks so much @BigstickCarpet and @keithamus! How does the release schedule work?

@keithamus
Copy link
Member

@austince someone will need to run make and generate a commit to commit the build artefacts into git. An example is here in the last release: #202. Generally I like to keep everything community lead - so if you'd like to make a pull request releasing 4.1.0 I'd be happy to accept that PR 😄

@austince
Copy link
Contributor Author

austince commented Aug 2, 2018

Got it - PR in progress!

@austince austince mentioned this pull request Aug 2, 2018
JamesMessinger added a commit that referenced this pull request Aug 14, 2018
4.1.0

Changes included:

- #219
- #217

Fixes:

- #218
- #189
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants