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

Maintain Capybara 2 behavior for Capybara 3 #95

Closed
codener opened this issue May 21, 2019 · 5 comments
Closed

Maintain Capybara 2 behavior for Capybara 3 #95

codener opened this issue May 21, 2019 · 5 comments

Comments

@codener
Copy link
Member

codener commented May 21, 2019

Capybara 3 has changed the page.has_text? method that is used e.g. by the I should (not) see … steps to respect white space, including \n. This breaks tests that rely on the previous behavior, which normalized all white space like Rails' squish.

Fortunately, the has_text? method takes options and passes them to assert_text. This method takes a :normalize_ws option that restores the previous behavior if set to true:

https://github.com/teamcapybara/capybara/blob/5146c4aa1f7bd95a0520582aa34ecf55243ea313/lib/capybara/node/matchers.rb#L662

An update for the I should see … steps could look like this:

patiently do
  expect(page).to have_text(text, normalize_ws: true)
end
@judithroth
Copy link
Contributor

I thought of this myself. What kept me from suggesting it is that I don't think we should try to polyfill Capybara. On the other hand I have seen people just overwriting the I should (not) see-step in their own project with that behaviour. And I don't want us to end up copying such steps from project to project.
I would prefer a) stripping whitespaces ourselves and thus having less dependencies on certain Capybara versions or b) adding an option to the step to activate normalize_ws.

@codener
Copy link
Member Author

codener commented May 22, 2019

I agree we should avoid making developers copy (overridden!) steps from project to project and instead give them a step that just works™ – after all that's what Spreewald is about.

We tried and overrode the I should (not) see step in a project, planning to strip white space ourselves. Unfortunately, this is not how Capybara works, as extracting the visible text from a webpage is a complex process (see the linked code in OP).

We also tried to add a modified step I should (not) see ... ignoring white space, but since we actually don't bother whether there's white space or not, we should not be explicit on that. Another attempt was to use regular expressions, but this is incredibly slower (about 5x).

I do not think we should ever consider newlines, tabs etc in the I should (not) see step because it does not even offer a way to express e.g. a newline character. Using the Capybara API to normalize white space is just the right way in my eyes.

@judithroth
Copy link
Contributor

@makmic found a nice solution to this: You can set Capybara.default_normalize_ws = true in your project to use the old behaviour of Capybara per default.
If this option is not available in future versions of Capybara, we could easily switch to stripping whitespace ourselves:

Then /^(?:|I )should see "([^"]*)"$/ do |text|
  patiently do
    actual_text = page.text.gsub(/\s+/, ' ')
    expected_text = text.gsub(/\s+/, ' ')
    expect(actual_text).to include(expected_text)
  end
end.overridable

Then /^(?:|I )should not see "([^"]*)"$/ do |text|
  patiently do
    actual_text = page.text.gsub(/\s+/, ' ')
    expected_text = text.gsub(/\s+/, ' ')
    expect(actual_text).not_to include(expected_text)
  end
end.overridable

We tested this in 3 of our lager projects and it worked like a charm. But we both prefer to set a project default for Capabara's whitespace stripping as long as it is possible, so that everyone can use it the way (s)he wants.

I'm closing this for now as I don't see a need for further discussion. If you don't agree please feel free to reopen the issue :)

@codener
Copy link
Member Author

codener commented May 24, 2019

The global Capybara.default_normalize_ws = true is a great fix for this issue. Full agreement 👍

@codener
Copy link
Member Author

codener commented May 24, 2019

Quick follow-up: we should tell users about the Capybara option. A couple of ideas:

  1. Adding a comment on the I should (not) see steps, so users may find it when debugging their app after a Capybara upgrade
  2. Adding a one-line post-install message, so users see it when upgrading. (However, they'll see it when upgrading Spreewald, not Capybara where it would be necessary.)
  3. Adding a note on the Readme under the Usage section

I would suggest to add 1 and 3.

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

No branches or pull requests

2 participants