-
-
Notifications
You must be signed in to change notification settings - Fork 698
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.
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
assert: add nestedInclude, deepNestedInclude, ownInclude and deepOwnInclude #964
assert: add nestedInclude, deepNestedInclude, ownInclude and deepOwnInclude #964
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zetamorph Thanks so much for working on this! The changes look very clean. I have one small request to test the custom message feature for negated assertions (which it looks like some of our existing tests failed to do; feel free to add those too in a separate commit, otherwise we'll tackle in another PR :D).
test/assert.js
Outdated
}, "expected { a: { b: [ 'x', 'y' ] } } to have nested property 'a.c'"); | ||
|
||
err(function () { | ||
assert.notNestedInclude({a: {b: ['x', 'y']}}, {'a.b[1]': 'y'}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a third arg 'blah'
here so that assert.notNestedInclude
has at least one test verifying that custom messages work. (I think it's an oversight that some of the existing test blocks like .notDeepInclude
don't have that).
test/assert.js
Outdated
}, "expected { a: { b: [ [Object] ] } } to have deep nested property 'a.c'"); | ||
|
||
err(function () { | ||
assert.notDeepNestedInclude({a: {b: [{x: 1}]}}, {'a.b[0]': {x: 1}}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here with adding a third arg 'blah'
.
test/assert.js
Outdated
}, "expected { a: { b: 2 } } to have deep own property 'toString'"); | ||
|
||
err(function () { | ||
assert.notDeepOwnInclude({a: {b: 2}}, {a: {b: 2}}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here with adding a third arg 'blah'
.
test/assert.js
Outdated
}, "expected { a: 1 } to have own property 'toString'"); | ||
|
||
err(function () { | ||
assert.notOwnInclude({a: 1}, {a: 1}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here with adding a third arg 'blah'
.
Thanks! I added the custom messages. I will also do the same for the other tests. |
LGTM! Let's get another set of eyes on this before merging though: @vieiralucas @lucasfcosta @keithamus @shvaikalesh |
LGTM except for naming: maybe we should come up with something more descriptive than |
@shvaikalesh I was initially trying to come up with something specific to these methods that´s still not too long, but failed. You´re right in that haystack and needle are much more descriptive, I will change it, then the documentation for include-related methods is also more uniform. |
Assert: made documentation more descriptive
LGTM |
I added these methods and their negated versions to the assert interface and adapted the expect-style tests to assert-style as discussed in this issue: #905.