Skip to content

Fix getByTestId nested custom text edge case #516

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

Closed
wants to merge 2 commits into from

Conversation

mym0404
Copy link

@mym0404 mym0404 commented Aug 20, 2020

This PR will close #515

  • Refactor getChildrenAsText function in getByAPI.js
  • Add getByApi.test.js, queryByApi.test.js test cases about nested <Text> component

#489 fixed case like the following.

test('getByText with nested native Text component get closest Text', () => {
  const NestedText = () => {
    return (
      <Text testID="outer">
        <Text testID="inner">Test</Text>
      </Text>
    );
  };

  const { getByText } = render(<NestedText />);

  expect(getByText('Test').props.testID).toBe('inner');
});

But not

test('getByText with nested custom Text component get closest Text', () => {
  const CustomText = ({ children, ...rest }) => (
    <Text {...rest}>{children}</Text>
  );

  const NestedText = () => {
    return (
      <CustomText testID="outer">
        <CustomText testID="inner">Test</CustomText>
      </CustomText>
    );
  };

  const { getByText } = render(<NestedText />);

  expect(getByText('Test').props.testID).toBe('inner');
});

This is because filterNodeByType(child, ...) is returned as false for custom text component.

So I handling the edge case for the above.
I can't sure this is the correct way, but this feature doesn't break before test cases.

Also, I refactored type checking code whether child is string or number for adding to textContent.

@mym0404
Copy link
Author

mym0404 commented Aug 20, 2020

I found some fail cases more... This PR should be pending now

@thymikee
Copy link
Member

The #515 mentions getByText with RegExp as an argument

@mym0404
Copy link
Author

mym0404 commented Aug 20, 2020

Yes, I realized the cause of this problem.

Case 1. Test failed only with RegExp

For example, the components exist like

    const NestedText = () => {
      return (
        <CustomText testID="outer">
          <CustomText testID="inner1">Test</CustomText>
          <CustomText testID="inner2">Test2</CustomText>
        </CustomText>
      );
    };

And, in getByAPI.js

    if (isTextComponent) {
      const textChildren = getChildrenAsText(node.props.children, Text);
      if (textChildren) {
        const textToTest = textChildren.join('');
        return typeof text === 'string'
          ? text === textToTest
          : text.test(textToTest);
      }
    }

The textToTest value of outer Text is TestTest2 and inner1 is Test.
So with string expectation, then inner1 is returned correctly. But in RegExp, outer is also matched with /Test/

Case 2. Test failed both

    const NestedText = () => {
      return (
        <CustomText testID="outer">
          <CustomText testID="inner1"></CustomText>
          <CustomText testID="inner2">Test</CustomText>
        </CustomText>
      );
    };

The textToTest value of outer Text is also Test... so getByText('Text') and getByText(/Text/) both matched to outer

@mym0404
Copy link
Author

mym0404 commented Aug 20, 2020

I think this is not about the bug and about the principle of the text matching test of this library.

In my case, I don't want to test like #489 with a recursive text collection way.
I just want to test the text in any <Text> component directly.

IMAO, the tests like the following should be failed.

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

But, this is just my opinion thank you for your comment. It helped me what is the main cause of the problem. 😃
I will test my components like this.

    const matches = testingLib.getAllByText(text);
    const element = matches[matches.length - 1];

@mym0404 mym0404 closed this Aug 20, 2020
@thymikee
Copy link
Member

IMAO, the tests like the following should be failed

I can't agree to that. The user would actually see the "Hello World!" text and this is what we try to achieve here. We can probably come up with a better way of doing so, which works well with both string and regex matches.

@mym0404
Copy link
Author

mym0404 commented Aug 20, 2020

Thank you for your concern

@mym0404 mym0404 deleted the fix/nest-custom-text branch August 20, 2020 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nested custom Text componentns not working correctly
2 participants