Support Chai@4 - Fix assertions broken by Chai 4.0

[meeber]

This PR addresses chaijs/chai#723. To summarize:

• A change in Chai 4.0 causes assertions to lose their promiseness unless the promisified assertion object is explicitly returned
• A single test started failing with Chai 4.0 due to the replacement of .deep with .nested
• With this PR, chai-as-promised should now work with Chai 4.0 while continuing to work with older versions of Chai

[lucasfcosta]

Nice catch.
Just summarizing this just to make other reviewer's job easier: this works because when we have a return inside the getter function then this if condition becomes true and therefore we return that instead of a new assertion with flags copied over.

[lucasfcosta]

Awesome work!

I just got to the bottom of this and this is what I found out: on the doAsserterAsyncAndAddThen function we have a single argument, as you can see here, this single argument for then will only be called if the promise gets resolved, if it gets rejected it should call the second argument, which doesn't exist in this case, so we don't do any assertion at all.

As a proof of concept, I changed that method to this:

function doAsserterAsyncAndAddThen(asserter, assertion, args) {
    // Since we're intercepting all methods/properties, we need to just pass through if they don't want
    // `eventually`, or if we've already fulfilled the promise (see below).
    if (!utils.flag(assertion, "eventually")) {
        return asserter.apply(assertion, args);
    }
    var derivedPromise = getBasePromise(assertion).then(function (value) {
        // Set up the environment for the asserter to actually run: `_obj` should be the fulfillment value, and
        // now that we have the value, we're no longer in "eventually" mode, so we won't run any of this code,
        // just the base Chai code that we get to via the short-circuit above.
        assertion._obj = value;
        utils.flag(assertion, "eventually", false);

        return args ? chaiAsPromised.transformAsserterArgs(args) : args;
    }, function(value) {
        // Here I'm doing the same thing as if the method got resolved, but this will be called if it gets rejected
        assertion._obj = value;
        utils.flag(assertion, "eventually", false);

        return args ? chaiAsPromised.transformAsserterArgs(args) : args;
    }).then(function (args) {
        asserter.apply(assertion, args);

        // Because asserters, for example `property`, can change the value of `_obj` (i.e. change the "object"
        // flag), we need to communicate this value change to subsequent chained asserters. Since we build a
        // promise chain paralleling the asserter chain, we can use it to communicate such changes.
        return assertion._obj;
    });

    chaiAsPromised.transferPromiseness(assertion, derivedPromise);
}

As you can see above I just added a second argument which does the same thing as the first one, but this will be called whenever the promise gets rejected.

Now every test is passing.
I'm just wondering why these tests weren't failing before, because the promise should've been getting rejected before that commit on Chai too.

Well, now we know what is causing the problem maybe we should check why this was working before.

Let me know if you need any help with this @meeber, you've been doing great commits 😄

Extra note: Isn't this line error prone? What if something that is not a promise has a then method?

[meeber]

@lucasfcosta Thanks for reviewing this!

I wonder the same thing about how those two tests were passing before. Would be interesting to dig deeper. Good job finding a solution!

However, I'm still not sure I understand why they are meant to pass in the first place? Why should performing a second assertion on the rejected promise result in a fulfilled promise?

[keithamus]

.rejected.and.eventually.equal(42) is a valid test. rejected effectively just swaps the resolved/rejected values around so that you can assert on the error.

To give a recent concrete example I've had: I've been using chai-http+chai-as-promised to assert on failed http requests like so:

it('should reject unauthed request with 401', () => 
  chai.request('http://foo...').should.eventually.be.rejected.and.eventually.have.status(401)
});

[meeber]

@keithamus Ahh I understand now. Thanks! :D

[lucasfcosta]

@meeber you're welcome!

I think this behavior, as @keithamus explained, is totally valid.
And I'd also like to point that my solution was just a PoC, we can have a cleaner and more beautiful implementation of that instead of repeating code inside both functions hahaha

Let me know if you need help with anything, I will be very happy to review this when it's done 😄

[meeber]

@lucasfcosta I just pushed an updated PR. Turns out the promiseness was getting lost at .eventually for the same reason it was getting lost at .fulfilled, .rejected, and .rejectedWith, so merely having .eventually return this did the trick.

The reason it works without that extra code in .doAsserterAsyncAndAddThen is because .rejected handles the rejected promise and returns a non-promise value here, in this case 42.

[lucasfcosta]

@meeber Ah, makes total sense! Yeah, I've seen that that line returns the value the promise was rejected with, but I haven't tought about this. Adding a property which turns on a flag must return this too in order to be caught on that if we've got in the addProperty util.
This LGTM.

Thoughts @keithamus and @domenic?

[meeber]

Just updated this PR to accommodate for additional changes in Chai v4.0. Specifically, every assertion now needs to explicitly return the promisified assertion object or else the promise will be lost.

Also a breaking change in Chai 4.0 caused a single test to start failing because the old deep.property behavior has moved to nested.property. This PR fixes the broken test (although that test now fails in Chai v3.5), which is why Travis says the build is failing. Will update this PR to bump the Chai version once v4.0 is officially released.

[Turbo87]

FYI chai@4 was just released

[meeber]

@domenic I've updated this PR to support the Chai v4 release.

[Turbo87]

@meeber is this also compatible with the old chai releases or only with v4?

[meeber]

@Turbo87 It's my belief that this is still compatible with older versions of Chai. I base that only off of running the test suite against older versions. One test fails in older versions of Chai, but it's not a compatibility issue; that test just happens to be using a flag that changed names while testing some other concept.

[lucasfcosta]

Hi everyone, I've been getting lots of questions about when this is going to be released and it seems ready to me.

This module is pretty important for our users, so I'd like to ping everyone, especially @domenic to see if we can get this released in a near future.

Sorry to bother you all, I certainly don't want you to rush or feel pressured, I'm just aiming on sending you a notification so that we can decide what our next steps are going to be.

Thanks everyone (especially @domenic) for the hard work and all the effort you've put in this module.

[domenic]

I've been sick recently, and have a lot of work to catch up on as a result of that, so the very earliest this will be addressed is this weekend, but perhaps not even then. I understand a lot of people use this, but my free time is limited, so hopefully people can be patient.

[lucasfcosta]

@domenic they sure can. Sorry for bothering you.
Thank you very much for your help and for these awesome plugins ❤️

[stevenvachon]

@domenic sick is no excuse!! 😉Be well.

[Azumi0]

How are things going with this merge? We are currently downloading & working on version from this PR, but we need it on npm to pass the project to the customer.

[meeber]

Just noting here that Chai 4's proxy protection feature doesn't work with this PR. I don't have time to look at it in detail right now. Should be a matter of adding util.proxify on a few return values. I think that could be added later in a separate PR with tests.

Edit: I've opened up a follow-up PR in #205 to address this.