-
Notifications
You must be signed in to change notification settings - Fork 108
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
Have promise rejection tests mirror how Chai tests exceptions. #167
Have promise rejection tests mirror how Chai tests exceptions. #167
Conversation
The problem is that expect().to.be.rejected/rejectedWith (and its negation) does not mirror how expect().to.throw (and its negation) behave. Same issue with assert.isRejected in relation to assert.throws.
The problem with issue chaijs#47 is that chai-as-promised somtimes treats the final string parameter as a custom error message. We want to test that it does not do this anymore so we check to make sure that such string is not used as a custom error message.
- `.fulfilled`, `.not.rejected` and `.not.rejectedWith` when successful: * The promise they return is fulfilled with the same value as the promise they were testing. - When `.not.fulfilled`, `.rejected` and `.rejectedWith` are successful: * The promise they return is fulfilled with the rejection reason. In all cases, when they fail, the promise they return is rejected and has for reason the assertion that failed. The changes above make it so that the `object` flag is automatically set when the promise returned is resolved, and thus also allows chaining. This fixes chaijs#113.
We now use the check-error package to get the constructor name of the error we expect to get with `rejectedWith`. For recent versions of Chai, this makes `rejectedWith` consistent in behavior with Chai's `throw`. Fixes chaijs#64.
In Chai `foo.should.not.throw(Constructor, 'string')` would fail only if the error thrown is an instance of `Constructor` **and** if its message includes `'string'`. An instance with a different message would not cause a failure. An instance constructed by something else than `Constructor` would not cause a failure. .not.rejectedWith's semantics were incompatible with the above. This commit fixes the test to work according to the correct semantics.
chai-as-promised's `.rejectedWith` and `assert.isRejected` now mirror how Chai's `.throws` works. Fixes issue chaijs#47.
Wow, thank you so much!! I'm at a week-long conference this week, so I'll be a bit slower in reviewing than I would like. But I will try to carve out time during the nights to work on this. Hopefully we should be able to merge by end of the week. |
I merged this with no changes, just to the commit messages :). I also then did a subsequent commit to remove the UMD stuff and just go with CommonJS. A new major version will be going out shortly. Thank you so much for your help! |
…=nathanhammond Update chai-as-promised to version 6.0.0 🚀 Hello lovely humans, [chai-as-promised](https://www.npmjs.com/package/chai-as-promised) just published its new version 6.0.0. <table> <tr> <th align=left> State </th> <td> Update 🚀 </td> </tr> <tr> <th align=left> Dependency </td> <td> chai-as-promised </td> </tr> <tr> <th align=left> New version </td> <td> 6.0.0 </td> </tr> <tr> <th align=left> Type </td> <td> devDependency </td> </tr> </table> This version is **not covered** by your **current version range**. Without accepting this pull request your project will work just like it did before. There might be a bunch of new features, fixes and perf improvements that the maintainers worked on for you though. I recommend you look into these changes and try to get onto the latest version of chai-as-promised. Given that you have a decent test suite, a passing build is a strong indicator that you can take advantage of these changes by merging the proposed change into your project. Otherwise this branch is a great starting point for you to work on the update. Do you have any ideas how I could improve these pull requests? Did I report anything you think isn’t right? Are you unsure about how things are supposed to work? There is a collection of [frequently asked questions](https://greenkeeper.io/faq.html) and while I’m just a bot, there is a group of people who are happy to teach me new things. [Let them know](https://github.com/greenkeeperio/greenkeeper/issues/new). Good luck with your project ✨ You rock! 🌴 --- [GitHub Release](https://github.com/domenic/chai-as-promised/releases/tag/v6.0.0) <p>In <a href="https://urls.greenkeeper.io/domenic/chai-as-promised/pull/167" class="issue-link js-issue-link" data-url="chaijs/chai-as-promised#167" data-id="177667891" data-error-text="Failed to load issue title" data-permission-text="Issue title is private">#167</a>, <a href="https://urls.greenkeeper.io/lddubeau" class="user-mention">@lddubeau</a> made many improvements to edge cases that have plagued users:</p> <ul> <li>Overhauls <code>rejectedWith</code> to behave more like Chai's <code>throws</code> asserter.</li> <li>Updates <code>.fulfilled</code>, <code>.rejected</code>, and <code>.rejectedWith</code> to change the asserter target to the fulfillment value or rejection reason, so that further assertions with <code>.and</code> act on them.</li> <li>Updates <code>.fulfilled</code>, when successful, to return a promise that fulfills with the fulfillment value</li> <li>Updates <code>.rejected</code> and <code>.rejectedWith</code>, when successful, to return a promise that fulfills with the rejection reason</li> </ul> <p>Also, Chai as Promised now only supplies a CommonJS-style module. To get AMD or <code><script></code>-compatibility, use a bundler tool like <a href="http://browserify.org/">browserify</a>.</p> --- The new version differs by 28 commits . - [`b2cfbdc`](chaijs/chai-as-promised@b2cfbdc) <code>6.0.0</code> - [`479f867`](chaijs/chai-as-promised@479f867) <code>Update copyright year</code> - [`bb51e07`](chaijs/chai-as-promised@bb51e07) <code>Switch to pure CommonJS</code> - [`fc9f868`](chaijs/chai-as-promised@fc9f868) <code>Load check-error in the browser too</code> - [`33ce9df`](chaijs/chai-as-promised@33ce9df) <code>Bring promise rejection tests in line with Chai.</code> - [`10beb9e`](chaijs/chai-as-promised@10beb9e) <code>Harmonize error messages with those of Chai</code> - [`a8b88d0`](chaijs/chai-as-promised@a8b88d0) <code>Fix the .not.rejectedWith tests</code> - [`1f7fc93`](chaijs/chai-as-promised@1f7fc93) <code>Use check-error to get expected constructor name</code> - [`e425136`](chaijs/chai-as-promised@e425136) <code>Redesign UMD code to allow imports</code> - [`823c15b`](chaijs/chai-as-promised@823c15b) <code>Allow chaining tests and promises.</code> - [`52beb1f`](chaijs/chai-as-promised@52beb1f) <code>Add a notMessage option</code> - [`40fe94e`](chaijs/chai-as-promised@40fe94e) <code>Restructure to avoid so much nesting</code> - [`62e137f`](chaijs/chai-as-promised@62e137f) <code>Add test cases that reproduce issue #113</code> - [`7662aca`](chaijs/chai-as-promised@7662aca) <code>Add a case that replicates issue #64</code> - [`deb505c`](chaijs/chai-as-promised@deb505c) <code>Add test cases to cover issue #47</code> There are 28 commits in total. See the [full diff](chaijs/chai-as-promised@5f20e6c...b2cfbdc). --- This pull request was created by [greenkeeper.io](https://greenkeeper.io/). <sub>Tired of seeing this sponsor message? ⚡ `greenkeeper upgrade`</sub>
This PR takes care primarily of #47. I've also dealt with #64 and #113 while working on this.
Some notes on this PR...
This PR introduces a bunch of breaking changes. There's no way around it if our goal is to have
.should.be.rejectedWith
andassert.isRejected
mirror what Chai does withshould.throw
andassert.throws
.Notably:
.fulfilled
,.rejected
and.rejectedWith
now change theobject
flag when they are successful. Code relying on the old behavior will fail, or in some cases it could be successful but not test what it used to test. Code that used chaining with these assertions should be hand-inspected and modified as needed..not.rejectedWith
now has different semantics. In previous versionsfoo.should.not.be.rejectedWith(Constructor, 'string')
would fail if the error was rejected with an instance ofConstructor
. If it was rejected with an instance ofConstructor
then the next parameter would not even be inspected. It would also fail if the error was rejected with an instance of something else that had for message'string'
.This was different from how
foo.should.not.throw(Constructor, 'string')
has been behaving in Chai. In Chai, this test would fail only if the error thrown is an instance ofConstructor
and if it has the message'string'
. AConstructor
instance with a different message would not cause a failure. An instance of a different class with the message'string'
would not cause a failure.Tests that were failing will now pass due to this change. All instances of
.not.rejectedWith
need inspection to see whether they need to be modified in light of the new semantics.While fixing #113 I've done the following:
.fulfilled
,.not.rejected
and.not.rejectedWith
when successful:.not.fulfilled
,.rejected
and.rejectedWith
are successful:In all cases, when they fail, the promise they return is rejected and has for reason the assertion that failed.
The changes above make it so that (as mentioned before) the
object
flag is automatically set when the promise returned is resolved, and thus also allows chaining. (I've tested chaining independently.)I have flip-flopped on whether the
eventually
flag should be automatically set by.fulfilled
.rejected
and.rejectedWith
. If it is set, then the tests that follow forcibly pertain to the fulfilled value of the promise they return. If it is not set, then if the user wants to test the fulfilled value, they have to use.eventually
. Otherwise the tests are done on the promise itself.The one thing that I don't like about not setting the
eventually
, it is really easy to end up not testing what one think they're testing. One may write this:When one means to write this:
The first will check that the promise returned by
fulfilled
does not have the propertynonexistent
. The second checks that the rejection reason does not have the propertynonexistent
.Using
check-error
turned out to be a bit problematic.Chai passes
check-error
to plugins as thecheckError
field on theutils
parameter. However, no version of Chai has been released yet that has this field onutils
. (Probably the next version after 3.5.0 would do it. Note that I've tried running chai-as-promised with the latestmaster
of Chai and got a whole slew of errors. Fixingchai-as-promised
to work with Chai@master is beyond the scope of my task.) It seems to me making chai-as-promised require that future version as a minimum peer dependency would be too restrictive.In the code proposed here, chai-as-promised now does this: if
utils.checkError
exists, use that. Otherwise, it use an instance ofcheck-error
loaded from the module system (or obtained globallywhen
chai-as-promised
is loaded throughscript
).