rejectedWith/isRejected is not perfect

[domenic]

Ideally it should behave the same as Chai's error assertions. But it doesn't. Here's a meta-bug tracking all the previous bugs people have opened. Add your problem as a comment here.

• isRejected not behaving as assert.throw #47 isRejected is not behaving as assert.throw
• Assertion does not fail if promise is rejected without reason #56 rejecting with undefined
• rejectedWith not reading constructor names #64 output not correct
• rejected rejects if returned reason is a rejecting thenable #79 problems if rejection reason is a thenable
• Custom error assertion #113 does not have the Chai semantics of changing the asserter target to the error
• For rejected promises that were expected to be fulfilled, log the error that was thrown #128 something about the output

The solution is #47 (comment) if someone has the time to make that work. It looks like chaijs/chai#683 was merged so maybe Chai has the ability to do this now?

[keithamus]

Yup. Just FYI we've split the majority of the check-error logic into it's own module - https://github.com/chaijs/check-error - which is on npm and can be used like any normal dependency. While there is still quite a bit of code in our throws assertion the real core bits live inside check-errors code. If you read through the chai throws code you'll see it's mostly boilerplate.

[domenic]

Jeez that is still a lot of code. Well I guess maybe it's easier now.

[keithamus]

Yeah... it's mostly argument munging or negotiating with the check-error API which we didn't want to black box. Sadly providing 10 different ways for people to assert on errors can make things complicated 😅

[lddubeau]

I have some tests that I'm sure are not testing what they appear to be testing because I did not realize that isRejected was not mirroring throws. Fixing #47 is a pressing matter for me. I may be able to put in a PR to fix that problem and some of the other issues here.

As for the other issues:

• Assertion does not fail if promise is rejected without reason #56 rejecting with undefined: I cannot reproduce the undesirable behavior reported there. I've copied the reporter's code. Mocha is doing something different from when the report was filed so I removed Mocha from the equation and found that the assertions do fail as expected.
• rejectedWith not reading constructor names #64 unable to read constructor name: I can reproduce the problem easily. Should be easily fixed.
• rejected rejects if returned reason is a rejecting thenable #79 problems if rejection reason is a thenable: I can reproduce the problem but I'm not sure that chai-as-promised should do something different here. It is moreover not a pressing matter for me.
• Custom error assertion #113 does not have the Chai semantics of changing the asserter target to the error: should be easily fixable.
• For rejected promises that were expected to be fulfilled, log the error that was thrown #128 something about the output: this is not a pressing matter for me.

So I can see putting in a PR that fixes #47, #64 and #113. It could happen that #79 and #128 are fixed while I work on the other issues (I can easily see #128 being fixed as a side effect fixing something else) but it does not feel right to declare that I'll deal with them when neither are pressing issues for me.

[domenic]

A pull request would be super-awesome. Thank you so much.

[lddubeau]

@domenic I see test suite code that uses Mocha's done callback rather than return promises. Is there some reason the suite needs to continue using done rather than return promises? Compatibility with older versions of Mocha?

I need to make some changes to test/support/common.js for instance and I figure I could easily modernize that code to return promises but I don't want to do this if it is going to break something else.

[domenic]

I think in a library focused on adding complex promise features it's best not to rely on the promise features in the test runner. I'd rather keep it using the done style for now.

[lddubeau]

I've run into a bizarre problem that is tangentially related to what I said I'd fix but I'm at a point where input would be useful.

The following code was run with [email protected] and [email protected]. Check "1" fails in a surprising way. The only trivial way I know how to prevent the surprising failure would be to make the eventually flag be turned on implicitly whenever .fulfilled .rejected and .rejectedWith are used but that would constitute a breaking change. Otherwise, I'm not seeing a simple fix.

const chai = require("chai");
const chaiAsPromised = require("chai-as-promised");

chai.use(chaiAsPromised);

const expect = chai.expect;

// This would pass, as expected.
// expect(1).to.equal(1).and.not.equal(2);

function check(p, name) {
    return p.then(
        () => {
            console.log(`passed ${name}`);
        },
        console.log.bind(console, `failed ${name}!!!\n`));
}

// This fails surprisingly because it is interpreted as to.not.be.fulfilled...
check(expect(Promise.resolve(1)).to.be.fulfilled.and.not.equal(2), "1");

// These two pass, as expected.
check(expect(Promise.resolve(1)).to.be.fulfilled.and.eventually.not.equal(2),
      "2");
check(expect(Promise.resolve(1)).to.eventually.be.fulfilled.and.not.equal(2),
      "3");

// If `.fulfilled` turns on the `eventually` flag implicitly, then this
// will no longer be possible.
const p = Promise.resolve(1);
check(expect(p).to.be.fulfilled.and.equal(p), "4");

check(expect(Promise.resolve(1)).to.be.fulfilled.and.eventually.equal(1), "5");
check(expect(Promise.resolve(1)).to.eventually.be.fulfilled.and.equal(1), "6");

When I run it, I get:

failed 1!!!
 { [AssertionError: expected promise not to be fulfilled but it was fulfilled with 1]
  message: 'expected promise not to be fulfilled but it was fulfilled with 1',
  showDiff: false,
  actual: 1,
  expected: undefined }
passed 4
passed 5
passed 2
passed 6
passed 3

[domenic]

We have a separate issue where .and doesn't work correctly (#85). If possible, I'd suggest not trying to solve that in this work.

[lddubeau]

Looking at #113 I took from this comment and this one that the preferable solution to the issue here would be to use .and so as to allow chaining. Otherwise, I don't see what the solution for #113 would be.

This being said, I'm actually not seeing anything actionable in #85. A good deal of the issues there are issue with things other than chai-as-promised or they look like misconceptions to me. (ETA: I actually tried to reproduce the problems reported there, and could not.)

[ArtemGovorov]

[email protected] was reporting a promise rejection reason as the actual object, however chai-as-promised@6 is now doing .toString() on the object if it's an error.

This makes it impossible to get/view/report the error stack, because the stack is basically thrown away.

Here's an example: wallaby.js uses actual object stack (if it's an Error) to highlight the error stack for the user, so that for a test like this:

it("should test promise throwing error", () => {
    const promiseThrowingError = () => {
        return new Promise((resolve, reject) => {
            // or just throw new Error("Error from promise");
            return reject(new Error("Error from promise"));
        });
    };
    return expect(promiseThrowingError()).to.not.be.rejected;
});

the error stack can be reported in [email protected]:

The raised AssertionError doesn't contain the original rejection Error stack, so the only way to report the stack was to use the actual object's stack (if it's an Error). Now in chai-as-promised@6 it is not possible because the stack is swallowed.

Is there any way the original error could still be reported so that tools like test runners/reporters could report the error stack?

[ArtemGovorov]

Perhaps chai could extend its API to include an original error into the AssertionError somehow. Because currently the stack from any promise error is simply lost, that is not great for debugging/visibility.

/cc @keithamus

[lddubeau]

@ArtemGovorov Chai-as-promised 6 aims to mirror what Chai does. (See here for instance.) Did you try to see what you get when you do a test similar to the one you show in your earlier comment but which tests synchronous exception throwing? Do you get a stack then? I would expect not.

Chai has config.includeStack, which can be used to turn on stack traces across the board. I suspect if exception assertions were to additionally add the stack of the exception itself, the end result would be hard to read when config.includeStack is also turned on.

Note that Chai-as-promised also benefits from config.includeStack however it is dependent on a Promise implementation that provides useful stack traces. If I use config.includeStack with the stock Promise implementation in Node.js I get something largely useless. If I use Bluebird and turn on long stack traces, then I get a stack that is more useful. It is not perfect through because it does not go back to the actual line where Error was created. I don't know whether it is a problem in Chai-as-promised or whether it can be fixed by setting transferPromiseness.

[ArtemGovorov]

@lddubeau I see what you mean about mirroring chai semantics. I think there's still an issue of not reporting the caught error back to the test runner though. This patch makes chai-as-promised to pass exceptions as actual objects, but it obviously doesn't mirror what chai does.

Using the config.includeStack is a good idea and could help, but without changing chai API I can't see how chai-as-promised could extend the raised AssertionError stack with the error that it captures as a promise rejection.

[thomasvanlankveld]

Turning on config.includeStack, I get the following:

AssertionError: expected promise to be rejected with 'ClientError' but it was fulfilled with undefined
    at assertIfNotNegated (<project-path>/node_modules/chai-as-promised/lib/chai-as-promised.js:62:19)
    at <project-path>/node_modules/chai-as-promised/lib/chai-as-promised.js:179:17
    at process._tickDomainCallback (internal/process/next_tick.js:129:7)

This stack doesn't tell me which test is failing.