Test upgrade #779
Test upgrade #779
Conversation
* Ignore some contents of the .yarn directory
* Rename input.spec.js to input.spec.jsx
* Remove @testing-library/react-hooks
|
The changes from #763 are not ready to be merged into main yet. I think it would be better to close this PR and create a new one when some of the other ongoing work in the dev branch has been completed. |
Sure, no problem. I will leave this open for now and keep making changes. You should still be able to see your comments and my responses. I marked them as resolved, but feel free to undo that.
Not planning on squashing anything 😄 |
|
|
||
| input.simulate('focus', focusEvent); | ||
| export function simulateFocusEvent(input) { | ||
| input.focus(); |
There was a problem hiding this comment.
Suggestion/Test Bug
So there are three cases how focus can happen.
- Due to mouse click on specific caret position. In which case the caret is placed at specific position. Which I think the usage is replaced now with simulateMousUpEvent.
- Due to tab press, in which case whole input value gets selected.
- Manual input.focus call, in which case also whole input value gets selected.
If we catering all the three cases with this util, we should have selectionStart and selectionEnd as param.
In case we are catering 2nd and 3rd, we need to manually set the selectionStart to 0 and selectionEnd at input.value.length
Reason: input.focus() in JSDom puts the caret position at the end, which is not same as browser.
|
@aashu16 Merged master and fixed some broken tests which was incorrect on master as well. Also, I see couple of eslint error for unused variables in testing files, maybe you can remove those as well. |
* The positive assertion makes more sense while still covering the negative assertion case.
* Key input now directly uses @test-library/user-event to type text into the input element being tested. Previously, in some cases, the function used `fireEvent.change` to update the value of the input element with a pre-computed value. * This change does not extend to "special keys" such as `Backspace` or `Delete`.
* The test now uses the `simulateKeyInput` function to set `input.selectionRange`. Pressing any key then replaces the selected text inside the input element.
* Added functions to select text inside an input element by, 1) double clicking inside it at a given position, and 2) pressing and dragging the mouse cursor.
* If `selectionEnd` is undefined, it is set to `selectionStart`, which matches the behaviour of `simulateKeyInput` function. * It also uses other functions to select text inside the input element instead of directly setting the `selectionRange`.
* The other `simulateFocus` should ideally be used only when necessary as clicking to focus handles the more general cases when the user takes an action to focus the input element.
* Add the todo annotation to four tests that are currently failing. These tests have been confirmed to fail in a browser upon manual testing. * Fix incorrect assertion in a test.
* Add a workaround to handle cases where `input.selectionStart != input.selectionEnd` by first removing the selected text in the input and then using `user.keyboard` to type the key pressed.
|
@s-yadav, I think this is just about ready to merge now. Let me know if you have any comments regarding the recent commits. (Please don't merge yet.) |
* Update node version. * Use the latest versions of setup-node and checkout actions. * Use `--frozen-lockfile` flag when running `yarn install`. * Update script to run the new test suite.
- Added better way to find change range which fixes issue around duplicate subsequent characters giving incorrect change range. - Remove findChangedIndex util, as it wasn't used anywhere. - Fix simulateKeyInput util - Fix incorrect test - Fix issue when complete text is selected and replaced with new value in pattern format. It was it was incorrectly pasting value.
|
Hey @aashu16, all changes look good. I think let's merge this as there are couple of bug fixes I was planning to do, and better to do with new tests. If there are any changes required we can do that on separate pr. Also, was making some changes to the core logic so wanted to do it on this branch only, seems like there was test for it, but earlier it was just giving false positives. In this branch, it was marked for todo. This are my changes. https://github.com/s-yadav/react-number-format/pull/779/files/91e8fd263a815f00d15f542a02dbad23c526ed6e..ca97625a2296f6fe39d33e9e4736e1cb7243cac2 |
Describe the issue/change
Add CodeSandbox link to illustrate the issue (If applicable)
Describe specs for failing cases if this is an issue (If applicable)
Describe the changes proposed/implemented in this PR
Link Github issue if this PR solved an existing issue
Example usage (If applicable)
Screenshot (If applicable)
Please check which browsers were used for testing