You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Error: Unable to find an element with text: "My text", while that text exist. #937
getByText can find element if text render component with text as not children (See "Steps to Reproduce" below)
-const Trans = p => p.i18nKey; // can find (error)+const Trans = p => p.children; // everything ok
Expected behavior
I see in debug output (see screenshot below) that text exists! But API can find it(
It should work properly!
Steps to Reproduce
test('test',()=>{const{ Text }=require('react-native');const{ render }=require('@testing-library/react-native');constTrans=p=>p.i18nKey;constscreen=render(<Text><Transi18nKey="My text"/></Text>,);screen.debug();expect(screen.getByText('My text'));// Error: Unable to find an element with text: My text});
@AugustinLF easy to reproduce but not easy to fix?
Reacted by Juan Marval, ArtemFedorchuk, hardikkarangiya, Krzysztof Borowski, Rajib Paudyal, Serge Olabin, Yudintsev George (Egor), Robinblad, Reneloper, Mateus Paulo da Silva and 3 more
@AugustinLF I know that this is a feature request. But is is possible to have the .debug() run automatically when a getBy* matcher fails? Recall react-testing-library had that and it was really useful to see how the component looks when debugging why it can't get a hold of it.
Reacted by Mark Lee, Louie Christie, Wiatan Oliveira, CheoR and CjordanIII
Not sure this is easy to fix, the type of <Trans i18nKey="text"/> isn't string so the getChildrenAsText method is looking for strings in its children but it doesn't have any. For this to work it would require to be able to tell that it renders as a string but i'm not sure it's easily doable.
@retyui this can be fixed on your end by mocking the Trans component like this
jest.mock('./Trans',()=>{const{ Text }=require('react-native');return{Trans: (props)=>{return<Text>{props.i18nKey}</Text>;},};});
However if there is an effective way to fix this it would be very nice, or maybe it could be documented somewhere
@AugustinLF I have found a solution but it's quite complex so I'm not sure it's something that should be done.
From what I've seen this can't be fixed if we use the type ReactTestInstance for getByText because there does not seem to be a way to know the component renders as a string, however it is doable by using the ReactTestRendererJSOn type. The thing is we also need to have access to the instance because it is what the query returns in the end. This is achievable by using the tree representation of type ReactTestRendererTree which combines the instance and a more json like modelization. This would however mean significant changes as it would almost completely change the way queries are done. I've been working on a draft pr and I think it would work and this type seems more appropriate for queries but I wanted to have your opinion first before spending more time on this
@pierrezimmermannbam that sounds like an interesting approach. Working with a "simpler" representation, based only on the platform components also feels more testing-library like.
One thing we'll need to cater for is that people do rely on firing fake events for some events (see #918 , I've also had to do that for momentum events on FlatList) . Some scroll events, swipes, etc. don't play well with the fact that we're not really rendering the native components (while jsdom does fire the real DOM events).
I'm not sure how that last point would play with this rearchitecture, but I think we should either offer a fallback API (current state) or figure a way to improve the behaviour.
@AugustinLF I'm not sure I understood your point about fake events. The changes I suggested would change the way queries are implemented internally but the api would remain the same as they would still return ReactTestInstance type so there shouldn't be any impact on the way fireEvent works. I'm not sure either how the issue if events not being supported could be fixed by using this approach but there are definitely things I'm missing here
@pierrezimmermannbam the current implementation of fireEvent crawls the tree up until it finds a ReactTestInstance with a prop corresponding to the given event, and fires it. Even if that's not a real event/a custom component.
I'll add tests for those cases in main, this way we can easily make sure there's no regressions and you shouldn't have to worry about that.
Regarding tests, I'll go through the repo and make sure that we have the desired coverage before we merge your PR. Luckily our tests do test external APIs so internal rewrite would be safe, but I'll make sure we're not missing any cases.
Regarding returning ReactTestInstance from queries – we considered replacing it with a QueryResult object or something, that would only give you access to specific fields instead of whole object. This is potentially breaking, but the direction where we'd like to go.
@AugustinLF@thymikee I have made some progress on this, I opened a draft pr but I've hit an issue. The queries by text work fine using the json representation but there are some problems.
First, I still need to get the ReactTestInstance from the renderer.root because the instances in the ReactTestRendererTree type are in fact not of type ReactTestInstance and do not have parents which is problematic for the fireEvent. I was able to fix this by finding the ReactTestInstance matching the result of the search though I'm not entirely sure it is reliable.
The second issue is with within. For within to work, the queries return type and what is used to build the api need to be of the same type and I broke that by building queries with a ReactTestRenderer type and returning the same type as before. It would be doable to have the following type :
However this would require to change not only the byText queries but all of them, which is more complex but shouldn't be too much of an issue if well refactored.
It would change the type of the api but not cause a breaking change as we'd be only adding a new field so it shouldn't be too problematic.
So before going further I wanted to have your inputs on this and make sure the implementation I started was what you also had in mind
string text output (not wrapped in Text object) is not traversable using ReactTestInstance.findAll method that all text queries use. However, you can fix the test quite easily by wrapping Trans output in Text instance like this:
constTrans=(p)=><Text>{p.i18nKey}</Text>;
Longer explanation
We we run getByText it internally uses ReactTestInstance.findAll to go through all elements of render tree.
We get findAll traversing following nodes of ReactTestInstance type:
NODE - Text composite class component
NODE - Text host component
NODE - Our Trans function component
As you can see there is no final "text node" with "My text" content. The leaf component is our Trans component.
From my investigation in what it offers in terms of API, there is no rendered output field available for easy examination. However when I inspect the actual object structure under tests we can see React fiber details:
Regarding using ReactTestRendererTree/ReactTestRendererJSON as a basis for working with queries that's an interesting question. It seems that there might be some benefit for using these however this would a very deep change for RNTL and should be careful considered whether it is fit for our purpose, also the light of the fact that all our queries return ReactTestInstance and users code is probably relying it, so changing that contract would be a really breaking change.
@mdjastrzebski The same test also fails in the RNTL repo with the patch-package applied, but that's because the testenvironment is node, I was able to make the test pass by switching the environment to jsdom
It works for the case with the link (not when rendering the view) if toJSON method return toJSON(this._fiber.child.stateNode) instead of toJSON(this._fiber.stateNode) so there seems to be cases where you'd want to use the child or maybe the children if there are several, I have no clue yet what's the logic behind this though
It also works when using getByText, but when trying to use toJSON on the container returned from the render function it fails
test('toJSON of Text with Trans',()=>{const{ container }=render(<View><TexttestID="subject"><Trans/></Text><Text>hello</Text></View>);expect(container.toJSON()).toMatchInlineSnapshot();});
In this case also using this_fiber.child.stateNode works but this time this._fiber_stateNode is not null but it doesn't have a tag property
I've created exported toJSON function that essentially calls the original toJSON when it is a host element, otherwise it recursively calls itself on all its children and collects the results.
@mdjastrzebski awesome! I see you're now exporting a function instead of adding a toJSON method to the ReactTestInstance class, why is that ? Wouldn't it be more convenient to have the method on the class ? It would allow for instance to take snapshots of elements returned by query without having to call the toJSON function and still have the JSON representation
@pierrezimmermannbam I've started with implementing toJSON as a method on ReactTestInstance, however I've encountered two issues that made free function a more natural approach:
testInstanceToJSON is recursive, as it needs to call itself for child nodes, and that requires ability to pass node on which to run the function
testInstanceToJSON accepts either ReactTestInstance | string as sometimes children might be string. Particular there was one existing unit test for renderer.toJSON() method that I could not duplicate as renderer.root.toJSON() because renderer.root was actually a string.
Thinking about this after your comment, I think that point 1 might be solvable by invoking child.toJSON() from testInstance.toJSON() and adding child type checks for string values. I will review my code tomorrow to see which feels like a better API.
It looks like this issue is fixed! I think this pr #1222 of @mdjastrzebski was the fix because we now don't look only at the direct children, so I guess from the beginning we only needed to look deeper as long as children weren't root elements
Activity
AugustinLF commentedon Mar 21, 2022
Yep, easy to reproduce, will try to figure out what happens but that should be fixable on our end.
retyui commentedon Mar 22, 2022
@AugustinLF easy to reproduce but not easy to fix?
AugustinLF commentedon Mar 22, 2022
I (or whoever's interested in) just need to take the time, but I don't think it should be very hard. I'll try to do that later this week, or next week
Norfeldt commentedon Mar 23, 2022
@AugustinLF I know that this is a feature request. But is is possible to have the
.debug()run automatically when agetBy*matcher fails? Recallreact-testing-libraryhad that and it was really useful to see how the component looks when debugging why it can't get a hold of it.pierrezimmermannbam commentedon Mar 24, 2022
Not sure this is easy to fix, the type of <Trans i18nKey="text"/> isn't string so the getChildrenAsText method is looking for strings in its children but it doesn't have any. For this to work it would require to be able to tell that it renders as a string but i'm not sure it's easily doable.
@retyui this can be fixed on your end by mocking the Trans component like this
However if there is an effective way to fix this it would be very nice, or maybe it could be documented somewhere
pierrezimmermannbam commentedon Mar 28, 2022
@AugustinLF I have found a solution but it's quite complex so I'm not sure it's something that should be done.
From what I've seen this can't be fixed if we use the type ReactTestInstance for getByText because there does not seem to be a way to know the component renders as a string, however it is doable by using the ReactTestRendererJSOn type. The thing is we also need to have access to the instance because it is what the query returns in the end. This is achievable by using the tree representation of type ReactTestRendererTree which combines the instance and a more json like modelization. This would however mean significant changes as it would almost completely change the way queries are done. I've been working on a draft pr and I think it would work and this type seems more appropriate for queries but I wanted to have your opinion first before spending more time on this
AugustinLF commentedon Mar 28, 2022
@pierrezimmermannbam that sounds like an interesting approach. Working with a "simpler" representation, based only on the platform components also feels more testing-library like.
One thing we'll need to cater for is that people do rely on firing fake events for some events (see #918 , I've also had to do that for momentum events on
FlatList) . Some scroll events, swipes, etc. don't play well with the fact that we're not really rendering the native components (while jsdom does fire the real DOM events).I'm not sure how that last point would play with this rearchitecture, but I think we should either offer a fallback API (current state) or figure a way to improve the behaviour.
pierrezimmermannbam commentedon Mar 28, 2022
@AugustinLF I'm not sure I understood your point about fake events. The changes I suggested would change the way queries are implemented internally but the api would remain the same as they would still return ReactTestInstance type so there shouldn't be any impact on the way fireEvent works. I'm not sure either how the issue if events not being supported could be fixed by using this approach but there are definitely things I'm missing here
AugustinLF commentedon Mar 29, 2022
@pierrezimmermannbam the current implementation of
fireEventcrawls the tree up until it finds aReactTestInstancewith a prop corresponding to the given event, and fires it. Even if that's not a real event/a custom component.I'll add tests for those cases in
main, this way we can easily make sure there's no regressions and you shouldn't have to worry about that.Regarding tests, I'll go through the repo and make sure that we have the desired coverage before we merge your PR. Luckily our tests do test external APIs so internal rewrite would be safe, but I'll make sure we're not missing any cases.
thymikee commentedon Mar 29, 2022
Regarding returning
ReactTestInstancefrom queries – we considered replacing it with aQueryResultobject or something, that would only give you access to specific fields instead of whole object. This is potentially breaking, but the direction where we'd like to go.pierrezimmermannbam commentedon May 1, 2022
@AugustinLF @thymikee I have made some progress on this, I opened a draft pr but I've hit an issue. The queries by text work fine using the json representation but there are some problems.
First, I still need to get the ReactTestInstance from the renderer.root because the instances in the ReactTestRendererTree type are in fact not of type ReactTestInstance and do not have parents which is problematic for the fireEvent. I was able to fix this by finding the ReactTestInstance matching the result of the search though I'm not entirely sure it is reliable.
The second issue is with within. For within to work, the queries return type and what is used to build the api need to be of the same type and I broke that by building queries with a ReactTestRenderer type and returning the same type as before. It would be doable to have the following type :
However this would require to change not only the byText queries but all of them, which is more complex but shouldn't be too much of an issue if well refactored.
It would change the type of the api but not cause a breaking change as we'd be only adding a new field so it shouldn't be too problematic.
So before going further I wanted to have your inputs on this and make sure the implementation I started was what you also had in mind
thymikee commentedon May 4, 2022
cc @mdjastrzebski
mdjastrzebski commentedon May 4, 2022
TLDR
string text output (not wrapped in
Textobject) is not traversable usingReactTestInstance.findAllmethod that all text queries use. However, you can fix the test quite easily by wrappingTransoutput inTextinstance like this:Longer explanation
We we run
getByTextit internally usesReactTestInstance.findAllto go through all elements of render tree.Having this as our render input:
We get
findAlltraversing following nodes ofReactTestInstancetype:As you can see there is no final "text node" with
"My text"content. The leaf component is ourTranscomponent.From my investigation in what it offers in terms of API, there is no rendered output field available for easy examination. However when I inspect the actual object structure under tests we can see React fiber details:
{ _fiber: <ref *1> FiberNode { tag: 0, key: null, elementType: [Function: Trans], type: [Function: Trans], // omitted for brevity child: FiberNode { tag: 6, key: null, elementType: null, type: null, stateNode: [Object], return: [Circular *1], child: null, sibling: null, index: 0, ref: null, pendingProps: 'My text', memoizedProps: 'My text', // ... omitted for brevity }, sibling: null, index: 0, ref: null, pendingProps: { i18nKey: 'My text' }, memoizedProps: { i18nKey: 'My text' }, // ... omitted for brevity } }It's pretty lengthy but under
_fiber.childwe find the actual text content. Otherwise there is not way to access the rendered output.I would be wary of building our code based on internal React representation and stick to public types only.
mdjastrzebski commentedon May 4, 2022
Regarding using
ReactTestRendererTree/ReactTestRendererJSONas a basis for working with queries that's an interesting question. It seems that there might be some benefit for using these however this would a very deep change for RNTL and should be careful considered whether it is fit for our purpose, also the light of the fact that all our queries returnReactTestInstanceand users code is probably relying it, so changing that contract would be a really breaking change.9 remaining items
pierrezimmermannbam commentedon Sep 20, 2022
@mdjastrzebski The same test also fails in the RNTL repo with the patch-package applied, but that's because the testenvironment is node, I was able to make the test pass by switching the environment to jsdom
mdjastrzebski commentedon Sep 20, 2022
@pierrezimmermannbam Hmmm that's an interesting development, did you make it pass in RN repo or RNTL repo by switching to
jsdomenv?pierrezimmermannbam commentedon Sep 20, 2022
in RNTL repo, I haven't tried in React repo
pierrezimmermannbam commentedon Sep 20, 2022
@mdjastrzebski nevermind it doesn't work either with jsdom environment, I mixed things up.
pierrezimmermannbam commentedon Sep 20, 2022
It works for the case with the link (not when rendering the view) if toJSON method return toJSON(this._fiber.child.stateNode) instead of toJSON(this._fiber.stateNode) so there seems to be cases where you'd want to use the child or maybe the children if there are several, I have no clue yet what's the logic behind this though
mdjastrzebski commentedon Sep 21, 2022
Maybe this is due to composite vs host element being passed. The RNTL test passed result of 'getByTestId' so a host view.
pierrezimmermannbam commentedon Sep 21, 2022
It also works when using getByText, but when trying to use toJSON on the container returned from the render function it fails
In this case also using this_fiber.child.stateNode works but this time this._fiber_stateNode is not null but it doesn't have a tag property
pierrezimmermannbam commentedon Sep 22, 2022
@mdjastrzebski The following implementation seems to work
You were right it does look like it only works on host component
mdjastrzebski commentedon Sep 25, 2022
@pierrezimmermannbam I've been able to make some reasonable progress, pls take a look at https://github.com/mdjastrzebski/react/blob/5ec75abd232a2315d90c30d3472ee5ce6130dbcf/packages/react-test-renderer/src/ReactTestRenderer.js#L132
I've created exported
toJSONfunction that essentially calls the originaltoJSONwhen it is a host element, otherwise it recursively calls itself on all its children and collects the results.Proposed implementation passes all RTR internal tests on equality between
renderer.toJSON()withtoJSON(renderer.root): https://github.com/mdjastrzebski/react/blob/5ec75abd232a2315d90c30d3472ee5ce6130dbcf/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.jspierrezimmermannbam commentedon Sep 26, 2022
@mdjastrzebski awesome! I see you're now exporting a function instead of adding a toJSON method to the ReactTestInstance class, why is that ? Wouldn't it be more convenient to have the method on the class ? It would allow for instance to take snapshots of elements returned by query without having to call the toJSON function and still have the JSON representation
mdjastrzebski commentedon Sep 26, 2022
@pierrezimmermannbam I've started with implementing
toJSONas a method onReactTestInstance, however I've encountered two issues that made free function a more natural approach:testInstanceToJSONis recursive, as it needs to call itself for child nodes, and that requires ability to pass node on which to run the functiontestInstanceToJSONaccepts eitherReactTestInstance | stringas sometimes children might be string. Particular there was one existing unit test forrenderer.toJSON()method that I could not duplicate asrenderer.root.toJSON()becauserenderer.rootwas actually astring.Thinking about this after your comment, I think that point 1 might be solvable by invoking
child.toJSON()fromtestInstance.toJSON()and adding child type checks forstringvalues. I will review my code tomorrow to see which feels like a better API.mdjastrzebski commentedon Sep 27, 2022
@pierrezimmermannbam I refactored back into
toJSONmethod onReactTestInstanceand I've submitted PR: react/react#25329Let's see how the review goes...
pierrezimmermannbam commentedon Mar 14, 2023
It looks like this issue is fixed! I think this pr #1222 of @mdjastrzebski was the fix because we now don't look only at the direct children, so I guess from the beginning we only needed to look deeper as long as children weren't root elements
mdjastrzebski commentedon Mar 16, 2023
@pierrezimmermannbam Indeed. This exact issue seems to be solved now.