feat: Add toHaveValue matcher #82 #90
Conversation
There was a problem hiding this comment.
I think I'm ok with all this, including what you decided about the checkbox and radio buttons. But I want to give it a second thought before approving, because right now I'm on a layover after missing a flight connection, and tired as hell. So just in case I'm not thinking clearly enough. In the mean time, I left a couple of comments on a small typo.
BTW, thanks for this!
|
@gnapse I have fix the typos. Another idea that I had for checkbox and radio inputs was to allow the user to pass a parent element of those elements: <div data-testid="contact">
<label><input type="radio" name="contact" value="email">Email</label>
<label><input type="radio" name="contact" value="phone">Phone</label>
<label><input type="radio" name="contact" value="post">Post</label>
</div>expect(getByTestId('contact')).toHaveValue('phone')But then probably you could just use Let me know what you think. BTW I see that one of the tests is failing (which is weird) - I will fix it once we agree on the final form of this PR |
|
What do we need to do to help get this across the finish line? |
|
I probably forgot to keep track of this and approve or request changes at the time. But now it seems there are conflicts, and also the build is failing for some reason (could it have something to do with the change of url of the github repo when moved to an org @kentcdodds? When I visit the travis details link above I get "We couldn't find the repository gnapse/jest-dom"). @lukaszfiszer sorry about not being on top of this. Can you please fix the conflicts? We'll figure out the travis issue in the mean time. |
|
@gnapse No worries. I will fix the conflicts over the weekend. |
|
@gnapse rebased against master and fixed the broken test. ready to merge |
extend-expect.d.ts
Outdated
| @@ -20,5 +20,6 @@ declare namespace jest { | |||
| text: string | RegExp, | |||
| options?: {normalizeWhitespace: boolean}, | |||
| ): R | |||
| toHaveValue(value?: string | string[]): R | |||
There was a problem hiding this comment.
Should this be value?: string | string[] | number?
README.md
Outdated
| ### `toHaveValue` | ||
|
|
||
| ```typescript | ||
| toHaveValue(value: string | string[]) |
There was a problem hiding this comment.
And should this one be value?: string | string[] | number?
src/__tests__/to-have-value.js
Outdated
| `) | ||
|
|
||
| expect(queryByTestId('with')).toHaveValue('foo') | ||
| expect(queryByTestId('with')).not.toHaveValue('bar') |
There was a problem hiding this comment.
Would it make sense to add a test case like this?
expect(queryByTestId('with')).toHaveValue();The corresponding .not.toHaveValue() case is already there for empty.
|
@jgoz Thanks for review. I've fixed the typings, added additional test cases and fixed the behaviour of |
|
@lukaszfiszer sorry for the delayed response, I missed the notification on this. Just two very minor issues that I can see, one of which is causing the CI failure. |
|
@jgoz fixed issues and squash-rebased against master. Hope to get it merged! |
README.md
Outdated
| ```javascript | ||
| const {getByTestId} = render(/* Rendered HTML */) | ||
|
|
||
| const textInput = getByTestId('[data-testid="input-text"]') |
There was a problem hiding this comment.
I think these should be: getByTestId('input-text') and so-forth.
There was a problem hiding this comment.
Good catch! Fixed.
|
🎉 This PR is included in version 3.5.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
What:
Add
toHaveValuematcherWhy:
As discussed in #82
How:
Reused a lot of existing logic from
toHaveFormValueand moved some internal functions toutils.jsso that they can be shared between both modules.I had a lot of thoughts on how to implement this matcher for
<input type="checkbox" />and<input type="radio" />, because those elements value can be different depending on other elements from the same group (sharing the samename). Finally I've decided to throw an error when a checkbox or radio input is passed, suggesting users to usetoHaveFormValueinstead. In the future, I think I would most make sense to implementtoBeCheckedmatcher - a perfect pair with totoHaveValuefor single form elements.Checklist: