Deprecate re-raising view exceptions from test client in tests

[Jon Dufresne]

This is a continuation of the work started in ticket #18707.

Set for removal in 4.0.

By no longer re-raising the view exceptions in tests, we'll move the test Client closer to behaving like a real world HTTP client. This assists integration tests by providing more realistic side effects to test against. Instead of altering the behavior under test, integration tests can more reliably test the full stack, including the error handlers (which were previously skipped in the default configuration).

Additionally, this behavior is duplicating the setting ​DEBUG_PROPAGATE_EXCEPTIONS.

Should a test want this behavior explicitly, it is already available by adjusting this setting. For example:

class MyTest(TestCase):
    @override_settings(DEBUG_PROPAGATE_EXCEPTIONS=True)
    def test_raises_exception(self):
        with self.assertRaises(KeyError):
            self.client.get('/broken_view/')

To assist in the deprecation period, I've opted to allow users to force the new behavior and silence warnings by setting DEBUG_PROPAGATE_EXCEPTIONS to the alternative falsey value None.

[Jon Dufresne]

​PR

[Carlton Gibson]

Ah, grrr. I'd rather not. (As much as I was and am +1 on #18707.)

Adjust below with "For me" whenever appropriate.

Taking an arbitrary change from the PR:

From this:

        with self.assertRaisesMessage(ImproperlyConfigured, msg):
	            self.client.get('/detail/author/invalid/qs/')

To this:

        response = self.client.get('/detail/author/invalid/qs/')
        self.assertEqual(response.status_code, 500)
        _, exc_value, _ = response.exc_info
        self.assertIsInstance(exc_value, ImproperlyConfigured)
        self.assertEqual(str(exc_value), msg)

It's both longer and less clear in intent.

Raising the exception is a great behaviour in most cases. It's a great default behaviour.
Yes, it's nice to have a switch (which we now do) for testing error views.
But the overall change isn't a win.

The @override_settings(DEBUG_PROPAGATE_EXCEPTIONS=False) is bad API.
I'm adjusting a setting to alter test client behaviour.
I want a kwarg for that.

Beyond all that I don't think it's worth the churn.

Again, all "For me" where needed.

As I say, grrr. -1 I'm afraid.

I'll leave this open for now to allow others to comment, before the inevitable trip to the mailing list, but I'd say wontfix here.
(Sorry to think that.)

[Jon Dufresne]

Hi Carlton, thanks for the response.

It's both longer ...

If you feel this is a matter of ergonomics, then I'm happy to add a helper assert method. Something like assertResponseException such that

response = self.client.get('/detail/author/invalid/qs/')
self.assertEqual(response.status_code, 500)
_, exc_value, _ = response.exc_info
self.assertIsInstance(exc_value, ImproperlyConfigured)
self.assertEqual(str(exc_value), msg)

Becomes:

response = self.client.get('/detail/author/invalid/qs/')
self.assertResponseException(response, ImproperlyConfigured, msg)

I could then document and use it throughout the Django test suite. WDYT?

... and less clear in intent.

IMO, the previous version is less clear of the actual behavior under test. If I didn't already know how the Django test client works, when reading:

with self.assertRaisesMessage(ImproperlyConfigured, msg):
    self.client.get('/detail/author/invalid/qs/')

Upon first read (again, if I didn't already know how the Django test client works) this leads me to believe that making a GET request on the provided URL results in an unhandled exception. However, this is not the case. The exception is handled by the default error handler, django.core.handlers.exception.handle_uncaught_exception and a 500 response is returned. Obviously, this is wrong as the test client has modified the code under test to make it appear as if the exception is unhandled. IMO, it is normally bad practice to modify the code under test unless testing for a very specific implementation detail. It should not be the default state of testing. Instead, we should be testing code as close to production as reasonable. By modifying the code under test, we've slightly moved away from the production code.

Raising the exception is a great behaviour in most cases. It's a great default behaviour.

Normally, I agree. But keep in mind, we're in the context of a test. Something will be asserted after the middleware + view executes (otherwise, what's the point of the test?) It could even be something as simple as self.assertEqual(response.status_code, 200). In the event of an unwanted view exception, these assertions will fail.

If you prefer the re-raising of exceptions for your project, then you can and should set the existing setting DEBUG_PROPAGATE_EXCEPTIONS to True in the test's settings.py. Then, the old behavior will be restored to eagerly throw exceptions.

I've created an alternate form of the PR with this set during Django's test suite here:

​https://github.com/django/django/compare/master...jdufresne:raise-request-exception-default-on

As you can see, this version has many fewer adjustments to tests. If this version is more agreeable, I'm happy to replace the PR with it.

The @override_settings(DEBUG_PROPAGATE_EXCEPTIONS=False) is bad API.

This setting already exits. I didn't invent it. It is documented here: ​https://docs.djangoproject.com/en/2.1/ref/settings/#debug-propagate-exceptions

I don't care much for this setting on its own, I only noticed that it already existed so I didn't invent a new mechanism. Having multiple ways to re-raise request exceptions seems unnecessary to me. I've never used it prior to this PR.

Beyond all that I don't think it's worth the churn.

To be clear, this isn't about churn. It is about moving the test client closer to simulating a real world HTTP client by avoiding modifying code under test, which I believe will result in more productive tests.

[Carlton Gibson]

Hey Jon.

Thanks for the follow-up.

Quick comments:

1. Some helper would be better yes. assertResponseException() or such. (But not sure it's worth adding unless we remove this option from TestClient, but, as per below, I don't think we should do that...)
2. The issue with the setting isn't that you invented it or not: rather it's using a setting, which has other purposes, to control the test-client behaviour.

I like and use the current behaviour. Maybe one-day we remove DEBUG_PROPOGATE_EXCEPTION, for a custom handler subclass or such. What do I do then? (For me 🙂) clearly the right answer is a flag on TestClient, which we already have.

(Even if we don't remove it, using the setting this way is—to use the official technical term—horrific 🙂: a flag on TestCient is just right, and I'm very glad you added it, after all these years.)

(Maybe then swapping the default...? — but meh... grrrr...)

(Re, the churn, yes, if we think it's an improvement then the churn is worth it — I'm just not there as yet, so it's "beyond all that...")

[Jon Dufresne]

Some helper would be better yes. assertResponseException() or such.

Yeah, I definitely agree. In my latest revision of the PR, I've added SimpleTestCase.assertRequestRaises(response, expected_exception, expected_message) along with documentation and tests.

With this in place, tests are now exactly the same size as before:

with self.assertRaises(AttributeError):
    self.client.get(reverse('admin:admin_views_simple_changelist'))

Becomes

response = self.client.get(reverse('admin:admin_views_simple_changelist'))
self.assertRequestRaises(response, AttributeError)

---

I'm open to taking a different approach to triggering the old behavior. Ultimately, my goal is to have the test client not skip parts of the Django request/response stack by default -- bring Client closer to acting like a real world HTTP client. If that is through the existing setting or a new kwarg, either is fine with me.

I'll respond to your comments above as they're presented, but again, I'm happy to compromise on an alternative path.

rather it's using a setting, which has other purposes, to control the test-client behaviour.

Sorry, I'm not sure I follow. This setting is documented as:

If set to True, Django’s exception handling of view functions (handler500, or the debug view if DEBUG is True) and logging of 500 responses (django.request) is skipped and exceptions propagate upwards.

This can be useful for some test setups.

IIUC, that is exactly what you're trying to achieve in your tests. That is, the 500 handler is skipped and the exception is raised by the caller (in this case, a test method).

I like and use the current behaviour.

Yup, I understand. This behavior will continue to be available to those that want it by using the existing setting. That isn't going away.

Maybe one-day we remove DEBUG_PROPOGATE_EXCEPTION, for a custom handler subclass or such. What do I do then? (For me 🙂) clearly the right answer is a flag on TestClient, which we already have.

No one is suggesting DEBUG_PROPOGATE_EXCEPTION be removed nor have I ever seen such a suggestion outside this ticket. I'm not sure where this is coming from.

---

If you still disagree with the overall aim of the ticket, then I'll prepare something to discuss further on the mailing list. If you disagree only on some of the details, then I think we can work out a compromise that fits both our needs.

Thanks again for the responses and discussion.

[Carlton Gibson]

Hey Jon.

You've only quoted part of the description for DEBUG_PROPAGATE_EXCEPTIONS. The whole thing goes like this:

This can be useful for some test setups. It shouldn’t be used on a live site unless you want your web server (instead of Django) to generate “Internal Server Error” responses. In that case, make sure your server doesn’t show the stack trace or other sensitive information in the response.

So the other use is having your web server handle the 500 (or whatever) rather than Django doing it. Nothing to do with testing.

Now I know you're not suggesting we get rid of it. My point was that as an action at a distance setting, controlling the exception handling behaviour, we might well want to phase it out at some point in the future.

It was the action at a distance nature of it that I thought you would, equally to me, not be keen on.

The point I (really!) imagined that we'd just agree on is that action at a distance type code is to be avoided. (Hence my use of "horrific" — using humour as I didn't think it a point where we wouldn't already agree.)

So the relevance for your suggestion here is that I don't think an action at a distance use of a setting to control the behaviour of the test client is a good move. Rather, setting a flag on the test client — the raise_request_exceptions flag — is the right locus for this behaviour.

As I say, I really thought this would be something you just agreed with. (Action at a distance is horrible right? 🙂)

Then...

Ultimately, my goal is to have the test client not skip parts of the Django request/response stack by default.

This I understand. It's totally reasonable. (I'm not sure I agree yet but I understand the desire.)

For me, the way to do this, if we want it, is to swap the default behaviour of the existing flag (i.e. change raise_request_exceptions from True to False)

So, can we agree that far? (That's a genuine question.)

Then it's "should we switch the default?" And, "is there another way to achieve similar?" One existing way would be to set client_class on the TestCase to return a suitably configured client.

[Carlton Gibson]

Hey Jon.

Looking at the PR, I don't think we can re-purpose DEBUG_PROPAGATE_EXCEPTIONS as per your suggestion, since it plays that "your web server (instead of Django) to generate “Internal Server Error” responses" role. (That's independent of also thinking that a flag on the client is the right place to have that switch.)

The new raise_request_exception is targeting 3.0, so there's still time to make adjustments.

Initial thought was something like...

from django import test


class Client(test.Client):
    """Client subclass defaulting to raise_request_exception=False"""
    def __init__(self, enforce_csrf_checks=False, raise_request_exception=False, **defaults):
        super().__init__(enforce_csrf_checks, raise_request_exception, **defaults)


class ClientMixin:
    client_class = Client

Where you just mixin using a appropriate client subclass. Maybe we could do something better making client a property or such? (Or similar...) — Really happy to think about suggestions here!

Maybe we should take this opportunity to also switch the default behaviour from "yes, raise" to "no, don't", but personally I'd leave it how it is.

I'm really happy if you want to take this to the mailing list to discuss. In the meantime I'm going mark as needsinfo, either on suggestions for improving the API, or whether we should change the default, or, indeed, whether we can/should/want to use the setting as you suggested.

Thanks!