Opened 11 years ago
Last modified 10 months ago
#20915 new Cleanup/optimization
Remove django.test.client dependency on django.contrib.auth (and .sessions?)
Reported by: | Ramiro Morales | Owned by: | nobody |
---|---|---|---|
Component: | Testing framework | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Ülgen Sarıkavak | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
As outlined in #8713 (more precisely on comment:13:ticket:8713). This is action item 2.
The Client methods call at some point the login(), logout(), authenticate() and get_user_model() functions from django.contrib.auth.
Also, Client code checks for the presence of the 'django.contrib.sessions' string in INSTALLED_APPS in a couple of places. Not sure if this counts as a dependency.
Change History (13)
follow-up: 2 comment:1 by , 11 years ago
comment:2 by , 11 years ago
Replying to mjtamlyn:
What would be the proposed solution here? The client should ideally retain the ability to do most of these things. Unless I'm mistaken,
contrib.auth
does not need to be installed for the client to function properly for general requests. I guess these are "optional" features of the client which are enabled when contrib.auth is available. It would be pretty awkward if you had to customise the client every time you wanted to use this functionality.
Obviously if the general functionality of the client doesn't work without
auth
then we have a problem we should fix. It may also be worth considering providing an 'auth-less' version of the test client.
I opened the ticket without initially without any solution in mind, just presenting the "issue". Mainly a place holder to possibly opening a django-dev discussion about if this is something worth changing in order to be able to say 'Django core (or non-contrib) has no deps on django.contrib'
Later, once I looked at the code a bit, second reason was to have an anchor to associate a PR (see below).
comment:4 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:5 by , 11 years ago
It'd be great to achieve this decoupling, can I help with sth to make it happen? Maybe I can submit the API doc needed in ramiro's patch?
comment:7 by , 11 years ago
An idle thought about possible implementations for this: If we move to a position where AuthConfig.ready() will always be invoked, we could install the login/logout methods on the TestClient using a contribute_to_class-like approach. That way, if you don't have auth installed, the test client won't have login/logout methods; if you do, it gains login/logout, and an auth dependency.
comment:8 by , 11 years ago
The more explicit / less monkeypatchy alternative would be to just have contrib.auth provide a Client subclass with these methods, and you have to import and use that one if you're using auth. That would require a deprecation process, though.
follow-up: 10 comment:9 by , 11 years ago
Russell's approach might be nice if there are other third party apps who wish to push utilities into the API. I could imagine a situation where something like contrib.admin or an API framework or CMS system could have utility assertions for common patterns of testing your configuration of them, which might feel appropriate to be client methods.
The issue with Carl's suggestion is that if auth provides a subclass and any other third party API provides a subclass merging them quickly gets boring, although it is the more explicit approach and it would remove the dependency more cleanly.
follow-up: 11 comment:10 by , 11 years ago
Replying to mjtamlyn:
Russell's approach might be nice if there are other third party apps who wish to push utilities into the API. I could imagine a situation where something like contrib.admin or an API framework or CMS system could have utility assertions for common patterns of testing your configuration of them, which might feel appropriate to be client methods.
The issue with Carl's suggestion is that if auth provides a subclass and any other third party API provides a subclass merging them quickly gets boring, although it is the more explicit approach and it would remove the dependency more cleanly.
Boring perhaps, but also trivially easy, takes advantage of ordinary Python programming techniques, and has all the flexibility of ordinary Python. Especially if the extra features are provided both as a mixin class and then as a Client subclass using the mixin.
I have trouble seeing the rationale for inventing our own magical monkeypatching framework for this specific case, when ordinary Python has the tools to solve the problem.
follow-up: 12 comment:11 by , 11 years ago
Replying to carljm:
I have trouble seeing the rationale for inventing our own magical monkeypatching framework for this specific case, when ordinary Python has the tools to solve the problem.
I'm thinking of the case where there are *other* apps that need to inject functionality into the test client. I'm following the "special cases are never *that* special" logic. Auth has a need to inject some extra functionality into the test client (however it's done). There's no reason to believe that other apps won't have the same need.
That said, to my knowledge, there isn't a huge community of 3rd party apps waiting to inject functionality into the test client. There's a possibility that this is a case of YAGNI.
And regardless, this could also be done with subclassing (or mixins).
This only gets complicated if there are two or more apps that provide a useful mixin - at that point, the end user doesn't have a prey-a-porter TestClient class; they need to have a testing utility library that composes the TestClient out of all the bits they need.
I'm not especially invested in either approach; I don't have the same visceral reaction against contribute_to_class, but I can appreciate where that reaction comes from.
comment:12 by , 11 years ago
Replying to russellm:
I'm thinking of the case where there are *other* apps that need to inject functionality into the test client. I'm following the "special cases are never *that* special" logic. Auth has a need to inject some extra functionality into the test client (however it's done). There's no reason to believe that other apps won't have the same need.
That said, to my knowledge, there isn't a huge community of 3rd party apps waiting to inject functionality into the test client. There's a possibility that this is a case of YAGNI.
It's possible there might be other apps that would use it, but I haven't seen the demand. I mean, if people want to add special features to the test client they already can subclass it (or monkeypatch it themselves); I haven't seen so many people doing that as to justify a new feature specifically to support it.
And what the Zen has to say about special cases is "special cases aren't special enough to break the rules", which I think weighs in the opposite direction :-)
And regardless, this could also be done with subclassing (or mixins).
This only gets complicated if there are two or more apps that provide a useful mixin - at that point, the end user doesn't have a prey-a-porter TestClient class; they need to have a testing utility library that composes the TestClient out of all the bits they need.
True. I guess I don't find that three-liner composition a problematic burden, for what I think would be an unusual case.
I'm not especially invested in either approach; I don't have the same visceral reaction against contribute_to_class, but I can appreciate where that reaction comes from.
Contribute_to_class
(as its currently used anyway) only touches a class that you've already attached something to explicitly. This proposal is for outright action-at-a-distance where-the-heck-did-that-come-from monkeypatching, which feels quite different to me.
comment:13 by , 10 months ago
Cc: | added |
---|
What would be the proposed solution here? The client should ideally retain the ability to do most of these things. Unless I'm mistaken,
contrib.auth
does not need to be installed for the client to function properly for general requests. I guess these are "optional" features of the client which are enabled when contrib.auth is available. It would be pretty awkward if you had to customise the client every time you wanted to use this functionality.Obviously if the general functionality of the client doesn't work without
auth
then we have a problem we should fix. It may also be worth considering providing an 'auth-less' version of the test client.