-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Variables in JSX strings put on new line #53
Variables in JSX strings put on new line #53
Comments
Given that in html any extra whitespace between words in text is not meaningful, and the resulting text shown to the user on screen is what you expect in your test above, I think it makes sense to take this into account when matching an element's text content. Actually I am now wondering if |
Yes. We should update the |
I'll take it, since I'll probably also have to do this to jest-dom's (BTW, this is really a PR for dom-testing-library) |
I'm still having this issue, any idea why it would continue to add white spaces? Updated to the latest dom-testing-library. |
@DarrylD your best bet for others to be able to help is to provide a sample repository or codesandbox where the problem can be reproduced. |
I have also continued to experience the same issue, and was able to diagnose it (as far as my case goes, at least). The problem is that the amendment to
will end up as Then, however, you'd be required to ensure a space is present after each separate word that's on a different line, so But I'm not really proposing this change be made, as you shouldn't have to ensure those spaces are present in your JSX. |
This is actually how HTML works. If you were to put this in the DOM: <div>
Pay
£
24.99
</div> The resulting output would appear to the user as:
This is why we do the |
That is a very good point. I guess then all you can do is ensure that you concatenate strings like this before inserting them into JSX. |
I'm running into the same problem, even with strings that don't have a space between the variable and normal text, e.g. In this situation, the DOM node's Live example: https://codesandbox.io/s/w718n5ojq7 |
Ah, this is tricky! What do you propose? |
Good question! I'm not at all familiar with the internals of this library, but following @maxcct's lead, replacing I haven't made a proper fork of I also wrote a quick (passing 🙌) test to see if the change works as expected across text nodes: test('can get elements by matching their text across adjacent text nodes', () => {
const textDiv = document.createElement('div')
const textNodeContent = ['£', '24', '.', '99']
textNodeContent
.map(text => document.createTextNode(text))
.forEach(textNode => textDiv.appendChild(textNode))
const {container, queryByText} = render('<div />')
container.appendChild(textDiv)
expect(queryByText('£24.99')).toBeInTheDOM()
}) In any event, since this issue doesn't seem specific to React, maybe it would be better to move the discussion over to an issue and/or PR in |
Yeah, this should be talked about in |
**What**: Changes the way `getNodeText` joins nodes. Instead of joining them with a space, it now joins them with an empty string. <!-- Why are these changes necessary? --> **Why**: This PR comes out of testing-library/react-testing-library#53 In React, if you have an element like `<span>{volts}V</span>`, then the generated element will have two text nodes, one for the variable `volts`, and one for the normal string. When browsers render this, there is no space between the text nodes. Likewise, the `textContent` property on the span will return a string without spaces (e.g. `300V`). However, the current implementation of `getNodeText` joins all of the nodes with a space, so if you try to use e.g. `queryByText('300V')` it will return `null`. For a live demo, see https://codesandbox.io/s/w718n5ojq7 <!-- How were these changes implemented? --> **How**: - changing `join(' ')` to `join('')` on [line 8 of `src/get-node-text.js`](https://github.com/kentcdodds/dom-testing-library/compare/master...bdchauvette:pr/get-node-text-whitespace?expand=1#diff-6350d468f7684d134aab9d42d96a77beR8) - adding [a test](https://github.com/kentcdodds/dom-testing-library/compare/master...bdchauvette:pr/get-node-text-whitespace?expand=1#diff-de31c4d0bed96b2a4211de164bb1b589R59) for querying against a DOM element containing adjacent text nodes without any whitespace <!-- Have you done all of these things? --> **Checklist**: <!-- add "N/A" to the end of each line that's irrelevant to your changes --> <!-- to check an item, place an "x" in the box like so: "- [x] Documentation" --> - [ ] Documentation - **N/A** (I think?) - [x] Tests - [x] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? --> - [x] Added myself to contributors table <!-- this is optional, see the contributing guidelines for instructions --> <!-- feel free to add additional comments --> Thanks! 😸 Closes testing-library/react-testing-library#53
* Add within API and document it * improve the `within` API example * Update README.md
I continue with the same problem. For example:
So getByText will fail saying Unable to find an element with the text when I try to do What can I do to make this work? |
This is also happening for me. My JSX:
The output: <h1
class="c-PJLV c-PJLV-ddalwR-size-large"
>
Leave
Foo
?
</h1> And the test: it('confirms with the user whether they wish to leave', async () => {
expect(
await screen.findByText('You are about to leave the Foo community'),
).toBeInTheDocument()
}) Not sure if this is actually the same issue, but it's definitely the same symptom. Noticed it when I upgraded to React 17 and started using the new JSX transform. I was able to work around this by changing my assertion to: expect(
await screen.findByText(/You are about to leave the(.*)Foo(.*)community./)
).toBeInTheDocument() |
I'm using an older version, so maybe an update (not an option right now) would solve this, but I got it working with a solution similar to yours: expect(screen.getByRole('button', { name: /Factors\s/ })).toBeInTheDocument(); |
This is still an issue for me. Is there going to be any solution for this? My use case:
I worked around by querying data-testid and then assert with
|
Is there any issue about changing the behaviour how this is rendered and so snapshots for me it is just looks false if I have component like this: <div>
{charactersLeft} {translate('sulu_admin.characters_left')}
<div> Is snapshoted as: <div>
-8
sulu_admin.characters_left
</div> Instead of and doesnt so represent what JSX is rendering: <div>
-8 sulu_admin.characters_left
</div> |
We managed to solve this problem by manually joining the strings like so: <div>
{charactersLeft + ' ' + translate('sulu_admin.characters_left')}
</div> The code above generates the following (desired) snapshot result: <div>
-8 sulu_admin.characters_left
</div> |
**What**: Changes the way `getNodeText` joins nodes. Instead of joining them with a space, it now joins them with an empty string. <!-- Why are these changes necessary? --> **Why**: This PR comes out of testing-library/react-testing-library#53 In React, if you have an element like `<span>{volts}V</span>`, then the generated element will have two text nodes, one for the variable `volts`, and one for the normal string. When browsers render this, there is no space between the text nodes. Likewise, the `textContent` property on the span will return a string without spaces (e.g. `300V`). However, the current implementation of `getNodeText` joins all of the nodes with a space, so if you try to use e.g. `queryByText('300V')` it will return `null`. For a live demo, see https://codesandbox.io/s/w718n5ojq7 <!-- How were these changes implemented? --> **How**: - changing `join(' ')` to `join('')` on [line 8 of `src/get-node-text.js`](https://github.com/kentcdodds/dom-testing-library/compare/master...bdchauvette:pr/get-node-text-whitespace?expand=1#diff-6350d468f7684d134aab9d42d96a77beR8) - adding [a test](https://github.com/kentcdodds/dom-testing-library/compare/master...bdchauvette:pr/get-node-text-whitespace?expand=1#diff-de31c4d0bed96b2a4211de164bb1b589R59) for querying against a DOM element containing adjacent text nodes without any whitespace <!-- Have you done all of these things? --> **Checklist**: <!-- add "N/A" to the end of each line that's irrelevant to your changes --> <!-- to check an item, place an "x" in the box like so: "- [x] Documentation" --> - [ ] Documentation - **N/A** (I think?) - [x] Tests - [x] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? --> - [x] Added myself to contributors table <!-- this is optional, see the contributing guidelines for instructions --> <!-- feel free to add additional comments --> Thanks! 😸 Closes testing-library/react-testing-library#53
getting something very similar here - very strange behaviour. This issue is not closed.
it's adding new lines above my elements |
Any workaround for the above mentioned issues? This is a really old issue by now, and no idea why its marked as closed, when no specific answer was provided (by anyone) on how should we handle such scenarios. I, as many others have already stated, am having an issue where the following JSX in my component: <div>
<h2>{status} text ({count})</h2>
</div> is rendered like this in the tests: <h2>
new
ads (
1
)
</h2> And I cannot use I tried using |
@pstevovski workaround string concat by @b3h3m0th should work in your case: #53 (comment) |
@alexander-schranz Thank you for your answer, and sorry for the late reply, no idea why but I didn't get any notification. The answer provided by @b3h3m0th doesn't really work for my case in terms of practicality, I'd have to go and change a lot of the pieces trough out the application to convert them to be manually concatenated like that. And also, to be fair, that is just a "workaround" (as you mentioned), but then again accessing and reading dynamic values in React (or in JS in general) has better ways like we're all already using (either that be template literals or the way we use them in react). =================== Closest thing I could work with was using I know this is an old issue, but I do believe that there really should be a way by RTL to be able to easily target these JSX variables with dynamic values. ================== And on the other hand, RTL also has problems when targeting elements like: <p>
<span>Example</span> <strong>text</strong>
</p> If wanted to target the entire type Query = (queryMatcher: MatcherFunction) => HTMLElement;
/**
*
* Helper matcher function to be used together with React Testing Library matchers.
*
* Helps find an element in the rendered testing UI that has nested HTML elements:
*
* @example
* <h1>
* <span>Title</span> with
* <strong>nested</strong> elements.
* </h1>
*
* @param query The query matcher function to be used, provided by RTL
* @param text The text by which we want to find the targeted element
*
* @returns The targeted element (if found). Otherwise, throws an error.
*
*/
export const getByTextWithMarkup = (query: Query, text: string): HTMLElement => {
return query((_content: string, node: any) => {
const hasText = (node: HTMLElement) => node.textContent === text;
const childrenDontHaveText = Array.from(node.children).every(
child => !hasText(child as HTMLElement),
);
return hasText(node) && childrenDontHaveText;
});
}; Leaving this here as it might help someone out there (although it doesn't really help with the main issue that was reported). |
react-testing-library
version: 2.1.0node
version: 8.9.3npm
(oryarn
) version:yarn
1.3.2Relevant code or config
Component to be tested:
What you did:
Test code:
What happened:
Reproduction repository:
This can be reproduced in the current repository test cases by inserting a variable into the JSX.
Problem description:
Variables in JSX strings cause new line issues (and failing tests). I believe that in the spirit of "not testing implementation details", this should be considered a harmful problem. If I add an extra space around the "1", the test passes (period signifies space):
Suggested solution:
I don't know why this is happening, but I'm guessing it is something to do with how the component is being serialised. I'm also guessing that this issue should be here and not dom-testing-library but I don't know for sure where the root of the problem lies.
The text was updated successfully, but these errors were encountered: