-
Notifications
You must be signed in to change notification settings - Fork 273
Error: Unable to find an element with text: "My text", while that text exist. #937
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
Comments
Yep, easy to reproduce, will try to figure out what happens but that should be fixable on our end. |
@AugustinLF easy to reproduce but not easy to fix? |
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 |
@AugustinLF I know that this is a feature request. But is is possible to have the |
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. |
@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 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 I'll add tests for those cases in 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 |
@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 : type QueryResult = ReactTestInstance & { tree : ReactTestRendererTree } 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. 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 |
TLDRstring text output (not wrapped in const Trans = (p) => <Text>{p.i18nKey}</Text>; Longer explanationWe we run Having this as our render input: const Trans = (p) => p.i18nKey;
const screen = render(
<Text>
<Trans i18nKey="My text" />
</Text>
); We get
As you can see there is no final "text node" with 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 I would be wary of building our code based on internal React representation and stick to public types only. |
Regarding using |
It would be nice if we could continue using Can we somehow make React Test Renderer to export |
@mdjastrzebski I think it is a very interesting approach, it could event be possible to still use the reactTestInstance.findAll method and then only use the ReactTestRendererJSON type for the getChildrenAsText method which would be a way less significant change. I'll try it and if it works ask on the React test renderer repo whether it is a change they are willing to make |
✕ renders learn react link (36 ms) ● renders learn react link
Test Suites: 1 failed, 1 total How to resolve this error? |
@nanda-kumar-k the posted errors seems to concern React Testing Library, and not React Native Testing Library, as the stack trace mentions |
@mdjastrzebski There is an issue on the react repo to export the toJSON method facebook/react#14539 but it's stale. Also I've looked a bit at react-test-renderer code and the toJSON method does not use the type ReactTestInstance, but something entirely different that's not exported either. And indeed it is probably impossible to go from ReactTestInstance to ReactTestRendererJSON type or else we should be able to fix this issue by using a ReactTestInstance. This means that this issue cannot be solved until the queries are using ReactTestInstance and since we should be able to do queries on elements returned by queries they should also return something else in order for this to be fixed |
@pierrezimmermannbam I've recently started working on PR to |
@mdjastrzebski yes I'm very interested ! How to you intend to manage this ? |
So I've stated with locally modifying the That allowed me to write some POC tests in RNTL repo, e.g.: import * as React from 'react';
import { View, Text } from 'react-native';
import { render, screen } from '../..';
const Trans = () => 'Hello';
test('toJSON of Text with Trans', () => {
render(
<View>
<Text testID="subject">
<Trans />
</Text>
</View>
);
const subject = screen.getByTestId('subject');
expect(subject.toJSON()).toMatchInlineSnapshot(`
<Text
testID="subject"
>
Hello
</Text>
`);
}); So far so good. Next I went to implement this as a PR for Here is the respective branch on my React repo fork: https://github.com/mdjastrzebski/react/tree/feature/test-instance-expose-toJSON However, when trying to write tests for // ReactTestRenderer-test.internal.js
describe('ReactTestRenderer', () => {
// This is original RTR test
it('renders a simple component', () => {
function Link() {
return <a role="link" />;
}
const renderer = ReactTestRenderer.create(<Link />);
expect(renderer.toJSON()).toEqual({
type: 'a',
props: {role: 'link'},
children: null,
});
// Below is my added part that fails
expect(renderer.root.toJSON()).toEqual({
type: 'a',
props: {role: 'link'},
children: null,
});
});
}); In the tests the @pierrezimmermannbam maybe you can spot some issue with code or tests? I've added you as a collaborator to my forked React repo. |
@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 |
@pierrezimmermannbam Hmmm that's an interesting development, did you make it pass in RN repo or RNTL repo by switching to |
in RNTL repo, I haven't tried in React repo |
@mdjastrzebski nevermind it doesn't work either with jsdom environment, I mixed things up. |
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 |
Maybe this is due to composite vs host element being passed. The RNTL test passed result of 'getByTestId' so a host view. |
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>
<Text testID="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 |
@mdjastrzebski The following implementation seems to work toJSON(): ReactTestRendererNode | null {
if (typeof this._fiber.type === 'string') {
return toJSON(this._fiber.stateNode);
}
return toJSON(this._fiber.child.stateNode);
} You were right it does look like it only works on host component |
@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 Proposed implementation passes all RTR internal tests on equality between |
@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
Thinking about this after your comment, I think that point 1 might be solvable by invoking |
@pierrezimmermannbam I refactored back into Let's see how the review goes... |
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 |
@pierrezimmermannbam Indeed. This exact issue seems to be solved now. |
Describe the bug
getByText
can find element if text render component with text as not children (See "Steps to Reproduce" below)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
Screenshots
Output:
Versions
The text was updated successfully, but these errors were encountered: