django icon indicating copy to clipboard operation
django copied to clipboard

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

Open cliffordgama opened this issue 7 months ago • 2 comments

Trac ticket number

ticket-35728

Branch description

There are two main approaches used.

  1. 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 db0cfda3a110392a99f31b54125b38ba2646a90d 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

  • [x] This PR targets the main branch.
  • [x] The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • [x] I have checked the "Has patch" ticket flag in the Trac system.
  • [x] 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.

cliffordgama avatar Apr 20 '25 10:04 cliffordgama

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?

cliffordgama avatar Apr 20 '25 10:04 cliffordgama

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.

adamchainz avatar May 26 '25 16:05 adamchainz

I took a stab at adding benchmarks in https://github.com/django/django-asv/pull/91/.

cliffordgama avatar Jun 25 '25 13:06 cliffordgama