-
Notifications
You must be signed in to change notification settings - Fork 406
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
Allow to use regex on toHaveDisplayValue #242
Allow to use regex on toHaveDisplayValue #242
Conversation
Codecov Report
@@ Coverage Diff @@
## master #242 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 21 21
Lines 281 289 +8
Branches 68 69 +1
=========================================
+ Hits 281 289 +8
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.
Looks good. I only have some requests to change the approach of reusability in tests and some naming. I also need some time to reflect about this:
This implementation comes with one (good?) side effect: unordered multiple values didn't match before it.
This means that this will be a breaking change, technically speaking, right? There's nothing wrong about that, just want to be sure because we need to release it as such and with a new major version.
cc @testing-library/core-maintainers can I have your opinion on this? See the PR description where this comment appears for the full context.
</select> | ||
`) | ||
describe('with multiple select', () => { | ||
let subject |
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.
We'd rather not share a variable across tests. No need for this or the afterEach
that resets it.
If you need to reuse the rendering part, extract it to a function in the module scope and call that function on each test that needs it.
src/to-have-display-value.js
Outdated
return expectedValue instanceof Array ? expectedValue : [expectedValue] | ||
} | ||
|
||
function getQtyOfMatchesBetweenArrays(arrayBase, array) { |
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'd rather have a fully readable name like getNumberOfMatchesBetweenArrays
. Same for the variables above that store this value.
Based on the comment above, it doesn't sound like a breaking change to me (unless someone was doing something very odd with this assertion). I think I'd be ok releasing this as a feature. |
I don't think so. There is nothing in the documentation talking about that. By the way, I thought it was working in this way. |
🎉 This PR is included in version 5.7.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
It closes #239.
All the current specs are kept. I just separate them in contexts to better describe the specs.
This implementation comes with one (good?) side effect: unordered multiple values didn't match before it.
Checklist: