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.

Sign up for GitHub

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

Jump to bottom

addMethod returns a new assertion with flags copied over instead of this #642

Merged
merged 3 commits into from
Mar 21, 2016
Merged

addMethod returns a new assertion with flags copied over instead of this #642

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1028fd2
addMethod returns a new assertion with flags copied over instead of this
lucasfcosta Mar 16, 2016
67a3ca5
Tests to check if addMethod returns a new Assertion instance
lucasfcosta Mar 19, 2016
8f8f186
Simplify adding language chains with a check for the getter argument
lucasfcosta Mar 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 1 addition & 3 deletions lib/chai/core/assertions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});

/**
Expand Down
7 changes: 6 additions & 1 deletion lib/chai/utils/addMethod.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

var config = require('../config');
var transferFlags = require('./transferFlags');

/**
* ### .addMethod (ctx, name, method)
Expand Down Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm getting ReferenceError: chai is not defined thrown here

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this problem has been shadowed by global.chai = require('../..'); in all tests...

Copy link
Member

Choose a reason for hiding this comment

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

Good catch, thanks @Turbo87. Fancy making a PR for this?

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, I see you already have 🎉 👍

transferFlags(this, newAssertion);

return result === undefined ? newAssertion: result;
};
};
8 changes: 7 additions & 1 deletion lib/chai/utils/addProperty.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

var config = require('../config');
var flag = require('./flag');
var transferFlags = require('./transferFlags');

/**
* ### addProperty (ctx, name, getter)
Expand Down Expand Up @@ -34,14 +35,19 @@ 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');
if (old_ssfi && config.includeStack === false)
flag(this, 'ssfi', addProperty);

var result = getter.call(this);
return result === undefined ? this : result;
var newAssertion = new chai.Assertion();
Copy link
Contributor

Choose a reason for hiding this comment

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

same here: ReferenceError: chai is not defined

transferFlags(this, newAssertion);

return result === undefined ? newAssertion: result;
}
, configurable: true
});
Expand Down
83 changes: 83 additions & 0 deletions test/utilities.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add some assertions that feature the an keyword, and also feature the length keyword, similar to the original issue.

expect([1, 2, 3]).to.be.an.instanceOf(Array).and.to.have.length.below(1000)

Copy link
Member Author

Choose a reason for hiding this comment

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

Well thought @keithamus.
This fix appears to work with the expression you posted, but I went to the issue to test other examples and this appears to fail:

expect(my_object).to.have.a.deep.property("foo.bar").that.is.an("array").with.a.length.of.at.least(1);

The reason is the same you explained on the issue, we are getting back an assert function instead of an Assertion object.

I'll take a further look at this to find the exact reason.

Thanks again for your feedback, it was really useful 😃

Copy link
Member

Choose a reason for hiding this comment

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

Hey @lucasfcosta thanks for doing that, and also awesome work adding some more tests and discovering another issue 😄

It looks like in that case, the .a. part turns the Assertion into a function, from which it doesn't recover. To wit:

> chai.expect({foo: [1,2,3]}).to
Assertion {
  __flags:
   { ssfi: [Function: addProperty],
     object: { foo: [Object] },
     message: undefined } }
> chai.expect({foo: [1,2,3]}).to.have
Assertion {
  __flags:
   { ssfi: [Function: addProperty],
     object: { foo: [Object] },
     message: undefined } }
> chai.expect({foo: [1,2,3]}).to.have.a
[Function: assert]
> chai.expect({foo: [1,2,3]}).to.have.a.deep
[Function: assert]

Luckily, it looks like the fix will be pretty much the same thing. L44 of addProperty has the same ternary.

Copy link
Member Author

Choose a reason for hiding this comment

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

EDIT: Please ignore this comment and just read this.

Ah, that makes sense.
Thanks for the insight here.

I was also doing some research on this too, shouldn't we create another assertion and transfer flags to it instead of returning this?

I'm not sure this is needed, I will do some further experiments with this, but it may be good to prevent further errors related to this.


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');
Expand Down Expand Up @@ -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 () {
Expand Down