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

Validate fireEvent.change #764

Open
WilliamPriorielloGarda opened this issue Aug 11, 2020 · 6 comments
Open

Validate fireEvent.change #764

WilliamPriorielloGarda opened this issue Aug 11, 2020 · 6 comments
Labels
enhancement New feature or request

Comments

@WilliamPriorielloGarda
Copy link

WilliamPriorielloGarda commented Aug 11, 2020

  • @testing-library/react version: "@testing-library/react": "^10.4.8",
  • Testing Framework and version: "jest": "^26.3.0", "ts-jest": "^26.2.0",
  • DOM Environment: "@testing-library/jest-dom": "^5.11.2",
"react": "^16.13.0",

Relevant code or config:

FULL CODE SANDBOX: https://codesandbox.io/s/silly-hertz-1v1uq

The test file: 'BasicSearchElement.test.tsx'

import React, { SyntheticEvent } from "react";
import {
  BasicSearchElement,
  BasicSearchElementProps
} from "../src/BasicSearchElement";
import { render, RenderResult, fireEvent } from "@testing-library/react";

describe("BasicSearchElement with props", () => {
  test("this test, which does not pass a value to 'renderBasicSearchElement', passes", async () => {
    const mockInputValue = "mockInputValue";
    const handleChange = jest.fn();
    const renderedComponent: RenderResult = renderBasicSearchElement({
      onChange: handleChange
      //value: mockInputValue THIS IS OMITTED IN THE PASSING TEST
    });

    const input = await renderedComponent.findByTestId("BasicSearchElement");

    const mockTypingEvent: Partial<SyntheticEvent> = {
      target: { value: mockInputValue }
    };
    fireEvent.change(input, mockTypingEvent);
    //handleChange is called once when we don't pass a value in renderBasicSearchElement
    expect(handleChange).toHaveBeenCalledTimes(1);
  });
  test("this test, which DOES pass a value to 'renderBasicSearchElement', FAILS", async () => {
    const mockInputValue = "mockInputValue";
    const handleChange = jest.fn();
    const renderedComponent: RenderResult = renderBasicSearchElement({
      onChange: handleChange,
      value: mockInputValue //HERE WE PASS A VALUE
    });

    const input = await renderedComponent.findByTestId("BasicSearchElement");

    const mockTypingEvent: Partial<SyntheticEvent> = {
      target: { value: "mockInputValue" }
    };
    fireEvent.change(input, mockTypingEvent);
    //handleChange is called once when we don't pass a value in renderBasicSearchElement
    expect(handleChange).toHaveBeenCalledTimes(1);
  });
});

function renderBasicSearchElement(
  props: Partial<BasicSearchElementProps> = {}
) {
  // @ts-ignore
  return render(<BasicSearchElement {...props} />);
}

What you did:

There are two tests defined in the testing file. In the first test, we do NOT pass a value to the input component. See line 14
//value: mockInputValue THIS IS OMITTED IN THE PASSING TEST

In the second test, we DO pass a value to the input component. See line 31
value: mockInputValue //HERE WE PASS A VALUE

What happened:

The first test passes the condition on line 24 (expect(handleChange).toHaveBeenCalledTimes(1);).
That is, the handleChange callback is called once after we call fireEvent.change(input, mockTypingEvent);

The second test fails the same condition, but on line 41. That is, the handleChange callback is NOT called after our fireEvent.change(input, mockTypingEvent);. The official failure message is: expect(jest.fn()).toHaveBeenCalledTimes(expected) Expected number of calls: 1 Received number of calls: 0

The only difference between the tests is whether or not we decide to pass a value to our input element (lines 14 and 31). That is, if we specify a "value" prop for the input element, the handleChange callback is not executed

Reproduction:

FULL CODE SANDBOX: https://codesandbox.io/s/silly-hertz-1v1uq

Problem description:

The handleChange callback should be called (after a call to fireEvent.change) even if we pass a value to the input element

Suggested solution:

I don't have time right now to investigate this more.

@eps1lon
Copy link
Member

eps1lon commented Aug 11, 2020

If you don't dispatch a change event with a changed value no onChange will be called. You have to pass a different value:

const mockTypingEvent: Partial<SyntheticEvent> = {
-  target: { value: "mockInputValue" }
+  target: { value: "changed-value" }
};
fireEvent.change(input, mockTypingEvent);
//handleChange is called once when we don't pass a value in renderBasicSearchElement
expect(handleChange).toHaveBeenCalledTimes(1);

The first test is passing because you do call it with a changed value.

I hope that helps.

@eps1lon eps1lon closed this as completed Aug 11, 2020
@WilliamPriorielloGarda
Copy link
Author

@eps1lon Figured it was something I was doing. Thanks a lot!

@roblevintennis
Copy link

roblevintennis commented Oct 21, 2020

@eps1lon point really helped me after hitting my head against the wall. Here's the vanilla JS working spec (extracted from Typescript example):

test('onChange fires', async () => {
  const changeHandler = jest.fn()
  const { getByRole } = render(<Input label="label is require prop haha" uniqueId="myUniqId" onChange={changeHandler} />);
  const input = getByRole('textbox')
  const mockTypingEvent = {
    target: {
      value: "changed-value"
    }
  };
  await fireEvent.change(input, mockTypingEvent)
  expect(changeHandler).toHaveBeenCalledTimes(1)
})

I feel like this should be documented more closely to what actually works but maybe it was me skipping over the docs that are already there I dunno ¯_(ツ)_/¯

@eps1lon
Copy link
Member

eps1lon commented Oct 21, 2020

I think there are some pre-flight checks we could do:

  • check if the value is actually different
  • check if the value makes sense for that input[type]

We can probably catch the most common issues while neglecting more complext components like <div contentEditable />.

I'm re-opening until we've done some investigation whether this is viable because, as far as I can tell, fireEvent.change is a common footgun.

@eps1lon eps1lon reopened this Oct 21, 2020
@eps1lon eps1lon added the enhancement New feature or request label Oct 21, 2020
@eps1lon eps1lon changed the title fireEvent.change(input, event) does not trigger 'handleChange' callback if we also specify an input value Validate fireEvent.change Oct 21, 2020
@radfahrer
Copy link

Is the solution here to have fireEvent.change throw an error if the value is the same and the existing value?

@nickmccurdy
Copy link
Member

I think warning the user is the best approach here. An error could potentially break tests that are written with this change cancellation in mind, and React doesn't error about this in production so it could be surprising. But it's also something I could see as confusing if it happened by accident.

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

No branches or pull requests

5 participants