rails / rails-dom-testing

Extracting DomAssertions and SelectorAssertions from ActionView.
MIT License
175 stars 57 forks source link

Ignoring whitespace differences `assert_dom_equal`? #62

Closed chancancode closed 1 year ago

chancancode commented 7 years ago

I think in practice a lot of people who tries to use assert_dom_equal will run into whitespace issues.

For example, if you have this template:

<div class="header">
  <h1><%= @post.title.titlecase %></h1>
</div>

...and in your test:

assert_dom_equal rendered.at('.header').inner_html, "<h1>Rails Is Omakase</h1>"

The test will fail due to whitespace differences.

In this simple case you can fix it by doing inner_html.strip, but consider this template:

          <div class="header">
            <h1><%= @post.title.titlecase %></h1>
            <h2>Published on <%= format_date @post.published_at %></h2>
          </div>

In this case you will have to do something more involved, perhaps using strip_heredoc, etc.

While they do produce different DOM structures, because they browser doesn't actually render these differences, and that the spirit of the helper is to test "equivalency" (e.g. it ignore attributes ordering), I highly doubt that the current failure is useful (maybe we can give you a strict mode, for edge cases like <pre> content etc).

Unfortunately, it is not easy to come up with a simply fix for this. Ideally, we would "just do what the browser does", but that is actually not so simple – for one it depends on inputs that we don't have, e.g. the language of the text and things like CSS rules.

That said, I think it's still worthwhile/possible to come up with a Good Enough™ set of heuristics that work for the 99% cases (and if it doesn't work, you can use the strict mode).

Does that sound reasonable? Is this a behavior that we can just change, or would it require an opt-in, and/or going through a deprecation cycle, etc?

chancancode commented 7 years ago

Using the link_to example in the documentation, and along with #60 and #61, we will be able to write tests like these:

test "link_to can accept a block" do
  render <<-ERB
    <%= link_to "https://www.example.com" do %>
      <%= content_tag :span, "Apples" %>
    <% end %>
  ERB

  assert_dom_equals <<-HTML
    <a href="https://www.example.com">
      <span>Apples</span>
    </a>
  HTML
end

Another case it comes up is when you have the "no-output" things in the template:

test "rendering a list" do
  assigns users: [User.new("David"), User.new("Jeremy"), User.new("Rafael")]

  render <<-ERB
    <div>
      <ul class="users">
        <% @users.each do |user| %>
          <li><%= user.name %></li>
        <% end %>
      </ul>
    </div>
  ERB

  assert_dom_equals <<-HTML
    <div>
      <ul class="users">
        <li>David</li>
        <li>Jeremy</li>
        <li>Rafael</li>
      </ul>
    </div>
  HTML
end

Because of the extra <% @users.each do |user| %>...<% end %>, the indentation would not normally match.

chancancode commented 7 years ago

@jeremy @matthewd @rafaelfranca @kaspth thoughts? 😬

I am happy to propose a patch/heuristics, just want to get a 👍 on the general direction before I do it – specifically...

1) do you think this is useful 2) is it okay that it "mostly does what you would expect", but isn't exactly the same as what happens in the browser's rendering engine (and there will be a strict mode for the current behavior) 3) how important is backwards compat here and how I should approach it (I don't think it will make any currently passing tests fail, but in theory it could change the meaning of your tests, but I currently believe that to be very rare)

jeremy commented 7 years ago
  1. Yes!
  2. Yes!
  3. Considering this doesn't break existing tests and only brings it closer to the (very clear) intent of the assertion, I think compatibility is not a concern.
kaspth commented 7 years ago

@chancancode yes please 😍

joshpencheon commented 6 years ago

I've opened a PR with an initial stab at this here: rails/rails-dom-testing/pull/71.