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

fix: .toHaveValue() / .toHaveFormValues() on input type range now expect a number #365

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Okazari
Copy link

@Okazari Okazari commented May 12, 2021

What:

This PR aim to fix #364.

Why:

In my use case on a react app, i'm using a constant that is a number for a default value of an input type range. I couldn't use it in my test without hacking a bit since .toHaveValue() were expecting a string instead of a number.

How:

It makes jest-dom consider <input type="range /> value getter the same as <input type="number" />

Checklist:

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

I consider this change as a breaking change since it would break any test related to range inputs.

@Okazari Okazari marked this pull request as ready for review May 12, 2021 10:04
@Okazari
Copy link
Author

Okazari commented May 12, 2021

Hey there 👋 the PR template mention to Update type definitions but i didn't manage to find any in the repo. Could you point me were are those ?

Thanks 😄

@nickserv
Copy link
Member

They're in DefinitelyTyped.

@codecov
Copy link

codecov bot commented Jul 15, 2021

Codecov Report

Merging #365 (4f4774b) into main (948d90f) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 4f4774b differs from pull request most recent head bf8c8c5. Consider uploading reports for the commit bf8c8c5 to get more accurate results

@@            Coverage Diff            @@
##              main      #365   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines          630       616   -14     
  Branches       236       224   -12     
=========================================
- Hits           630       616   -14     
Impacted Files Coverage Δ
src/utils.js 100.00% <100.00%> (ø)
src/to-be-invalid.js 100.00% <0.00%> (ø)
src/to-be-visible.js 100.00% <0.00%> (ø)
src/to-have-class.js 100.00% <0.00%> (ø)
src/to-have-focus.js 100.00% <0.00%> (ø)
src/to-be-disabled.js 100.00% <0.00%> (ø)
src/to-have-accessible-name.js 100.00% <0.00%> (ø)
src/to-have-accessible-description.js 100.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@nickserv
Copy link
Member

This looks good, but should we leave the breaking changes for a major release or consider this a bug fix?

@gnapse
Copy link
Member

gnapse commented Jul 16, 2021

FWIW, I was planning to take some time next week to gather this and at least one other PR that are breaking changes, and make a single new major release with them all. I was also planning to include in this major release the removal of all the custom matchers that we already have marked as deprecated for various versions now. There are 2 or 3 of them.

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

Successfully merging this pull request may close these issues.

.toHaveValue expect string values for input type range
3 participants