Opened 10 years ago
Closed 8 years ago
#24112 closed Bug (fixed)
Inconsistency in TestCase.assertInHTML
Reported by: | Andrew Plummer | Owned by: | Adam Zapletal |
---|---|---|---|
Component: | Testing framework | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
assertInHTML(needle, haystack) has the following behaviour
assertInHTML('<p>a</p>', '<div><p>a</p><p>b</p></div>')
passes: clearly correct
assertInHTML('<p>a</p><p>b</p>', '<p>a</p><p>b</p>')
passes: possibly correct
assertInHTML('<p>a</p><p>b</p>', '<div><p>a</p><p>b</p></div>')
fails with an assertion error:
File ".../django/test/testcases.py", line 673, in assertInHTML msg_prefix + "Couldn't find '%s' in response" % needle) AssertionError: Couldn't find '<p> a </p><p> b </p>' in response
Which seems wrong. It doesn't handle the case correctly if the needle doesn't have a single top-level element (unless the two bits of html are equivalent). This is down to the _count
method of django.test.html.Element
.
This means that the test will fail (with the error above) rather than erroring.
I think this should throw a ValueError if the needle has multiple top level elements. However, this does change existing behaviour for the case where the html fragments are equivalent. An alternative would be to try to correctly handle the count in this case, which is possibly a bit fiddly, and the current implementation appears to make no attempt to do this.
I have a patch for the first case: https://github.com/plumdog/django/commit/bfdfda315ad74d067be52888a236ab7a4aadcf96
Change History (16)
comment:1 by , 10 years ago
Description: | modified (diff) |
---|
comment:2 by , 10 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Uncategorized → Bug |
comment:4 by , 10 years ago
Has patch: | set |
---|---|
Needs tests: | set |
comment:5 by , 10 years ago
I've updated my PR to include tests, and changed my proposed fix to preserve existing behaviour where I believe the existing behaviour to be correct, namely where the two HTML fragments are equal.
comment:6 by , 10 years ago
Needs tests: | unset |
---|
comment:8 by , 8 years ago
I'm willing to clean up the patch and open a new PR, or do you want to finish it yourself, plumdog?
comment:10 by , 8 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:12 by , 8 years ago
Patch needs improvement: | unset |
---|---|
Version: | 1.7 → master |
comment:13 by , 8 years ago
Patch needs improvement: | set |
---|
comment:14 by , 8 years ago
PR is updated with an attempt at an actual fix, not just a cleanup of plumdog's original PR
comment:15 by , 8 years ago
Patch needs improvement: | unset |
---|
Please make a pull request with your proposed fix and we'll go from there.