Skip to content

onChange event not triggering function #175

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

Closed
alexwendte opened this issue Sep 12, 2018 · 21 comments
Closed

onChange event not triggering function #175

alexwendte opened this issue Sep 12, 2018 · 21 comments

Comments

@alexwendte
Copy link
Contributor

alexwendte commented Sep 12, 2018

  • react-testing-library version: 5.0.1
  • react version: 16.5.0
  • node version: 9.11.1 or 8.11.1 (tested to see if it made a difference)
  • npm (or yarn) version: 5.6.0

Relevant code or config:

// CostInput.js
import React from 'react';

const CostInput = ({ handleChange }) => (
  <div>
    <label htmlFor="cost">Item Cost</label>
    <input type="string" id="cost" onChange={handleChange} />
  </div>
);
export default CostInput;

// CostInput.test.js
import React from "react";
import { render, cleanup, fireEvent } from "react-testing-library";
import CostInput from "./CostInput";

afterEach(cleanup);
it("handleChange function called", () => {
  const spy = jest.fn();
  const { getByLabelText } = render(<CostInput handleChange={spy} />);
  let input = getByLabelText("Item Cost");
  expect(input.value).toBe("");
  fireEvent.change(input, { target: { value: "23" } });
  expect(spy).toHaveBeenCalledTimes(1);
});

What you did:

I wrote a test to check that the handleChange function would be called. I also ran the same code on CodeSandbox at https://codesandbox.io/s/jvw4499xzw.

What happened:

The test fails on my local environment which is a CRA application that only has those 2 files in it.
It works on the Code Sandbox without a problem.

Reproduction:

The working Code Sandbox can be found here: https://codesandbox.io/s/jvw4499xzw
The broken repo is found here: https://github.com/themostcolm/react-testing-bug
If possible, please create a repository that reproduces the issue with the
minimal amount of code possible.

Problem description:

It is making my app untestable.

Suggested solution:

I honestly have no idea, I'm sorry. I tried using waitForElement, but it didn't make a difference.

@kentcdodds
Copy link
Member

Weird. Thanks for reporting this. Could you do a little digging to figure out what's going on?

@alexwendte
Copy link
Contributor Author

I'm happy to do that, where would you recommend I start? I have been trying to figure out what is wrong for a few hours now. I'm not sure what to do next.

@gnapse
Copy link
Member

gnapse commented Sep 12, 2018

When you say you tried using waitForElement what exactly did you mean? Because I was thinking to try putting the toHaveBeenCalled expectation inside a await wait(() => { ... }) just to make sure is not something about jsdom having a tick delay in calling the change handler. Other than that, no idea.

@alexwendte
Copy link
Contributor Author

I now have the following test:

it('handleChange function called', async () => {
  const spy = jest.fn();
  const { getByLabelText } = render(<CostInput handleChange={spy} />);
  let input = getByLabelText('Item Cost');
  expect(input.value).toBe('');
  fireEvent.change(input, { target: { value: '23' } });
  await wait(() => {
    expect(spy).toHaveBeenCalledTimes(1);
  });

It still works on the code sandbox but not locally.

@zachboatwright
Copy link

I can confirm that I'm having the same problem. I assumed I was doing something incorrectly until I saw this issue. I also attempted to use waitForElement with no luck.

@alexwendte
Copy link
Contributor Author

alexwendte commented Sep 12, 2018

@SZBoatwright what operating system are you using? I am using Windows 10.

@denchen
Copy link

denchen commented Sep 13, 2018

I'm having issues with fireEvent.change() as well, but my errors are different. I'm adding my comment here in the hopes that they're somehow related.

The following is my test for my component called <Input>, which is a thin wrapper around <input> (it more or less adds an outer div with some styling):

const onChange = jest.fn();
const { getByTestId } = render(
  <Input name="test-input" value="old value" onChange={onChange} />
);
const inputNode = getByTestId('input-node'); // The actual <input>
expect(onChange).toHaveBeenCalledTimes(0);
fireEvent.change(inputNode, { target: { value: 'new value' } });
expect(onChange).toHaveBeenCalledTimes(1);

The above test passes. However, if I change the last line to:

expect(onChange).toHaveBeenCalledWith({ target: { value: 'new value' } });

Then I get a ton of errors all of the form:

Warning: This synthetic event is reused for performance reasons. If you're seeing this, you're
accessing the property `nativeEvent` on a released/nullified synthetic event. This is set to null.
If you must keep the original synthetic event around, use event.persist().
See https://fb.me/react-event-pooling for more information.

The difference in all the errors is the property. So the above error is for "nativeEvent", then there's one for "target", "currentTarget", "eventPhase", etc.

Out of curiosity, I added a wait() to my test, and I get even more of the same errors as before.

@zachboatwright
Copy link

@themostcolm I'm on a mac with Sierra. I have a desktop with Windows 10 on it that I could try on as well.

@kentcdodds
Copy link
Member

@denchen your issue is likely unrelated. Read the information in the error message link.

I'll try to give this a look when I get the chance, but probably not until next week.

@Enikol
Copy link
Contributor

Enikol commented Sep 17, 2018

I seem to have a similar problem: I can't trigger onChange handler on my checkbox component.
My component (I use an action from mobx store for handling change):

    <input className={this.props.styles['checkbox__input']}
        data-testid='checkbox'
        type='checkbox' 
        checked={this.props.store.current}
        disabled={this.props.disabled}
        onChange={(event) => this.props.store.change(event.target.checked)}/>

Store:

    @action
    change = (value) => {
        console.log('I ran') //doesn't run during tests
        this.current = value;
    }

My tests:

    test("Checkbox changes value", () => {
        const store = new CheckboxStore({});
        const { getByTestId } = render(<Checkbox store={store} />);
        console.log(getByTestId("checkbox").checked); //logs false
        fireEvent.change(getByTestId("checkbox"), { target: { checked: true } });
        console.log(getByTestId("checkbox").checked); // logs true, but the handler wasn't called
        expect(store.current).toBe(true); //logs false

I also tried changing the attribure by hand and calling fireEvemt afterwards:

    console.log(getByTestId("checkbox").checked); //false
    getByTestId("checkbox").checked = true;
    console.log(getByTestId("checkbox").checked); //true
    fireEvent.change(getByTestId("checkbox")); //no effect

The component itself works fine. If I'm doing something wrong, I'd appreciate the help with figuring it out!

@Gpx
Copy link
Member

Gpx commented Sep 17, 2018

Hey @Enikol, I tried to reproduce your code and it works for me.
https://codesandbox.io/s/oj5lk0qk6y?module=%2Fsrc%2Findex.test.js

Is it possible that the change action happens asynchronously?

@Enikol
Copy link
Contributor

Enikol commented Sep 17, 2018

Hi @Gpx ! Thank you for taking time to dig into this. No, I don't think it's possible, but I tried wrapping it in wait just to be sure:

test("Checkbox changes value", async () => {
  const { getByTestId } = render(<Checkbox store={store} />);
  console.log(getByTestId("checkbox").checked);
  fireEvent.change(getByTestId("checkbox"), { target: { checked: true } });
  await wait(() => expect(store.current).toBe(true))
  console.log(getByTestId("checkbox").checked);
});

Test just fails because of the 5ms timeout, because change never runs.
Since your codesandbox is a little different from my code due to the absence of mobx store, I created my own sandbox (should've thought of it sooner, thanks fot the idea!), and no, it still doesn't work https://codesandbox.io/s/w75qnpoppk
The event handler is never called. I put a console.log in the onChange callback function and it never shows up. I also fork your codesandbox and put a console.log in the same event hadler; it's also never called (I think, the reason your test passes might be because you don't actually check state of the app, just console.log checked attribute).

@kentcdodds
Copy link
Member

Ok, so you are all probably experiencing slightly different problems.

@themostcolm, your issue is that your CostInput renders an input which has a type of string but that's not a supported value for type (you probably want text). Changing it to text makes everything work fine: https://codesandbox.io/s/zn0wzonq94

@SZBoatwright, my guess is you're experiencing the same issue.

@denchen, the reason you're getting all those errors is because jest is reading properties from the Synthetic Event that React called your event handler with and that synthetic event has already been returned to the pool (learn more about the synthetic event system here: https://www.youtube.com/watch?v=dRo_egw7tBc&list=PLV5CVI1eNcJi8sor_aQ2AzOeQ3On3suOr). The solution is to not make an assertion about how your event handler was called because that's an implementation detail. Assert on the DOM output, how the your prop function callbacks were called, or how other modules were called.

@Enikol, this should probably be documented better, but with checkboxes you don't actually fire change events, you should fire click events instead. Would you like to open a pull request to document that in the README?

Thanks everyone! I'm going to go ahead and close this now :)

kentcdodds pushed a commit that referenced this issue Sep 18, 2018
**What**:
Added a note about triggering `onChange` event handler on a checkbox to README.md

**Why**:
Discussed in #175 

**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
- [x] Tests"N/A"
- [x] Ready to be merged
      <!-- In your opinion, is this ready to be merged as soon as it's reviewed? -->
- [x] Added myself to contributors table
      <!-- this is optional, see the contributing guidelines for instructions -->
julienw pushed a commit to julienw/react-testing-library that referenced this issue Dec 20, 2018
**What**:

Move `CODE_OF_CONDUCT.md` from `other` to root of repo

**Why**:

`README.md` links to this file and expects to find it in the root.

**Checklist**:

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

denisw commented Feb 26, 2019

The solution is to not make an assertion about how your event handler was called because that's an implementation detail. Assert on the DOM output, how the your prop function callbacks were called, or how other modules were called.

It's certainly not an implementation detail when testing a reusable text input component, where the event being propagated is part of the component's interface contract. This is the problem I ran into today when trying to test a custom TextInput component. The only solutions I could find is:

  • Don't assert the event's contents (leaving part of the component contract untested).
  • Use event.persist() (negatively affects performance in production).
  • Change the component interface to pass the text value to the exposed callback rather than the event.

@kentcdodds
Copy link
Member

You could also make a component that uses the API you want to test and test that component instead. That has the benefit of making the tests better suited as documentation of intent.

@denisw
Copy link

denisw commented Mar 5, 2019

You could also make a component that uses the API you want to test and test that component instead. That has the benefit of making the tests better suited as documentation of intent.

@kentcdodds Thanks for the tip, this is basically what we did now.

@konstantin-tarasov-clearscale
Copy link

konstantin-tarasov-clearscale commented Jun 2, 2020

I seem to have a similar problem: I can't trigger onChange handler on my checkbox component.
My component (I use an action from mobx store for handling change):

@Enikol did you find a problem? I experience same issue with mobx and checkbox field now.
Jest react to mobx store changes.
But when I apply the mobx state to the checkbox jest doesn't behave right unless you fire events manually.

@MatanBobi
Copy link
Member

Hi @ktarasov-clearscale , did you see @kentcdodds answer about using click event?

@Enikol, this should probably be documented better, but with checkboxes you don't actually fire change events, you should fire click events instead. Would you like to open a pull request to document that in the README?

@konstantin-tarasov-clearscale
Copy link

konstantin-tarasov-clearscale commented Jun 2, 2020

@MatanBobi yes, I've seen that answer.
But why should I produce synthetic events?
I want to test the state of the checkbox based on mobx store's state.

Let's assume I have a scenario with a checkbox input which checked by default.
And to set this default state I use an observable variable in the store.

Do you think I should fire a click event to test that scenario?
I guess it is not the right way to test components.

@MatanBobi
Copy link
Member

MatanBobi commented Jun 2, 2020

@ktarasov-clearscale So let me just understand the scenario, you're talking about an initial value and not a default value.. You're getting your data from the server or something?
If that's just an initial value then you shouldn't trigger an event just to imitate it, you should try to resemble how the user will see the screen.
If you show a loader while loading you should wait for that loader to disappear and then check the checkbox value.

@konstantin-tarasov-clearscale
Copy link

konstantin-tarasov-clearscale commented Jun 2, 2020

@MatanBobi seems my issue is related to final forms as soon as final forms have own state.

No problems with mobx and jest. Just need to construct (find) selectors from wrapper element (provided by mount/render/shallow) to avoid sideeffects.

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

No branches or pull requests

10 participants