search

kucaahbe / rspec-html-matchers

Old school have_tag, with_tag(and more) matchers for rspec 3 (Nokogiri powered)
http://rubygems.org/gems/rspec-html-matchers
MIT License
199 stars 90 forks source link

Restore the support for matching `html` and `body` #75

Closed CvX closed 3 years ago

CvX commented 4 years ago

This effectively reverts 306b8f41ba73280379bc1c6c88c615097a4091ce and adds specs for matching html and body elements.

The issue reported in #62 isn't solvable using DOM assertions. DOM tree has to have a root element (html). Using DocumentFragment extracts a part of the tree, so it won't contain html or body elements, but that doesn't mean they weren't a part of the input.

kucaahbe commented 4 years ago

looks like this bug was a thing for a while: https://stackoverflow.com/questions/37692178/rspec-html-matchers-finds-html-and-body-tags-in-html-without-them

CvX commented 3 years ago

Just to reiterate – this PR doesn't fix the issue raised in #62 or that StackOverflow question. I argue that neither did 306b8f41ba73280379bc1c6c88c615097a4091ce - hence the revert of the breaking change and addition of specs to prevent the breakage in the future.

I don't think it's possible to support fragment assertions via Nokogiri.

If you use Nokogiri::HTML::Document (which is what this PR reverts to) the html and body elements are always present. This allows to e.g. assert the existence of classes on those elements:

expect('<html class="test"><body>abc</body></html>').to have_tag('html', with: { class: 'test' })

# but:
expect('<p>test</p>').to_not have_tag('body') # fails

(Losing the ability to assert classes on these elements broke the specs after the 0.9.3 update e.g. here: https://github.com/discourse/discourse/pull/10664)

If you use Nokogiri::HTML::DocumentFragment (as in the reverted commit) html and body elements are always absent:

expect('<html><body>test</body></html>').to have_tag('body') # fails
kucaahbe commented 3 years ago

@CvX thanks for explanation, the argument regarding matching html/body tags attributes sounds good to me, but quirk about matching these tags presence/absence should be signalled.

Looks like we should stick with Nokogiri::HTML, also will make cases like have_tag('html')/have_tag('body') raise ArgumentError with appropriate explanation.

kucaahbe commented 3 years ago

this PR was included into 0.9.4 version