Skip to content

assert_dom :text collapses whitespace #123

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

Merged

Conversation

jyeharry
Copy link
Contributor

@jyeharry jyeharry commented Mar 10, 2025

  • Add macos to supported platforms in Gemfile.lock
  • Add failing test for assert_dom collapsing whitespace
  • Collapse whitespace from :text but not :html

Fixes #121

This PR is my preferred solution over #122.

Instead of making use of a :strict operator to determine whether or not to collapse excess whitespace as the browser does, we instead use the existing :html equality operator to match for text with all whitespace included. The :text equality operator is then optimised to collapse all whitespace.

In my opinion, :text should match what would be returned by Element.innerText in javascript land, in that it does not include the excess whitespace. Element.innerHTML on the other hand, does include all of the whitespace that was in the document which I think is what the :html operator should match (which it does already) and so :html could instead be used as a :strict parameter.

The only difference between what Element.innerText and the updated :text equality operator does is that Element.innerText replaces <br> tags with \n, whereas our :text operator just removes them without replacing them. But this is out of our control as it is Nokogiri that does this. Regardless, this is my preferred solution but I made the two PRs since the original issue I raised specifically mentions a strict parameter.

@flavorjones
Copy link
Member

flavorjones commented Mar 10, 2025

There seems to be an issue with the CI workflows related to the Logger gem. I'll fix it on main and then you can rebase this and #122.

@flavorjones
Copy link
Member

flavorjones commented Mar 10, 2025

OK, main is green, so please rebase when you have a chance.

@jyeharry
Copy link
Contributor Author

So to confirm, you won't both PRs merged, not just this one? I made the two PRs so that there were a couple of options for how this would be implemented, with the intention that only one would get chosen. Let me know what you think is best.

I should probably also update the commented docs just before the assert_dom definition to make the updated behaviour of :text and :html a little more clear.

@flavorjones
Copy link
Member

flavorjones commented Mar 14, 2025

So to confirm, you won't both PRs merged, not just this one

I just want whatever you want to submit rebased so I can see if the tests pass, please! Or let me know if you want me to do it, that's fine, too.

I'm sorry I've been busy this week, but in general I want to see:

  • green CI (tests passing)
  • then I will look at the code and review it
  • then we can make a decision about merging

and we're still on that first item until the branch is rebased on the current main. I hope that makes sense and clarifies? 🙏

@jyeharry
Copy link
Contributor Author

Ahhh right sorry for some reason I read "rebase" as merge the pull request. I'll rebase now.

@jyeharry jyeharry force-pushed the feature/assert_dom-ignore-whitespace-v2 branch from 27f1469 to c052922 Compare March 15, 2025 02:47
@jyeharry
Copy link
Contributor Author

@flavorjones Rebased and tests and ci is passing

@jyeharry
Copy link
Contributor Author

jyeharry commented Apr 1, 2025

@flavorjones just following up on this

@rafaelfranca rafaelfranca force-pushed the feature/assert_dom-ignore-whitespace-v2 branch from c052922 to 4241aa0 Compare May 20, 2025 17:43
@rafaelfranca rafaelfranca merged commit 6a4b9db into rails:main May 20, 2025
@joelhawksley
Copy link

joelhawksley commented May 21, 2025

👋🏻 just wanted to share that this change broke the ViewComponent test suite: https://github.com/ViewComponent/view_component/actions/runs/15148835486/job/42590881883?pr=2309.

The fix was pretty minor (https://github.com/ViewComponent/view_component/pull/2309/files#diff-0553d15257b3cb95586c15820fe4dbe55ce1d4e3bae03f0f37d88fafcc50709b), but I would argue that this PR was a breaking change.

I'm 💯 OK with leaving this be as the fix was very minor on our end, but figured I'd flag the issue in case others run into it.

Edouard-chin added a commit to Edouard-chin/rails that referenced this pull request May 22, 2025
- Fix rails#55093
- The new version of dom testing now strip whitespaces when making
  a `text` assertion (See rails/rails-dom-testing#123).
  Changing to an html assertion to preserve the previous behaviour
  and make the test pass on rails-dom-testing 2.3
Edouard-chin added a commit to Edouard-chin/rails that referenced this pull request May 23, 2025
- Fix rails#55093
- The new version of dom testing now strip whitespaces when making
  a `text` assertion (See rails/rails-dom-testing#123).
  Changing to an html assertion to preserve the previous behaviour
  and make the test pass on rails-dom-testing 2.3
Edouard-chin added a commit to Edouard-chin/rails that referenced this pull request May 23, 2025
- Fix rails#55093
- The new version of dom testing now strip whitespaces when making
  a `text` assertion (See rails/rails-dom-testing#123).
  Changing to an html assertion to preserve the previous behaviour
  and make the test pass on rails-dom-testing 2.3
@ccasabona
Copy link

Changing the behavior of collapsing the white space has broken hundreds of tests. We'll have to stick with 2.2.0.

@jyeharry
Copy link
Contributor Author

@rafaelfranca should some documentation or changelog be created to let users know that the update is a breaking change and that the :html equality operator must be used to match all whitespace?

patrickpatrickpatrick added a commit to alphagov/whitehall that referenced this pull request Jun 4, 2025
In rails/rails-dom-testing#123 a change was made to `assert_select`
which meant that `text:` now collapses whitespace. The tests impacted
by this have been amended.
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.

assert_dom should ignore whitespace just like assert_dom_equal
5 participants