add tests to should not fire on non-editable TextInput getting by testID

[douglasjunior]

Summary

Add tests to simulate the problem related in issue #476

If you get the component instance using getByTestID the editable={false} is ignored.

Test plan

Just clone the repo and run yarn test.

[mdjastrzebski]

If we are to merge this PR, all tests need be pass green, so either we disable this test (e.g. xtest()) or provide a fix. @thymikee , @douglasjunior wdyt?

[thymikee]

So far the tests on CI are not running for this PR 🤔

[douglasjunior]

Sorry for delay, guys.

@mdjastrzebski I created this PR so @thymikee can analyze the TextInput problem.

I don't know why the tests don't run automatically. 😬

[pierrezimmermannbam]

This happens because the queries by testID return host components whereas other queries return composite components. When verifying that the event is enabled, the isTextInput function checks that the type of the component is the TextInput component but in the case of host components, the type is a string, thus the element is not considered as a TextInput and we don't look at the editable prop.
A simple fix is to check if the type is TextInput component or string TextInput, I opened a pr to fix it #1092.

I'm wondering though why do other queries return composite components. Making them return host components doesn't break any test and at first glance I'd say everything should work as before, but I may be missing something. This would however be a breaking change as the return of the queries would be a bit different but I can't think of a way it could break a test. Having the same behaviour for all queries would improve maintainability imo (this bug for instance would have been spotted earlier), @mdjastrzebski @thymikee wdyt ?

[AugustinLF]

I'm wondering though why do other queries return composite components. Making them return host components doesn't break any test and at first glance I'd say everything should work as before, but I may be missing something. This would however be a breaking change as the return of the queries would be a bit different but I can't think of a way it could break a test. Having the same behaviour for all queries would improve maintainability imo (this bug for instance would have been spotted earlier), wdyt ?

Yeah, that definitely doesn't feel right to me. I'm unsure how problematic this change would be though. I definitely think it's worth opening an issue for discussing that. I'd feel it's the kind of changes that would be nice to test on real codebase, and see if it breaks anything, or rolling out behind a flag.

[douglasjunior]

@pierrezimmermannbam When using withTheme from "@callstack/react-theme-provider", the test fail even with getByPlaceholderText.

PS: I also tried renaming the property to label, and passing it as placeholder just for the native component.

https://github.com/douglasjunior/TestingLibrarySample/blob/cbfd92f7268edef5f020522244cff2b4e33edc72/__tests__/App-test.tsx#L65-L89

Would that be the same scenario?

[pierrezimmermannbam]

@pierrezimmermannbam When using withTheme from "@callstack/react-theme-provider", the test fail even with getByPlaceholderText.

PS: I also tried renaming the property to label, and passing it as placeholder just for the native component.

https://github.com/douglasjunior/TestingLibrarySample/blob/cbfd92f7268edef5f020522244cff2b4e33edc72/__tests__/App-test.tsx#L65-L89

Would that be the same scenario?

This is yet another problem. fireEvent correctly detects that the textinput is non editable so it then tries to trigger the event on the parent (it tries triggering the event on the parent when it can't find an appropriate handler on an element) and since your custom component has an onFocus prop it calls it. The custom component is not considered non editable because it's not a textinput.

This is a way more complex problem to solve, but I guess the way to fix it would be to make fireEvent work only on host components, but it would require that all queries return host components instead of composite one and it would also be a breaking change because then you wouldn't be able to trigger custom events based on the name of a custom component prop, but maybe it should work that way, @AugustinLF wdyt ?

[douglasjunior]

Thank you very much for the information @pierrezimmermannbam , this is interesting.

How do you guys solve this kind of problem within the @callstack in real projects?

[AugustinLF]

you wouldn't be able to trigger custom events based on the name of a custom component prop

This is not a solution for me. We rely on this behaviour to trigger events handled at the native level (for instance onScrollMomentumBegin on a FlatList). And I assume such behaviour could also be possible with regular events (a press event being triggered at the native level, while the custom native component takes an onClick (or whatever) prop.

[mdjastrzebski]

@pierrezimmermannbam I think that RNTL already has assumption that all queries return host components. The reasoning behind it is that only host components result directly with visible user interface. This is analogous to RTL returning you only DOM elements (aka web host components).

The only exception to that rule are UNSAFE_ queries, which can return composite components, and UNSAFE prefix is meant to exactly signify that fact.

So no breaking change here :-)

[mdjastrzebski]

@AugustinLF I'm not sure I understand your point here:

you wouldn't be able to trigger custom events based on the name of a custom component prop

This is not a solution for me. We rely on this behaviour to trigger events handled at the native level (for instance onScrollMomentumBegin on a FlatList).

Can you please elaborate on what behaviour would you expect from fireEvent regarding host/composite components and custom event names?

[pierrezimmermannbam]

@mdjastrzebski I'm not sure I understood your point. RNTL queries, except the ones by id do return composite components for now so changing this would be a breaking change even though it may not be very impactful. However if fireEvent only triggered on host components fireEvent.press wouldn't work anymore because native components have an onClick prop instead of an onPress (unless the implemtentation of fireEvent.press was changed so it triggers the onClick prop). Also we are currently able to call any prop on any component using fireEvent, which wouldn't be possible anymore if it was restricted to host components props (this is not as much of an issue I think, although it would be a breaking change it's not used much or at least i've not seen it being used extensively and maybe it shouldn't be allowed as I believe it would be better to interact only with host components)

[mdjastrzebski]

@pierrezimmermannbam Thanks for brining that to my attention. I've done some research regarding what our queries return in order to learn more about the current state of RNTL, and here are the results:

Queries returning host components (typeof node.type === 'string'):

• testId: host & matches normalised testID prop
• role: host & matches accessibilityRole prop
• hintText: host & matches accessibilityHint prop
• labelText: host & matches accessibilityLabel prop
• a11yState: host & matches accessibilityState prop
• a11yValue: host & matches accessibilityValue prop

Queries returning composite components:

• text: composite Text from RN, matches against normalised and flattened children prop
• displayValue: composite TextInput from RN, matches normalised value or defaultValue
• placeholderText: composite TextInput from RN, matches normalised placeholder prop

I've omitted UNSAFE_ queries as they are special case and generally are not recommended.

So you are right that we have a number of queries which return composite components, although this should be strictly RN Text or TextInput, and not arbitrary RN or user composite components, if my analysis is correct.

To shed some light on why we are in such state, originally all queries were intended to return host components, basically because they represent visible UI. That however proved problematic in some cases, namely Text and TextInput, so these two match against respective composite components exported by RN.

This is certainly not perfect, but since there is no Jest environment simulating RN, so RNTL tries to do it itself.

[mdjastrzebski]

Seems like the easiest solution for now is to tweak the isTextInput detection in fireEvent so that it can check editable props on both composite (exported) TextInput as well as child host TextInput.

[pierrezimmermannbam]

@pierrezimmermannbam Thanks for brining that to my attention. I've done some research regarding what our queries return in order to learn more about the current state of RNTL, and here are the results:

Queries returning host components (typeof node.type === 'string'):

• testId: host & matches normalised testID prop
• role: host & matches accessibilityRole prop
• hintText: host & matches accessibilityHint prop
• labelText: host & matches accessibilityLabel prop
• a11yState: host & matches accessibilityState prop
• a11yValue: host & matches accessibilityValue prop

Queries returning composite components:

• text: composite Text from RN, matches against normalised and flattened children prop
• displayValue: composite TextInput from RN, matches normalised value or defaultValue
• placeholderText: composite TextInput from RN, matches normalised placeholder prop

I've omitted UNSAFE_ queries as they are special case and generally are not recommended.

So you are right that we have a number of queries which return composite components, although this should be strictly RN Text or TextInput, and not arbitrary RN or user composite components, if my analysis is correct.

To shed some light on why we are in such state, originally all queries were intended to return host components, basically because they represent visible UI. That however proved problematic in some cases, namely Text and TextInput, so these two match against respective composite components exported by RN.

This is certainly not perfect, but since there is no Jest environment simulating RN, so RNTL tries to do it itself.

@mdjastrzebski thanks this a very interesting ! I am curious however if you know how returning host components proved problematic in the cases of TextInput and Text ? As far as I can tell I don't see any issues so I must be missing something here

[AugustinLF]

@mdjastrzebski I opened a PR to add tests covering the use case I care about regarding native event.

I don't have strong opinions (and am not super knowledgeable) about the host vs composite element approach. I just think that returning either, depending on the query is likely to lead to other subtle bugs like that.

[mdjastrzebski]

Alternative resolution merged: #1092 1092