-
Notifications
You must be signed in to change notification settings - Fork 273
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
Conversation
I found some fail cases more... This PR should be pending now |
The #515 mentions |
Yes, I realized the cause of this problem. Case 1. Test failed only with
|
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. 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. 😃 const matches = testingLib.getAllByText(text);
const element = matches[matches.length - 1]; |
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. |
Thank you for your concern |
This PR will close #515
getChildrenAsText
function ingetByAPI.js
getByApi.test.js
,queryByApi.test.js
test cases about nested<Text>
component#489 fixed case like the following.
But not
This is because
filterNodeByType(child, ...)
is returned asfalse
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
isstring
ornumber
for adding totextContent
.