markusenglund / react-switch

A draggable toggle-switch component for React. Check out the demo at:
https://react-switch.netlify.com/
MIT License
1.33k stars 101 forks source link

onChange event still being triggered when disabled #98

Closed winsjos closed 3 years ago

winsjos commented 3 years ago

Hello, this component seems to be acting strange when I started writing tests around it. I pass in the disabled property set to true, and I'm expecting the onChange callback to not be called, or if it is called, to be called with checked set to false. But what I'm seeing is onChange is being called, and checked is true. Maybe this is expected behavior? Let me know if I'm not using this component correctly.

Here is my test suite: Using Jest and React Testing Library

describe("React-Switch", () => {
  it("Disabled test", () => {
    const testId = "switch";
    let spy = jest.fn();
    const { getByTestId } = render(<ReactSwitch checked={false} onChange={(checked) => spy(checked)} disabled={true} data-testid={testId} />);

    fireEvent.click(getByTestId(testId));

    expect(spy).not.toHaveBeenCalled();
    // Fails - Received number of calls: 1
  })

  it("Disabled test 2", () => {
    const testId = "switch";
    let spy = jest.fn();
    const { getByTestId } = render(<ReactSwitch checked={false} onChange={(checked) => spy(checked)} disabled={true} data-testid={testId} />);

    fireEvent.click(getByTestId(testId));

    expect(spy).toHaveBeenCalledWith(false);
    // Fails - Received: true
  })
})
djD-REK commented 3 years ago

Hi, I'm not a maintainer but would suggest that the test itself is fragile. Wouldn't it be better to test something closer to the user behavior, like what's supposed to happen when the switch is clicked on, or just the internal state (disabled) of the switch? Just my 2 cents. See also https://kentcdodds.com/blog/stop-mocking-fetch

markusenglund commented 3 years ago

Does this impact actual human users? If so, could you fork this sandbox and reproduce the issue?

joeyfigaro commented 3 years ago

Hi, I'm not a maintainer but would suggest that the test itself is fragile. Wouldn't it be better to test something closer to the user behavior, like what's supposed to happen when the switch is clicked on, or just the internal state (disabled) of the switch? Just my 2 cents. See also https://kentcdodds.com/blog/stop-mocking-fetch

I don't think OP mocked fetch anywhere, did they?

winsjos commented 3 years ago

@joeyfigaro I have not mocked fetch anywhere, the component is not using fetch to fetch any data.

Just to clarify, the component works as intended in actuality. The event does not get triggered when the component is disabled as expected. Although, I'm not sure if it's React Testing Library or something faulty in this component, the behavior above is being observed while testing. This doesn't allow me to test my component that is using this component thoroughly(expecting events to not be triggered on click - fails). I just wanted to bring this to attention incase anyone else is seeing this behavior, and might be a simple fix.

winsjos commented 3 years ago

I will fork the repo and try to reproduce this by the end of the week, if not I will close the issue.

djD-REK commented 3 years ago

The OP is obviously not mocking fetch. But the point is you should never mock anything you don have to. The whole "issue" is that the mock isn't firing, which is a complete non-issue. The OP should write a functional test seeing if the React app exhibits the correct behavior, not trying to inspect if Jest fires a mock. The article I linked is the closest that Kent C Dodds' blog has on the topic; for more than that you'd have to buy his Testing JavaScript course.