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

Support Chai@4 - Fix assertions broken by Chai 4.0 #157

Closed
wants to merge 1 commit into from
Closed

Support Chai@4 - Fix assertions broken by Chai 4.0 #157

wants to merge 1 commit into from

Conversation

meeber
Copy link
Contributor

@meeber meeber commented Jun 14, 2016

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

@@ -119,6 +119,7 @@
);

chaiAsPromised.transferPromiseness(that, derivedPromise);
return that;
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

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
Copy link
Contributor Author

meeber commented Jun 17, 2016

@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
Copy link
Member

.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
Copy link
Contributor Author

meeber commented Jun 17, 2016

@keithamus Ahh I understand now. Thanks! :D

@lucasfcosta
Copy link
Member

lucasfcosta commented Jun 17, 2016

@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
Copy link
Contributor Author

meeber commented Jun 17, 2016

@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
Copy link
Member

@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
Copy link
Contributor Author

meeber commented Dec 17, 2016

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
Copy link

Turbo87 commented May 26, 2017

FYI chai@4 was just released

@meeber
Copy link
Contributor Author

meeber commented May 26, 2017

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

@Turbo87
Copy link

Turbo87 commented May 26, 2017

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

@meeber
Copy link
Contributor Author

meeber commented May 26, 2017

@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.

@keithamus keithamus changed the title WIP: Fix assertions broken by latest Chai Support Chai@4 - Fix assertions broken by Chai 4.0 May 30, 2017
@lucasfcosta
Copy link
Member

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
Copy link
Collaborator

domenic commented May 30, 2017

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
Copy link
Member

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

@stevenvachon
Copy link

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

igorshapiro added a commit to WiserSolutions/quadro that referenced this pull request Jun 6, 2017
igorshapiro added a commit to WiserSolutions/quadro that referenced this pull request Jun 6, 2017
igorshapiro added a commit to WiserSolutions/quadro that referenced this pull request Jun 6, 2017
@Azumi0
Copy link

Azumi0 commented Jun 8, 2017

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
Copy link
Contributor Author

meeber commented Jun 10, 2017

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.

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.

7 participants