feat(breaking): add text match options a.k.a string precision API #554

AugustinLF · 2020-09-28T11:40:50Z

Continuation of @RafikiTiki's work of #541

Summary

This does two things, adding some options to the queries (see initial PR), and remove matching strings spread in several levels of text. Namely the following example will not be valid anymore.

render(
      <Text>
        Hello <CustomText>World!</CustomText>	
      </Text>	
    ).queryByText('Hello World!')	
  ).toBeTruthy();

This is a breaking change, but it aligns the APIs and behaviour with @testing-library/react.

Closes #541
Closes #514

Co-authored-by: Maciej Jastrzebski <[email protected]>

AugustinLF · 2020-10-06T07:56:01Z

@thymikee is there anything I can do to help this PR to move forward?

thymikee · 2020-10-06T08:10:27Z

src/__tests__/queryByApi.test.js

@@ -48,7 +48,7 @@ test('queryByText not found', () => {
  ).toBeFalsy();
 });

-test('queryByText nested text across multiple <Text> in <Text>', () => {
+test('queryByText does not match nested text across multiple <Text> in <Text>', () => {


So we're regressing this functionality to be compatible with Testing Library, which also doesn't support this?

Yes. I checked, testing library doesn't match in this case.

How does this PR change this functionality? Finding the text in nodes still recurses through children

Yes https://github.com/callstack/react-native-testing-library/pull/554/files#diff-28f6e2b77eef7ef8b7583339cfdf593fR43

I must be missing something but how does the logic added to getNodeByText end up not matching in this case? As I see it the the logic in getChildrenAsText will return ['Hello', ' ', 'World', '!'] which when joined will be "Hello World!", which is an exact match for what you are looking for.

Yes, the change is to keep matching component with multiple children, but not <Text>Hello <Text>World</Text></Text, because now getChildrenAsText don't read its children's children

Ah, so if the test render looked like this:

<Text> <Text>Hello</Text> World! </Text>

Would this then match the outer text?

This currently match is the current released version, but wouldn't match in my current implementation. It's the topic of discussion in #554 (comment)

src/__tests__/queryByApi.test.js

thymikee

Before merging, I need to make sure that @RafikiTiki gets attribution for his work. Since there are 2 breaking changes here now, I'm considering releasing it in a major update

AugustinLF · 2020-10-06T09:50:19Z

Yes, I think this should be a major release. I only see the recursive matching as breaking (don't know which other one you're talking about) though. I can help with the release notes if needed.

thymikee · 2020-10-06T09:52:01Z

src/__tests__/findByApi.test.js

  await expect(
-    findByPlaceholderText('Placeholder Text', options)
+    findByPlaceholderText('Placeholder Text', {}, options)


this is the second breaking change I mentioned

We could provide an easy migration path by looking for waitFor options (timeout, interval) also in the queryOptions only for findBy queries and only till next major release. While displaying a console warning to inform the developer. Wdyt?

That's a good idea that I missed, I fixed it in the latest commit d2f9f33

src/helpers/findByAPI.js

thymikee · 2020-10-07T16:55:28Z

FYI, there's an ongoing discussion in DOM Testing Library regarding matching nested text: testing-library/dom-testing-library#473 (comment). Also, seems like getByLabelText already behaves similar to what we have today.

Hope you understand why I'm so hesitant merging this. We're likely regressing a use case that we'd eventually had to add anyway. I'd like to think of a way to not break matching nested nodes using exact: false

mdjastrzebski

Wow a lot of good code! 👍

I've added some comments as there seems to be some minor errors (e.g. typescript types) and opportunities to improve readability/code structure.

mdjastrzebski · 2020-10-07T17:01:59Z

src/__tests__/getByApi.test.js

+      </View>
+    );
+
+    expect(getByText('Text and whitespace')).toBeTruthy();


I like that one 👍

mdjastrzebski · 2020-10-07T17:08:43Z

src/__tests__/queryByApi.test.js

+  expect(queryByTextFirstCase('My text')).toBe(null);
  expect(
-    render(
-      <Text>
-        Hello <CustomText>World!</CustomText>
-      </Text>
-    ).queryByText('Hello World!')
-  ).toBeTruthy();
+    queryByTextSecondCase('My text', { exact: false })?.props.nativeID
+  ).toBe('2');


Does seem strange a bit, that we use one component structure with the default exact: true and other component structure with exact: false. If we were to contast exact true/false IMO we should use the same component structure.

Looks like we should do one of the following (in order of preference):

merge queryByTextFirstCase + queryByTextSecondCase

add queryByTextFirstCase + exact: false & queryByTextSecondCase + exact: true

split the test into two unreleated tests

I do overall believe that tests should be restructured, a lot of cases are tested only because the internal APIs are the same (a lot of edge cases for findByText are only tested in getByText for instance), so I'd like to have more tests, and make them more verbose. But I believe it's out of the scope of this PR.

But this specific case makes sense, I'll follow your suggestion, I got a bit lazy and monkey patched some tests without really reconsidering them.

mdjastrzebski · 2020-10-07T17:09:04Z

src/__tests__/queryByApi.test.js

+  ).toBe(null);
+});
+
+test('queryAllByText does not match several times the same text', () => {


src/matches.js

src/helpers/getByAPI.js

mdjastrzebski · 2020-10-07T17:19:15Z

src/helpers/getByAPI.js

    try {
-      return instance.find((node) => getNodeByText(node, text));
+      const results = getAllByText(instance)(text, options);


mdjastrzebski · 2020-10-07T17:23:29Z

typings/index.d.ts

  findByText: (
-    text: string | RegExp,
-    waitForOptions?: WaitForOptions
+    text: TextMatch,
+    waitForOptions?: WaitForOptions,
+    options?: TextMatchOptions,
  ) => FindReturn;
  findByPlaceholderText: (
-    placeholder: string | RegExp,
-    waitForOptions?: WaitForOptions
+    placeholder: TextMatch,
+    waitForOptions?: WaitForOptions,
+    options?: TextMatchOptions,
  ) => FindReturn;
  findByDisplayValue: (
-    value: string | RegExp,
-    waitForOptions?: WaitForOptions
+    value: TextMatch,
+    waitForOptions?: WaitForOptions,
+    options?: TextMatchOptions,
  ) => FindReturn;
-  findByTestId: (testID: string | RegExp, waitForOptions?: WaitForOptions) => FindReturn;
+  findByTestId: (testID: TextMatch, waitForOptions?: WaitForOptions) => FindReturn;
  findAllByText: (
-    text: string | RegExp,
-    waitForOptions?: WaitForOptions
+    text: TextMatch,
+    waitForOptions?: WaitForOptions,
+    options?: TextMatchOptions,
  ) => FindAllReturn;
  findAllByPlaceholderText: (
-    placeholder: string | RegExp,
-    waitForOptions?: WaitForOptions
+    placeholder: TextMatch,
+    waitForOptions?: WaitForOptions,
+    options?: TextMatchOptions,
  ) => FindAllReturn;
  findAllByDisplayValue: (
-    value: string | RegExp,
-    waitForOptions?: WaitForOptions
+    value: TextMatch,
+    waitForOptions?: WaitForOptions,
+    options?: TextMatchOptions,
  ) => FindAllReturn;
  findAllByTestId: (
-    testID: string | RegExp,
+    testID: TextMatch,
    waitForOptions?: WaitForOptions
  ) => FindAllReturn;


Looks like typescript types for findBy have incorrect options args order.

Oops forgot to update, thanks!

typings/index.d.ts

…match-options

AugustinLF · 2021-11-01T09:32:51Z

Hey folks, it's never too late to pick up a stale PR! I've addressed one of the comments to reduce the breaking changes, but there's still the main question, which is the issue with matching across multiple nodes and the difference in the API with @testing-library/react. @thymikee you were unsure about this change and raised some concerns. I've written higher in this PR what my opinion is (i.e. same behaviour on both platform), but also discussed some other alternatives, what do you think? Do you see a solution to move forward?

thymikee · 2021-11-03T09:31:38Z

Hey, I'm gonna look at this once again soon. We already release v8, so we're clear to introduce other unrelated breaking changes 💪🏼

AugustinLF · 2021-11-22T16:52:01Z

Hey hey @thymikee, anything I can do to help? I admit that current status is a bit messy since the PR went over several months and building on top of another contribution. If that helps, I could try to make a recap of the whole discussion/status.

thymikee · 2021-11-22T20:07:16Z

@AugustinLF that won't hurt for sure! I'm pretty sure this is the way we want to move forward. It would help to gather all the breaking changes this can cause and ways to migrate.

I'm a bit swamped lately with work and personal stuff and having a really hard time to fit this into my pipeline. But maybe I could only have a brief look and release this as an alpha version for testing. Appreciate you stick with us all this time 🙌🏼

* origin/main: (24 commits) v8.0.0 chore(deps): bump dns-packet from 1.3.1 to 1.3.4 in /website (callstack#742) chore(deps-dev): bump @babel/preset-flow from 7.13.13 to 7.14.5 (callstack#765) chore(deps-dev): bump typescript from 4.4.2 to 4.4.4 (callstack#845) chore(deps-dev): bump release-it from 14.6.1 to 14.11.6 (callstack#830) chore(deps-dev): bump @babel/plugin-proposal-class-properties (callstack#846) chore(deps-dev): bump @babel/cli from 7.13.16 to 7.15.7 (callstack#833) chore(deps-dev): bump @testing-library/jest-native from 3.4.3 to 4.0.2 (callstack#842) chore(deps): bump color-string from 1.5.3 to 1.6.0 in /website (callstack#844) chore(deps): bump prismjs from 1.23.0 to 1.25.0 in /website (callstack#843) Forward extra data to press event (callstack#828) chore(deps-dev): bump @babel/preset-react from 7.13.13 to 7.14.5 (callstack#839) chore(deps): bump url-parse from 1.5.1 to 1.5.3 in /website (callstack#813) chore(deps-dev): bump @types/react from 17.0.2 to 17.0.30 (callstack#838) chore(deps): bump postcss from 7.0.29 to 7.0.36 in /website (callstack#756) chore(deps): bump prismjs from 1.23.0 to 1.25.0 in /website (callstack#823) chore(deps): bump path-parse from 1.0.6 to 1.0.7 in /website (callstack#796) chore(deps): bump merge-deep from 3.0.2 to 3.0.3 in /website (callstack#754) chore(deps): bump ws from 6.2.1 to 6.2.2 in /website (callstack#752) chore(deps): bump path-parse from 1.0.6 to 1.0.7 (callstack#795) ...

AugustinLF · 2021-11-23T14:49:56Z

This PR includes two main changes: it adds TextMatch options, and changes the way nested text is matched:

Support to TextMatch in all text based queries. This moves the current waitForOptions parameter of the findBy* queries from the 2nd place to the 3rd place. For backward compatibility, we still read it from the 2nd parameter but log a deprecation warning.

Nested text matching changes, it will not match text broken across several nodes:

// This examples matches before this PR and will not after
render(
      <Text>
        Hello <CustomText>World!</CustomText>	
      </Text>	
    ).getByText('Hello World!')

Extra care has been put to make sure that the relevant node gets matched, when several nodes could match (in the previous example, when matching "World", it'll match CustomText and not Text.

These two changes have the benefit of making the API closer to the older implementation of testing-library/react-native (before being transferred to callstack), and to make it closer to the web implementation.

thymikee · 2021-11-23T14:54:56Z

@AugustinLF I've pushed a commit with a few updates, please check it. One thing that we're still missing is matching a11y queries, but I'm thinking about introducing it in a separate PR

AugustinLF · 2021-11-23T15:26:38Z

Great, that looks good.

The a11y queries are very high on my list (to be honest we've reimplemented them in user-land on our side). But I'd love to take a step back and try to get a more holistic view and figure out what we're missing, if anything needs to be improved, I'm planning to try to get some feedback from our devs here at Klarna. I hope to be able to free more time to improve our test infra and @testing-library/react-native is a very important part of it, so looking forward helping.

Any plan to switch to TS? 👀

thymikee · 2021-11-23T15:30:01Z

The a11y queries are very high on my list (to be honest we've reimplemented them in user-land on our side). But I'd love to take a step back and try to get a more holistic view and figure out what we're missing

Yes, let's take some time to make it right. We may even want to revisit the API and maybe somehow mimic the getByRole from the web, as accessibility matures on RN? Not sure if that's even viable, so treating it as an idea / open question for now.

Any plan to switch to TS? 👀

Yes please!

thymikee · 2021-11-23T15:40:00Z

Published a pre-release: https://github.com/callstack/react-native-testing-library/releases/tag/v9.0.0-alpha.0. Thank you once again @AugustinLF, much appreciated!

janpe · 2021-12-02T08:04:58Z

@thymikee any estimate on when 9.0.0 could be released?

AugustinLF · 2021-12-02T10:40:30Z

We need to publish some announcement around those change, I hope to pick that work today or tomorrow #862 (reply in thread)

RafikiTiki and others added 17 commits September 9, 2020 15:27

Add TextMatch options to getByAPI

34ffda8

Add TextMatch options to queryByAPI

d95ff2f

Add TextMatch options to findByAPI

b616319

Add tests covering new TextMatch options

3857870

Add documentation and examples for TextMatch options

7c4c074

Implement text match normalization customization

f9e7d8d

Implement tests for normalization options

e3719bd

Add text normalization docs

73c6aed

Refactor typings

0884d86

Add export for getDefaultNormalizer function

c14446c

Remove undocumented config for normalizer function

677a78e

Update website/docs/Queries.md

cb6d122

Co-authored-by: Maciej Jastrzebski <[email protected]>

BREAKING: make findBy* queries arguments order compatible with RTL

b7f0b74

Fix tests for findBy queries after changing their API

5f1d266

Fix matching nested Text components with queryOptions exact set to true

e5e977b

Remove recursion matching for *byText APIs

4118a23

Reuse getAllByText in getByText

4087aff

AugustinLF mentioned this pull request Sep 29, 2020

V7 getByText no longer find translation string #553

Closed

thymikee reviewed Oct 6, 2020

View reviewed changes

src/__tests__/queryByApi.test.js Outdated Show resolved Hide resolved

thymikee approved these changes Oct 6, 2020

View reviewed changes

thymikee requested review from mdjastrzebski and cross19xx October 6, 2020 08:56

thymikee reviewed Oct 6, 2020

View reviewed changes

src/helpers/findByAPI.js Outdated Show resolved Hide resolved

Add Textmatch options to *byTestId

6d8caf4

mdjastrzebski reviewed Oct 7, 2020

View reviewed changes

Damian Sznajder added 3 commits May 17, 2021 17:16

refactor: change params position and description for matches

e1ad2b4

Merge branch 'master' into AugustinLF/non-recursive-text-match-options

ff10a3a

refactor: move & remove duplicated tests after merge

6eb071a

This was referenced Jul 6, 2021

Bug: Found multiple elements with text #776

Closed

test: add failing tests for #776 #778

Merged

gmaclennan mentioned this pull request Jul 16, 2021

Add react-testing-library and Jest suite digidem/mapeo-mobile#519

Merged

AugustinLF added 2 commits October 30, 2021 08:19

Merge remote-tracking branch 'origin/master' into non-recursive-text-…

3bafdff

…match-options

Make waitForOptions param change non breaking change

d2f9f33

AugustinLF force-pushed the non-recursive-text-match-options branch from 677c78a to d2f9f33 Compare November 1, 2021 08:53

thymikee added 2 commits November 23, 2021 15:19

chore: cleanup and type fixups

af62597

thymikee and others added 4 commits November 23, 2021 16:10

tests: update snapshots

864155a

chore: use default arg value for normalizer

ed34a69

Restore mock after use

223247b

fix: wrong snapshot update with outdated deps

bd17c86

thymikee merged commit 31ef0b9 into callstack:main Nov 23, 2021

thymikee changed the title ~~feat: add text match options a.k.a string precision API~~ feat(breaking): add text match options a.k.a string precision API Nov 23, 2021

AugustinLF deleted the non-recursive-text-match-options branch November 23, 2021 15:34

AugustinLF mentioned this pull request Dec 3, 2021

Native TS support #877

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(breaking): add text match options a.k.a string precision API #554

feat(breaking): add text match options a.k.a string precision API #554

AugustinLF commented Sep 28, 2020 •

edited by thymikee

Loading

AugustinLF commented Oct 6, 2020

thymikee Oct 6, 2020

AugustinLF Oct 6, 2020

mikeduminy Oct 10, 2020

AugustinLF Oct 10, 2020

mikeduminy Oct 10, 2020

AugustinLF Oct 12, 2020

mikeduminy Oct 12, 2020

AugustinLF Oct 12, 2020

thymikee left a comment

AugustinLF commented Oct 6, 2020

thymikee Oct 6, 2020

mdjastrzebski Oct 7, 2020

AugustinLF Nov 1, 2021

thymikee commented Oct 7, 2020

mdjastrzebski left a comment

mdjastrzebski Oct 7, 2020

mdjastrzebski Oct 7, 2020

AugustinLF Oct 8, 2020

mdjastrzebski Oct 7, 2020

mdjastrzebski Oct 7, 2020

mdjastrzebski Oct 7, 2020

AugustinLF Oct 8, 2020

AugustinLF commented Nov 1, 2021

thymikee commented Nov 3, 2021

AugustinLF commented Nov 22, 2021

thymikee commented Nov 22, 2021

AugustinLF commented Nov 23, 2021

thymikee commented Nov 23, 2021

AugustinLF commented Nov 23, 2021 •

edited

Loading

thymikee commented Nov 23, 2021

thymikee commented Nov 23, 2021

janpe commented Dec 2, 2021

AugustinLF commented Dec 2, 2021

feat(breaking): add text match options a.k.a string precision API #554

feat(breaking): add text match options a.k.a string precision API #554

Conversation

AugustinLF commented Sep 28, 2020 • edited by thymikee Loading

Summary

AugustinLF commented Oct 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thymikee left a comment

Choose a reason for hiding this comment

AugustinLF commented Oct 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thymikee commented Oct 7, 2020

mdjastrzebski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AugustinLF commented Nov 1, 2021

thymikee commented Nov 3, 2021

AugustinLF commented Nov 22, 2021

thymikee commented Nov 22, 2021

AugustinLF commented Nov 23, 2021

thymikee commented Nov 23, 2021

AugustinLF commented Nov 23, 2021 • edited Loading

thymikee commented Nov 23, 2021

thymikee commented Nov 23, 2021

janpe commented Dec 2, 2021

AugustinLF commented Dec 2, 2021

AugustinLF commented Sep 28, 2020 •

edited by thymikee

Loading

AugustinLF commented Nov 23, 2021 •

edited

Loading