Fix properties with `undefined` value pass property assertion #308

Merged
merged 7 commits into from Dec 2, 2014

Projects

None yet

4 participants

@joshperry
Contributor

I was doing some interface assertions and noticed that properties that have a value of undefined assert as not existing. This is an initial hack at how I think this could be resolved. I just wanted to get this up and get some suggestions and comments to tighten this up.

var obj = { foo: undefined };
Object.defineProperty(obj, 'bar', {
    get: function() { }
});
// These both assert without this change
obj.should.have.property('foo');
obj.should.have.property('bar');

Fixes #184

@FredKSchott

+1 it looks like @devand123 stopped responding to comments in #210, happy to move discussion/work over to this PR

@keithamus keithamus commented on an outdated diff Nov 18, 2014
lib/chai/core/assertions.js
@@ -765,11 +765,20 @@ module.exports = function (chai, _) {
Assertion.addMethod('property', function (name, val, msg) {
if (msg) flag(this, 'message', msg);
+ function safeHasProperty(name, obj) {
+ return (typeof obj === 'object' || typeof obj === 'string' || typeof obj === 'function')
+ && name in (typeof obj !== 'string' ? obj : new String(obj))
+ }
+
@keithamus
keithamus Nov 18, 2014 Member

Can't this function be defined outside the addMethod anon function?

@keithamus keithamus commented on an outdated diff Nov 18, 2014
lib/chai/utils/getPathValue.js
+ } else {
+ return _getPathValue(parsed, obj);
+ }
+};
+
+function _getPathPropertyName(parsed) {
+ var part = parsed[parsed.length - 1];
+ if(part.p) {
+ return part.p;
+ } else if(part.i) {
+ throw new Error('The provided path is an array index, not a property');
+ }
+}
+
+var getPathParent = function(parsed) {
+ return _getPathParent(parsed, obj);
};
@keithamus
keithamus Nov 18, 2014 Member

This feels a bit confusing to me. Why is getPathParent() calling out to _getPathParent(). Can't they be the same method?

@keithamus
Member

Good work @joshperry 😄. I'd like to see a couple of comments addressed above, but largely this is looking in better shape than #210

@joshperry
Contributor

I wasn't handling array indexes terminating the path, I figured an array index isn't technically a property. However, it looks like the documentation sells this, so I added the code in the documentation to the unit tests and added this functionality.

I also did some deduplication and cleanup of the code.

@keithamus keithamus and 2 others commented on an outdated diff Nov 19, 2014
lib/chai/core/assertions.js
@@ -765,11 +765,21 @@ module.exports = function (chai, _) {
Assertion.addMethod('property', function (name, val, msg) {
if (msg) flag(this, 'message', msg);
+ function safeHasProperty(name, obj, isArrayIndex) {
+ return (isArrayIndex && obj && ('length' in obj) && obj.length > name) ||
+ ((typeof obj === 'object' || typeof obj === 'string' || typeof obj === 'function') &&
+ name in (typeof obj !== 'string' ? obj : new String(obj)))
+ }
+
@keithamus
keithamus Nov 19, 2014 Member

Still looks like this function could be defined outside of Assertion.addMethod('property', function (name, val, msg) { (i.e. above line 765).

@joshperry
joshperry Nov 19, 2014 Contributor

I'm not against moving this outside, but it has no general applicability outside of this code. My own personal style is to put functions that are akin to variables inside of the scope where they are used, if it is more in-line with the project's style to have this outside then I'm cool with that.

@logicalparadox
logicalparadox Nov 19, 2014 Member

What you have done here is quite clever, however it is not very maintainable as its logic is quite dense. By moving it out of .addProperty you can provide first class documentation for its workings. Perhaps it might be relevant to add it as new utility and expose it for other plugin developers (just in case it has further use)?

Furthermore, any reason you are using typeof instead of our type-detect utility? I would also recommend just doing the type detection once to reduce overhead.

@joshperry
joshperry Nov 20, 2014 Contributor

Thanks for the feedback guys. I honestly wasn't trying to be clever, just adding functionality as I added more and more tests.

Ok, here's my plan:

  • A new util function getPathInfo that returns the property info struct including whether the property actually exists or not.
  • Keep getPathValue for back compat, but it can easily be implemented by calling getPropertyInfo to share the bulk of the code.

This will allow me to properly document the general functionality and make it available to others.

I'll take a look at the type utility you mentioned, I'm still getting introduced to the codebase.

@joshperry
Contributor

I think this wraps things up guys, let me know what you think.

@logicalparadox logicalparadox commented on an outdated diff Nov 21, 2014
lib/chai/utils/getPathValue.js
@@ -1,10 +1,12 @@
/*!
- * Chai - getPathValue utility
+ * Chai - getPathInfo utility
@logicalparadox
logicalparadox Nov 21, 2014 Member

This is still getPathValue...

@logicalparadox logicalparadox commented on the diff Nov 21, 2014
lib/chai/utils/hasProperty.js
+};
+
+module.exports = function hasProperty(name, obj) {
+ var ot = type(obj);
+
+ // Bad Object, obviously no props at all
+ if(ot === 'null' || ot === 'undefined')
+ return false;
+
+ // The `in` operator does not work with certain literals
+ // box these before the check
+ if(literals[ot] && typeof obj !== 'object')
+ obj = new literals[ot](obj);
+
+ return name in obj;
+};
@logicalparadox
logicalparadox Nov 21, 2014 Member

Oh yeah, this looks much better!

@logicalparadox
Member

I'd like @keithamus to get eyes on this as well, but you got my 👍

@keithamus
Member

Sorry, must have missed this notification. Given it a thorough look over and it looks amazing to me. Great work @joshperry!

@keithamus keithamus merged commit 7de269a into chaijs:master Dec 2, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@keithamus keithamus referenced this pull request Feb 12, 2015
Closed

Get a proper changelog #366

@koulmomo koulmomo added a commit to yahoo/fluxible-action-utils that referenced this pull request Feb 26, 2015
@koulmomo koulmomo ⬆️ bump chai devdep to ^2.0
modify unit tests due to breaking changes from chai 1.10 -> 2.0
chaijs/chai#308
chaijs/chai#306
90e5ec1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment