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

Form Inputs not updating after change event is fired #152

Closed
pcfields opened this issue Aug 3, 2018 · 21 comments · Fixed by testing-library/dom-testing-library#85
Closed
Labels
help wanted Extra attention is needed needs investigation

Comments

@pcfields
Copy link

pcfields commented Aug 3, 2018

  • react-testing-library version: 4.1.5
  • react version: 16.4.1
  • node version: 8.6.0
  • npm (or yarn) version: 6.2.0

Relevant code or config:

test('submits form successfully', async () => {

const { getByText, queryByText, getByLabelText, debug } = renderWithRouter(
<SignIn setCurrentUser={jest.fn()} isSignedIn={false} history={{}} />
);

const submitButton = getByText('Sign In');
const emailInput = getByLabelText('Email');
const passwordInput = getByLabelText('Password');

emailInput.value = 'test@test.com';
passwordInput.value = '12345678';

fireEvent.change(emailInput);
fireEvent.change(passwordInput);

debug();
/* 
In debug email has value of 'test@test.com' and password has a value of ' ' 
*/

fireEvent.click(submitButton);

const emailErrorNode = queryByText('Email is required');
const passwordErrorNode = queryByText('Password is required')

expect(emailErrorNode).toBeNull();
expect(passwordErrorNode).toBeNull();

});

What you did:

Test a sign in form by setting username to 'test@test.com' and password to '12345678'

What happened:

When updating 2 inputs in a test, only one input gets updated the second input's value is ' ' (empty)

Reproduction:

https://codesandbox.io/s/0xwn98wo1p

Problem description:

I can't test forms with more than one field change

Suggested solution:

@kentcdodds
Copy link
Member

Hi @pcfields,

Thanks for this. Please ping in here when you've added the minimal reproduction codesandbox. I definitely want to improve this so testing this kind of experience is more obvious.

FWIW, change is one of the events that we do some trickery for because the change event works differently with React than it does in a real browser. I'd love to do away entirely with this code: https://github.com/kentcdodds/react-testing-library/blob/9667967a085e438919fe3d02bc36761735bb2732/src/index.js#L50-L56

@pcfields
Copy link
Author

pcfields commented Aug 3, 2018

@kentcdodds I've updated the codesandbox https://codesandbox.io/s/0xwn98wo1p

@pcfields
Copy link
Author

pcfields commented Aug 3, 2018

Let me know how I can help you fix this? This is my first ever Github issue or contribution so i'm more than happy to help

@kentcdodds
Copy link
Member

Sounds great! Could you strip down your example to be as minimal as possible? The less code in there to reproduce the issue the better as it makes it easier for us to determine what could cause the issue.

@kentcdodds kentcdodds added help wanted Extra attention is needed needs investigation labels Aug 3, 2018
@pcfields
Copy link
Author

pcfields commented Aug 4, 2018

@kentcdodds I've stripped down the code. Let me know if this is enough.

@kentcdodds
Copy link
Member

Let's try for something even simpler: https://codesandbox.io/s/2pokxwn2y0

I try to reduce it down to the smallest possible amount of code to still reproduce the bug :)

@kentcdodds
Copy link
Member

Ok, so I'm pretty sure that if we just remove this code we're golden. I didn't put that code there and I didn't quite understand why we needed it in the first place. But if I remove it then that test case works. Can you try removing that code from your node_modules to see if that causes any issues for you?

@antsmartian
Copy link
Collaborator

antsmartian commented Aug 4, 2018

@kentcdodds Yes. But I remember, we had the discussion on the same lines of code before somewhere, I tried to search our issues/PR but no luck. Looks like Jomaxx have added these files, when we moved from React Simulate to native events. Not sure the reason behind it. @jomaxx any reasons?

@jomaxx
Copy link
Collaborator

jomaxx commented Aug 4, 2018 via email

@kentcdodds
Copy link
Member

Hi friends! So it doesn't quite do it for us. Test cases definitely fail because even though the value does change, the event handlers are not called. I've figured out a way to fix this for onChange, but still need to work on the other three. I think I want these changes in dom-testing-library instead though.

I've opened a PR for this in react-testing-library to show how it works: #153

@kentcdodds
Copy link
Member

kentcdodds commented Aug 4, 2018

@pcfields, if you'd like to open a PR that makes this work in dom-testing-library that'd be great. I think that's the right place for this change because even though it's kind of an implementation detail for react it doesn't impact how things work for other frameworks and I believe that Cypress (another framework-agnostic tool) handles this case.

We'll definitely want some good comments for this though...

kentcdodds pushed a commit to testing-library/dom-testing-library that referenced this issue Aug 4, 2018
Specifically this is so the change event can set the value of the node
in a way that works for React (and all other frameworks and
non-frameworks) nicely.

fixes: testing-library/react-testing-library#152
kentcdodds pushed a commit to testing-library/dom-testing-library that referenced this issue Aug 4, 2018
Specifically this is so the change event can set the value of the node
in a way that works for React (and all other frameworks and
non-frameworks) nicely.

fixes: testing-library/react-testing-library#152
kentcdodds pushed a commit to testing-library/dom-testing-library that referenced this issue Aug 4, 2018
Specifically this is so the change event can set the value of the node
in a way that works for React (and all other frameworks and
non-frameworks) nicely.

fixes: testing-library/react-testing-library#152
kentcdodds pushed a commit to testing-library/dom-testing-library that referenced this issue Aug 4, 2018
Specifically this is so the change event can set the value of the node
in a way that works for React (and all other frameworks and
non-frameworks) nicely.

fixes: testing-library/react-testing-library#152
kentcdodds pushed a commit to testing-library/dom-testing-library that referenced this issue Aug 4, 2018
Specifically this is so the change event can set the value of the node
in a way that works for React (and all other frameworks and
non-frameworks) nicely.

fixes: testing-library/react-testing-library#152
@pcfields
Copy link
Author

pcfields commented Aug 6, 2018

@kentcdodds I am only now seeing your message. What would you like me to do?

@kentcdodds
Copy link
Member

Could you review this: testing-library/dom-testing-library#85

Thanks!

kentcdodds pushed a commit to testing-library/dom-testing-library that referenced this issue Aug 6, 2018
Specifically this is so the change event can set the value of the node
in a way that works for React (and all other frameworks and
non-frameworks) nicely.

fixes: testing-library/react-testing-library#152
@kentcdodds
Copy link
Member

Ok, so this bug happened due to the way React works, and now dom-testing-library's fireEvent utility supports setting properties on the target element when you fire an event. This allows you to set the value at the time of the event and dom-testing-library will set it in a way that works around how React tracks input value changes.

Here's how you use it: https://codesandbox.io/s/github/kentcdodds/react-testing-library-examples/tree/master/?module=%2Fsrc%2F__tests__%2Fon-change.js&previewwindow=tests

Here's the basic example:

fireEvent.change(passwordInput, {target: {value: '12345678'}})

🎉

Thanks! Now we gotta figure out how to help people avoid this in the future 🤔

@noinkling
Copy link

Is that now the recommended way to change an input value then? Should the readme be updated?

@kentcdodds
Copy link
Member

Yes, if you're going to fire a change event then that's how the value should be changed. Otherwise manipulating the value should work fine.

@weyert
Copy link
Contributor

weyert commented Aug 13, 2018

Hmm, should this work with the latest version of this package on npm? It's not for working for me

@kentcdodds
Copy link
Member

Please be more specific @weyert. A reproduction would be helpful.

@weyert
Copy link
Contributor

weyert commented Aug 13, 2018

@kentcdodds Sorry, I am was goofing up! Checked the wrong form field! All working as expected. Amazingly awesome :D 🎉

@ankitsinghaniyaz
Copy link
Contributor

fireEvent.change(passwordInput, {target: {value: '12345678'}})

This should definitely be added to the README file.

@gnapse
Copy link
Member

gnapse commented Oct 18, 2018

It is in the readme. Search for the string "fireEvent.change" in the README, and you'll find this:

fireEvent.change(comment, {
  target: {value: 'Great advice, I love your posts!'},
})

julienw pushed a commit to julienw/react-testing-library that referenced this issue Dec 20, 2018
Hey, this fixes small typo in `README.md` in function parameters example for `getText`.

<!--
Thanks for your interest in the project. Bugs filed and PRs submitted are appreciated!

Please make sure that you are familiar with and follow the Code of Conduct for
this project (found in the CODE_OF_CONDUCT.md file).

Also, please make sure you're familiar with and follow the instructions in the
contributing guidelines (found in the CONTRIBUTING.md file).

If you're new to contributing to open source projects, you might find this free
video course helpful: http://kcd.im/pull-request

Please fill out the information below to expedite the review and (hopefully)
merge of your pull request!
-->

<!-- What changes are being made? (What feature/bug is being fixed here?) -->

**What**:

Documentation typo.

<!-- Why are these changes necessary? -->

**Why**:

So documentation is more precise :)

<!-- How were these changes implemented? -->

**How**:

<!-- Have you done all of these things?  -->

**Checklist**:

<!-- add "N/A" to the end of each line that's irrelevant to your changes -->

<!-- to check an item, place an "x" in the box like so: "- [x] Documentation" -->

- [x] Documentation
- [ ] Tests N/A
- [x] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? -->
- [ ] Added myself to contributors table <!-- this is optional, see the contributing guidelines for instructions -->

<!-- feel free to add additional comments -->
lucbpz pushed a commit to lucbpz/react-testing-library that referenced this issue Jul 26, 2020
Closes testing-library#152

BREAKING CHANGE: disabled or readonly inputs won't be typeable anymore
qijixin2 pushed a commit to qijixin2/dom-testing-library that referenced this issue Aug 26, 2022
Specifically this is so the change event can set the value of the node
in a way that works for React (and all other frameworks and
non-frameworks) nicely.

fixes: testing-library/react-testing-library#152
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed needs investigation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants