[Django] #20915: Remove django.test.client dependency on django.contrib.auth (and .sessions?)
Django
.sessions?)
------------------------------------------------+------------------------
Reporter: ramiro | 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 |
------------------------------------------------+------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20915>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
.sessions?)
Reporter: ramiro | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Testing framework | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20915#comment:1>
Django
.sessions?)
Reporter: ramiro | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Testing framework | Resolution:
Severity: Normal | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by ramiro):
Replying to [comment:1 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).
--
Ticket URL: <https://code.djangoproject.com/ticket/20915#comment:2>
Django
.sessions?)
Reporter: ramiro | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Testing framework | Resolution:
Severity: Normal | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by ramiro):
See https://github.com/django/django/pull/1471 for an approach.
--
Ticket URL: <https://code.djangoproject.com/ticket/20915#comment:3>
Django
.sessions?)
Reporter: ramiro | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/20915#comment:4>
Django
.sessions?)
Reporter: ramiro | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jcatalan):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/20915#comment:5>
Django
.sessions?)
Reporter: ramiro | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by russellm):
See #21835 for a related problem with contenttypes.
--
Ticket URL: <https://code.djangoproject.com/ticket/20915#comment:6>
Django
.sessions?)
Reporter: ramiro | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by russellm):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20915#comment:7>
Django
.sessions?)
Reporter: ramiro | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20915#comment:8>
Django
.sessions?)
Reporter: ramiro | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20915#comment:9>
Django
.sessions?)
Reporter: ramiro | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
Replying to [comment:9 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20915#comment:10>
Django
.sessions?)
Reporter: ramiro | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by russellm):
Replying to [comment:10 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20915#comment:11>
Django
.sessions?)
Reporter: ramiro | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
Replying to [comment:11 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20915#comment:12>
Django
.sessions?)
Reporter: Ramiro Morales | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Ülgen Sarıkavak):
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20915#comment:13>