Fixed #24112 -- Fixed counting of html elements if needle has no root element #4041

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
@plumdog
Contributor

plumdog commented Feb 3, 2015

The parse_html method (at the bottom of test/html.py) creates a instance of Parser and feeds in the html. The parser starts with a RootElement and adds to it as it parses, meaning that what parse_html gets back always has an additional RootElement above the rest of the HTML. It then removes this if it can, ie, if the RootElement only has one child (ie, the given HTML had a root element of its own).

This means that if the html passed to parse_html has no root node, it will retain its RootElement from the parser.

We can then check this when counting to see if the needle HTML has multiple top level elements. If it does, then counting becomes much more fiddly, so my current patch just raises a ValueError.

There aren't any tests as I couldn't find any existing ones for this module. Should things within the test module get tests?

@claudep

This comment has been minimized.

Show comment
Hide comment
@claudep

claudep Feb 3, 2015

Member

Yes, tests should be added in tests/test_utils/tests.py. Even if assertInHTML itself is not in those tests, you'll find related tests (testing parse_html and count).

Member

claudep commented Feb 3, 2015

Yes, tests should be added in tests/test_utils/tests.py. Even if assertInHTML itself is not in those tests, you'll find related tests (testing parse_html and count).

@plumdog

This comment has been minimized.

Show comment
Hide comment
@plumdog

plumdog Feb 3, 2015

Contributor

OK, thanks for that. I'll add relevant tests ASAP.

Contributor

plumdog commented Feb 3, 2015

OK, thanks for that. I'll add relevant tests ASAP.

@plumdog

This comment has been minimized.

Show comment
Hide comment
@plumdog

plumdog Feb 5, 2015

Contributor

The PR currently contains two commits. (I know I'll want to squash these before a merge) The first (1a56cea) adds a failing test that demonstrates what I believe is incorrect behaviour.

Specifically:

FAIL: test_html_contain (test_utils.tests.HTMLEqualTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File ".../django/tests/test_utils/tests.py", line 579, in test_html_contain
    self.assertIn(dom1, dom2)
AssertionError: <p>
foo
</p><p>
bar
</p> not found in <div>
<p>
foo
</p><p>
bar
</p>
</div>

which seems wrong to be saying "not found in".

The second commit (430531c) rectifies this by throwing a ValueError if the needle HTML has no root element, unless if the two elements are equal, which preserves existing functionality.

I believe that the net effect of my change is that if where before you got AssertionError("... not found in ...") and it made no sense, you now get ValueError('Needle HTML does not have a root element').

Contributor

plumdog commented Feb 5, 2015

The PR currently contains two commits. (I know I'll want to squash these before a merge) The first (1a56cea) adds a failing test that demonstrates what I believe is incorrect behaviour.

Specifically:

FAIL: test_html_contain (test_utils.tests.HTMLEqualTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File ".../django/tests/test_utils/tests.py", line 579, in test_html_contain
    self.assertIn(dom1, dom2)
AssertionError: <p>
foo
</p><p>
bar
</p> not found in <div>
<p>
foo
</p><p>
bar
</p>
</div>

which seems wrong to be saying "not found in".

The second commit (430531c) rectifies this by throwing a ValueError if the needle HTML has no root element, unless if the two elements are equal, which preserves existing functionality.

I believe that the net effect of my change is that if where before you got AssertionError("... not found in ...") and it made no sense, you now get ValueError('Needle HTML does not have a root element').

+ # when a root element is used for the needle but not the haystack
+ dom1 = parse_html('<p>foo</p><p>bar</p>')
+ dom2 = parse_html('<div><p>foo</p><p>bar</p></div>')
+ with self.assertRaises(ValueError):

This comment has been minimized.

@timgraham

timgraham Mar 13, 2015

Member

with self.assertRaisesMessage('Needle HTML does not have a root element'):

@timgraham

timgraham Mar 13, 2015

Member

with self.assertRaisesMessage('Needle HTML does not have a root element'):

@@ -95,6 +95,9 @@ def _count(self, element, count=True):
if not isinstance(element, six.string_types):
if self == element:
return 1
+ if isinstance(element, RootElement):
+ raise ValueError('Needle HTML does not have a root element')

This comment has been minimized.

@timgraham

timgraham Mar 13, 2015

Member

Should we add some context to the message (like part of the HTML snippet) to help identify the needle? I'm a bit worried developers will have a tough time figuring out what the message means.

@timgraham

timgraham Mar 13, 2015

Member

Should we add some context to the message (like part of the HTML snippet) to help identify the needle? I'm a bit worried developers will have a tough time figuring out what the message means.

@timgraham timgraham changed the title from Fix counting of html elements if needle has no root element to Fixed #24112 -- Fixed counting of html elements if needle has no root element Mar 14, 2015

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Apr 23, 2015

Member

Please send a new PR if you can reply and/or update per my comments, thanks!

Member

timgraham commented Apr 23, 2015

Please send a new PR if you can reply and/or update per my comments, thanks!

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