Skip to content

feat: #9: Normalized whitespace using dom-testing-library #56

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

Merged
merged 6 commits into from
Oct 2, 2018
Merged

feat: #9: Normalized whitespace using dom-testing-library #56

merged 6 commits into from
Oct 2, 2018

Conversation

smacpherson64
Copy link
Collaborator

@smacpherson64 smacpherson64 commented Sep 7, 2018

What:
Implementing #9, it required an adjustment from dom-testing-library as testing-library/dom-testing-library#31 makes fuzzy logic optional.

Why:
To allow users to test against text with spacing that varies. The text should be normalized to only have one space.

How:
As discussed in #9 used dom-testing-library's get-node-text function and also included the regex multiple space to one space replacement.

Checklist:

  • Documentation N/A
  • Tests
  • Updated Type Definitions N/A
  • Ready to be merged
  • Added myself to contributors table N/A

@smacpherson64 smacpherson64 requested a review from gnapse September 7, 2018 19:54
Copy link
Member

@gnapse gnapse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. BTW, don't merge yet. I wonder if this is breaking change. It looks like it.

import {checkHtmlElement, getMessage, matches} from './utils'

export function toHaveTextContent(htmlElement, checkWith) {
checkHtmlElement(htmlElement, toHaveTextContent, this)
const textContent = htmlElement.textContent
const textContent = getNodeText(htmlElement).replace(/\s+/g, ' ')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what happens with  s in the text? I guess it's a borderline feature and can probably wait until someone has the pain though.

Copy link
Collaborator Author

@smacpherson64 smacpherson64 Sep 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm, this is a great question. I believe element.textContent will see them as spaces and we would normalize them down to once space which is incorrect in that case (haha, as they are non breaking spaces). Do you think we should make the normalization (of extra spaces) to be optional like dom-testing-library does?

Step
1
of 4
</span>`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest indent this two spaces under the c of const. The extra spaces on the string won't make a difference, and it would be respecting more the code style.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do!

expect(() =>
expect(queryByTestId('count-value')).not.toHaveTextContent('2'),
).toThrowError()
test('handles negative test cases', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good that you started separating the test cases. It was becoming hard to read. 👏 👏 👏

@smacpherson64
Copy link
Collaborator Author

I do believe this is a breaking change. If someone was relying on the spacing their tests would start breaking. If we added this in optionally then maybe not, something like:

expect(element).toHaveTextContent('hey there', { normalizeSpaces: true });

@smacpherson64
Copy link
Collaborator Author

Hey @gnapse, on the last commit, I set the normalization of spaces to be optional (just in case a user desires to test against &nbsp; and intentionally wants the spaces. I am going to invert it in another item, currently I have it turned off, but I think turned on by default (although a breaking change) would make more sense.

@smacpherson64
Copy link
Collaborator Author

smacpherson64 commented Oct 1, 2018

Hi @gnapse, Just checking in on this one!

The last we left off we said we want to hold off on merging:

Looks good. BTW, don't merge yet. I wonder if this is breaking change. It looks like it.

I agree that it is a breaking change, it is set to normalize spaces by default, with an option to turn if off.

Let me know your thoughts,
Thanks for your time!

@gnapse
Copy link
Member

gnapse commented Oct 2, 2018

Ok, let's merge it then!

@gnapse gnapse merged commit 40db857 into testing-library:master Oct 2, 2018
@smacpherson64 smacpherson64 deleted the pr/to-have-text-content-whitespace branch October 2, 2018 13:21
@smacpherson64
Copy link
Collaborator Author

Whoohoo! :-)!

@noinkling
Copy link

noinkling commented Oct 3, 2018

This change breaks testing for text within child elements (nested text), because you switched to using getNodeText from dom-testing-library, which only considers immediate text node children.

Example:

<div class="error">
  <ul>
    <li>Email format is invalid</li>
  </ul>
</div>

I just want to write something like expect(errorBox).toHaveTextContent('email format'), instead of tightly coupling my tests to whatever the internal structure may be. This worked fine in the previous version because it used .textContent (which is what I would expect a method named "toHaveTextContent" to use).

Since it now fails, the best I can come up with is something like expect(errorBox.textContent).toMatch(/email format/i), which isn't nearly as nice.

@kentcdodds
Copy link
Member

expect(errorBox).toHaveTextContent('email format')

I don't think that would have worked before. The regex is how I would have done that before and now 🤔

@smacpherson64
Copy link
Collaborator Author

Hrm, these are really good points! @kentcdodds and @noinkling I am going to copy the conversation to #9 so it doesn't get lost!

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 this pull request may close these issues.

4 participants