Opened 8 months ago

Closed 4 months ago

#33348 closed Cleanup/optimization (fixed)

Change API of assertFormError to take an actual form object

Reported by: Baptiste Mispelon Owned by: Baptiste Mispelon
Component: Testing framework Version: dev
Severity: Normal Keywords:
Cc: Baptiste Mispelon Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (13)

comment:1 Changed 8 months ago by Baptiste Mispelon

As an argument in favor of this change, may I present this lines from tests/admin_views/tests.py:6540 [1]

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:

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:

for i, formset in enumerate(response.context['inline_admin_formsets']):
    with self.subTest(i=i):
        self.assertEqual(formset.non_form_errors(), [])

[1] https://github.com/django/django/blob/8a4e5067605e608c3fcbb5ca11e0019eac8b40aa/tests/admin_views/tests.py#L6540

comment:2 Changed 8 months ago by Mariusz Felisiak

Triage Stage: UnreviewedAccepted

Agreed, let's change the first argument to form/formset and deprecate passing a response in it.

comment:3 Changed 8 months ago by Baptiste Mispelon

Has patch: set

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).

[1] https://github.com/django/django/blob/2f73e5406d54cb8945e187eff302a3a3373350be/tests/admin_views/tests.py#L5558

comment:4 Changed 8 months ago by Mariusz Felisiak

Owner: changed from nobody to Baptiste Mispelon

comment:5 Changed 8 months ago by Mariusz Felisiak

Patch needs improvement: set

comment:6 Changed 7 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In c67e1cf4:

Refs #33348 -- Deprecated passing errors=None to SimpleTestCase.assertFormError()/assertFormsetErrors().

comment:7 Changed 6 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 7986028:

Refs #33348 -- Made SimpleTestCase.assertFormError()/assertFormsetErrors() raise an error for unbound forms/formsets.

comment:8 Changed 6 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 9bb13def:

Refs #33348 -- Made SimpleTestCase.assertFormsetErrors() raise an error when form_index is too big.

comment:9 Changed 6 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In d84cd91e:

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.

comment:10 Changed 6 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In d4c9dab:

Refs #33348 -- Fixed SimpleTestCase.assertFormError() error message raised for unbound forms.

comment:11 Changed 6 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In f7e0bffa:

Refs #33348 -- Made SimpleTestCase.assertFormError() raise ValueError when "field" is passed without "form_index".

comment:12 Changed 4 months ago by Mariusz Felisiak

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:13 Changed 4 months ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In 50e1e7ef:

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.

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