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

Wrap non-chainable methods in proxies #789

Merged
merged 1 commit into from
Sep 26, 2016
Merged

Wrap non-chainable methods in proxies #789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 5 additions & 2 deletions lib/chai/utils/addMethod.js
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@

var chai = require('../../chai');
var flag = require('./flag');
var proxify = require('./proxify');
var transferFlags = require('./transferFlags');

/**
Expand Down Expand Up @@ -35,11 +36,11 @@ var transferFlags = require('./transferFlags');
*/

module.exports = function (ctx, name, method) {
ctx[name] = function () {
var fn = function () {
var keep_ssfi = flag(this, 'keep_ssfi');
var old_ssfi = flag(this, 'ssfi');
if (!keep_ssfi && old_ssfi)
flag(this, 'ssfi', ctx[name]);
flag(this, 'ssfi', fn);

var result = method.apply(this, arguments);
if (result !== undefined)
Expand All @@ -49,4 +50,6 @@ module.exports = function (ctx, name, method) {
transferFlags(this, newAssertion);
return newAssertion;
};

ctx[name] = proxify(fn, name);
};
7 changes: 5 additions & 2 deletions lib/chai/utils/overwriteMethod.js
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@

var chai = require('../../chai');
var flag = require('./flag');
var proxify = require('./proxify');
var transferFlags = require('./transferFlags');

/**
Expand Down Expand Up @@ -51,11 +52,11 @@ module.exports = function (ctx, name, method) {
if (_method && 'function' === typeof _method)
_super = _method;

ctx[name] = function () {
var fn = function () {
var keep_ssfi = flag(this, 'keep_ssfi');
var old_ssfi = flag(this, 'ssfi');
if (!keep_ssfi && old_ssfi)
flag(this, 'ssfi', ctx[name]);
flag(this, 'ssfi', fn);

flag(this, 'keep_ssfi', true);
var result = method(_super).apply(this, arguments);
Expand All @@ -69,4 +70,6 @@ module.exports = function (ctx, name, method) {
transferFlags(this, newAssertion);
return newAssertion;
}

ctx[name] = proxify(fn, name);
};
20 changes: 17 additions & 3 deletions lib/chai/utils/proxify.js
View file
Open in desktop
Expand Up @@ -11,15 +11,22 @@ var getProperties = require('./getProperties');
* # proxify(object)
*
* Return a proxy of given object that throws an error when a non-existent
* property is read. (If Proxy or Reflect is undefined, then return object
* without modification.)
* property is read. By default, the root cause is assumed to be a misspelled
* property, and thus an attempt is made to offer a reasonable suggestion from
* the list of existing properties. However, if a nonChainableMethodName is
* provided, then the root cause is instead a failure to invoke a non-chainable
* method prior to reading the non-existent property.
*
* If proxies are unsupported or disabled via the user's Chai config, then
* return object without modification.
*
* @param {Object} obj
* @param {String} nonChainableMethodName
* @namespace Utils
* @name proxify
*/

module.exports = function proxify (obj) {
module.exports = function proxify (obj, nonChainableMethodName) {
if (!config.useProxy || typeof Proxy === 'undefined' || typeof Reflect === 'undefined')
return obj;

Expand All @@ -32,6 +39,13 @@ module.exports = function proxify (obj) {
if (typeof property === 'string' &&
config.proxyExcludedKeys.indexOf(property) === -1 &&
!Reflect.has(target, property)) {
// Special message for invalid property access of non-chainable methods.
if (nonChainableMethodName) {
throw Error('Invalid Chai property: ' + nonChainableMethodName + '.' +
property + '. See docs for proper usage of "' +
nonChainableMethodName + '".');
}
Copy link
Member

Choose a reason for hiding this comment

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

This error message is a bit unfriendly, I feel like it doesn't explicitly point out where the user went wrong. Could we instead have something like Invalid Chai property equal.pizza. Did you mean equal(pizza)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@keithamus Agreed it can be better. I struggled with the phrasing quite a bit actually. The reason I ended up with the current phrasing is because many of the alternatives I explored only offer sensible advice part of the time.

For example:

expect(x).to.be.equal.to.true;
// Invalid Chai property: equal.to. Did you mean equal(to)?

expect(x).to.be.a.string.with.length(4);
// Invalid Chai property: string.with. Did you mean string(with)?

expect(x).to.throw.an.error;
// Invalid Chai property: throw.an. Did you mean throw(an)?

expect(x).to.be.within.2.and.4;
// Invalid Chai property: within.2. Did you mean within(2)?

I feel uncomfortable with Chai reacting to the user's mistake by suggesting an alternative that's also incorrect, especially in the last case since it demonstrates using within with only one argument when it actually requires two.

Could do something like: Invalid Chai property: equal.pizza. See docs for proper usage of the "equal" assertion.

Or the high-effort solution with customized messages for each method: Invalid usage of .within. Correct usage: .within(start, finish).

Copy link
Member

@keithamus keithamus Sep 9, 2016 •
edited

Choose a reason for hiding this comment

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

Okay, I like the idea of "See docs...". I agree that we dont want to present wrong info, but I also what to give users a chance to fix without immediately resorting to a support channel (like our issue tracker 😵)


var orderedProperties = getProperties(target).filter(function(property) {
return !Object.prototype.hasOwnProperty(property) &&
['__flags', '__methods', '_obj', 'assert'].indexOf(property) === -1;
Expand Down
132 changes: 115 additions & 17 deletions test/expect.js
View file
Open in desktop
Expand Up @@ -10,29 +10,127 @@ describe('expect', function () {
expect('foo').to.equal('foo');
});

it('invalid property', function () {
describe('invalid property', function () {
if (typeof Proxy === 'undefined' || typeof Reflect === 'undefined') return;

err(function () {
expect(42).pizza;
}, 'Invalid Chai property: pizza');
before(function () {
chai.util.addProperty(chai.Assertion.prototype, 'tmpProperty', function () {
new chai.Assertion(42).equal(42);
});
chai.util.overwriteProperty(chai.Assertion.prototype, 'tmpProperty', function (_super) {
return function () {
_super.call(this);
};
});

err(function () {
expect(42).to.pizza;
}, 'Invalid Chai property: pizza');
chai.util.addMethod(chai.Assertion.prototype, 'tmpMethod', function () {
new chai.Assertion(42).equal(42);
});
chai.util.overwriteMethod(chai.Assertion.prototype, 'tmpMethod', function (_super) {
return function () {
_super.call(this);
};
});

err(function () {
expect(42).to.be.a.pizza;
}, 'Invalid Chai property: pizza');
chai.util.addChainableMethod(chai.Assertion.prototype, 'tmpChainableMethod', function () {
new chai.Assertion(42).equal(42);
}, function () {
new chai.Assertion(42).equal(42);
});
chai.util.overwriteChainableMethod(chai.Assertion.prototype, 'tmpChainableMethod', function (_super) {
return function () {
_super.call(this);
};
}, function (_super) {
return function () {
_super.call(this);
};
});
});

err(function () {
expect(42).to.equal(42).pizza;
}, 'Invalid Chai property: pizza');
after(function () {
delete chai.Assertion.prototype.tmpProperty;
delete chai.Assertion.prototype.tmpMethod;
delete chai.Assertion.prototype.tmpChainableMethod;
});

// .then is excluded from property validation for promise support
expect(function () {
expect(42).then;
}).to.not.throw();
it('throws when invalid property follows expect', function () {
err(function () {
expect(42).pizza;
}, 'Invalid Chai property: pizza');
});

it('throws when invalid property follows language chain', function () {
err(function () {
expect(42).to.pizza;
}, 'Invalid Chai property: pizza');
});

it('throws when invalid property follows property assertion', function () {
err(function () {
expect(42).ok.pizza;
}, 'Invalid Chai property: pizza');
});

it('throws when invalid property follows overwritten property assertion', function () {
err(function () {
expect(42).tmpProperty.pizza;
}, 'Invalid Chai property: pizza');
});

it('throws when invalid property follows uncalled method assertion', function () {
err(function () {
expect(42).equal.pizza;
}, 'Invalid Chai property: equal.pizza. See docs for proper usage of "equal".');
});

it('throws when invalid property follows called method assertion', function () {
err(function () {
expect(42).equal(42).pizza;
}, 'Invalid Chai property: pizza');
});

it('throws when invalid property follows uncalled overwritten method assertion', function () {
err(function () {
expect(42).tmpMethod.pizza;
}, 'Invalid Chai property: tmpMethod.pizza. See docs for proper usage of "tmpMethod".');
});

it('throws when invalid property follows called overwritten method assertion', function () {
err(function () {
expect(42).tmpMethod().pizza;
}, 'Invalid Chai property: pizza');
});

it('throws when invalid property follows uncalled chainable method assertion', function () {
err(function () {
expect(42).a.pizza;
}, 'Invalid Chai property: pizza');
});

it('throws when invalid property follows called chainable method assertion', function () {
err(function () {
expect(42).a('number').pizza;
}, 'Invalid Chai property: pizza');
});

it('throws when invalid property follows uncalled overwritten chainable method assertion', function () {
err(function () {
expect(42).tmpChainableMethod.pizza;
}, 'Invalid Chai property: pizza');
});

it('throws when invalid property follows called overwritten chainable method assertion', function () {
err(function () {
expect(42).tmpChainableMethod().pizza;
}, 'Invalid Chai property: pizza');
});

it('doesn\'t throw if invalid property is excluded via config', function () {
expect(function () {
expect(42).then;
}).to.not.throw();
});
});

it('no-op chains', function() {
Expand Down
132 changes: 115 additions & 17 deletions test/should.js
View file
Open in desktop
Expand Up @@ -7,29 +7,127 @@ describe('should', function() {
should.not.equal('foo', 'bar');
});

it('invalid property', function () {
describe('invalid property', function () {
if (typeof Proxy === 'undefined' || typeof Reflect === 'undefined') return;

err(function () {
(42).should.pizza;
}, 'Invalid Chai property: pizza');
before(function () {
chai.util.addProperty(chai.Assertion.prototype, 'tmpProperty', function () {
new chai.Assertion(42).equal(42);
});
chai.util.overwriteProperty(chai.Assertion.prototype, 'tmpProperty', function (_super) {
return function () {
_super.call(this);
};
});

err(function () {
(42).should.be.pizza;
}, 'Invalid Chai property: pizza');
chai.util.addMethod(chai.Assertion.prototype, 'tmpMethod', function () {
new chai.Assertion(42).equal(42);
});
chai.util.overwriteMethod(chai.Assertion.prototype, 'tmpMethod', function (_super) {
return function () {
_super.call(this);
};
});

err(function () {
(42).should.be.a.pizza;
}, 'Invalid Chai property: pizza');
chai.util.addChainableMethod(chai.Assertion.prototype, 'tmpChainableMethod', function () {
new chai.Assertion(42).equal(42);
}, function () {
new chai.Assertion(42).equal(42);
});
chai.util.overwriteChainableMethod(chai.Assertion.prototype, 'tmpChainableMethod', function (_super) {
return function () {
_super.call(this);
};
}, function (_super) {
return function () {
_super.call(this);
};
});
});

err(function () {
(42).should.equal(42).pizza;
}, 'Invalid Chai property: pizza');
after(function () {
delete chai.Assertion.prototype.tmpProperty;
delete chai.Assertion.prototype.tmpMethod;
delete chai.Assertion.prototype.tmpChainableMethod;
});

// .then is excluded from property validation for promise support
(function () {
(42).should.then;
}).should.not.throw();
it('throws when invalid property follows should', function () {
err(function () {
(42).should.pizza;
}, 'Invalid Chai property: pizza');
});

it('throws when invalid property follows language chain', function () {
err(function () {
(42).should.to.pizza;
}, 'Invalid Chai property: pizza');
});

it('throws when invalid property follows property assertion', function () {
err(function () {
(42).should.ok.pizza;
}, 'Invalid Chai property: pizza');
});

it('throws when invalid property follows overwritten property assertion', function () {
err(function () {
(42).should.tmpProperty.pizza;
}, 'Invalid Chai property: pizza');
});

it('throws when invalid property follows uncalled method assertion', function () {
err(function () {
(42).should.equal.pizza;
}, 'Invalid Chai property: equal.pizza. See docs for proper usage of "equal".');
});

it('throws when invalid property follows called method assertion', function () {
err(function () {
(42).should.equal(42).pizza;
}, 'Invalid Chai property: pizza');
});

it('throws when invalid property follows uncalled overwritten method assertion', function () {
err(function () {
(42).should.tmpMethod.pizza;
}, 'Invalid Chai property: tmpMethod.pizza. See docs for proper usage of "tmpMethod".');
});

it('throws when invalid property follows called overwritten method assertion', function () {
err(function () {
(42).should.tmpMethod().pizza;
}, 'Invalid Chai property: pizza');
});

it('throws when invalid property follows uncalled chainable method assertion', function () {
err(function () {
(42).should.a.pizza;
}, 'Invalid Chai property: pizza');
});

it('throws when invalid property follows called chainable method assertion', function () {
err(function () {
(42).should.a('number').pizza;
}, 'Invalid Chai property: pizza');
});

it('throws when invalid property follows uncalled overwritten chainable method assertion', function () {
err(function () {
(42).should.tmpChainableMethod.pizza;
}, 'Invalid Chai property: pizza');
});

it('throws when invalid property follows called overwritten chainable method assertion', function () {
err(function () {
(42).should.tmpChainableMethod().pizza;
}, 'Invalid Chai property: pizza');
});

it('doesn\'t throw if invalid property is excluded via config', function () {
(function () {
(42).should.then;
}).should.not.throw();
});
});

it('no-op chains', function() {
Expand Down