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

Restore the support for matching html and body #75

Merged
merged 1 commit into from Oct 6, 2020

Conversation

CvX
Copy link
Contributor

@CvX CvX commented Oct 2, 2020

This effectively reverts 306b8f4 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.

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

The issue reported in kucaahbe#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
Copy link
Owner

kucaahbe commented Oct 2, 2020


it 'should find the body element' do
expect(rendered).to have_tag('body')
end
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest adding examples for .to_not have_tag(...), without them it may be easy to introduce same bug in future again.

@CvX
Copy link
Contributor Author

CvX commented Oct 3, 2020

Just to reiterate – this PR doesn't fix the issue raised in #62 or that StackOverflow question. I argue that neither did 306b8f4 - 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: discourse/discourse#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
Copy link
Owner

kucaahbe commented Oct 6, 2020

@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 kucaahbe merged commit 4746224 into kucaahbe:master Oct 6, 2020
@kucaahbe
Copy link
Owner

kucaahbe commented Oct 6, 2020

this PR was included into 0.9.4 version

@CvX CvX deleted the fragment-issue branch October 7, 2020 13:49
end

it 'should find the body element' do
expect(rendered).to have_tag('body')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand, the ArgumentError is telling me to add additional options. and it works, I have added with: {...}
but your spec don't have any ?!?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a good point, that's because this code is not fully relevant (as original author was proposing restoring functionality), it was consumed and changed, recent version contains this part changed a bit

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

Successfully merging this pull request may close these issues.

None yet

3 participants