Opened 11 months ago
Closed 10 months ago
#36038 closed Cleanup/optimization (wontfix)
Added a test case for the display_for_field function when a FileField is passed.
| Reported by: | Antoliny | Owned by: | Antoliny |
|---|---|---|---|
| Component: | contrib.admin | Version: | dev |
| Severity: | Normal | Keywords: | display_for_field, FileFIeld |
| Cc: | Triage Stage: | Accepted | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | yes |
| Easy pickings: | no | UI/UX: | no |
Description (last modified by )
def display_for_field(value, field, empty_value_display):
from django.contrib.admin.templatetags.admin_list import _boolean_icon
...
elif isinstance(field, (models.IntegerField, models.FloatField)):
return formats.number_format(value)
elif isinstance(field, models.FileField) and value:
return format_html('<a href="{}">{}</a>', value.url, value)
...
There is no test case for the display_for_field function when a FileField object is passed. --> admin_utils test code
When the FileField handling was added in the above function, I found that the test was conducted indirectly through the FileWidget readonly attribute. --> test code
However, I believe it would be better to have a direct test case for when a FileField is passed, considering that display_for_field function is globally accessible.
Change History (9)
comment:1 by , 11 months ago
| Owner: | set to |
|---|
comment:2 by , 11 months ago
| Description: | modified (diff) |
|---|
comment:3 by , 11 months ago
| Description: | modified (diff) |
|---|
comment:4 by , 11 months ago
| Component: | Utilities → contrib.admin |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
| Version: | 5.1 → dev |
comment:6 by , 11 months ago
| Has patch: | set |
|---|
comment:7 by , 10 months ago
You're right that this is already covered by existing tests and this PR would add double cover.
The existing assert is "strong" enough that changes to display_for_field for FileField would make the existing test fail.
I would prefer we add tests where we are missing coverage or in parallel with a ticket for which the tests add value. Otherwise, it feels unnecessary
comment:8 by , 10 months ago
| Patch needs improvement: | set |
|---|
comment:9 by , 10 months ago
| Resolution: | → wontfix |
|---|---|
| Status: | assigned → closed |
Thank you Antoliny! Missing tests are always welcomed.