[Django] #30249: Deprecate re-raising view exceptions from test client in tests
Django
------------------------------------------------+------------------------
Reporter: Jon Dufresne | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
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
[https://docs.djangoproject.com/en/2.2/ref/settings/#debug-propagate-
exceptions 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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/30249>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Jon Dufresne | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jon Dufresne):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/11077 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/30249#comment:1>
Django
Reporter: Jon Dufresne | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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 say wontfix here.
(Sorry to think that.)
--
Ticket URL: <https://code.djangoproject.com/ticket/30249#comment:2>
Django
Reporter: Jon Dufresne | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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 this 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, response, msg)
}}}
I could then document it 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30249#comment:3>
Django
Reporter: Jon Dufresne | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Hey Jon.
Thanks for the follow-up.
Quick comments:
1. Some helper would be better yes. `assertResponseException()` or such.
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.
(**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.)
--
Ticket URL: <https://code.djangoproject.com/ticket/30249#comment:4>
Django
Reporter: Jon Dufresne | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30249#comment:5>
Django
Reporter: Jon Dufresne | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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 fut
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 if "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 (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30249#comment:6>
Django
Reporter: Jon Dufresne | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* needs_better_patch: 0 => 1
* resolution: => needsinfo
Comment:
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 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!
--
Ticket URL: <https://code.djangoproject.com/ticket/30249#comment:7>