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

Ignoring whitespace differences assert_dom_equal? #62

Closed
chancancode opened this issue Feb 24, 2017 · 5 comments
Closed

Ignoring whitespace differences assert_dom_equal? #62

chancancode opened this issue Feb 24, 2017 · 5 comments

Comments

@chancancode
Copy link
Member

chancancode commented Feb 24, 2017

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 chancancode changed the title Collapsing whitespace assert_dom_equal? Ignoring whitespace differences assert_dom_equal? Feb 24, 2017
@chancancode
Copy link
Member Author

chancancode commented Feb 24, 2017

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
Copy link
Member Author

@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
Copy link
Member

jeremy commented Feb 26, 2017

  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
Copy link
Contributor

kaspth commented Mar 9, 2017

@chancancode yes please 😍

@joshpencheon
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants