Fixed #35728 -- Computed error messages in assertions only on test failures. #19402

cliff688 · 2025-04-20T10:40:33Z

Trac ticket number

Branch description

There are two main approaches used.

Return early if the assertion will pass; if not compute error messages and pass them on to unittest's assertions. Passing on to unittest's assertions has the benefit of sometimes providing more helpful error messages, e.g. when asserting sequence equality, the difference is shown in the report. But sometimes this is just prepends False is not true to an error message.
~~2. Compute the error message in the failing conditional branch, for self.fail(msg).~~

A ~~additional~~ (now removed) commit db0cfda makes sure that we use __contains__ for checking existence of an element in HTML assertions. This has the benefit of using Element._count(elem, count=False) under the hood, and short-circuiting when a single match is found.

from django.test.html import parse_html

parsed_needle = parse_html("<span>Hello</span>")
parsed_haystack = parse_html(
    "<div>" + ("<span>Other</span>" * 100) +
    "<span>Hello</span>" * 2 +
    ("<span>Other</span>" * 100) + "</div>"
)

%timeit parsed_needle in parsed_haystack 
%timeit parsed_haystack.count(parsed_needle)  # Double the time.

Checklist

This PR targets the main branch.
The commit message is written in past tense, mentions the ticket number, and ends with a period.
I have checked the "Has patch" ticket flag in the Trac system.
I have added or updated relevant tests.
I have added or updated relevant docs, including release notes if applicable.
I have attached screenshots in both light and dark modes for any UI changes.

django/test/testcases.py

cliff688 · 2025-04-20T10:49:36Z

django/test/testcases.py

+        if count is None or count == 0:
+            # Does needle exist regardless of count?
+            real_count = int(parsed_needle in parsed_haystack)
+        else:
+            real_count = parsed_haystack.count(parsed_needle)
+        if (count is None and real_count > 0) or count == real_count:
+            # Shortcut.
+            return
+


The performance gains of this are difficult to test using assertInHTML because parse_html() incurs somewhere from 50–100x the cost of that assertion (compared to _count()) when I run it using the HTML in the PR description, checking for Hello for example.

cliff688 · 2025-04-20T10:52:00Z

Hello @adamchainz! Should I extend the patch for the other assertions as well, e.g. assertTemplateUsed() / assertRedirect() where computing the messages does not seem particularly expensive?

adamchainz · 2025-05-26T16:01:17Z

Hello @adamchainz! Should I extend the patch for the other assertions as well, e.g. assertTemplateUsed() / assertRedirect() where computing the messages does not seem particularly expensive?

No, I think those are safe.

sarahboyce

Thank you @cliff688. I've added a thought that I had looking through this

It would be great if we add benchmarks to django-asv for our custom test assertions as that might have been able to catch that this is a performance regression from 1dae65d (Django 5.1) (refs ticket-34657)
You might want to ref this ticket/commit in some commit messages

sarahboyce · 2025-06-20T14:08:56Z

django/test/testcases.py

+        if (count is None and real_count > 0) or (
+            (count is not None and real_count == count)
+        ):
+            return


By doing these early returns, we have already done the work of whether the assertion should fail and could just fail rather than doing the comparisons. This could look something like:

--- a/django/test/testcases.py +++ b/django/test/testcases.py @@ -606,23 +606,16 @@ class SimpleTestCase(unittest.TestCase): text_repr = self._text_repr(text, force_string=html) if count is not None: - self.assertEqual( - real_count, - count, - ( - f"{msg_prefix}Found {real_count} instances of {text_repr} " - f"(expected {count}) in the following response\n" - f"{response_content!r}" - ), + msg = ( + f"Found {real_count} instances of {text_repr} (expected {count}) in " + f"the following response\n{response_content!r}" ) else: - self.assertTrue( - real_count != 0, - ( - f"{msg_prefix}Couldn't find {text_repr} in the following response\n" - f"{response_content!r}" - ), - ), + msg = ( + f"Couldn't find {text_repr} in the following response\n" + f"{response_content!r}" + ) + self.fail(f"{msg_prefix}{msg}") def assertNotContains( self, response, text, status_code=200, msg_prefix="", html=False @@ -1012,15 +1005,9 @@ class SimpleTestCase(unittest.TestCase): f"Found {real_count} instances of {needle!r} (expected {count}) in " f"the following response\n{haystack_repr}" ) - self.assertEqual(real_count, count, f"{msg_prefix}{msg}") else: - self.assertTrue( - real_count != 0, - ( - f"{msg_prefix}Couldn't find {needle!r} in the following response\n" - f"{haystack_repr}" - ), - ) + msg = f"Couldn't find {needle!r} in the following response\n{haystack_repr}" + self.fail(f"{msg_prefix}{msg}") def assertNotInHTML(self, needle, haystack, msg_prefix=""): self.assertInHTML(needle, haystack, count=0, msg_prefix=msg_prefix) diff --git a/tests/test_utils/tests.py b/tests/test_utils/tests.py index 16692500e3..99154d16ce 100644 --- a/tests/test_utils/tests.py +++ b/tests/test_utils/tests.py @@ -990,15 +990,14 @@ class HTMLEqualTests(SimpleTestCase): class InHTMLTests(SimpleTestCase): def test_needle_msg(self): msg = ( - "False is not true : Couldn't find 'Hello' in the following " - "response\n'Test'" + "Couldn't find 'Hello' in the following response\n'Test'" ) with self.assertRaisesMessage(AssertionError, msg): self.assertInHTML("Hello", "Test") def test_msg_prefix(self): msg = ( - "False is not true : Prefix: Couldn't find 'Hello' in the following " + "Prefix: Couldn't find 'Hello' in the following " 'response\n\'<input type="text" name="Hello" />\'' ) with self.assertRaisesMessage(AssertionError, msg): @@ -1010,9 +1009,8 @@ class InHTMLTests(SimpleTestCase): def test_count_msg_prefix(self): msg = ( - "2 != 1 : Prefix: Found 2 instances of 'Hello' (expected 1) in the " + "Prefix: Found 2 instances of 'Hello' (expected 1) in the " "following response\n'HelloHello'" - "" ) with self.assertRaisesMessage(AssertionError, msg): self.assertInHTML(

Thanks @sarahboyce for the review!

The only motivation for using unittest's assertions, knowing the assertion has already failed, was to preserve the message format. But I can drop them if that's okay.

I think we can keep the assertion messages the same.

So something like:

--- a/django/test/testcases.py +++ b/django/test/testcases.py @@ -606,23 +606,17 @@ class SimpleTestCase(unittest.TestCase): text_repr = self._text_repr(text, force_string=html) if count is not None: if count is not None: - self.assertEqual( - real_count, - count, - ( - f"{msg_prefix}Found {real_count} instances of {text_repr} " - f"(expected {count}) in the following response\n" - f"{response_content!r}" - ), + msg = ( + f"{real_count} != {count} : {msg_prefix}Found {real_count} instances " + f"of {text_repr} (expected {count}) in the following response\n" + f"{response_content!r}" ) else: - self.assertTrue( - real_count != 0, - ( - f"{msg_prefix}Couldn't find {text_repr} in the following response\n" - f"{response_content!r}" - ), - ), + msg = ( + f"False is not true : {msg_prefix}Couldn't find {text_repr} in the " + f"following response\n{response_content!r}" + ) + self.fail(msg) def assertNotContains( self, response, text, status_code=200, msg_prefix="", html=False @@ -639,9 +633,9 @@ class SimpleTestCase(unittest.TestCase): if real_count != 0: text_repr = self._text_repr(text, force_string=html) self.fail( - f"{msg_prefix}{text_repr} unexpectedly found in the following response" - f"\n{response_content!r}" - ), + f"{real_count} != {0} : {msg_prefix}{text_repr} unexpectedly found in " + f"the following response\n{response_content!r}" + ) def _check_test_client_response(self, response, attribute, method_name): """ @@ -1012,14 +1006,11 @@ class SimpleTestCase(unittest.TestCase): f"Found {real_count} instances of {needle!r} (expected {count}) in " f"the following response\n{haystack_repr}" ) - self.assertEqual(real_count, count, f"{msg_prefix}{msg}") + self.fail(f"{real_count} != {count} : {msg_prefix}{msg}") else: - self.assertTrue( - real_count != 0, - ( - f"{msg_prefix}Couldn't find {needle!r} in the following response\n" - f"{haystack_repr}" - ), + self.fail( + f"False is not true : {msg_prefix}Couldn't find {needle!r} in the " + f"following response\n{haystack_repr}" )

I noticed that you had updated assertNotContains from an assertEqual to a fail, which has updated the message format but no test has failed so perhaps we want to make sure all of the failure messages are covered by tests

I think we can keep the assertion messages the same.

Yes, that would work... How do you feel about preserving f"{real_count} != {count}, etc., which sort of summarize the failure, and OTH dropping False is not true which doesn't add any value to the message?

perhaps we want to make sure all of the failure messages are covered by tests

I did an audit and added tests in #19588 👍🏾. Let me know if you think we should also add for cases like [] is not true and False is not true (which I am inclined to replace with self.fail() in this PR).

How do you feel about preserving f"{real_count} != {count}, etc., which sort of summarize the failure, and OTH dropping False is not true which doesn't add any value to the message?

I don't think either prefix adds too much value given the messages that follow. For someone to be relying on our failure message would be strange.
So I'm open to these changing, but would appreciate more opinions (maybe @adamchainz @nessita )

…sertNotContains only on test failure. Thanks to Adam Johnson for the report. Performance regression in 1dae65d. Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com>

… test failure.

Thanks to Adam Johnson for the report and to Sarah Boyce for the review.

cliff688 · 2025-06-25T13:31:09Z

I took a stab at adding benchmarks in django/django-asv#91.

cliff688 commented Apr 20, 2025

View reviewed changes

cliff688 marked this pull request as draft April 22, 2025 10:52

cliff688 mentioned this pull request May 14, 2025

Fixed #33312 -- Raised explicit exception when copying deferred model instances. #19445

Merged

cliff688 force-pushed the ticket-35728 branch from 4eb0357 to 2bcbf13 Compare June 2, 2025 21:42

cliff688 marked this pull request as ready for review June 3, 2025 07:33

cliff688 force-pushed the ticket-35728 branch from 2bcbf13 to 096b61d Compare June 3, 2025 07:41

sarahboyce reviewed Jun 20, 2025

View reviewed changes

cliff688 and others added 2 commits June 25, 2025 14:30

Refs #35728 -- Computed error messages for TestCase.assertContains|as…

07c995b

…sertNotContains only on test failure. Thanks to Adam Johnson for the report. Performance regression in 1dae65d. Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com>

Refs #35728 -- Computed error messages for assert_form_errors only on…

9aaee89

… test failure.

cliff688 force-pushed the ticket-35728 branch from 096b61d to 251eed5 Compare June 25, 2025 12:32

Refs #35728 -- Computed assertInHTML failure messages only on failure.

62f1594

Thanks to Adam Johnson for the report and to Sarah Boyce for the review.

cliff688 force-pushed the ticket-35728 branch from 251eed5 to 62f1594 Compare June 25, 2025 12:35

cliff688 mentioned this pull request Jun 27, 2025

Refs #35728 -- Added standard failure prefixes to assertion message tests. #19588

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fixed #35728 -- Computed error messages in assertions only on test failures. #19402

Fixed #35728 -- Computed error messages in assertions only on test failures. #19402

Uh oh!

cliff688 commented Apr 20, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

cliff688 Apr 20, 2025

Uh oh!

cliff688 commented Apr 20, 2025 •

edited

Loading

Uh oh!

adamchainz commented May 26, 2025

Uh oh!

sarahboyce left a comment

Uh oh!

sarahboyce Jun 20, 2025

Uh oh!

cliff688 Jun 21, 2025

Uh oh!

sarahboyce Jun 23, 2025

Uh oh!

cliff688 Jun 24, 2025

Uh oh!

sarahboyce Jun 25, 2025

Uh oh!

cliff688 commented Jun 25, 2025

Uh oh!

Uh oh!

Uh oh!

Fixed #35728 -- Computed error messages in assertions only on test failures. #19402

Are you sure you want to change the base?

Fixed #35728 -- Computed error messages in assertions only on test failures. #19402

Uh oh!

Conversation

cliff688 commented Apr 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Trac ticket number

Branch description

Checklist

Uh oh!

Uh oh!

Uh oh!

cliff688 Apr 20, 2025

Choose a reason for hiding this comment

Uh oh!

cliff688 commented Apr 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamchainz commented May 26, 2025

Uh oh!

sarahboyce left a comment

Choose a reason for hiding this comment

Uh oh!

sarahboyce Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

cliff688 Jun 21, 2025

Choose a reason for hiding this comment

Uh oh!

sarahboyce Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

cliff688 Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

sarahboyce Jun 25, 2025

Choose a reason for hiding this comment

Uh oh!

cliff688 commented Jun 25, 2025

Uh oh!

Uh oh!

cliff688 commented Apr 20, 2025 •

edited

Loading

cliff688 commented Apr 20, 2025 •

edited

Loading