assertRaises vs. assertRaisesMessage

[Shai Berger]

Hello all,

A recent PR[1] seeks to replace most of the assertRaises() calls in the test-
suite with assertRaisesMessage(). The argument for it is that it then becomes
easier to find the test for some error handling, by grepping the test for the
message text. Also, in some cases, an existing assertRaises() is followed by a
separate, explicit verification of the message text[2], which surely indicates
that assertRaisesMessage() should have been used; and actually, this whole
move initiated as part of the Py2-support-deprecation changes.

However, I don't think we should generally prefer assertRaisesMessage to
assertRaises, because it means that trivial phrasing changes need to be made
twice (at least) instead of once. I don't believe exception messages should be
considered part of the stable API (as opposed to exception types), and so, I
think it is, in many cases, actually wrong to test them as such.

In some places, it makes sense to assert that certain information is included
in the message, and then assertRaisesRegex could be used. Tim made the
practical argument that regexes in these circumstances introduce more
complexity than they're worth, and that is certainly a valid argument; I can
agree with a general policy of "prefer assertRaisesMessage to
assertRaisesRegex". But I think in most places, we should only test for the
exception type, and if more info is required, it should be structured -- that
is, presented as attributes on the exception object rather than text in the
message.

What do you think?

Thanks,
Shai.

[1] https://github.com/django/django/pull/8257
[2] https://github.com/django/django/pull/8257#pullrequestreview-30486470

[Tim Graham]

Hi Shai,

Without testing for a message, there's also a possibility that the exception you think you're testing isn't the one the test is actually raising. I think the readability advantages of including messages in the tests outweigh the cost of updating tests when a message changes.

[Shai Berger]

Hi Tim,

> Without testing for a message, there's also a possibility that the
> exception you think you're testing isn't the one the test is actually
> raising.

If you need to verify the error message for that reason, then the exception is
probably not specific enough. Testing the message, in this case, is just
working around faulty design.

> I think the readability advantages of including messages in the
> tests outweigh the cost of updating tests when a message changes.

In some cases, of course; but I see a significant distance between that and a
general recommendation to prefer assertRaisesMessage.

Shai.

[Adam Johnson]

I agree with Tim. He's also probably the most familiar with the cost of message changes 😊

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscribe@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/e1e8010e-903f-45dd-8e15-9f5da8150665%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Adam

[Adam Johnson]

That said more structured attributes on exceptions can be useful for Django users, if you know any obvious places they are missing.

--

Adam

[James Pic]

Without diving into implementation details, I recon I've been taught the same as Shai. An exception type should have only one purpose, thus testing the type /should/ be sufficient imho. That said, if you want to TDD the message, you're going to code a test for it, is there really another way ? ;)

[Josh Smeaton]

We throw lots of ValueErrors and TypeErrors rather than creating new exception types all over the place. There's definitely an argument to be made that different exception types can be created. But I know that I've definitely run afoul of believing I was testing one error (assertRaises) when I wasn't. A different ValueError was being caught and the test was incorrectly passing. I would prefer assertRaises[Message|Regex] for verifying the location of the error (not the exact string) where it makes sense, and a simple assertRaises where the behaviour is immediately clear.

[Mads Jensen]

On Sunday, April 9, 2017 at 3:24:42 PM UTC+2, Josh Smeaton wrote:

We throw lots of ValueErrors and TypeErrors rather than creating new exception types all over the place. There's definitely an argument to be made that different exception types can be created. But I know that I've definitely run afoul of believing I was testing one error (assertRaises) when I wasn't. A different ValueError was being caught and the test was incorrectly passing. I would prefer assertRaises[Message|Regex] for verifying the location of the error (not the exact string) where it makes sense, and a simple assertRaises where the behaviour is immediately clear.

There are quite a lot of these calls. I noticed that tests don't always inherit from django.test.TestCase and thus assertRaisesMessage is not available.
I'm not sure if this PR should be exhaustive. I've added the message to all instances of some exceptions, but TypeError/ValueError (as highlighted by Josh Smeaton) are all over the place.

While working at this, I noticed this inconsistency (I think the same check should be in the base-backend).
https://github.com/django/django/blob/master/django/db/backends/sqlite3/operations.py#L262-L263
https://github.com/django/django/blob/master/tests/expressions/tests.py#L1164-L1166

~ Mads

[Shai Berger]

Hi,

I apologize for re-raising a thread that's mostly done and decided, but I just
ran into [1] where it says:

"""
Note

Exception messages are not part of the Python API. Their contents may change
from one version of Python to the next without warning and should not be
relied on by code which will run under multiple versions of the interpreter.
"""

I don't think that's enough to overturn the decision, but I thought this
should be noted in this thread.

[1] https://docs.python.org/3/reference/executionmodel.html#exceptions

[Mads Jensen]

On Thursday, May 25, 2017 at 5:03:54 PM UTC+2, Shai Berger wrote:

Hi,

I apologize for re-raising a thread that's mostly done and decided, but I just
ran into [1] where it says:

"""
Note

Exception messages are not part of the Python API. Their contents may change
from one version of Python to the next without warning and should not be
relied on by code which will run under multiple versions of the interpreter.
"""

I don't think that's enough to overturn the decision, but I thought this
should be noted in this thread.

I asked Tim on IRC about this some weeks ago. I think it suffices to restrict the PR to Django's own exception messages. There are quite a lot of tests for IndexError, TypeError etc. which use messages from Python.

I created https://code.djangoproject.com/ticket/28224 while doing work on this PR.

[1] https://docs.python.org/3/reference/executionmodel.html#exceptions

~ Mads