assertFormsetError() crashes on formset named "form".

[OutOfFocus4]

I started updating a project from Django 3 to Django 4. When I ran my unit tests after the update, one of them failed with an AttributeError.

The full stack trace is

Traceback (most recent call last):
  File "/private/tmp/example/example/tests.py", line 17, in test_formset
    self.assertFormsetError(
  File "/private/tmp/example/.venv/lib/python3.9/site-packages/django/test/testcases.py", line 565, in assertFormsetError
    if field in context[formset].forms[form_index].errors:
AttributeError: 'ManagementForm' object has no attribute 'forms'

It looks like rendering the ManagementForm of a formset adds a context to response.context which contains a "form". Calling "assertFormsetError" to check for errors in a formset called "form" will check the formset's errors, but will also try to look at the errors in any "form" in all contexts, including the ManagementForm.

[OutOfFocus4]

Basic Django application to demonstrate the bug. Just install Django 4 and run "manage.py test"

[Mariusz Felisiak]

Thanks for the report, it's niche but we should definitely fix this bug in assertFormsetError().

Regression in 456466d932830b096d39806e291fe23ec5ed38d5.

[Baptiste Mispelon]

I did some digging around assertFormsetError (and assertFormError) while working on #33301 and came to the conclusion that they might be fundamentally broken.

Both methods work by looking for a specific name in the context of each template rendered during the execution of the view. When the name is not found in the template, it is skipped. If the name is found then the context[name] object is checked for the assertion.

The issue is that you cannot always control which templates are rendered during your views, and especially what names those templates are using in their respective contexts.
The situation is even worse now that Django uses templates to render forms, formsets and widgets.

Restricting the search to the first context would probably fix most issues like the one reported here, but it's not 100% correct (and backwards-incompatible).

The only way out that I could think of would be to deprecate passing a response to assertFormError/assertFormsetError in favor of passing the form/formset object directly. I think it would be a more natural API anway (I never understood why the assertions were based on the response to begin with).

[Mariusz Felisiak]

Replying to Baptiste Mispelon:

...
Restricting the search to the first context would probably fix most issues like the one reported here, but it's not 100% correct (and backwards-incompatible).

What do you think about skipping context values that are not a FormSet instance? or don't have the forms attribute. This should be backward compatible.

The only way out that I could think of would be to deprecate passing a response to assertFormError/assertFormsetError in favor of passing the form/formset object directly. I think it would be a more natural API anway (I never understood why the assertions were based on the response to begin with).

We cannot change or deprecate an existing and documented API as a part of patch which is intended for backport. This can be discussed separately. Personally, I like the idea.

[Baptiste Mispelon]

Replying to Mariusz Felisiak:

What do you think about skipping context values that are not a FormSet instance? or don't have the forms attribute. This should be backward compatible.

I haven't thought it through completely, but my gut feeling is that your proposed fix would work for the reported regression but there might still be corner cases that could fail. But those corner cases might have already been broken so it's probably ok.

I'll start working on a PR, it'll be easier for me to think it through with some concrete examples.

Replying to Mariusz Felisiak:

We cannot change or deprecate an existing and documented API as a part of patch which is intended for backport. This can be discussed separately. Personally, I like the idea.

Agreed, I'll open a separate ticket.

[Baptiste Mispelon]

PR here: ​https://github.com/django/django/pull/15165

I went with the isinstance check wich seems a bit more robust since forms seems like quite a common attribute name and might catch other non-formset things.
I went with hasattr(context[formset], 'forms') because the isinstance check broke some admin tests (InlineAdminFormSet is not a subclass of BaseFormset 😿 ).

[Mariusz Felisiak <felisiak.mariusz@…>]

In cb383753:

Fixed #33346 -- Fixed SimpleTestCase.assertFormsetError() crash on a formset named "form".

Thanks OutOfFocus4 for the report.

Regression in 456466d932830b096d39806e291fe23ec5ed38d5.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 15031852:

[4.0.x] Fixed #33346 -- Fixed SimpleTestCase.assertFormsetError() crash on a formset named "form".

Thanks OutOfFocus4 for the report.

Regression in 456466d932830b096d39806e291fe23ec5ed38d5.

Backport of cb383753c0e0eb52306e1024d32a782549c27e61 from main.