Make assert_dom_equal ignore insignificant whitespace when walking the node tree

[jduff]

Yet another take on fixing #62

Prior art: #71, #66 and #83

This version ignores the whitespace as we are walking the tree instead of trying to pre-process or clean the html strings. Also included a strict option to preserve the assertion including whitespace (default to false).

I'm not a fan of passing strict through the method calls but wasn't sure of a better approach.

Let me know what you think and if there are any changes I should make!

[chancancode]

I think this is a pretty solid attempt and the best one we've had so far. I think once we fix the interior whitespace issue, it seems good to land this and iterate further when there are bug reports. Thanks for working on this!

[jduff]

@chancancode thanks for taking a look! I just updated this PR with a test and fix for the issue with interior whitespace you mentioned. If there is anything else let me know.

[chancancode]

Urgh, seems like CI config is super old and 1.9 does not support kwarg. You can either change it to take a hash there to avoid the syntax error, or you can fix the CI config to drop support for EOL'ed Rubies and Rails, and add matrix rows for newer Rubies + Rails, in which case we can cut a breaking change release after landing this.

[jduff]

@chancancode instead of lumping it all together I created another PR updating the test matrix https://github.com/rails/rails-dom-testing/pull/86

[kaspth]

I don't have much overhead for intricate reviews right now. @chancancode if you want to help carry this, I'll happily merge 🙏

[chancancode]

I think this is basically good to go if we merge the CI fix and rebase this on top. We will have to make a new breaking release after due to dropping old ruby versions.

[chancancode]

However, I apparently can’t merge 😛

[jduff]

I just pushed up a rebase!

[chancancode]

@kaspth This seems good to me. If you can look into getting my commit/publish bit back then I can help with the merge/release as well, I assume that got lost in shuffling the GH teams since I still have the rails/rails one, and this seems much lower stakes than that. I also don't mind not having it if you want to do the merge/publish.

@jduff Since we stopped testing Rails 4.2 on CI, we should probably bump this to require the versions we are testing. It doesn't have to be part of this PR, but it does need to be changed before we release.

[kaspth]

@chancancode done, appreciate the help 👍 — and thank you for the PR, @jduff 🙌

[jduff]

Thanks for helping see this through! I've been using master in one of my projects and it's working great 👌