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.

Sign up for GitHub

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

Jump to bottom

Have promise rejection tests mirror how Chai tests exceptions. #167

Closed
wants to merge 12 commits into from
Closed

Have promise rejection tests mirror how Chai tests exceptions. #167

wants to merge 12 commits into from

Conversation

lddubeau
Copy link
Contributor

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 and assert.isRejected mirror what Chai does with should.throw and assert.throws.

Notably:

  • .fulfilled, .rejected and .rejectedWith now change the object 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 versions foo.should.not.be.rejectedWith(Constructor, 'string') would fail if the error was rejected with an instance of Constructor. If it was rejected with an instance of Constructor 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 of Constructor and if it has the message 'string'. A Constructor 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:
    • 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 (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:

promise.should.not.be.fulfilled.and.have.property("nonexistent")

When one means to write this:

promise.should.not.be.fulfilled.and.eventually.have.property("nonexistent")

The first will check that the promise returned by fulfilled does not have the property nonexistent. The second checks that the rejection reason does not have the property nonexistent.

Using check-error turned out to be a bit problematic.

Chai passes check-error to plugins as the checkError field on the utils parameter. However, no version of Chai has been released yet that has this field on utils. (Probably the next version after 3.5.0 would do it. Note that I've tried running chai-as-promised with the latest master of Chai and got a whole slew of errors. Fixing chai-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 of check-error loaded from the module system (or obtained globally
when chai-as-promised is loaded through script).

@lddubeau
Added test cases to cover issue chaijs#47.
56ea133 
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.
@lddubeau
Added a case that replicates issue chaijs#64.
f06f849
@lddubeau
Added test cases that reproduce issue chaijs#113.
17d7dd8
@lddubeau
Restructure to avoid so much nesting.
cd4f4b9
@lddubeau
Add a notMessage option.
828c2d2 
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.
@lddubeau
Allow chaining tests and promises.
1e629d3 
- `.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.
@lddubeau
Redesigned UMD code to allow imports.
79f12ac
@lddubeau
Use check-error to get expected constructor name.
4656d67 
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.
@lddubeau
Fix the .not.rejectedWith tests.
150b666 
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.
@lddubeau
Harmonize error messages with those of Chai.
ddc7bf7
@lddubeau
Bring promise rejection tests in line with Chai.
c6188f4 
chai-as-promised's `.rejectedWith` and `assert.isRejected` now mirror
how Chai's `.throws` works. Fixes issue chaijs#47.
@lddubeau
Load check-error in the browser too!
8f1fa2b
@domenic
Copy link
Collaborator

domenic commented Sep 18, 2016

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.

@lddubeau lddubeau mentioned this pull request Sep 19, 2016
Closed
@domenic
Copy link
Collaborator

domenic commented Sep 27, 2016

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!

@domenic domenic closed this Sep 27, 2016
homu added a commit to ember-cli/ember-cli that referenced this pull request Sep 28, 2016
@homu
Auto merge of #6302 - ember-cli:greenkeeper-chai-as-promised-6.0.0, r…
6f0c9ae 
…=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>&lt;script&gt;</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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@lddubeau @domenic