[Django] #34327: Test client session does not work as described when using signed cookie engine
Django
engine
-------------------------------------+-------------------------------------
Reporter: Sergei | Owner: nobody
Type: Bug | Status: new
Component: Testing | Version: dev
framework | Keywords: session
Severity: Normal | signed_cookies
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
The following snippet from
[https://docs.djangoproject.com/en/dev/topics/testing/tools/#django.test.Client.session
the documentation] does not work when using
`django.contrib.sessions.backends.signed_cookies`:
{{{
#!python
def test_something(self):
session = self.client.session
session['somekey'] = 'test'
session.save()
}}}
`session.save()` is not enough in this case: session key is session data
itself and test client cookies need to be updated.
Like `session` getter does when creating session store object
(`django/test/client.py:732`):
{{{
#!python
self.cookies[settings.SESSION_COOKIE_NAME] = session.session_key
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34327>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
engine
-------------------------------------+-------------------------------------
Reporter: Sergei | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: session | Triage Stage:
signed_cookies | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sergei):
Also, can I fix this and do a PR?
I think it's possible to make a test client API like this:
{{{
#!python
# introduce setter for session
# short-circuit if passing SessionBase type
client.session = session_store
# if not SessionBase, then assume it's a map type -> .items()
# completely replace session data
client.session = {'foo': 'bar'}
# update
session = client.session
session.update({'foo': 'bar'})
client.session = session
}}}
Old way from the docs would continue to work (if not signed cookies
store).
--
Ticket URL: <https://code.djangoproject.com/ticket/34327#comment:1>
Django
engine
-------------------------------------+-------------------------------------
Reporter: Sergei | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: session | Triage Stage:
signed_cookies | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [ticket:34327 Sergei]:
> The following snippet from
[https://docs.djangoproject.com/en/dev/topics/testing/tools/#django.test.Client.session
the documentation] does not work when using
`django.contrib.sessions.backends.signed_cookies`:
This is not an universal snippet intended to work with all kind of built-
in and 3-party session engines. It's only an example of how the session
can be modified. We could clarify this with:
{{{#!diff
diff --git a/docs/topics/testing/tools.txt b/docs/topics/testing/tools.txt
index 85652095a8..94d11197ca 100644
--- a/docs/topics/testing/tools.txt
+++ b/docs/topics/testing/tools.txt
@@ -688,7 +688,8 @@ access these properties as part of a test condition.
To modify the session and then save it, it must be stored in a
variable
first (because a new ``SessionStore`` is created every time this
property
- is accessed)::
+ is accessed). For example, if you use
+ ``'django.contrib.sessions.backends.db'`` engine (the default):
def test_something(self):
session = self.client.session
}}}
What do you think?
--
Ticket URL: <https://code.djangoproject.com/ticket/34327#comment:2>
Django
engine
-------------------------------------+-------------------------------------
Reporter: Sergei | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: session | Triage Stage:
signed_cookies | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sergei):
Replying to [comment:2 Mariusz Felisiak]:
> What do you think?
Thanks for the suggestion! I guess it could be solved with documentation.
Then I think there is a need to explain what's going on. Maybe like this?
{{{#!diff
diff --git a/docs/topics/testing/tools.txt b/docs/topics/testing/tools.txt
index 85652095a8..b3deb2b377 100644
--- a/docs/topics/testing/tools.txt
+++ b/docs/topics/testing/tools.txt
@@ -695,6 +695,12 @@ access these properties as part of a test condition.
session['somekey'] = 'test'
session.save()
+ # if a particular session store changes session key on
modification
+ # (like signed cookie store does), it's required to update
the
+ # session key in cookies
+ from django.conf import settings
+ self.client.cookies[settings.SESSION_COOKIE_NAME] =
session.session_key
+
}}}
But I still think it's too much details spilling into a test, could be
handled internally in the `ClientMixin`, independent of a concrete store
implementation (it already does everything actually).
--
Ticket URL: <https://code.djangoproject.com/ticket/34327#comment:3>
Django
engine
-------------------------------------+-------------------------------------
Reporter: Sergei | Owner: nobody
Component: Testing framework | Version: dev
Keywords: session | Triage Stage:
signed_cookies | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => wontfix
Comment:
Hi Sergei.
I'm going to mark this as `wontfix`, as I'm not sure it's worth even the
clarification Mariusz offered. (For me, it clearly reads as assume the
default backend — it's an example...) If you want to make that tweak, I
don't object though.
> ...could be handled internally in the ClientMixin, independent of a
concrete store implementation (it already does everything actually).
I don't see from what's written here exactly what you have in mind. If you
wanted to make a draft PR showing the API changes then we could see if it
looks worthwhile.
(If folks think so, then we can reopen here.)
I hope that makes sense. Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/34327#comment:4>