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

make assert_dom_equal ignore insignificant whitespace #83

Closed
wants to merge 1 commit into from

Conversation

jduff
Copy link
Contributor

@jduff jduff commented May 18, 2020

Another take on fixing #62 . This version strips any newlines, or newlines with surrounding whitespace, from the html before testing if they are equal. There is also an added option, strict to preserve the assertion including whitespace (default to false).

If we want to change this to opt in to the new behaviour we can change the default to strict: true.

Previous attempts: #71 and #66

@chancancode
Copy link
Member

I think I mentioned this in the previous threads, but I don't think we can use a regex for this. IMO, for correctness, we will have to walk the nokogiri tree and normalize the whitespace we encounter.

@chancancode
Copy link
Member

For example, one test case you can write to demonstrate why a regex doesn't work is <div data-wow="Don't strip this">.

Alternatively, instead of preprocessing the nokogiri trees, we can also just try to do it when walking the trees for the actual comparison.

@jduff
Copy link
Contributor Author

jduff commented May 19, 2020

Sorry, I missed the comment about a regex not being preferred. That being said, the regex I used is /[\s]*\n[\s]*/ which requires a newline and removes the newline and any surrounding whitespace, so the example you posted works as expected.

assert_dom_equal(
  %(<div data-wow="Don't strip this">),
  %(<div data-wow="Don't strip this">)
) # pass

# Add an extra space within the attribute so they shouldn't match
assert_dom_equal(
  %(<div data-wow="Don't  strip this">),
  %(<div data-wow="Don't strip this">)
) # fail

Doing it while walking the trees doesn't look too bad either, if we want to go that way. It looks like rejecting blank text nodes here for both expected and actual, then stripping the strings before comparing them here will do it (for the tests I have at least). Making it opt out becomes a bit more complicated though because we need to add conditionals in a few places instead of one. If that is preferred let me know and I can submit another PR with that approach.

@jduff
Copy link
Contributor Author

jduff commented Jun 16, 2020

@chancancode would you be able to take a look at the PR I opened, #84, that implements handling this while walking the tree?

@chancancode
Copy link
Member

Reviewed, thanks!

@jduff
Copy link
Contributor Author

jduff commented Jun 18, 2020

Closing in favor of #84

@jduff jduff closed this Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants