Opened 2 years ago

Closed 7 months ago

#24112 closed Bug (fixed)

Inconsistency in TestCase.assertInHTML

Reported by: Andrew Plummer Owned by: Adam Zapletal
Component: Testing framework Version: master
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 Andrew Plummer)

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 Changed 2 years ago by Andrew Plummer

Description: modified (diff)

comment:2 Changed 2 years ago by Tim Graham

Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

Please make a pull request with your proposed fix and we'll go from there.

comment:3 Changed 2 years ago by Andrew Plummer

comment:4 Changed 2 years ago by Claude Paroz

Has patch: set
Needs tests: set

comment:5 Changed 2 years ago by Andrew Plummer

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 Changed 2 years ago by Andrew Plummer

Needs tests: unset

comment:7 Changed 2 years ago by Tim Graham

Patch needs improvement: set

Comments for improvement on PR.

comment:8 Changed 8 months ago by Adam Zapletal

I'm willing to clean up the patch and open a new PR, or do you want to finish it yourself, plumdog?

comment:9 Changed 8 months ago by Claude Paroz

After 18 months, any taker can freely go ahead!

comment:10 Changed 8 months ago by Adam Zapletal

Owner: changed from nobody to Adam Zapletal
Status: newassigned

comment:12 Changed 8 months ago by Claude Paroz

Patch needs improvement: unset
Version: 1.7master

comment:13 Changed 7 months ago by Tim Graham

Patch needs improvement: set

comment:14 Changed 7 months ago by Adam Zapletal

PR is updated with an attempt at an actual fix, not just a cleanup of plumdog's original PR

https://github.com/django/django/pull/7079

comment:15 Changed 7 months ago by Claude Paroz

Patch needs improvement: unset

comment:16 Changed 7 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In ca2ccf54:

Fixed #24112 -- Fixed assertInHTML()'s counting if needle has no root element.

Note: See TracTickets for help on using tickets.
Back to Top