issue 184: assert.property should pass for defined keys with undefined values #210

Closed
wants to merge 9 commits into
from

Conversation

Projects
None yet
9 participants

Fixes #184

Coverage Status

Coverage remained the same when pulling bd87d2c on devand123:master into f82f43f on chaijs:master.

Owner

vesln commented Nov 16, 2013

That's a great start. Please consider the following too:

var foo = { bar: { foo: undefined }};
assert.deepProperty(foo, 'bar.foo')

Unfortunately, it's currently failing.

Owner

logicalparadox commented Nov 19, 2013

I agree this behavior needs to be changed to support the existence of deep properties and not just undefined values. I think changes will need to be made to util/getPathValue.js in order for this to work properly. However, a little discussion first.

Building on @vesln 's example:

var obj = { foo: { bar: undefined }};
assert.deepProperty(obj, 'foo.bar'); // => should pass

This makes sense. The deep property exists even though it is undefined. But what about:

/* a */ assert.deepProperty(obj, 'foo.bar.baz'); // => should fail?
/* b */ assert.deepProperty(obj, 'foo.bar[0]'); // => should fail?

I think a reasonable expectation here is that:

  • a: since bar is undefined, baz cannot exist. assertion failed
  • b: since bar is not an array, index 0 cannot exist. assertion failed

This is my opinion anyway. Pinging @vesln @domenic for your thoughts.

Also, @devand123: There is a lot of change/revert going on in your PR. Can you please squash all your commits into a single commit then force push to your fork? It will update this PR automatically.

devand123 added some commits Nov 15, 2013

issue 184: assert.property should not fail for a specified property o…
…n an object with a value of undefined.

convert to camel case styling.

Fixing test.

issue: 184 Make sure properties assigned with undefined value still test true for assert.property

change assertion back to original file

Revert "issue: 184 Make sure properties assigned with undefined value still test true for assert.property"

This reverts commit 8fd7cf6.

issue: 184 Make sure properties assigned with undefined value still test true for assert.property

Issue 184, allowing assert.property to not fail for undefined values for NON-deep objects.

Coverage Status

Coverage remained the same when pulling 3125bc3 on devand123:master into f82f43f on chaijs:master.

👍 for this. This behavior should be changed. But sadly this issue remains.

👍 👍 objects can have undefined properties, this should really be fixed

@@ -1699,8 +1699,11 @@ module.exports = function (chai, _) {
throw new Error(msg + _.inspect(obj) + ' has no ' + descriptor + _.inspect(name));
}
} else {
+ var finalValue = value || flag(this, 'deep') ?
@charlierudolph

charlierudolph Aug 18, 2014

Contributor

Don't include changes to this file. It will get updated on releases.

this.assert(
- undefined !== value
+ finalValue
@charlierudolph

charlierudolph Aug 18, 2014

Contributor

This variable should probably be named ok instead of finalValue.
That's how something similar is done elsewhere in the file.

Owner

keithamus commented Oct 24, 2014

Hey @devand123, this looks like a good PR. Thanks for adding some good test coverage. If you could follow @charlierudolph's points, and maybe add some more tests to verify that @vesln's code would work, then I think we could look to getting this merged 😄

Owner

keithamus commented Nov 6, 2014

@devand123 - do you have any time for this? It'd be great to see the PR updated with the above points ⬆️

Contributor

joshperry commented Nov 17, 2014

Wow, I didn't realize there was a PR for this already. I was just running into this problem doing some TDD and wrote #308. Happy to help with this if I can, I'd love to get fix into the mainline.

Owner

keithamus commented Dec 2, 2014

Closing this, as we went ahead with @joshperry's version. Thanks for all of your efforts with this though @devand123 - please don't let it put you off from making more great pull requests!

@keithamus keithamus closed this Dec 2, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment