#34657 closed New feature (fixed)

Testing assertions `assertContains` and `assertInHTML` should output the haystack on failure

Reported by: Thibaud Colas Owned by: Chinmoy
Component: Testing framework Version: dev
Severity: Normal Keywords: HTML, assertions, testing
Cc: Chinmoy 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 (last modified by Thibaud Colas)

If I use the vanilal Python assertIn or assertRegex, when the assertion fails, it’s very simple to assess what went wrong from the test error only:

    self.assertIn("<b>hey</b>", "<p>Howdy!</p>")
AssertionError: '<b>hey</b>' not found in '<p>Howdy!</p>'

With Django’s assertContains and assertInHTML, this gets much harder:

    self.assertInHTML("<b>hey</b>", "<p>Howdy!</p>")
  File "/Users/thibaudcolas/Dev/django/html-testing-with-django/.venv/lib/python3.11/site-packages/django/test/testcases.py", line 1076, in assertInHTML
    self.assertTrue(
AssertionError: False is not true : Couldn't find '<b>
hey
</b>' in response

and:

    self.assertContains(res, "<b>hey</b>")
  File "/Users/thibaudcolas/Dev/django/html-testing-with-django/.venv/lib/python3.11/site-packages/django/test/testcases.py", line 660, in assertContains
    self.assertTrue(
AssertionError: False is not true : Couldn't find '<b>hey</b>' in response

---

In both cases, Django doesn’t display the haystack – so I have to waste a lot of time going back to my HTML templates to check what they might be outputting / or potentially load the same scenario in a browser / or manually add print statements to my test cases. This is all very time-consuming. Instead, it’d be much better if the haystack was just present.

Additionally for assertInHTML – it’s annoying that needle as displayed in the failure message is the parser’s output, which is therefore broken up over multiple lines. It’d be much nicer if the example above resulted in AssertionError: False is not true : Couldn't find '<b>hey</b>' in response.

---

Test suite I used to compare output for reference: https://github.com/thibaudcolas/html-testing-with-django/blob/39c82c410cce4bea71ac54be27be67057f4d8dd8/testing_tests/tests.py#L5

And link to Python’s assertIn / assertNotIn implementations: https://github.com/python/cpython/blob/101d5ec7d7fe122fa81a377c8ab8b562d1add9ee/Lib/unittest/case.py#L1147-L1159

Change History (25)

comment:1 by Thibaud Colas, 17 months ago

Description: modified (diff)

comment:2 by Natalia Bidart, 17 months ago

Keywords: testcases unit tests removed
Triage Stage: UnreviewedAccepted

Tentatively accepting because this is related to the testing framework, though I'm not 100% sure on procedure whether this specific case needs a forum post since it's marked as a new feature. I would argue this is a cleanup/optimization :thinking:

comment:3 by Sarah Boyce, 17 months ago

I can see the value of this. The react testing library we use has a setting DEBUG_PRINT_LIMIT (https://testing-library.com/docs/dom-testing-library/api-debugging/#automatic-logging) with a default of 7000 characters but you can increase it if you need to see more printed. I recommend that we have a similar setting 👍

in reply to:  2 comment:4 by Mariusz Felisiak, 17 months ago

Replying to Sarah Boyce:

I can see the value of this. The react testing library we use has a setting DEBUG_PRINT_LIMIT (https://testing-library.com/docs/dom-testing-library/api-debugging/#automatic-logging) with a default of 7000 characters but you can increase it if you need to see more printed. I recommend that we have a similar setting 👍

Creating a new setting is always controversial (we already have many of them) and users can already control it with maxDiff.

Last edited 17 months ago by Mariusz Felisiak (previous) (diff)

comment:5 by Thibaud Colas, 17 months ago

Sorry, I didn’t realise feature requests require a forum post! I’ll know for next time.

comment:6 by Shamil Abdulaev, 16 months ago

I have fixed this bug and sent a PR https://github.com/django/django/pull/17077 for consideration!)

Last edited 16 months ago by Shamil Abdulaev (previous) (diff)

comment:7 by Sarah Boyce, 16 months ago

Has patch: set
Needs documentation: set
Needs tests: set
Patch needs improvement: set

comment:8 by Sarah Boyce, 16 months ago

Owner: changed from nobody to Shamil Abdulaev
Status: newassigned

comment:9 by Shamil Abdulaev, 16 months ago

Resolution: fixed
Status: assignedclosed

comment:10 by Sarah Boyce, 16 months ago

Resolution: fixed
Status: closednew

Hi Shamil, we still need to review and merge in a PR before we mark a ticket as fixed. In general I would leave the status alone as mostly the fellows take care of this
I've written a couple of comments on the PR, once they're sorted we can update the ticket to go into the review queue

comment:11 by Chinmoy, 16 months ago

Hey, is this still being worked upon? Can I assign myself to this ticket?

in reply to:  11 comment:12 by Natalia Bidart, 16 months ago

Owner: changed from Shamil Abdulaev to Chinmoy
Status: newassigned

Replying to Chinmoy:

Hey, is this still being worked upon? Can I assign myself to this ticket?

Hi! Thank you for your interest in contributing. I checked and both related PRs are closed so if you can work on this, it'll be certainly welcomed! Thanks.

comment:13 by Natalia Bidart, 16 months ago

Has patch: unset
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

Resetting patch flags.

comment:14 by Chinmoy, 15 months ago

Cc: Chinmoy added

comment:15 by Chinmoy, 15 months ago

Hey, sorry I missed this in my mail. Will submit a PR soon.

comment:16 by Chinmoy, 15 months ago

Has patch: set

comment:17 by Mariusz Felisiak, 15 months ago

Needs documentation: set
Patch needs improvement: set
Version: dev

comment:18 by Chinmoy, 15 months ago

Needs documentation: unset
Patch needs improvement: unset

comment:19 by Mariusz Felisiak, 15 months ago

Needs tests: set

comment:20 by Chinmoy, 15 months ago

Needs tests: unset

comment:21 by Mariusz Felisiak <felisiak.mariusz@…>, 14 months ago

In e99c7d88:

Refs #34657 -- Made assertInHTML() use unparsed needle in error messages.

comment:22 by Mariusz Felisiak, 14 months ago

Patch needs improvement: set

comment:23 by GitHub <noreply@…>, 14 months ago

In 679212a:

Refs #34657 -- Made msg_prefix handling in assertURLEqual()/assertInHTML consistent with other assertions.

Co-authored-by: Chinmoy Chakraborty <chinmoy12c@…>

comment:24 by Mariusz Felisiak, 14 months ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:25 by Mariusz Felisiak <felisiak.mariusz@…>, 14 months ago

Resolution: fixed
Status: assignedclosed

In 1dae65dc:

Fixed #34657 -- Made assert(Not)Contains/assertInHTML display haystacks in error messages.

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