Repository: chaijs/chai
Pull request #642 (2016-03-16 21:58:31)

addMethod returns a new assertion with flags copied over instead of this
This aims to solve #562.

Special thanks to @keithamus for the whole idea here, I just made some minor adjustments.

This PR should include:
- [x] `addMethod` returning a `new Assertion` with flags copied over
- [x] Tests for it
- [x] Fix [this](https://github.com/chaijs/chai/pull/642#discussion_r56754878) - EDIT: Unfortunately there is one case we won't be able to cover, so this is only *partially* resolved. [Please read this](https://github.com/chaijs/chai/pull/642#issuecomment-198979726).

I'm currently writing the tests and I'm not sure if they're complete enough.
I was thinking about testing chaining some assertions as described on #562, testing the type of their return and if methods are really returning a new Assertion with the flags which should have been copied. Is this enough? If not, what else should be verified?

EDIT: This needs a review, please see [this](https://github.com/chaijs/chai/pull/642#discussion_r56754878). I'll take a further look at it.

lucasfcosta: Here are the tests I thought were right for this.
If you guys think of more things that should be covered by this test, please share.
lucasfcosta: Well, I've done some further studies on this and I've discovered that the explanation for why this (`expect(my_object).to.have.a.deep.property("foo.bar").that.is.an("array").with.a.length.of.at.least(1)`) is not working is the following:

We've got [this line into our `addChainableMethod.js` file](https://github.com/chaijs/chai/blob/master/lib/chai/utils/addChainableMethod.js#L108) which returns a `function` as the getter for the property being added, therefore, when calling `.length` right after using the `a` getter we get back a function (the function which executes the `chainingBehavior`, [as you can see here](https://github.com/chaijs/chai/blob/master/lib/chai/utils/addChainableMethod.js#L80), so it has got its own `.length` property, instead of the `.length` property of the assertion.

What I've done on this PR partially solves our problem because when we return a `new Assertion` instead of this, it means we're returning an object which is not a `function` and therefore does not have the `length` property, that is why `an.<something_here_will_return_new_Assertion>.length` works (because we are "transforming" the assert function into another object which is not a function) and `a.length` does not (because `a` immediately returns a function, which has a `length` property).

--------------------------------------------

## ** How we could solve this: **

This would be a very, very easy fix if we could just do:  `delete length` before returning `assert` [here](https://github.com/chaijs/chai/blob/master/lib/chai/utils/addChainableMethod.js#L108). This would cause the `.length` property to be called from the `prototype`, which means it will call the assertion instead of the function's length property.

**Unfortunately this is not available on all browsers** because the `.length` property is configurable only on Firefox(Gecko) >= 37 and on Node.js.

**Being able to delete `.length` would enable us to solve this with a single line of code**, unfortunately this is not possible on older environments (or even on PhantomJS 2.1).

Currently we can only solve this for some cases, like the ones described above, in which we return a `new Assertion` when using `addMethod` or `addProperty`.

I hope I was clear, but if I wasn't please ask.

Patch:
diff --git a/lib/chai/core/assertions.js b/lib/chai/core/assertions.js
index 68f8c27..6c3b9c5 100644
--- a/lib/chai/core/assertions.js
+++ b/lib/chai/core/assertions.js
@@ -43,9 +43,7 @@ module.exports = function (chai, _) {
   , 'is', 'and', 'has', 'have'
   , 'with', 'that', 'which', 'at'
   , 'of', 'same', 'but' ].forEach(function (chain) {
-    Assertion.addProperty(chain, function () {
-      return this;
-    });
+    Assertion.addProperty(chain);
   });
 
   /**
diff --git a/lib/chai/utils/addMethod.js b/lib/chai/utils/addMethod.js
index a2676cd..7c97a96 100644
--- a/lib/chai/utils/addMethod.js
+++ b/lib/chai/utils/addMethod.js
@@ -5,6 +5,7 @@
  */
 
 var config = require('../config');
+var transferFlags = require('./transferFlags');
 
 /**
  * ### .addMethod (ctx, name, method)
@@ -39,6 +40,10 @@ module.exports = function (ctx, name, method) {
     if (old_ssfi && config.includeStack === false)
       flag(this, 'ssfi', ctx[name]);
     var result = method.apply(this, arguments);
-    return result === undefined ? this : result;
+
+    var newAssertion = new chai.Assertion();
+    transferFlags(this, newAssertion);
+
+    return result === undefined ? newAssertion: result;
   };
 };
diff --git a/lib/chai/utils/addProperty.js b/lib/chai/utils/addProperty.js
index 07d624c..64e69ba 100644
--- a/lib/chai/utils/addProperty.js
+++ b/lib/chai/utils/addProperty.js
@@ -6,6 +6,7 @@
 
 var config = require('../config');
 var flag = require('./flag');
+var transferFlags = require('./transferFlags');
 
 /**
  * ### addProperty (ctx, name, getter)
@@ -34,6 +35,8 @@ var flag = require('./flag');
  */
 
 module.exports = function (ctx, name, getter) {
+  getter = getter === undefined ? new Function() : getter;
+
   Object.defineProperty(ctx, name,
     { get: function addProperty() {
         var old_ssfi = flag(this, 'ssfi');
@@ -41,7 +44,10 @@ module.exports = function (ctx, name, getter) {
           flag(this, 'ssfi', addProperty);
 
         var result = getter.call(this);
-        return result === undefined ? this : result;
+        var newAssertion = new chai.Assertion();
+        transferFlags(this, newAssertion);
+
+        return result === undefined ? newAssertion: result;
       }
     , configurable: true
   });


Test patch:
diff --git a/test/utilities.js b/test/utilities.js
index 4cdb86c..ecea26d 100644
--- a/test/utilities.js
+++ b/test/utilities.js
@@ -232,6 +232,46 @@ describe('utilities', function () {
     expect(expect('foo').result()).to.equal('result');
   });
 
+  it('addMethod returns new assertion with flags copied over', function () {
+    var assertionConstructor;
+
+    chai.use(function(_chai, utils) {
+      assertionConstructor = _chai.Assertion;
+      _chai.Assertion.addMethod('returnNewAssertion', function () {
+        utils.flag(this, 'mySpecificFlag', 'value1');
+        utils.flag(this, 'ultraSpecificFlag', 'value2');
+      });
+
+      _chai.Assertion.addMethod('checkFlags', function() {
+        this.assert(
+            utils.flag(this, 'mySpecificFlag') === 'value1' &&
+            utils.flag(this, 'ultraSpecificFlag') === 'value2'
+          , 'expected assertion to have specific flags'
+          , "this doesn't matter"
+        );
+      });
+    });
+
+    assertion1 = expect('foo');
+    assertion2 = assertion1.to.returnNewAssertion();
+
+    // Checking if a new assertion was returned
+    expect(assertion1).to.not.be.equal(assertion2);
+
+    // Check if flags were copied
+    assertion2.checkFlags();
+
+    // Checking if it's really an instance of an Assertion
+    expect(assertion2).to.be.instanceOf(assertionConstructor);
+
+    // Test chaining `.length` after a method to guarantee it is not a function's `length`
+    var anAssertion = expect([1, 2, 3]).to.be.an.instanceof(Array);
+    expect(anAssertion.length).to.be.an.instanceof(assertionConstructor);
+
+    var anotherAssertion = expect([1, 2, 3]).to.have.a.lengthOf(3).and.to.be.ok;
+    expect(anotherAssertion.length).to.be.an.instanceof(assertionConstructor);
+  });
+
   it('overwriteMethod', function () {
     chai.use(function (_chai, _) {
       expect(_chai.Assertion).to.respondTo('eqqqual');
@@ -291,6 +331,49 @@ describe('utilities', function () {
     expect(assert.__flags.tea).to.equal('chai');
   });
 
+  it('addProperty returns a new assertion with flags copied over', function () {
+    var assertionConstructor = chai.Assertion;
+
+    chai.use(function(_chai, utils) {
+      assertionConstructor = _chai.Assertion;
+      _chai.Assertion.addProperty('thing', function () {
+        utils.flag(this, 'mySpecificFlag', 'value1');
+        utils.flag(this, 'ultraSpecificFlag', 'value2');
+      });
+
+      _chai.Assertion.addMethod('checkFlags', function() {
+        this.assert(
+            utils.flag(this, 'mySpecificFlag') === 'value1' &&
+            utils.flag(this, 'ultraSpecificFlag') === 'value2'
+          , 'expected assertion to have specific flags'
+          , "this doesn't matter"
+        );
+      });
+    });
+
+    assertion1 = expect('foo');
+    assertion2 = assertion1.is.thing;
+
+    // Checking if a new assertion was returned
+    expect(assertion1).to.not.be.equal(assertion2);
+
+    // Check if flags were copied
+    assertion2.checkFlags();
+
+    // If it is, calling length on it should return an assertion, not a function
+    expect([1, 2, 3]).to.be.an.instanceof(Array);
+
+    // Checking if it's really an instance of an Assertion
+    expect(assertion2).to.be.instanceOf(assertionConstructor);
+
+    // Test chaining `.length` after a property to guarantee it is not a function's `length`
+    expect([1, 2, 3]).to.be.a.thing.with.length.above(2);
+    expect([1, 2, 3]).to.be.an.instanceOf(Array).and.have.length.below(4);
+
+    expect(expect([1, 2, 3]).be).to.be.an.instanceOf(assertionConstructor);
+    expect(expect([1, 2, 3]).thing).to.be.an.instanceOf(assertionConstructor);
+  });
+
   it('addProperty returning result', function () {
     chai.use(function(_chai, _) {
       _chai.Assertion.addProperty('result', function () {