peacechen / react-native-modal-selector

A cross-platform (iOS / Android), selector/picker component for React Native that is highly customizable and supports sections.
MIT License
369 stars 129 forks source link

The testID story can be improved #77

Open luisnaranjo733 opened 6 years ago

luisnaranjo733 commented 6 years ago

The way that test automation frameworks like Detox work is that they match native views based on the testID prop, which RN maps to the native view. This facilitates the writing of tests that are less brittle, because components can be matched via a non-content/non-accessibility string. So that changing your strings won't royally break your tests.

react-native-modal-selector partially accomplishes this with the passThruProps prop, but I think there is some room for improvement here.

Requirements: 1) There should be a way to pass the testID prop to the trigger button that you tap to bring up the modal. This way the component can be found and tapped by test automation. 2) There should be a way to pass the testID prop to the that appears in the modal. This way the component can be found and scrolled by test automation. 3) There should be a way to pass a unique testID prop to the individual option components. This way the individual options can be selected by test automation. 4) Each testID should be unique. react-native-modal-selector shouldn't force you to pass the same prop to the components for requirements 1, 2, and 3. The whole point of testID is that it is a unique cross-platform identifier that doesn't mess with accessibility.

How react-native-modal-selector stacks up to these requirements today: 1) It is possible to pass testID to the trigger button via the passThruProps, but this still violates requirement 4. 2) There is no way to pass testID to the ScrollView, so it is not possible to scroll from a test automation standpoint 3) There is a way to pass testID to the individual option components, but it is the same testID that is passed to the trigger button, so this also violates requirement 4. 4) The implementations for 1 and 3 violate this requirement because the same testID prop is spread to the components.

peacechen commented 6 years ago

Thanks @luisnaranjo733 for the detailed explanation. Would you be interested in creating a PR to implement this?

My initial thought is that there are too many unique testID props to pass in individually. What about specifying a prefix prop which gets prepended to predefined IDs? For example, if testIdPrefix={'myComponentA'}, the trigger button's testID would become myComponentA-triggerButton. If there are multiple modal selectors, pass in testIdPrefix={'myComponentB'} and so on.

luisnaranjo733 commented 6 years ago

I'm interested in creating a PR for this, but I can't give an ETA (tied up with other stuff at the moment for at least another week or two).

I like that idea. That way we could pass a unique testID to the trigger button and to the scroll view based on the same prefix (that solves requirements 1 & 2).

To solve 3 and 4, what do you think about changing the shape of the data object to use an optional testID key/value pair? I think that custom keys are already supported and passed to the onChange callback. Is it possible to pass testID to TouchableOpacity if it is present on that option?

For instance: { key: index++, label: 'Cherries', testID: "cherriesOption" },

would end up as <TouchableOpacity testID={"cherriesOption"} />

peacechen commented 6 years ago

That should be straight-forward. Add it to the TouchableOpacity props, falling back to a prefixed version. For example at https://github.com/peacechen/react-native-modal-selector/blob/master/index.js#L164

<TouchableOpacity
    key={this.props.keyExtractor(option)}
    testId={option.testID || (this.props.testIdPrefix + '-' + option.label)}
    ...

It would help to add a default prop value for testIdPrefix so the testID doesn't end up as "undefined-Cherries"

Merlier commented 4 years ago

I'm interesting by adding testID for each options to support test automation. So I opened a PR: https://github.com/peacechen/react-native-modal-selector/pull/148

Hope it will help PS: Let me know about the PR if something breaks your rules as it's my first contribution :)

peacechen commented 4 years ago

Thanks @Merlier for submitting the PR. I'll review and provide feedback.

peacechen commented 4 years ago

I merged @Merlier 's PR 148 with minor tweaks. This is currently on the master branch (hopefully Github will rename that to main). Until the next version is published, you can use this in your app by pointing to this repo in package.json:

dependencies: {
  "react-native-modal-selector": "https://github.com/peacechen/react-native-modal-selector.git",
}
Merlier commented 4 years ago

I merged @Merlier 's PR 148 with minor tweaks. This is currently on the master branch (hopefully Github will rename that to main). Until the next version is published, you can use this in your app by pointing to this repo in package.json:

dependencies: {
  "react-native-modal-selector": "https://github.com/peacechen/react-native-modal-selector.git",
}

Really great! Cool 👍 Thanks @peacechen