issue 184: assert.property should pass for defined keys with undefined values #210
Conversation
…n an object with a value of undefined.
…est true for assert.property
… still test true for assert.property" This reverts commit 8fd7cf6.
…est true for assert.property
|
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. |
|
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 Building on @vesln 's example: var obj = { foo: { bar: undefined }};
assert.deepProperty(obj, 'foo.bar'); // => should passThis 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:
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. |
…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.
|
👍 for this. This behavior should be changed. But sadly this issue remains. |
|
👍 👍 objects can have |
| @@ -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') ? | |||
There was a problem hiding this comment.
Don't include changes to this file. It will get updated on releases.
|
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 😄 |
|
@devand123 - do you have any time for this? It'd be great to see the PR updated with the above points ⬆️ |
|
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. |
|
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! |
Fixes #184