#34327 closed Bug (wontfix)

Test client session does not work as described when using signed cookie engine

Reported by: Sergei Owned by: nobody
Component: Testing framework Version: dev
Severity: Normal Keywords: session signed_cookies
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The following snippet from the documentation does not work when using django.contrib.sessions.backends.signed_cookies:

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):

self.cookies[settings.SESSION_COOKIE_NAME] = session.session_key

Change History (4)

comment:1 by Sergei, 22 months ago

Also, can I fix this and do a PR?

I think it's possible to make a test client API like this:

# 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).

in reply to:  description ; comment:2 by Mariusz Felisiak, 22 months ago

Replying to Sergei:

The following snippet from 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:

  • docs/topics/testing/tools.txt

    diff --git a/docs/topics/testing/tools.txt b/docs/topics/testing/tools.txt
    index 85652095a8..94d11197ca 100644
    a b access these properties as part of a test condition.  
    688688
    689689    To modify the session and then save it, it must be stored in a variable
    690690    first (because a new ``SessionStore`` is created every time this property
    691     is accessed)::
     691    is accessed). For example, if you use
     692    ``'django.contrib.sessions.backends.db'`` engine (the default):
    692693
    693694        def test_something(self):
    694695            session = self.client.session

What do you think?

in reply to:  2 comment:3 by Sergei, 22 months ago

Replying to 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?

  • docs/topics/testing/tools.txt

    diff --git a/docs/topics/testing/tools.txt b/docs/topics/testing/tools.txt
    index 85652095a8..b3deb2b377 100644
    a b access these properties as part of a test condition.  
    695695            session['somekey'] = 'test'
    696696            session.save()
    697697
     698            # if a particular session store changes session key on modification
     699            # (like signed cookie store does), it's required to update the
     700            # session key in cookies
     701            from django.conf import settings
     702            self.client.cookies[settings.SESSION_COOKIE_NAME] = session.session_key
     703

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).

comment:4 by Carlton Gibson, 22 months ago

Resolution: wontfix
Status: newclosed

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.

Note: See TracTickets for help on using tickets.
Back to Top