[Django] #25188: inconsistent escaping in assertRaisesMessage test output
Django
-----------------------------------+---------------------------------
Reporter: cjerdonek | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Severity: Normal | Keywords: assertRaisesMessage
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 1 | UI/UX: 0
-----------------------------------+---------------------------------
If `assertRaisesMessage` fails with differing exception text, then the
test output escapes the two strings differently in the display.
For example--
{{{#!python
def test(self):
with self.assertRaisesMessage(Exception, "two words"):
raise Exception("no, three words")
}}}
yields--
{{{
----------------------------------------------------------------------
Exception: no, three words
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "***.py", line 178, in test
raise Exception("no, three words")
AssertionError: "two\ words" does not match "no, three words"
}}}
Observe the spurious slash ("\") before the space. The test output is
especially annoying if the string is long and has many spaces. It can
look something like--
{{{
\{\
\ \ \ \ \"
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25188>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: cjerdonek | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Keywords: assertRaisesMessage | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by timgraham):
* needs_better_patch: => 0
* easy: 1 => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
I don't see an easy way to address this issue. The expected message is
passed through `re.escape(expected_message)` before it's passed to
`six.assertRaisesRegex`. It seems we would have to reimplement large parts
of `six.assertRaisesRegex` in order to unescape the pattern in the error
message.
--
Ticket URL: <https://code.djangoproject.com/ticket/25188#comment:1>
Django
Reporter: cjerdonek | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Keywords: assertRaisesMessage | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by cjerdonek):
Since only a plain string comparison is needed, is there a reason the
method needs to be using `assertRaisesRegex`? For example, couldn't the
implementation bypass `assertRaisesRegex` and go something like this?
(This is just an idea so shouldn't be taken as is.)
{{{#!python
@contextmanager
def assertRaisesMessage(self, expected_exception, expected_message):
with self.assertRaises(expected_exception) as cm:
yield
err = cm.exception
self.assertEqual(str(err), expected_message)
}}}
An implementation like this would have the further advantage that in the
case of a string mismatch the test output would show ''where'' the strings
differ by using unittest's `assertEqual`, which is better equipped for
this and has this added functionality.
--
Ticket URL: <https://code.djangoproject.com/ticket/25188#comment:2>
Django
Reporter: cjerdonek | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Keywords: assertRaisesMessage | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
The attached patch passes the test suite, however, it doesn't fail
correctly when `assertRaisesMessage()` is used in a non-context manager
context. If this issue can be solved, I don't see a reason not to use that
approach.
--
Ticket URL: <https://code.djangoproject.com/ticket/25188#comment:3>
Django
Reporter: cjerdonek | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Keywords: assertRaisesMessage | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
Changes (by timgraham):
* Attachment "25188.diff" added.
Django
Reporter: cjerdonek | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Keywords: assertRaisesMessage | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
Comment (by cjerdonek):
What about something like this?
{{{#!python
@contextmanager
def _assert_raises_message_cm(self, expected_exception, expected_message):
with self.assertRaises(expected_exception) as cm:
yield cm
err = cm.exception
self.assertEqual(str(err), expected_message)
def assertRaisesMessage(self, expected_exception, expected_message,
_callable=None, *args, **kwargs):
cm = self._assert_raises_message_cm(expected_exception,
expected_message)
if _callable is None:
# Then return the context manager.
return cm
# Otherwise, call the callable inside the context manager.
with cm:
_callable(*args, **kwargs)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25188#comment:4>
Django
Reporter: cjerdonek | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Keywords: assertRaisesMessage | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
Comment (by cjerdonek):
(Actually, if `callable` is supposed to be positional-only, then it could
be pulled as the first element of `*args`.)
--
Ticket URL: <https://code.djangoproject.com/ticket/25188#comment:5>
Django
Reporter: cjerdonek | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Keywords: assertRaisesMessage | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
* needs_better_patch: 1 => 0
Comment:
Yes, that seems to work. Thanks for the help.
[https://github.com/django/django/pull/5072 PR] for review.
--
Ticket URL: <https://code.djangoproject.com/ticket/25188#comment:6>
Django
Reporter: cjerdonek | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Keywords: assertRaisesMessage | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
Comment (by cjerdonek):
Thanks. I'm not sure if you prefer using GitHub's inline commenting
feature. But my only comment is: are you really supposed to be using
`assertIn()` here? The method name and documentation suggest that an
exact match is required (and that was my understanding before). If it
really should be `assertIn()`, perhaps the documentation can be updated to
clarify?
--
Ticket URL: <https://code.djangoproject.com/ticket/25188#comment:7>
Django
Reporter: cjerdonek | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Keywords: assertRaisesMessage | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
Comment (by cjerdonek):
From the docstring: "Asserts that the message in a raised exception
matches the passed value."
--
Ticket URL: <https://code.djangoproject.com/ticket/25188#comment:8>
Django
Reporter: cjerdonek | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.8
Keywords: assertRaisesMessage | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
Comment (by timgraham):
Yes, that's the current behavior since it uses a regular expression match.
Some tests in Django's test suite rely on this, so I don't think changing
it would be a good idea. I'll update the docs.
--
Ticket URL: <https://code.djangoproject.com/ticket/25188#comment:9>
Django
Reporter: cjerdonek | Owner: nobody
Type: Bug | Status: closed
Component: Testing framework | Version: 1.8
Keywords: assertRaisesMessage | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"1c7c782d6e7cdce09a982f05d468d538698c3004" 1c7c782]:
{{{
#!CommitTicketReference repository=""
revision="1c7c782d6e7cdce09a982f05d468d538698c3004"
Fixed #25188 -- Improved message raised by
SimpleTestCase.assertRaisesMessage().
Thanks Chris Jerdonek for the suggestion and help with the patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25188#comment:10>