-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Array keyPath for toHaveProperty #5220
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5220 +/- ##
=======================================
Coverage 60.91% 60.91%
=======================================
Files 202 202
Lines 6731 6731
Branches 3 3
=======================================
Hits 4100 4100
Misses 2630 2630
Partials 1 1 Continue to review full report at Codecov.
|
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.
Nice, I'm surprised it just worked this way. Left some comments inlined.
CHANGELOG.md
Outdated
@@ -13,10 +13,17 @@ | |||
([#5166](https://github.com/facebook/jest/pull/5166)) | |||
* `[jest-config]` Allow configuration objects inside `projects` array | |||
([#5176](https://github.com/facebook/jest/pull/5176)) | |||
* `[jest-cli]` Add support to .toHaveProperty matcher to accept the keyPath |
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.
This should be [expect]
. Also, can you add backticks?
`[expect]` Add support to `.toHaveProperty` matcher to accept the keyPath
" | ||
`; | ||
|
||
exports[`.toHaveProperty() {pass: true} expect({"a": {"b": {"c": {"d": 1}}}}).toHaveProperty('a,b,c,d')' 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.
Btw, this is out of scope of this PR, but could you please remove this extra single-quote here?
Also, not super relevant, but this should say .toHaveProperty(['a','b','c','d'])
instead .toHaveProperty('a,b,c,d')
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.
I fixed the changelog and remove the extra single-quote. How would you prefer for the snapshot output be changed (if at all for this PR)? I tried using the stringify
util function but ended up with a different issue where the output looked like .toHaveProperty('["a","b","c","d"]')
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.
If I had to choose, I'd pick the stringified version (it's just a name of the test anyway)
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.
Taking a second look, it also had an adverse effect of adding additional double quotes to normal keypaths like so : exports['.toHaveProperty() {pass: false} expect("abc").toHaveProperty('"a.b.c"') 1']
. This might be better off fixed in the pretty-format
package in a separate PR to prevent really cluttering up the matcher tests with string parsing logic.
Hi. It seems that this fine feature lacks the update of flow type-def. toHaveProperty(propPath: string, value?: any): void; I'd post the update, but I'm at loss where's the correct place |
Flow-typed repo |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
Add support to
.toHaveProperty
matcher to accept the keyPath argument as an array of properties/indices.Discussion can be found at #3873
Test plan
Update the
.toHaveProperty
tests to test against array as the keyPath argument. Include test for testing that an object has a flattened property like:{'a.b.c.d': 1}
and against array indices.