easier manipulation of sessions by test client

[Rick Dean]

Creating and modifying sessions is painful for the test client...

class SimpleTest(TestCase):
   def test_stuff(self):

      # ugly and long create of session if session doesn't exist
      from django.conf import settings
      engine = import_module(settings.SESSION_ENGINE)
      store = engine.SessionStore()
      store.save()  # we need to make load() work, or the cookie is worthless
      self.cookies[settings.SESSION_COOKIE_NAME] = store.session_key

      #   ugly and long set of session
      session = self.client.session 
      session['foo'] = 33
      session.save()

      # pretty and faster
      self.client.session['foo'] = 33

      #   ugly and long pop of session
      session = self.client.session 
      val = session.pop('foo')
      session.save()

      # pretty and faster
      val = self.client.session.pop('foo')

The attached patch makes the "pretty and faster" possible.
It's faster because every session get doesn't have to go
to the database. The pretty code fails before the patch
because each fetch of self.client.session creates a new
SessionStore with a small scope, not lasting long enough
to be saved.

[Rick Dean]

Here is an updated patch...

1. Only save sessions when actually modified
2. Only session.save() when django.contrib.sessions is used.
3. Survive calls of cycle_key()
4. Simplify login()
5. Rename new _session_cache to _session_store to avoid confusion
6. Updated comments

[Russell Keith-Magee]

(In [10686]) Fixed #10899 -- Ensured that log messages for deletions in the admin contain useful descriptions. Thanks to Jeremy Dunck for the patch.

[Russell Keith-Magee]

(In [10720]) [1.0.X] Fixed #10899 -- Ensured that log messages for deletions in the admin contain useful descriptions. Thanks to Jeremy Dunck for the patch.

Merge of r10686 from trunk.

[Ludvig Ericson]

Seems like you got the wrong ticket number, Russell.

As an aside, here's how I access the session in a very session backend agnostic way:

from django.test import TestCase
from django.test.client import ClientHandler

class SessionHandler(ClientHandler):
    def get_response(self, request):
        response = super(SessionHandler, self).get_response(request)
        response.session = request.session.copy()
        return response

class SessionTestCase(TestCase):
    def _pre_setup(self):
        super(SessionTestCase, self)._pre_setup()
        self.client.handler = SessionHandler()

class FooTestCase(SessionTestCase):
    def test_session_exists(self):
        resp = self.client.get("/")
        self.assert_(hasattr(resp, "session"))
        self.assert_("my_session_key" in resp.session)
        self.assertEqual(resp.session["my_session_key"], "Hello world!")

As you can see, it'd be very easy for the original ClientHandler class to just copy over the session to the response object.

[Chris Beaven]

Good job - I scratched my head for a bit figuring out why my session tests were failing (due to my implementation of the obvious "pretty and faster" code).

Hint: you don't need the _failure method for property - just don't provide a set or del method for the property and Python will raise an attribute error if someone tries to.

I'd leave out your ClientHandler changes from this ticket - open that in a new ticket if you want, but no point in making this one any more complex.

[Preston Timmons]

Smiley Chris,

I attached a new patch that applies cleanly to trunk as of [14099]. It includes tests and passes the Django test suite. It doesn't have docs yet. That's because when I went to write them I noticed that this functionality is already provided in a fairly straightforward manner, with a caveat:

See ​http://docs.djangoproject.com/en/dev/topics/testing/#django.test.client.Client.session.

The caveat is that the docs don't explain that the session has to already be created, by calling a view or another manner, else it returns a dictionary object rather than a SessionStore. That means if you run the provided code sample in a test case you get an attribute error:

AttributeError: 'dict' object has no attribute 'save'

Could you give a decision on what to do with this next?

1. Go with the requested behavior of this patch and add the needed docs. It's a slight improvement over the existing way of setting session variables, although it's less explicit because the save behavior is happening behind the scenes.

2. Keep the documented behavior but add the disclaimer that the session needs to be created by calling a view with the client before editing the session.

3. Open a different ticket for the existing caveat to be fixed and change the test client to always return a session store when 'django.contrib.sessions' is installed by creating one if it doesn't exist.

The changes I made are also on Github: ​http://github.com/prestontimmons/django-10899

Thanks.

Preston

[Preston Timmons]

Patch with tests

[Russell Keith-Magee]

I think the patch is on the right track. The currently documented behavior needs to continue to work for backwards compatibility purposes, but I don't see the proposed behavior affecting that.

I have two comments on the patch:

Firstly, can the call to _session_save() be pushed further down the stack into request()? That would removes some code duplication, and I can't see any obvious reason why it wouldn't work the same (feel free to prove me wrong!)

Secondly, there is some loss of cookie properties from the login method. This needs to be checked to make sure there aren't any backwards incompatible changes to the way the login cookie is being set during testing.

Regarding docs -- you're correct that this new behavior is less explicit, so explaining exactly how and when the session is persisted will be an important thing to explain well.

[Michael Radziej]

The patch does not apply to current trunk - do you have an up to date patch?

[Preston Timmons]

Updated patch against r16086

[Preston Timmons]

Attached is an updated patch against r16086. I'd appreciate if somebody could review this.

My questions are:

1) Are the doc's I added enough? What needs to be said on this? Should the old example be removed?

2) Since the session is being cached on the Client, problems arise with functions that change the session key. This patch makes sure the session key is updated both when a view is called and when the login/logout Client methods are called. Are there any other functions that change the session key I need to be aware of?

Thanks.

[Michael Radziej]

Thanks for the updated patch!

With the patch, I ran the test suite (using the supplied test_sqlite settings). There are a few tests that check the number of queries, so a failure is no surprise. But the strange thing is: I get 9 queries instead of 1. Why so many? And how do we want to deal with the problem that the patch might break users' test suite? I have no idea what the policy is, but IMHO it should be expected that the number of SQL queries can change with a new release.

But in any case, your patch needs to modify the affected tests in the Django test suite.

Here's the test run:

> ./tests/runtests.py --settings=test_sqlite test_utils
Creating test database for alias 'default'...
Creating test database for alias 'other'...
..F.F...
======================================================================
FAIL: test_with_client (regressiontests.test_utils.tests.AssertNumQueriesContextManagerTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/mir/kunden/django/workspace/django/tests/regressiontests/test_utils/tests.py", line 80, in test_with_client
    self.client.get("/test_utils/get_person/%s/" % person.pk)
  File "/home/mir/kunden/django/workspace/links/django/test/testcases.py", line 234, in __exit__
    executed, self.num
AssertionError: 9 queries executed, 1 expected

======================================================================
FAIL: test_assert_num_queries_with_client (regressiontests.test_utils.tests.AssertNumQueriesTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/mir/kunden/django/workspace/django/tests/regressiontests/test_utils/tests.py", line 38, in test_assert_num_queries_with_client
    "/test_utils/get_person/%s/" % person.pk
  File "/home/mir/kunden/django/workspace/links/django/test/testcases.py", line 537, in assertNumQueries
    func(*args, **kwargs)
  File "/home/mir/kunden/django/workspace/links/django/test/testcases.py", line 234, in __exit__
    executed, self.num
AssertionError: 9 queries executed, 1 expected

----------------------------------------------------------------------
Ran 8 tests in 0.025s

FAILED (failures=2)

I have another rather formal suggestion regarding your patch: Your patch changes the number of blank lines between functions in a few places. It shouldn't, first because 2 blank lines are standard in all Django files, second you should not touch code places that are not really related to the ticket, this can get a real hassle when merging in other changes. Would you like to clean this up by yourself?

Re documentation: It is a bit short but might be sufficient. I don't like the wording, but I'm no native speaker and I guess the final committer will polish it up anyway. But ... I think the preceding paragraph does not apply after your patch, does it?

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

        def test_something(self):
            session = self.client.session
            session['somekey'] = 'test'
            session.save()

[Preston Timmons]

Updated ticket to not produce extra queries

[Preston Timmons]

Whoops. Let's try this again.

[Preston Timmons]

Thanks for reviewing that mirs.

[Michael Radziej]

The test suits passes now and it looks fine, but I need a bit more time.

[gennad]

Any progress on this guys?

[Preston Timmons]

Updated patch

[Preston Timmons]

I added an updated patch against r16527. It needs a second set of eyes to review it and verify it covers all cases.

[Preston Holmes]

Tests failing:

FAIL: test_group_permission_performance (regressiontests.admin_views.tests.GroupAdminTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/admin/Projects/django/django-git/tests/regressiontests/admin_views/tests.py", line 2983, in test_group_permission_performance
    self.assertEqual(response.status_code, 200)
  File "/Users/admin/Projects/django/django-git/django/test/testcases.py", line 246, in __exit__
    executed, self.num
AssertionError: 8 != 6 : 8 queries executed, 6 expected

======================================================================
FAIL: test_user_permission_performance (regressiontests.admin_views.tests.UserAdminTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/admin/Projects/django/django-git/tests/regressiontests/admin_views/tests.py", line 2952, in test_user_permission_performance
    self.assertEqual(response.status_code, 200)
  File "/Users/admin/Projects/django/django-git/django/test/testcases.py", line 246, in __exit__
    executed, self.num
AssertionError: 9 != 7 : 9 queries executed, 7 expected

----------------------------------------------------------------------
Ran 4228 tests in 340.354s

FAILED (failures=2, skipped=69, expected failures=3)

[Mark Lavin]

I was looking into these test failures in the recent NC sprint. From what I could tell the two additional queries are related to a session read and write now that the test sessions work more like the real sessions. Starting a new project, creating a superuser and viewing the superuser in the admin shows 9 queries using django-debug-toolbar rather than 7 asserted in the test case.

[Deni Bertovic]

What's the progress on this?

[Deni Bertovic]

Fixed in: ​https://github.com/django/django/pull/1167

[Aymeric Augustin]

While reviewing the patch together I made a few changes, see attachement.

I don't understand why the block added at line 415 is necessary; the session middleware should take care of saving the session.

If this piece of code is actually useful, could you explain why? Thank you!

[Deni Bertovic]

There's been some work on some parts of the code that overlap with this issue.
I'm not really sure anymore as to how to resolve this so it would be nice if someone with more insight
would take a look.

Also I'm not sure if the patch is the best solution because of the part on line 415, as Aymeric mentioned.
It is questionable in the sense that it's there to enable *direct session manipulation* (as outlined in the tests in the pull request)
but the part that checks if the session was modified, and therefore saves it, is the middleware. The test client
goes through the middleware and there should be a way to modify it to enable direct session manipulation.

[anonymous]

Hi guys,
This makes unit testing of views impossible when they use session variables, unless creating a view that insert data into the session but this is a ugly and dangerous hack.
Are you planning to fix this issue or is it considered as a no fix like : https://code.djangoproject.com/ticket/11475

[Preston Timmons]

In light of #21357, I think this ticket should be closed as wontfix.

1. It's no longer necessary to manually import and instantiate the session engine.

2. The proposed patches here modify the test client to automatically call save on the session object if it's modified. That's better handled explicitly or by the session middleware.

[Adam Zapletal]

As prestontimmons said above, this seems much easier now since the engine does not need to be instantiated. Also, it is documented that the session must be stored in a variable: ​https://docs.djangoproject.com/en/1.10/topics/testing/tools/#django.test.Client.session

So maybe this ticket can be closed?