[Django] #32556: assertHTMLEqual gives a confusing error message with empty attributes
Django
---------------------------------------------+------------------------
Reporter: Baptiste Mispelon | Owner: nobody
Type: Uncategorized | Status: new
Component: Testing framework | Version: dev
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------------+------------------------
If you try the following assertion:
```
self.assertHTMLEqual('<input value>', '<input value="">')
```
You get a test failure and the following message:
```
AssertionError: <input value> != <input value>
<input value>
```
I'm getting mixed signals here: either the test should pass or the error
message should show the difference between the two strings.
I'm not actually sure which option would be correct in this case so I'm
leaving the ticket as "uncategorized" instead of "bug" or "cleanup".
--
Ticket URL: <https://code.djangoproject.com/ticket/32556>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Baptiste Mispelon | Owner: nobody
Type: Uncategorized | Status: new
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Baptiste Mispelon:
Old description:
> If you try the following assertion:
> ```
> self.assertHTMLEqual('<input value>', '<input value="">')
> ```
>
> You get a test failure and the following message:
> ```
> AssertionError: <input value> != <input value>
> <input value>
>
> ```
>
> I'm getting mixed signals here: either the test should pass or the error
> message should show the difference between the two strings.
>
> I'm not actually sure which option would be correct in this case so I'm
> leaving the ticket as "uncategorized" instead of "bug" or "cleanup".
New description:
If you try the following assertion:
{{{
self.assertHTMLEqual('<input value>', '<input value="">')
}}}
You get a test failure and the following message:
{{{
AssertionError: <input value> != <input value>
<input value>
}}}
I'm getting mixed signals here: either the test should pass or the error
message should show the difference between the two strings.
I'm not actually sure which option would be correct in this case so I'm
leaving the ticket as "uncategorized" instead of "bug" or "cleanup".
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32556#comment:1>
Django
Reporter: Baptiste Mispelon | Owner: nobody
Type: Uncategorized | Status: new
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Baptiste Mispelon):
Digging into this, the issue appears to be that `django.test.html.Element`
has a different logic in its `__eq__` vs `__str__`.
In particular, `__eq__` [1] has this comment:
{{{
# attributes without a value is same as attribute with value that
# equals the attributes name:
# <input checked> == <input checked="checked">
}}}
Whereas `__str__` will simply drop the value if it's false-ish.
But I'm not sure that's a correct interpretation of the HTML spec on
boolean attributes [2].
From what I understand not all HTML attributes can be used without a
value, but I can't seem to find an exhaustive list. A random comment on
the internet suggests going through the complete HTML5 spec [3] and
looking for the phrase "is a boolean".
With Django's current approach, the two elements `<input value>` and
`<input value="value">` would be considered equivalent but I'm not sure
that's correct.
[1]
https://github.com/django/django/blob/54d91795408c878ad335226a87a44961d7f646a9/django/test/html.py#L61
[2] https://www.w3.org/TR/html52/infrastructure.html#sec-boolean-
attributes
[3] https://html.spec.whatwg.org/
--
Ticket URL: <https://code.djangoproject.com/ticket/32556#comment:2>
Django
Reporter: Baptiste Mispelon | Owner: nobody
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
Comment:
Thanks for this report.
> ''A boolean attribute without a value assigned to it (e.g. checked) is
implicitly equivalent to one that has the empty string assigned to it
(i.e. checked="")''
I think we should stick to this, without checking if an attribute is
really a boolean attribute or not according to spec. `assertHTMLEqual()`
already allows invalid HTML (see #32547). I suggest checking if the values
are truthy:
{{{
diff --git a/django/test/html.py b/django/test/html.py
index 486a0d358d..d443c118e2 100644
--- a/django/test/html.py
+++ b/django/test/html.py
@@ -64,9 +64,9 @@ class Element:
for i in range(len(self.attributes)):
attr, value = self.attributes[i]
other_attr, other_value = element.attributes[i]
- if value is None:
+ if not value:
value = attr
- if other_value is None:
+ if not other_value:
other_value = other_attr
if attr != other_attr or value != other_value:
return False
}}}
Would you like to provide a patch?
--
Ticket URL: <https://code.djangoproject.com/ticket/32556#comment:3>
Django
Reporter: Baptiste Mispelon | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Hasan Ramezani):
* owner: nobody => Hasan Ramezani
* status: new => assigned
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/14144 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/32556#comment:4>
Django
Reporter: Baptiste Mispelon | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32556#comment:5>
Django
Reporter: Baptiste Mispelon | Owner: Hasan
| Ramezani
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"9bf5e9418f425666726559c9f1981a516da30aab" 9bf5e941]:
{{{
#!CommitTicketReference repository=""
revision="9bf5e9418f425666726559c9f1981a516da30aab"
Fixed #32556 -- Fixed assertHTMLEqual() to handle empty string as boolean
attributes value.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32556#comment:6>
Django
Reporter: Baptiste Mispelon | Owner: Hasan
| Ramezani
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Baptiste Mispelon):
* status: closed => new
* resolution: fixed =>
Comment:
Oh, that was fast. Thanks for the quick turnaround on this, but if I may
I'd like to reopen this ticket and argue for a revert.
This test was passing before and now fails (both assertions fail):
{{{#!py
def test_boolean_attribute2(self):
html1 = '<input value="">'
html2 = '<input value="value">'
self.assertHTMLNotEqual(html1, html2)
self.assertNotEqual(parse_html(html1), parse_html(html2))
}}}
I've been trying to think about how I would fix the original issue but I
keep coming back to having an exhaustive list of known boolean attributes
(the parser already has `SELF_CLOSING_TAGS` so there's some precedent). I
don't see how else to fix this correctly.
On the other hand I'm not opposed to having an incorrect parser with known
limitations but I think the limitations should be documented.
I also think that having `__eq__` and `__str__` use separate codepaths
will lead to more issues. Why can't we have `def __eq__(self, other):
return str(self) == str(other)`?
--
Ticket URL: <https://code.djangoproject.com/ticket/32556#comment:7>
Django
Reporter: Baptiste Mispelon | Owner: Hasan
| Ramezani
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Ready for checkin => Accepted
Comment:
> I've been trying to think about how I would fix the original issue but I
keep coming back to having an exhaustive list of known boolean attributes
(the parser already has SELF_CLOSING_TAGS so there's some precedent). I
don't see how else to fix this correctly.
Can you prepare such a list? (based on spec). Maybe: `allowfullscreen`,
`async`, `autofocus`, `autoplay`, `checked`, `controls`, `default`,
`defer` , `disabled`, `formnovalidate`, `hidden`, `ismap`, `itemscope`,
`loop`, `multiple`, `muted`, `nomodule`, `novalidate`, `open`,
`playsinline`, `readonly`, `required`, `reversed`, `selected` (see
https://html.spec.whatwg.org/#attributes-3).
--
Ticket URL: <https://code.djangoproject.com/ticket/32556#comment:8>
Django
Reporter: Baptiste Mispelon | Owner: Hasan
| Ramezani
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Baptiste Mispelon):
Using the table you linked to, I get the same list of 24 attributes. The
table is labelled as "non-normative" but I think it would be good enough
for this usecase.
I tried various other ways of extracting the list from the spec but this
table seems like the best method. It's missing `truespeed` which is a
boolean attribute for the `<marquee>` tag (which is deprecated) so I think
it's acceptable.
For reference, here's the script I used to get the list (piped to `sort
-u` to remove duplicates):
{{{#!py
from urllib.request import urlopen
from lxml import html # needs to be pip-installed (`pip install lxml`)
if __name__ == '__main__':
# The spec is an 11Mb file, downloading it takes a while
response = urlopen('https://html.spec.whatwg.org')
tree = html.parse(response)
for i, row in
enumerate(tree.xpath('.//table[@id="attributes-1"]/tbody/tr')):
assert len(row) == 4, f"Expected 4 columns on row {i}, got
{len(rows)}"
attr, elements, description, value = (node.text_content().strip()
for node in row)
if value == "Boolean attribute":
print(attr)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32556#comment:9>
Django
Reporter: Baptiste Mispelon | Owner: Hasan
| Ramezani
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Baptiste Mispelon):
I wrote a [https://github.com/django/django/pull/14148 new PR] using this
approach:
- declaring a list of known boolean attributes
- using a single logic between `__str__` and `__eq__` (which is now a one-
liner)
While writing the patch I noticed that the previous behavior is sortof
documented, as the documentation for `assertHTMLEqual` [1] says:
> Attributes without an argument are equal to attributes that equal in
name and value (see the examples).
But the example only uses `checked` which is a boolean attribute so I'd
argue it's not a breaking change.
--
Ticket URL: <https://code.djangoproject.com/ticket/32556#comment:10>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
| Mispelon
Type: Bug | Status: assigned
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: Hasan Ramezani => Baptiste Mispelon
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32556#comment:11>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
| Mispelon
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32556#comment:12>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
| Mispelon
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Baptiste Mispelon):
* needs_better_patch: 1 => 0
Comment:
PR updated
--
Ticket URL: <https://code.djangoproject.com/ticket/32556#comment:13>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
| Mispelon
Type: Bug | Status: assigned
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32556#comment:14>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
| Mispelon
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"41e6b2a3c5e723256506b9ff49437d52a1f3bf43" 41e6b2a3]:
{{{
#!CommitTicketReference repository=""
revision="41e6b2a3c5e723256506b9ff49437d52a1f3bf43"
Fixed #32556 -- Fixed handling empty string as non-boolean attributes
value by assertHTMLEqual().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32556#comment:16>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
| Mispelon
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"98abf80cde0a7b6846f2612ee8ec9189adccdc3c" 98abf80c]:
{{{
#!CommitTicketReference repository=""
revision="98abf80cde0a7b6846f2612ee8ec9189adccdc3c"
Refs #32556 -- Added django.test.html.normalize_attributes().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32556#comment:15>