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

feature: fireEvent should provide generated event object #714

Closed
Natteke opened this issue Apr 14, 2021 · 10 comments
Closed

feature: fireEvent should provide generated event object #714

Natteke opened this issue Apr 14, 2021 · 10 comments
Labels
help wanted Extra attention is needed

Comments

@Natteke
Copy link

Natteke commented Apr 14, 2021

Describe the Feature

If I understand the behavior correctly, fireEven looks for properties like "onPress" / "onScroll" in the DOM and calls these functions, but does not create an event object. This is a strong difference from the actual behavior of the application.

I understand the potential difficulties in creating these objects in nodejs environment, but it is possible for example to create objects at least partially similar to real events. It's also possible to document which event properties are relevant and which are not.

Even the very fact that a callback will receive an object, will be closer to real situations.

Possible Implementations

  • Generate an event object and pass it to the fired event.
  • Fill the object with data from this event. All fields should be presented as in real event.
  • Fields for which no valid values can be created must be populated with fake values and documented

Problem example

// short example component 
interface Props {
    /* Button's value to return */
    value?: any;
    /* Press callback with passed event and value */
    onPress?: (value: any, event: GestureResponderEvent) => void;
}

const Button: FunctionComponent<Props> = ({ value, onPress }) => {
    const onButtonPress = useCallback(
        (event: GestureResponderEvent): void => {
            onPress?.(value, event);
        },
        [onPress, value],
    );

    return <TouchableWithoutFeedback onPress={onButtonPress} />
};

// short tests example
fireEvent(button, 'press');
expect(typeof onPress.mock.calls[0][1]).toBe(typeof 'object');
  • The last test will fail because the event is undefined
@AlanSl
Copy link

AlanSl commented Jan 18, 2022

This would be very useful.

It's possible to do a partial workaround by passing an additional arg to fireEvent; in your example:

fireEvent(button, 'press', {});
expect(typeof onPress.mock.calls[0][1]).toBe(typeof 'object');

...which is better than nothing, but is a flawed approach overall which can give false positives.

For example, TextInput's onChangeText does not pass an event, it only passes one text string. This example test will pass, but it does not reflect reality:

const onChangeText = jest.fn()
const { getByA11yLabel } = render(<TextInput accessibilityLabel="test" onChangeText={onChangeText} />)
fireEvent.changeText(getByA11yLabel('test'), 'some text', { test: 'event' })

// This passes, but real-life onChangeText doesn't get any extra args beyond the string
expect(onChangeText).toHaveBeenLastCalledWith('some text', { test: 'event' })

It'd be better for fireEvent to reflect reality, passing appropriate events to handlers that take them in real life and not passing them to those that don't.

@mdjastrzebski
Copy link
Member

It seems that RTL fireEvent implementation passes some default Event object in case user omitted the event.

We should match that if feasible.

@mdjastrzebski mdjastrzebski added the good first issue Good for newcomers label Aug 9, 2022
@AugustinLF
Copy link
Collaborator

Yes, I ran into the same problem a while ago, where reading some properties from an event handler broke my tests since I was not passing a fake event. Finding smart values is not trivial, but it's definitely worth pursuing.

@mdjastrzebski mdjastrzebski added the help wanted Extra attention is needed label Sep 26, 2022
@CodingItWrong
Copy link
Contributor

I am hoping to be able to implement this feature. I have a hackathon on Oct 13 where I'm hoping to make significant progress on this. It's possible my time will be pulled away with some workshops, though, so if someone else would like to work on this feature, don't let me stop you 👍 I will post updates here.

@mdjastrzebski
Copy link
Member

@CodingItWrong looking forward to your PR. If you make a meaningful partial progress, do not hesitate to submit a draft PR.

@CodingItWrong
Copy link
Contributor

In a hackathon today we ended up exploring how the arguments to callbacks should work. We haven’t looked into the structure of the fake event so far.

I’ve pushed up a draft PR that shows the cases we covered, and the form the code took so far.

This investigation uncovered two questions that need to be answered to land this work:

1. In What Scenarios to Pass the Generated Event Object

Which types of event should pass a generated event object to the handler and which should not? It seems that press and scroll should receive an event object argument, and changeText should not, since it receives text instead. Custom events should probably not receive an event object argument, because custom events are functions called from user code with arbitrary arguments. What about other built-in events—do all of them other than changeText receive an event object?

Even if we had an “allowlist” of event types that should receive the generated event object, there is the risk that someone would add a custom event to their component that matches the name of a built-in event type. Maybe the allowlist should to specify both the events and the component types; that should allow it to correctly simulate the cases where RN generates events.

2. Whether to Handle Multiple Event Handler Arguments

Currently the API allows an arbitrary number of arguments to be passed to fireEvent, and they are forwarded along to the event handler. Is this behavior we should preserve?

In all the examples I could find, events receive just one argument. Are there built-in RN events that receive multiple arguments?

It seems like at least custom events should be able to receive multiple arguments, because again they can be called in arbitrary ways. But if we are already differentiating custom and built-in events due to question 1 above, maybe it is more realistic to not pass along multiple arguments for built-in events.

@mdjastrzebski
Copy link
Member

@CodingItWrong thanks for taking care of this issue.

Couple of key points I've noticed while reviewing your code and researching RTL/DTL implementation here

  1. We generally try to match RTL/DTL behaviour to maintain familiar behaviour with our bigger brother. But we also need to account for things that make React Native different from React DOM.
  2. We try to match real RN behavior, so when in doubt observe events types/props/etc send by real RN app.
  3. DTL's seems to apply default events only for fireEvent.xxx call style, while ignoring it for fireEvent('xxx') style: see: https://github.com/testing-library/dom-testing-library/blob/edffb7c5ec2e4afd7f6bedf842c669ddbfb73297/src/events.js#L95
  4. DTL has a map of event names to event characteristics, like constructor name, default init values, etc. We could mimic that approach if feasible. It would allow us to add more event types in the future. That being said onChangeText seems to be an odd outlier and might deserve its own if in the implementation ;-)

Answering your questions:

  1. Generate object when user calls fireEvent.xxx() but not when calls fireEvent('xxx'). Nearly all event handlers, so for now press and scroll should probably receive some event types. Pls conduct some experiments how normal RN apps behaves for these events (on device/emu, not in jest tests). That should give use answer whether to use base Event type or some subtype/other type.

  2. I am not aware of any events not accepting a single event param. Imo we can support only a single arg for now, and refactor in case there would be such real case. For custom events users should be able to call fireEvent('name') with 2+ args, if they can do it atm.

@mdjastrzebski
Copy link
Member

Seems like DTL has pretty good docs:

We might also want to expose createEvent which seems to be a helper for creating event objects for fireEvent[event]().

@mdjastrzebski
Copy link
Member

@CodingItWrong It seems like progress on this issue has staled, so I plan to start working on it.

@mdjastrzebski mdjastrzebski changed the title FireEvent should provide generated event object featuer: fireEvent should provide generated event object Mar 16, 2023
@mdjastrzebski mdjastrzebski changed the title featuer: fireEvent should provide generated event object feature: fireEvent should provide generated event object Mar 16, 2023
@mdjastrzebski
Copy link
Member

Closing in favor of upcoming User Event feature.

@mdjastrzebski mdjastrzebski closed this as not planned Won't fix, can't repro, duplicate, stale Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants