Used assertRaisesMessage() to test Django's error messages. #8257

atombrella · 2017-03-28T09:17:41Z

As requested in #8251 a separate PR to replace self.assertRaises with self.assertRaisesMessage.

self.assertRaisesRegex is not used anywhere in the testsuite, so instead of replacing everything (which takes time), I've introduced it in a few places.

IntegrityError, DatabaseError, DoesNotExist will be ignored.

timgraham

In my experience, regular expressions introduce unneeded complexity, that's why assertRaisesRegex isn't used. I'm not vehemently opposed to it if it makes sense in some places, but I think it can typically be avoided.

timgraham · 2017-03-28T13:57:23Z

tests/null_queries/tests.py

@@ -38,11 +38,12 @@ def test_none_as_null(self):
        )

        # Valid query, but fails because foo isn't a keyword
-        with self.assertRaises(FieldError):
+        with self.assertRaisesMessage(


Please use this style in places like this:

msg = "..." with self.assertRaisesMessage(FieldError, msg):

timgraham · 2017-03-28T13:58:33Z

tests/aggregation_regress/tests.py

@@ -430,13 +430,14 @@ def test_decimal_aggregate_annotation_filter(self):

    def test_field_error(self):
        # Bad field requests in aggregates are caught and reported
-        with self.assertRaises(FieldError):
+        msg = "Cannot resolve keyword 'foo' into field. Choices are: [A-Za-z, _\-]+"
+        with self.assertRaisesRegex(FieldError, msg):


I'd use assertRaisesMessage() with Cannot resolve keyword 'foo' into field. Choices are: -- there's not much value in a regex for the choices, I think.

timgraham · 2017-03-28T13:59:38Z

tests/one_to_one/tests.py

@@ -226,7 +226,8 @@ def test_related_object_cache(self):
        setattr(p, 'restaurant', None)

        # You also can't assign an object of the wrong type here
-        with self.assertRaises(ValueError):
+        with self.assertRaisesMessage(
+                ValueError, 'Cannot assign "%s": "Place.restaurant" must be a "Restaurant" instance.' % repr(p)):


Use %r in the message.

timgraham · 2017-03-28T14:04:20Z

tests/custom_managers/tests.py

@@ -47,7 +47,8 @@ def test_queryset_copied_to_default(self):
            # Public methods are copied
            manager.public_method()
            # Private methods are not copied
-            with self.assertRaises(AttributeError):
+            msg = "'(BaseCustom)?ManagerFromCustomQuerySet' object has no attribute '_private_method'"


Can you interpolate the manager name in the message instead?

timgraham · 2017-03-30T16:15:45Z

tests/aggregation_regress/tests.py

@@ -430,13 +430,17 @@ def test_decimal_aggregate_annotation_filter(self):

    def test_field_error(self):
        # Bad field requests in aggregates are caught and reported
-        with self.assertRaises(FieldError):
+        msg = "Cannot resolve keyword 'foo' into field. Choices are: authors, contact, contact_id, hardbackbook, " \


The usual style is (wrapped at 79 chars):

msg = ( "Cannot resolve " " " )

Is 79 a hard limit? There are a few places where a single name could be fitted onto the previous line which would exceed the limit (but only by a little bit).

No, it's not.

timgraham · 2017-03-30T16:16:59Z

tests/aggregation_regress/tests.py

@@ -777,7 +782,8 @@ def test_values_queryset_non_conflict(self):
    def test_reverse_relation_name_conflict(self):
        # Regression for #11256 - providing an aggregate name
        # that conflicts with a reverse-related name on the model raises ValueError
-        with self.assertRaises(ValueError):
+        with self.assertRaisesMessage(
+                ValueError, "The annotation 'book_contact_set' conflicts with a field on the model."):


use msg="..." style

timgraham · 2017-03-30T16:18:54Z

tests/model_fields/test_decimalfield.py

@@ -14,7 +14,8 @@ def test_to_python(self):
        f = models.DecimalField(max_digits=4, decimal_places=2)
        self.assertEqual(f.to_python(3), Decimal('3'))
        self.assertEqual(f.to_python('3.14'), Decimal('3.14'))
-        with self.assertRaises(ValidationError):
+        msg = "%r value must be a decimal number."


No need to interpolate 'abc' in the message -- this might be done in some existing tests for Python 2 compatibility to account for u'' prefixes on Python 2 but could be removed now.

timgraham · 2017-03-30T16:20:31Z

tests/pagination/tests.py

@@ -302,7 +302,7 @@ def test_page_getitem(self):
        # Make sure object_list queryset is not evaluated by an invalid __getitem__ call.
        # (this happens from the template engine when using eg: {% page_obj.has_previous %})
        self.assertIsNone(p.object_list._result_cache)
-        with self.assertRaises(TypeError):
+        with self.assertRaisesMessage(TypeError, ''):


This is asserting '' is in the exception message which doesn't do anything useful.

OK. Actually the exception message is just blank.

timgraham · 2017-03-30T16:21:35Z

tests/one_to_one/tests.py

@@ -226,7 +226,8 @@ def test_related_object_cache(self):
        setattr(p, 'restaurant', None)

        # You also can't assign an object of the wrong type here
-        with self.assertRaises(ValueError):
+        msg = 'Cannot assign "%s": "Place.restaurant" must be a "Restaurant" instance.'


You could use %r or just hard-code the repr of the class in the expected message.

timgraham · 2017-03-30T16:23:56Z

tests/prefetch_related/tests.py

            list(qs)

        self.assertIn('prefetch_related', str(cm.exception))
        self.assertIn("name", str(cm.exception))

    def test_forward_m2m_to_attr_conflict(self):
-        msg = 'to_attr=authors conflicts with a field on the Book model.'


I don't see a big advantage to this change. The coding style says to use longer lines if it makes things easier to read -- my taste is to use msg = '...' if with self.assertRaisesMessage(ValueError, '....'):. is much over 79 chars.

timgraham · 2017-03-30T16:24:24Z

tests/raw_query/tests.py

@@ -276,7 +276,7 @@ def test_get_item(self):
        first_two = Author.objects.raw(query)[0:2]
        self.assertEqual(len(first_two), 2)

-        with self.assertRaises(TypeError):
+        with self.assertRaisesMessage(TypeError, ''):


Not useful as previously commented.

timgraham · 2017-03-30T16:25:21Z

tests/settings_tests/tests.py

@@ -174,28 +175,30 @@ def test_override_change(self):
        del settings.TEST

    def test_override_doesnt_leak(self):
-        with self.assertRaises(AttributeError):
+        msg = "'Settings' object has no attribute %r" % 'TEST'


I guess you meant to use self.msg here?

timgraham · 2017-03-30T16:26:12Z

tests/settings_tests/tests.py

@@ -148,6 +148,7 @@ class SettingsTests(SimpleTestCase):
    def setUp(self):
        self.testvalue = None
        signals.setting_changed.connect(self.signal_callback)
+        self.msg = "'Settings' object has no attribute %r"


That can be a class-level attribute -- no need to assign it in setUp for each test. Probably should have a more descriptive name like no_attribute_msg.

timgraham · 2017-03-30T16:34:11Z

tests/timezones/tests.py

@@ -123,7 +123,8 @@ def test_aware_datetime_in_other_timezone(self):
    @skipIfDBFeature('supports_timezones')
    def test_aware_datetime_unsupported(self):
        dt = datetime.datetime(2011, 9, 1, 13, 20, 30, tzinfo=EAT)
-        with self.assertRaises(ValueError):
+        msg = r'%s backend does not support timezone-aware datetimes when USE_TZ is False.' % connection.vendor


I'd use assertRaisesMessage() with msg = 'backend does not support timezone-aware datetimes when USE_TZ is False.'. There's a small chance that messages from third-party backends might not match the connection.vendor convention and I don't think it's a critical part of the assertion.

atombrella · 2017-03-31T18:13:21Z

I have still some places to change; it's not going to be exhaustive (there are more than 1000, and in some places assertRaisesMessage is not available). I think it makes sense that all error messages can be found in the test suite.

atombrella · 2017-04-03T10:58:02Z

tests/forms_tests/field_tests/test_uuidfield.py

@@ -18,7 +18,8 @@ def test_uuidfield_2(self):

    def test_uuidfield_3(self):
        field = UUIDField()
-        with self.assertRaises(ValidationError) as cm:
+        msg = 'Enter a valid UUID.'
+        with self.assertRaisesMessage(ValidationError, msg) as cm:
            field.clean('550e8400')
        self.assertEqual(cm.exception.messages[0], 'Enter a valid UUID.')


I suppose this line could be deleted because of assertRaisesMessage?

I forget if there was a reason to prefer the current version but yes, we don't need the check twice.

There are more instances of this pattern if you grep for cm.exception.messages. However, in some of them, e.g., in tests/auth_tests/test_validators.py), more checks are done are done on the exception (args, code) and it's a bit different for the forms.

shaib · 2017-04-07T09:33:06Z

Hello, I disagree with the general gist of this PR and posted about it to django-developers.

timgraham

This is enough for one commit, more can be done later if needed. Please add 0846e40.

timgraham · 2017-07-24T20:41:39Z

tests/update_only_fields/tests.py

            s.save(update_fields=['first_name'])

-        with self.assertRaises(ValueError):
+        with self.assertRaisesMessage(ValueError, msg % ''):


'' -> 'name'?

timgraham · 2017-07-24T20:43:02Z

tests/aggregation_regress/tests.py

@@ -738,19 +748,22 @@ def test_more_more(self):

    def test_duplicate_alias(self):
        # Regression for #11256 - duplicating a default alias raises ValueError.
-        with self.assertRaises(ValueError):
+        msg = "The named annotation 'authors__age__avg' conflicts with the default name for another annotation."


multiline please

timgraham · 2017-07-24T20:45:07Z

tests/basic/tests.py

@@ -397,7 +397,7 @@ def test_eq(self):
    def test_hash(self):
        # Value based on PK
        self.assertEqual(hash(Article(id=1)), hash(1))
-        with self.assertRaises(TypeError):
+        with self.assertRaisesMessage(TypeError, 'Model instances without primary key value are unhashable'):


use a msg variable

timgraham · 2017-07-24T20:46:21Z

tests/custom_managers/tests.py

@@ -90,7 +92,7 @@ def test_manager_attributes(self):
        querysets.
        """
        Person.custom_queryset_custom_manager.manager_only()
-        with self.assertRaises(AttributeError):
+        with self.assertRaisesMessage(AttributeError, "'CustomQuerySet' object has no attribute 'manager_only'"):


use a msg variable

timgraham · 2017-07-24T20:47:09Z

tests/expressions/tests.py

@@ -281,7 +282,8 @@ def test_new_object_create(self):

    def test_object_create_with_aggregate(self):
        # Aggregates are not allowed when inserting new data
-        with self.assertRaisesMessage(FieldError, 'Aggregate functions are not allowed in this query'):
+        msg = 'Aggregate functions are not allowed in this query'


Don't need to change this.

timgraham · 2017-07-24T20:53:55Z

tests/migrations/test_commands.py

                call_command("makemigrations")
+        exception_message = str(context.exception)
+        self.assertIn((


self.assertIn( 'Conflicting migrations detected; multiple leaf nodes ' 'in the migration graph:', exception_message )

timgraham · 2017-07-24T20:55:13Z

tests/multiple_database/tests.py

@@ -321,27 +321,30 @@ def test_m2m_cross_database_protection(self):

        mark = Person.objects.using('other').create(name="Mark Pilgrim")
        # Set a foreign key set with an object from a different database
-        with self.assertRaises(ValueError):
+        msg = 'Cannot assign "<Person: Marty Alchin>": the current database router prevents this relation.'


multiline messages much longer than 79 chars please

timgraham · 2017-07-24T20:55:51Z

tests/sitemaps_tests/test_http.py

@@ -17,6 +17,11 @@

 class HTTPSitemapTests(SitemapTestsBase):



no blank line before class attrs needed

timgraham · 2017-07-24T20:56:12Z

tests/template_tests/syntax_tests/test_with.py

@@ -6,6 +6,7 @@


 class WithTagTests(SimpleTestCase):
+    atleast_with_one_msg = "'with' expected at least one variable assignment"


timgraham · 2017-07-24T20:57:28Z

tests/update_only_fields/tests.py

@@ -120,7 +120,8 @@ def test_update_fields_m2m(self):
        a2 = Account.objects.create(num=2)
        e1.accounts.set([a1, a2])

-        with self.assertRaises(ValueError):
+        msg = 'The following fields do not exist in this model or are m2m fields: accounts'


Could go on the class-level to avoid duplicating it in two tests.

timgraham reviewed Mar 28, 2017

View reviewed changes

atombrella force-pushed the assertRaises branch 3 times, most recently from 6434ed1 to f62f8eb Compare March 30, 2017 14:06

timgraham reviewed Mar 30, 2017

View reviewed changes

atombrella force-pushed the assertRaises branch 2 times, most recently from 8b1d9bb to b8cd00d Compare March 31, 2017 16:54

atombrella force-pushed the assertRaises branch 3 times, most recently from 46593df to 74e6405 Compare April 3, 2017 10:42

atombrella commented Apr 3, 2017

View reviewed changes

felixxm mentioned this pull request Apr 7, 2017

Fixed #23147 -- Disabled a limit/offset on a query with select_for_update on Oracle. #8164

Merged

atombrella force-pushed the assertRaises branch 3 times, most recently from 7294d6e to 6e84568 Compare May 19, 2017 16:21

timgraham reviewed Jul 24, 2017

View reviewed changes

atombrella force-pushed the assertRaises branch 2 times, most recently from 3f3f52e to b268248 Compare July 28, 2017 21:08

Used assertRaisesMessage() to test Django's error messages.

a51c4de

timgraham force-pushed the assertRaises branch from b268248 to a51c4de Compare July 29, 2017 23:50

timgraham merged commit a51c4de into django:master Jul 30, 2017

timgraham changed the title ~~[WIP] Prefer assertRaisesMessage/assertRaisesRegex over assertRaises~~ Used assertRaisesMessage() to test Django's error messages. Jul 30, 2017

atombrella deleted the assertRaises branch July 31, 2017 05:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Used assertRaisesMessage() to test Django's error messages. #8257

Used assertRaisesMessage() to test Django's error messages. #8257

atombrella commented Mar 28, 2017

timgraham left a comment

timgraham Mar 28, 2017

timgraham Mar 28, 2017

timgraham Mar 28, 2017

timgraham Mar 28, 2017

timgraham Mar 30, 2017

atombrella Mar 31, 2017

timgraham Mar 31, 2017

timgraham Mar 30, 2017

timgraham Mar 30, 2017

timgraham Mar 30, 2017

atombrella Mar 31, 2017

timgraham Mar 30, 2017

timgraham Mar 30, 2017

timgraham Mar 30, 2017

timgraham Mar 30, 2017

timgraham Mar 30, 2017

timgraham Mar 30, 2017

atombrella commented Mar 31, 2017

atombrella Apr 3, 2017

timgraham Apr 3, 2017

atombrella Apr 4, 2017 •

edited

shaib commented Apr 7, 2017

timgraham left a comment

timgraham Jul 24, 2017

timgraham Jul 24, 2017

timgraham Jul 24, 2017

timgraham Jul 24, 2017

timgraham Jul 24, 2017

timgraham Jul 24, 2017

timgraham Jul 24, 2017

timgraham Jul 24, 2017

timgraham Jul 24, 2017

timgraham Jul 24, 2017

		@@ -6,6 +6,7 @@


		class WithTagTests(SimpleTestCase):
		atleast_with_one_msg = "'with' expected at least one variable assignment"

Used assertRaisesMessage() to test Django's error messages. #8257

Used assertRaisesMessage() to test Django's error messages. #8257

Conversation

atombrella commented Mar 28, 2017

timgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atombrella commented Mar 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atombrella Apr 4, 2017 • edited

Choose a reason for hiding this comment

shaib commented Apr 7, 2017

timgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atombrella Apr 4, 2017 •

edited