Skip to content
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

Merged
merged 10 commits into from May 7, 2020
Merged

Allow to use regex on toHaveDisplayValue #242

merged 10 commits into from May 7, 2020

Conversation

lourenci
Copy link
Contributor

@lourenci lourenci commented May 6, 2020

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.

<select id="fruits" data-testid="select" multiple>
  <option value="">Select a fruit...</option>
  <option value="ananas" selected>Ananas</option>
  <option value="banana">Banana</option>
  <option value="avocado" selected>Avocado</option>
</select>

// This was not true before this PR. It's true now.
expect(subject.queryByTestId('select')).toHaveDisplayValue([
  'Avocado',
  'Ananas',
])

Checklist:

  • Documentation
  • Tests
  • Updated Type Definitions
  • Ready to be merged

@codecov
Copy link

codecov bot commented May 6, 2020

Codecov Report

Merging #242 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #242   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines          281       289    +8     
  Branches        68        69    +1     
=========================================
+ Hits           281       289    +8     
Impacted Files Coverage Δ
src/to-have-display-value.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17787f3...1e63854. Read the comment docs.

@lourenci lourenci marked this pull request as ready for review May 6, 2020 23:23
Copy link
Member

@gnapse gnapse left a 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
Copy link
Member

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.

return expectedValue instanceof Array ? expectedValue : [expectedValue]
}

function getQtyOfMatchesBetweenArrays(arrayBase, array) {
Copy link
Member

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.

@kentcdodds
Copy link
Member

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.

@lourenci
Copy link
Contributor Author

lourenci commented May 7, 2020

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.

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.

@lourenci lourenci requested a review from gnapse May 7, 2020 20:41
@gnapse gnapse merged commit 5c9e8e5 into testing-library:master May 7, 2020
@gnapse
Copy link
Member

gnapse commented May 7, 2020

🎉 This PR is included in version 5.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

toHaveDisplayValue doesn't support regex
3 participants