react-native-testing-library icon indicating copy to clipboard operation
react-native-testing-library copied to clipboard

Allow providing custom queries

Open mdjastrzebski opened this issue 3 years ago • 13 comments

Describe the Feature

Allow to add custom queries in render call. Similar to RTL API.

Possible Implementations

Option 1 (RTL-like):

  • Provide a new queries option to render & within functions as in RTL,.
  • Since RTL API requires you to import original queries if you want to retain them (and you generally want this), so we probably need to provider such export.
  • Correctly managing TS types might be the most difficult part of the task since we would want render and within output to be typed with the actual queries passed
  • Implementation might consider changing way we internally export the queries from bindXxxQueries to named particular queries, e.g. bindGetByText: (instance) => (text, options) => ReactTestInstance or event introducing "unbound" queries (getByText: (instance, text, options) => ReactTestInstance) as in Dom Testing Library if that simplifies the exposed API / type management
  • Following RTL we should also export makeQueries internal utils, so that users need only to prepare queryAll variant of the query.

Option 2 (roll our own API):

  • We could probably simplify the API surface by accepting list of additional queries only in queryAllBy verb and then invoking makeQuery internally.
  • We could automatically forward all built-in queries and only append new queries from the user. That we we would keep the strong typing for built-in queries with potentially more lax typing of user provided queries (more usage any for query predicate / options).
  • This options seems to have much better trade off between lower increasing code complexity & niche popularity of the feature. However it is not API compatible with RTL.

Option 3 (expose generic query function accepting user predicate):

  • We could expose a full set of queries (get, etc) for ByPredicate query accepting a function (instance: ReactTestInstance) => boolean.
  • User then could define his own predicates and feed them to our getByPredicate, etc queries. That would give user total flexibility in types of built queries: number of accepted params, typing, logic, etc
  • These custom queries would do thing like filtering our composite components for the user, so he can focus on query logic.
  • Pros: least increase of code & API surface complexity, proper typing for all queries
  • Cons: not compatible with RTL, but seems so good that we can suggest RTL to add it ;-)
  • Quick and dirty POC implementation of this option: #973

Related Issues

  • Initial implementation attempt: #573 but the code is stale. Should be easy to re-apply. Some implementation and tests can be salved from that PR

mdjastrzebski avatar May 04 '22 11:05 mdjastrzebski

@thymikee @AugustinLF @pvinis: what do you think?

mdjastrzebski avatar May 04 '22 16:05 mdjastrzebski

@mdjastrzebski I'd love to contribute to this issue, is this up for grabs?

V1shvesh avatar Jun 21 '22 07:06 V1shvesh

@V1shvesh sure :)

thymikee avatar Jun 21 '22 07:06 thymikee

@thymikee @V1shvesh wait guys! In the issue description I've presented 3 options how we can get the problem solved. Let's have some discussion before jumping to code.

mdjastrzebski avatar Jun 22 '22 17:06 mdjastrzebski

@mdjastrzebski I went through that as well. Would love to connect and get more context around this!

V1shvesh avatar Jun 22 '22 17:06 V1shvesh

@V1shvesh In general we try to be as compatible in API terms as possible with RTL for easily the life of developers using RNTL. Also RTL is the original Testing Library, and they have put a lot of time and work, and re-work, in polishing their APIs.

That being said, their custom query implementation has number of problems:

  1. User needs to provide all query variants (getBy, getAllBy, queryBy, queryAllBy, findBy, findAllBy) instead of only making queryAllBy and deriving other queries for it. RTL provides a helper for that, but still it's up to the user to do it.
  2. When providing custom queries, user needs to spread the existing queries, or otherwise they will get removed from his render output.
  3. Typing: RenderResult is hence generically typed based on passed queries. Which complicates the typing for various usages.

It seems that these problems come from importing their queries in the format of DOM Testing Library queries. Since we do not have our version of "DOM Testing Library", I think we should seriously evaluate pros and cons of alternative approaches to providing custom queries.

I've highlighted the few possible options in the issue description. All could provide the custom queries feature to our users, but it's a matter of deciding which one offers the best trade-offs between various aspects: ease of use, compatibility with RTL, etc.

mdjastrzebski avatar Jun 23 '22 09:06 mdjastrzebski

Hey @mdjastrzebski, I went through your POC draft https://github.com/callstack/react-native-testing-library/pull/973, accepting the predicate logic from user really makes sense to me as a functional implementation for this API.

Can you shed more light on the ramifications of breaking compatibility with RTL? I'm really hopeful for having the same API in RTL as well!

V1shvesh avatar Jun 28 '22 19:06 V1shvesh

@mdjastrzebski @thymikee If we can agree on the 3rd approach, shall I begin working on it?

V1shvesh avatar Jul 01 '22 10:07 V1shvesh

@thymikee @AugustinLF @MattAgn @pierrezimmermannbam Could you give your feedback on the proposal (initial post)?

mdjastrzebski avatar Aug 19 '22 21:08 mdjastrzebski

@mdjastrzebski I love the 3rd option! As you say, considering the low amount of people using this feature, I think RNTL can have its own API And as @V1shvesh said, the 3rd option also seems like the easiest to use and understand from a user perspective

MattAgn avatar Aug 22 '22 10:08 MattAgn

@mdjastrzebski @MattAgn awesome then, I'll start working on this 👍🏻

V1shvesh avatar Aug 22 '22 11:08 V1shvesh

I haven't used this feature ever but I don't really see the benefits of not retaining the original queries so I think it would be better if they were kept by default, but I may be missing a particular use case ? (maybe we should propose testing-library/react to change their api). I also think the 3rd option is a very good one at least for a beginning considering the differences in the internal implementation of queries, and maybe work progressively towards a common api for both librairies

pierrezimmermannbam avatar Aug 22 '22 11:08 pierrezimmermannbam

To me the poor typing support of the first solution is a deal-breaker. I don't have a strong use case for custom queries, but since this seems to be a user requested feature, I do think the third one is the one that makes the most sense, make users life easier, while still making the API maintainable.

AugustinLF avatar Aug 25 '22 08:08 AugustinLF

@V1shvesh have you been able to make some progress on this ?

pierrezimmermannbam avatar Nov 10 '22 11:11 pierrezimmermannbam

@pierrezimmermannbam I went quite far with the protype implementation of options 3 in #973. You can start by reviewing that PR.

mdjastrzebski avatar Nov 10 '22 12:11 mdjastrzebski

@thymikee @AugustinLF @MattAgn @pierrezimmermannbam feel free to express whether our users would actually benefit from this feature or it might be just unused piece of coda that we would have to maintain.

I treat this feature as an escape hatch for more advanced users who need more flexibility in querying items. We already have one escape hatch in form of *ByTestID queries. The fact that RTL does have custom queries (although in a bit different shape but functionally equivalent) does not mean that we need to add this as well if there is no valid use case for it. I have my own doubts here, so looking forward for your take.

mdjastrzebski avatar Dec 12 '22 12:12 mdjastrzebski

Not sure as well that this is valuable, I don't really see use cases for this feature. I think I'd rather use regular queries and build a custom jest matcher if needed and not available in testing-library/jest-native. Custom matchers have the advantage of being available globally whereas this would be used in a single place if we don't have the possibility to extend the screen object (which would be way more complicated to implement)

pierrezimmermannbam avatar Dec 13 '22 13:12 pierrezimmermannbam

Closing as we struggle to come up with any reasonable use case. We can come back to this idea if our users request it and have a reasonable use case for it.

mdjastrzebski avatar Dec 29 '22 08:12 mdjastrzebski