[Django] #24112: Inconsistency in TestCase.assertInHTML
Django
-----------------------------------+--------------------
Reporter: plumdog | Owner: nobody
Type: Uncategorized | Status: new
Component: Testing framework | Version: 1.7
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------
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`
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
--
Ticket URL: <https://code.djangoproject.com/ticket/24112>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-----------------------------------+--------------------------------------
Reporter: plumdog | Owner: nobody
Type: Uncategorized | Status: new
Component: Testing framework | Version: 1.7
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by plumdog):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Old description:
New description:
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
--
--
Ticket URL: <https://code.djangoproject.com/ticket/24112#comment:1>
Django
-----------------------------------+------------------------------------
Reporter: plumdog | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
Comment:
Please make a pull request with your proposed fix and we'll go from there.
--
Ticket URL: <https://code.djangoproject.com/ticket/24112#comment:2>
Django
-----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Testing framework | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by plumdog):
Pull Request created:
https://github.com/django/django/pull/4041
--
Ticket URL: <https://code.djangoproject.com/ticket/24112#comment:3>
Django
-----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Testing framework | Version: 1.7
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* has_patch: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/24112#comment:4>
Django
-----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Testing framework | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by plumdog):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24112#comment:5>
Django
-----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Testing framework | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by plumdog):
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/24112#comment:6>
Django
-----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Testing framework | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
Comments for improvement on PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/24112#comment:7>
Django
-----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Testing framework | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by adamzap):
I'm willing to clean up the patch and open a new PR, or do you want to
finish it yourself, plumdog?
--
Ticket URL: <https://code.djangoproject.com/ticket/24112#comment:8>
Django
-----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Testing framework | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by claudep):
After 18 months, any taker can freely go ahead!
--
Ticket URL: <https://code.djangoproject.com/ticket/24112#comment:9>
Django
-----------------------------------+------------------------------------
Type: Bug | Status: assigned
Component: Testing framework | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by adamzap):
* owner: nobody => adamzap
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/24112#comment:10>
Django
-----------------------------------+------------------------------------
Reporter: plumdog | Owner: adamzap
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by adamzap):
PR is here: https://github.com/django/django/pull/7079
--
Ticket URL: <https://code.djangoproject.com/ticket/24112#comment:11>
Django
-----------------------------------+------------------------------------
Reporter: plumdog | Owner: adamzap
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* needs_better_patch: 1 => 0
* version: 1.7 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/24112#comment:12>
Django
-----------------------------------+------------------------------------
Reporter: plumdog | Owner: adamzap
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/24112#comment:13>
Django
-----------------------------------+------------------------------------
Reporter: plumdog | Owner: adamzap
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by adamzap):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/24112#comment:14>
Django
-----------------------------------+------------------------------------
Reporter: plumdog | Owner: adamzap
Type: Bug | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/24112#comment:15>
Django
-----------------------------------+------------------------------------
Reporter: plumdog | Owner: adamzap
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"ca2ccf54ffa95cf001260b917dd267fda60e93d5" ca2ccf54]:
{{{
#!CommitTicketReference repository=""
revision="ca2ccf54ffa95cf001260b917dd267fda60e93d5"
Fixed #24112 -- Fixed assertInHTML()'s counting if needle has no root
element.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24112#comment:16>