callstack / react-native-testing-library

πŸ¦‰ Simple and complete React Native testing utilities that encourage good testing practices.
https://callstack.github.io/react-native-testing-library/
MIT License
3.1k stars 273 forks source link

fireEvent seems to always trigger event (on disabled TouchableOpacity) #28

Closed fwal closed 4 years ago

fwal commented 6 years ago

I'm trying to figure out why it seems like the fireEvent.press function always seem to trigger a callback even though it shouldn't be registered. My use case is that I'm trying to write a test that makes sure that onPress isn't called if the component is "disabled".

Trying to mimic the behaviour below...

  const MyComponent = _props => (
    <View>
      <TouchableHighlight testID={'touchable'}>
        <Text>{'Foo'}</Text>
      </TouchableHighlight>
    </View>
  )

  const pressedCallback = jest.fn()
  const { getByTestId } = render(<MyComponent onPress={() => pressedCallback('bar')} />)
  fireEvent.press(getByTestId('touchable'))
  expect(pressedCallback).not.toBeCalled() //Expected mock function not to be called but it was called with: ["bar"]
fwal commented 6 years ago

Hmm seems like fireEvent traverses up the tree and call the function if found, that would make my use-case a bit trickier... https://github.com/callstack/react-native-testing-library/blob/master/src/fireEvent.js#L18

thymikee commented 6 years ago

Yup, that's a desired behavior – user may press a text, which in fact may trigger a parent press event handler.

fwal commented 6 years ago

Any idea on how I could test my behaviour? I basically don't set onPress on the touchable component if my prop disabled is true...

thymikee commented 6 years ago

cc @Esemesek

thymikee commented 6 years ago

I think it makes sense to stop bubbling just before root component, that would prevent this and seems like a sane solution, but haven't thought it through yet

fwal commented 6 years ago

Ah I ~could~ probably should swallow the onPress event in the TouchableHighlight if disabled is true rather than omitting to set the prop... πŸ€”

fwal commented 6 years ago

For example this works:

  const MyComponent = ({ disabled, onPress }) => (
    <View>
      <TouchableHighlight onPress={disabled ? () => {} : onPress} testID={'touchable'}>
        <Text>{'Foo'}</Text>
      </TouchableHighlight>
    </View>
  )

But then again I'm changing implementation to satisfy the test πŸ˜…

thymikee commented 6 years ago

I usually do both just in case, but I guess we could detect the "disabled" prop on the Core RN components like Touchable* and native Button.

fwal commented 6 years ago

There's probably more cases like this, like onScroll and scrollEnabled in ScrollView. However In my button case I'll probably leave it and rely on that the core components are properly tested.

thymikee commented 6 years ago

Yup, we have an idea on how to make it more generic without changing implementation of fireEvent. Bare with us while we're fixing this.

thymikee commented 6 years ago

Just merged #30 so the bug is fixed now (not released yet). As for disabled prop, this is something that should be fixed in RN mocks tbh. We'll need to think about simulating native events capturing as close as possible though (e.g. including capturing phase).

will-stone commented 5 years ago

Hi @thymikee, thanks for all the hard work on the lib. I know this issue is closed and was a while ago but what was the conclusion on how to deal with a disabled prop? I'm struggling to get my test to pass.

thymikee commented 5 years ago

For test purposes you'll need to make sure by yourself that your events won't fire when disabled prop is passed, there's not really other way until the mocks are fixed.

We may, however, consider some special treatment for Touchable* components when in RN context (usual), as these are special and e.g. cancel-out event bubbling, unlike one the web where events propagate up freely unless stopped explicitly by the user.

will-stone commented 5 years ago

So this...

<Button onPress={() => {
  if(thing && otherThing) {
    this.props.onButtonPress()
  }
}} disabled={!thing || !otherThing}>
  Press Me
</Button>

is the way to go for now?

thymikee commented 5 years ago

I'd extract disabled to a variable since it's reused, but yes.

reaktivo commented 5 years ago

Hi @thymikee, I just bumped into this same issue myself. Is there any newer workaround that doesn't involve modifying the component itself? Can this ticket be reopened?

thymikee commented 5 years ago

You can mock the TouchableOpacity and pass it default implementation plus disabled prop set correctly.

We may introduce some special handling to disabled prop in touchable elements, e.g. as an option to fireEvent, but this is clearly a hack and we don't want to bake it into the library for good.

This is unfortunately a shortcoming of our event triggering system and we'd like to create something better, but on the other hand not scoped to React Native internals – and this is gonna be hard, provided no documentation of the event/touch system and platform inconsistencies. I'm however looking forward to what the React Flare project brings. If successful, would make it way easier for us to implement robust and cross-platform React event handling.

reaktivo commented 5 years ago

Thanks for the quick reply, your response is quite reasonable. For now as a temporary workaround I'm first checking the element's disabled prop and running my expectations based on that.

alexborton commented 5 years ago

We also just came across this issues. I have added a mock in the setup.js to keep it tucked away and ignore moving forward πŸ™„

jest.mock('TouchableOpacity', () => {
  const RealComponent = require.requireActual('TouchableOpacity')
  const MockTouchableOpacity = props => {
    const { children, disabled, onPress } = props
    return React.createElement(
      'TouchableOpacity',
      { ...props, onPress: disabled ? () => {} : onPress },
      children,
    )
  }
  MockTouchableOpacity.propTypes = RealComponent.propTypes
  return MockTouchableOpacity
})
swingywc commented 5 years ago

We also just came across this issues. I have added a mock in the setup.js to keep it tucked away and ignore moving forward πŸ™„

jest.mock('TouchableOpacity', () => {
  const RealComponent = require.requireActual('TouchableOpacity')
  const MockTouchableOpacity = props => {
    const { children, disabled, onPress } = props
    return React.createElement(
      'TouchableOpacity',
      { ...props, onPress: disabled ? () => {} : onPress },
      children,
    )
  }
  MockTouchableOpacity.propTypes = RealComponent.propTypes
  return MockTouchableOpacity
})

In my point of view, I don't think testing should override any component, and should not change the implementation to satisfy the test. @thymikee Is there any update on the disabled property in the fireEvent? Thank you.

thymikee commented 5 years ago

@swingywc no updates yet.

In my point of view, I don't think testing should override any component, and should not change the implementation to satisfy the test.

I see your point, but I have to disagree here. Sometimes, an inability to write a test for a piece of code indicates an accessibility issue with it – e.g. it's not usable by someone using a screen reader.

MattAgn commented 4 years ago

I stumbled upon the same issue and the best workaround I found was to use this jest extension for react native. It extends the jest "expect" function with helpers like .toBeDisabled, toBeEmpty, toHaveProp etc... Very useful!

sanjeevyadavIT commented 4 years ago

Yup, we have an idea on how to make it more generic without changing implementation of fireEvent. Bare with us while we're fixing this.

@thymikee Are you working on this bug???

I see your point, but I have to disagree here. Sometimes, an inability to write a test for a piece of code indicates an accessibility issue with it – e.g. it's not usable by someone using a screen reader.

I disagree with you, I don't understand how this falls under accessibility issue you are talking about, sending different function when disabled, is literally changing code just to pass my test, not because it is requirement, because I expect onPress to be disabled when pass disabled prop on Touch listener provided my react-native. ☺️

thymikee commented 4 years ago

@alexakasanjeev we're not currently working on this. But the proper place to fix it is the Touchable* components in RN core. I won't patch the library to work around specific logic that lives on the native side.

For example, there's a new Pressable component coming up, which may or may not have this issue. Shall we patch it then as well? What about other components? I hope you see my point. This is an upstream "bug" (or limitation) and I only leave it open for folks to be aware.

I can only advise to always wrap RN touchable components into custom components which you can control. This will also make touchables unified across your app, so it's worth doing anyway. A sample implementation: https://github.com/react-native-community/react-native-platform-touchable/blob/master/PlatformTouchable.js of such primitive

TDAK1509 commented 4 years ago

I test the disable status by using snapshot to check the style. (When disabled, my button has a different color)

    // Default is disabled
    it("matches snapshot", () => {
      expect(wrapper).toMatchSnapshot();
    });

    // Button is enabled when an image is selected
    it("matches snapshot when image is selected", () => {
      const { getAllByTestId } = wrapper;
      const checkBox = getAllByTestId("checkbox");
      fireEvent.press(checkBox[0]);
      expect(wrapper).toMatchSnapshot();
    });

After doing this, in snapshot file there will be 2 cases for the button, which will be disabled state and enabled state with different colors.

<View
    accessible={true}
    focusable={true}
    onClick={[Function]}
    onResponderGrant={[Function]}
    onResponderMove={[Function]}
    onResponderRelease={[Function]}
    onResponderTerminate={[Function]}
    onResponderTerminationRequest={[Function]}
    onStartShouldSetResponder={[Function]}
    style={
      Object {
        "alignItems": "center",
        "borderRadius": 50,
        "bottom": 80,
        "justifyContent": "center",
        "opacity": 1,
        "position": "absolute",
        "right": 30,
        "zIndex": 10,
      }
    }
    testID="deleteButton"
  >
    <Text
      allowFontScaling={false}
      style={
        Array [
          Object {
            "color": "#948f8f", <--- grey for disabled state
            "fontSize": 40,
          },
          undefined,
          Object {
            "fontFamily": "anticon",
            "fontStyle": "normal",
            "fontWeight": "normal",
          },
          Object {},
        ]
      }
    >
      
    </Text>
  </View>
<View
    accessible={true}
    focusable={true}
    onClick={[Function]}
    onResponderGrant={[Function]}
    onResponderMove={[Function]}
    onResponderRelease={[Function]}
    onResponderTerminate={[Function]}
    onResponderTerminationRequest={[Function]}
    onStartShouldSetResponder={[Function]}
    style={
      Object {
        "alignItems": "center",
        "borderRadius": 50,
        "bottom": 80,
        "justifyContent": "center",
        "opacity": 1,
        "position": "absolute",
        "right": 30,
        "zIndex": 10,
      }
    }
    testID="deleteButton"
  >
    <Text
      allowFontScaling={false}
      style={
        Array [
          Object {
            "color": "#000", <--- black for enabled state
            "fontSize": 40,
          },
          undefined,
          Object {
            "fontFamily": "anticon",
            "fontStyle": "normal",
            "fontWeight": "normal",
          },
          Object {},
        ]
      }
    >
      
    </Text>
  </View>
thymikee commented 4 years ago

@TDAK1509 I encourage you to use https://github.com/jest-community/snapshot-diff in such cases :)

thymikee commented 4 years ago

Good news, we're fixing that in the next major release (which will involve rename to @testing-library/react-native).

Closed via #460

ManuViola77 commented 2 years ago

could it be this bug is back again? Because Im testing with the extended version that is disabled like this:

expect(something).toBeDisabled();

and this passes correctly but then I do this:

fireEvent.press(something);
expect(function).toHaveBeenCalledTimes(0);

and this fails saying it was called 1 time 😒

Im using "@testing-library/react-native": "^9.1.0"

Med-Amine-Fredj commented 1 year ago

Any idea on how I could test my behaviour? I basically don't set onPress on the touchable component if my prop disabled is true...

I was asking my self the same question , so for me i tried to test the prop it self if it is passed correctly passed :

it("should be disabled when the disabled prop is true", () => {
    const { getByTestId } = render(<AppButton title="Test" disabled={true} />);
    const button = getByTestId("AppButton-test");
    expect(button.props.accessibilityState.disabled).toBeTruthy();
  });

this is my AppButton Component :

const AppButton: React.FC<AppButtonProps> = ({
  title = "",
  onPress = () => {},
  disabled = false,
}) => {
  return (
    <TouchableOpacity
      testID="AppButton-test"
      disabled={disabled}
      onPress={onPress}>
      <AppText>
        {title}
      </AppText>
    </TouchableOpacity>
  );
};
mdjastrzebski commented 1 year ago

@Med-Amine-Fredj I would suggest using Jest Native matchers to improve the test code:

it("should be disabled when the disabled prop is true", () => {
    const { getByTestId } = render(<AppButton title="Test" disabled={true} />);
    const button = getByTestId("AppButton-test");
    expect(button).toBeDisabled();
  });
robertwt7 commented 4 months ago

This is still happening for me when using Pressable component? I am on version 11.4.0

How is everyone testing their disabled component now?