[Django] #33348: Change API of assertFormError to take an actual form object
Django
------------------------------------------------+--------------------------
Reporter: Baptiste Mispelon | Owner: nobody
Type: Cleanup/optimization | Status: assigned
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 |
------------------------------------------------+--------------------------
The current signature of `assertFormError` is `(response, form, field,
errors, msg_prefix='')` where:
- `response` is a response object (specifically one generated by the test
client because it needs `response.context`)
- `form` is the name of a form (string) to be found in the response's
context
- `field` is a name of a field in the form (string)
- `errors` is either a list of error strings, a single error string
(equivalent to `[error]`) or `None` (equivalent to `[]`)
- `msg_prefix` is a string added to the test failure message
Personally I've always found this `response`/`form` API to be clunky since
I often want to test a form directly without having to go through the test
client.
It would be a lot easier to use if we could pass a form object directly. I
don't really understand why the API was implemented like this to begin
with.
On top of that, now that Django uses template-based rendering for forms,
formsets and widgets there are a lot of contexts to search in
`response.context` and a lot of possibilities for clashing names (see
#33346).
I would therefore propose the following new signature:
`assertFormError(form, field, errors, msg_prefix='')` with `form` being an
actual `Form` object and all other parameters staying the same.
The deprecation should be straightforward to implement by checking if
`form` is a response object (`isinstance` check) or if `response` kwarg
has been passed.
With that change `assertFormsetError` becomes mostly useless since one can
simply do `assertFormError(formset.forms[i], field_name, errors)`. The one
usecase that wouldn't be covered is `i = None + field_name=None` which
checks the `errors` list against the formset's `non_form_errors()`.
We could either leave `assertFormsetError` in place as a convenience
method (with a similar change to its API to accept a formset object
directly) or deprecate more agressively by suggesting the user tests the
output of `formset.non_field_errors()` directly.
--
Ticket URL: <https://code.djangoproject.com/ticket/33348>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Baptiste Mispelon | Owner: nobody
Cleanup/optimization |
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):
As an argument in favor of this change, may I present this lines from
`tests/admin_views/tests.py:6540` [1]
{{{#!py
msg = "The formset 'inline_admin_formset' in context 22 does not contain
any non-form errors."
with self.assertRaisesMessage(AssertionError, msg):
self.assertFormsetError(response, 'inline_admin_formset', None, None,
['Error'])
}}}
Over the years as more and more templates got involved in the rendering of
admin views, the exact number needed to make this test pass went from 4 to
10 to 12 to 22 (and every time, I suspect someone changed it manually to
whichever new number the test failure asked for).
With the change I propose, the test would look like this:
{{{#!py
for formset in response.context['inline_admin_formsets']:
with self.assertRaisesMessage("The formset does not contain any non-
form errors"):
self.assertFormsetError(formset, None, None, ['error']
}}}
Though on second thought, in this case I think we can skip
`assertFormsetError` altogether:
{{{#!py
for i, formset in enumerate(response.context['inline_admin_formsets']):
with self.subTest(i=i):
self.assertEqual(formset.non_form_errors(), [])
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33348#comment:1>
Django
Reporter: Baptiste Mispelon | Owner: nobody
Type: Cleanup/optimization | Status: assigned
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):
* stage: Unreviewed => Accepted
Comment:
Agreed, let's change the first argument to `form`/`formset` and deprecate
passing a `response` in it.
--
Ticket URL: <https://code.djangoproject.com/ticket/33348#comment:2>
Django
Reporter: Baptiste Mispelon | Owner: nobody
Type: Cleanup/optimization | 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 Baptiste Mispelon):
* has_patch: 0 => 1
Comment:
PR is ready for review: https://github.com/django/django/pull/15179
Overall the changes are backwards-compatible but there are some
exceptions:
1) The failure messages are now different
2) The behavior of `errors=[]` is completely different
3) The behavior in the case of multiple `errors` is different
For 1), I don't know if our compatibility policy applies here. As noted in
comment:1 the failure message were already prone to change arbitrarily as
Django rendered more and more templates in its request/response cycle.
For 2), the old implementation of `assertFormError` (and
`assertFormsetError`) would **always** pass when using `errors=[]`. Even
if the field had no errors, or even if the field was not present on the
form. I'd argue that this is a prety nasty bug and fixing it actually
caught an incorrect tests in Django's own test suite [1]
For 3), the old implementation would only make sure that all the `errors`
passed to `assertFormError` (and `assertFormsetError`) were present in the
field's error. If the field happened to have extra errors the test would
still pass. I changed that behavior so that the two lists must match
exactly. This makes the implementation simpler (we can use a plain
`assertEqual` and let Python give a nice diff in case of errors) and I
think it's also more correct. The documentation wasn't very clear anyway
so I think the change is acceptable.
As part of the ticket, I also removed the undocumented ability to use
`errors=None` as an equivalent to `errors=[]`. I don't really see the
usecase for that "shortcut" and "one obvious way" something something.
Finally, the PR adds a nicer `repr` for formsets, similar to what was done
in #23167 (it made the tests easier to write).
--
Ticket URL: <https://code.djangoproject.com/ticket/33348#comment:3>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
Type: | Mispelon
Cleanup/optimization | 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: nobody => Baptiste Mispelon
--
Ticket URL: <https://code.djangoproject.com/ticket/33348#comment:4>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
Type: | Mispelon
Component: Testing framework | Version: dev
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/33348#comment:5>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
Type: | Mispelon
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"c67e1cf44f17c36139e25b1eae92216cb8baad77" c67e1cf4]:
{{{
#!CommitTicketReference repository=""
revision="c67e1cf44f17c36139e25b1eae92216cb8baad77"
Refs #33348 -- Deprecated passing errors=None to
SimpleTestCase.assertFormError()/assertFormsetErrors().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33348#comment:6>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
Type: | Mispelon
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"7986028e3fbc9f84fca5a25a03cecfbc85ca5ce7" 7986028]:
{{{
#!CommitTicketReference repository=""
revision="7986028e3fbc9f84fca5a25a03cecfbc85ca5ce7"
Refs #33348 -- Made SimpleTestCase.assertFormError()/assertFormsetErrors()
raise an error for unbound forms/formsets.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33348#comment:7>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
Type: | Mispelon
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"9bb13def5d416ff3d5d1928a2def5babac0e19f6" 9bb13def]:
{{{
#!CommitTicketReference repository=""
revision="9bb13def5d416ff3d5d1928a2def5babac0e19f6"
Refs #33348 -- Made SimpleTestCase.assertFormsetErrors() raise an error
when form_index is too big.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33348#comment:8>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
Type: | Mispelon
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"d84cd91e90cb0871e43f98cba8c53be99053e903" d84cd91e]:
{{{
#!CommitTicketReference repository=""
revision="d84cd91e90cb0871e43f98cba8c53be99053e903"
Refs #33348 -- Improved messages raised by
SimpleTestCase.assertFormError()/assertFormsetErrors().
This makes messages use BaseFormSet/BaseForm.__repr__() instead of
context, and adds the _assert_form_error() helper.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33348#comment:9>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
Type: | Mispelon
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"f7e0bffa2e9c8820c008e742f76b71feaf7dd89f" f7e0bffa]:
{{{
#!CommitTicketReference repository=""
revision="f7e0bffa2e9c8820c008e742f76b71feaf7dd89f"
Refs #33348 -- Made SimpleTestCase.assertFormError() raise ValueError when
"field" is passed without "form_index".
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33348#comment:11>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
Type: | Mispelon
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"d4c9dab74bc29c240c33879a1167e5fd0971c7bc" d4c9dab]:
{{{
#!CommitTicketReference repository=""
revision="d4c9dab74bc29c240c33879a1167e5fd0971c7bc"
Refs #33348 -- Fixed SimpleTestCase.assertFormError() error message raised
for unbound forms.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33348#comment:10>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
Type: | Mispelon
Component: Testing framework | Version: dev
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):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33348#comment:12>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
Type: | 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:"50e1e7ef8ef63271717f8bcab76d7151ccf4bb18" 50e1e7ef]:
{{{
#!CommitTicketReference repository=""
revision="50e1e7ef8ef63271717f8bcab76d7151ccf4bb18"
Fixed #33348 -- Changed
SimpleTestCase.assertFormError()/assertFormsetErrors() to take
form/formset.
Instead of taking a response object and a context name for
the form/formset, the two methods now take the object directly.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33348#comment:13>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
Type: | Mispelon
Cleanup/optimization | Status: closed
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:"71d1203b07d3092062aa9ebc0ab198abfc144d54" 71d1203b]:
{{{
#!CommitTicketReference repository=""
revision="71d1203b07d3092062aa9ebc0ab198abfc144d54"
Refs #33348 -- Removed support for passing response object and
form/formset name to
SimpleTestCase.assertFormError()/assertFormSetError().
Per deprecation timeline.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33348#comment:15>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
Type: | Mispelon
Cleanup/optimization | Status: closed
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:"23c87874397a22ca80c0d6fa8648fb58ee3ab28a" 23c8787]:
{{{
#!CommitTicketReference repository=""
revision="23c87874397a22ca80c0d6fa8648fb58ee3ab28a"
Refs #33348 -- Removed support for passing errors=None to
SimpleTestCase.assertFormError()/assertFormsetErrors().
Per deprecation timeline.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33348#comment:14>