Skip to content
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

Closed
thchia opened this issue Apr 11, 2018 · 22 comments · Fixed by testing-library/dom-testing-library#19
Closed

Variables in JSX strings put on new line #53

thchia opened this issue Apr 11, 2018 · 22 comments · Fixed by testing-library/dom-testing-library#19

Comments

@thchia
Copy link
Contributor

thchia commented Apr 11, 2018

  • react-testing-library version: 2.1.0
  • node version: 8.9.3
  • npm (or yarn) version: yarn 1.3.2

Relevant code or config

Component to be tested:

const Component = ({ currentStep }) => <div>Step {currentStep} of 4</div>

What you did:

Test code:

const { getByText } = render(<Component currentStep={1} />)

expect(getByText('Step 1 of 4')).toBeDefined()

What happened:

Unable to find an element with the text: Step 1 of 4. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.

<div>
    Step
    1 // <--------- newline
      of 4
 </div>

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):

expect(getByText('Step..1..of 4')).toBeDefined() // passing

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.

@gnapse
Copy link
Member

gnapse commented Apr 11, 2018

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 .toHaveTextContent should also take this into account.

@kentcdodds
Copy link
Member

Yes. We should update the matcher function in dom-testing-library to replace multiple spaces with one before it attempts to do any comparison. I consider this a bug and would love a pull request for it 👍

@gnapse
Copy link
Member

gnapse commented Apr 11, 2018

I'll take it, since I'll probably also have to do this to jest-dom's .toHaveTextContent ;)

(BTW, this is really a PR for dom-testing-library)

@DarrylD
Copy link

DarrylD commented May 14, 2018

I'm still having this issue, any idea why it would continue to add white spaces? Updated to the latest dom-testing-library.

@gnapse
Copy link
Member

gnapse commented May 14, 2018

@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.

@maxcct
Copy link

maxcct commented May 31, 2018

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 getText presupposes that when text is divided over multiple lines, the text on each line will constitute a separate word. Things go wrong when this is not the case; for example:

Pay
£
24.99

will end up as Pay £ 24.99. This problem is remedied if line 73 here is changed from .join(' ') to .join('').

Then, however, you'd be required to ensure a space is present after each separate word that's on a different line, so Pay would have to be Pay (i.e. with a space after it on the same line) in my example. Otherwise, there's no way to differentiate between words on different lines that should be separated by spaces on the one hand, and word fragments/characters on separate lines that should be concatenated on the other.

But I'm not really proposing this change be made, as you shouldn't have to ensure those spaces are present in your JSX.

@kentcdodds
Copy link
Member

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:

Pay £ 24.99

This is why we do the join(' ') in dom-testing-library.

@maxcct
Copy link

maxcct commented May 31, 2018

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.

@bdchauvette
Copy link

I'm running into the same problem, even with strings that don't have a space between the variable and normal text, e.g. <span>{volts}V<span>.

In this situation, the DOM node's textContent property contains a string without spaces, even though there are two Text nodes behind the scenes.

Live example: https://codesandbox.io/s/w718n5ojq7

@kentcdodds
Copy link
Member

Ah, this is tricky! What do you propose?

@bdchauvette
Copy link

Good question!

I'm not at all familiar with the internals of this library, but following @maxcct's lead, replacing join(' ') with join('') in getNodeText seems to do the trick.

I haven't made a proper fork of dom-testing-library yet, but I checked out master, ran the test suite locally with this change, and everything still passed.

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 dom-testing-library?

@kentcdodds
Copy link
Member

Yeah, this should be talked about in dom-testing-library. Could you make a PR there and we can discuss how to solve this there. Thanks!

kentcdodds pushed a commit to testing-library/dom-testing-library that referenced this issue Aug 10, 2018
**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
julienw pushed a commit to julienw/react-testing-library that referenced this issue Dec 20, 2018
* Add within API and document it

* improve the `within` API example

* Update README.md
@anapanadero
Copy link

I continue with the same problem. For example:
<span>My name is (${name})</span>
The result for this code is:

<span> 
My name is ( 
 Ana
)
</span>

So getByText will fail saying Unable to find an element with the text when I try to do
screen.getByText(`My name is ${name})`)

What can I do to make this work?

@tubbo
Copy link

tubbo commented Jun 29, 2021

This is also happening for me.

My JSX:

          <Text as="h1" size="large">
            Leave {data.community.name}?
          </Text>

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()

@klauskpm
Copy link

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();

@OttlikG
Copy link

OttlikG commented Jun 17, 2022

This is still an issue for me. Is there going to be any solution for this?
Can we consider reopening the issue?

My use case:

<Text textStyle="p12" color="typography.titleNormal" mb={6} data-testid="change-created-at">
   Created {format(createdAt, DEFAULT_DATE_PATTERN)}
</Text>

I worked around by querying data-testid and then assert with toHaveTextContent

it('should display created at text', () => {
    renderWithRouter(<Change {...props} />);

    expect(screen.getByTestId('change-created-at')).toHaveTextContent('Created 15 June 2022')
});

@alexander-schranz
Copy link

alexander-schranz commented Jul 28, 2022

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>

@b3h3m0th
Copy link

b3h3m0th commented Jul 28, 2022

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>

qijixin2 pushed a commit to qijixin2/dom-testing-library that referenced this issue Aug 26, 2022
**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
@evanjmg
Copy link

evanjmg commented Dec 13, 2022

getting something very similar here - very strange behaviour. This issue is not closed.

  • Snapshot - 0

    • Received + 1

    @@ -1,6 +1,7 @@

    +

it's adding new lines above my elements

@pstevovski
Copy link

pstevovski commented Jun 30, 2023

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 getByText matcher to target it, as it says its unable to find the element. So.. how can I target it? I do not want to include data-testid attribute in this case (as this is part of a dynamically generated content for a table).

I tried using await screen.findByText(/new text (1)/gim) with the global and multi-line flags attached to the regex, but that is not working either.

@alexander-schranz
Copy link

alexander-schranz commented Jun 30, 2023

@pstevovski workaround string concat by @b3h3m0th should work in your case: #53 (comment)

@pstevovski
Copy link

pstevovski commented Jul 18, 2023

@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 getByText("Example text", { exact: false }), but even that didn't always worked.

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 p element based on the entire text, that wouldn't work like we're expecting. So workaround at least for that type of issue would be a helper matcher function like:

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).

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 a pull request may close this issue.