Return new assertion with flags copied over instead of this. Fix #791 #799

vieiralucas · 2016-09-14T02:12:11Z

Hello!

This PR addresses #791.

overwriteProperty
overwriteMethod
addChainableMethod
overwriteChainableMethod

keithamus · 2016-09-14T09:20:19Z

lib/chai/utils/overwriteProperty.js

@@ -4,7 +4,9 @@
 * MIT Licensed
 */

+var chai = require('../../chai');


Does this create a circular dependency? Have we done this before? I feel like this might create issues

Currently addMethod and addProperty have this too and there's been no issue reported about it. However, I'd like to have more time to check this before giving a confident response.

I'm a bit confused how it's working.

test/bootstrap/index.js requires index.js

index.js requires lib/chai.js

lib/chai.js requires lib/chai/utils/index.js

lib/chai/utils/index.js requires lib/chai/utils/addProperty.js

lib/chai/utils/addProperty.js requires lib/chai.js

Thus circular dependency. -scratches head-

I think it works because chai.js only sets module.exports and this file only calls stuff from chai.js inside of the methods, so it all kind of works out eventually.

Hello there, While I was working on #799, I discovered that you cannot call `JSON.stringify` on an assertion because o proxify. ```javascript const { expect } = require('chai'); const equal = expect(1).to.equal(1); JSON.stringify(equal); // This will throw Error: Invalid Chai property: toJSON ``

vieiralucas · 2016-09-15T03:11:22Z

I believe that I was creating more confusion than explaining something (at least to myself).

IMHO, we should return a new Assertion with flags copied over not only to prevent bugs with length, but to keep consistency as well.

meeber

@vieiralucas Awesome work sir, thank you for doing this! I tested this branch with my work-in-progress on proxifying methods, and it solves the problem I was running into. Yay!

I noticed you added delete statements to a bunch of existing tests that were adding new properties. I agree that we should make sure every test cleans up after itself. My only suggestion is that we restructure some of these tests so that they clean up after themselves even if they fail. Therefore, we'll need to wrap those tests in describe blocks and move the cleanup code into after blocks, as I noticed some of the newer tests already do.

meeber · 2016-09-19T00:36:15Z

test/utilities.js

@@ -230,6 +230,8 @@ describe('utilities', function () {
    });

    expect(expect('foo').result()).to.equal('result');
+
+    delete chai.Assertion.prototype.result;


It's a great idea to add this so the test cleans up after itself, but we need it to run the cleanup operation regardless of success or failure. Therefore, I think we should wrap this test in a describe with an after function to contain the cleanup activities.

Of course, you're absolutely right. Thanks for noticing.
I'll make the changes.

meeber · 2016-09-19T00:36:56Z

test/utilities.js

@@ -273,6 +275,9 @@ describe('utilities', function () {

    var anotherAssertion = expect([1, 2, 3]).to.have.a.lengthOf(3).and.to.be.ok;
    expect(anotherAssertion.length.constructor).to.equal(assertionConstructor);
+
+    delete chai.Assertion.prototype.returnNewAssertion;
+    delete chai.Assertion.prototype.checkFlags;
