Code

Opened 5 years ago

Last modified 5 months ago

#10899 new New feature

easier manipulation of sessions by test client

Reported by: tallfred Owned by:
Component: Testing framework Version: master
Severity: Normal Keywords:
Cc: magma.chambers@…, prestontimmons, bmihelac@…, mlowicki, sergeykolosov, kitsunde@…, deni@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Attachments (7)

easy_session_manipulation.diff (3.9 KB) - added by tallfred 5 years ago.
easy_session_manipulation6.diff (5.5 KB) - added by tallfred 5 years ago.
easy_session_manipulation_against_14099_with_tests.diff (7.1 KB) - added by prestontimmons 4 years ago.
Patch with tests
10899_at_r16086.diff (6.0 KB) - added by prestontimmons 3 years ago.
Updated patch against r16086
10899_at_r16097.diff (6.5 KB) - added by prestontimmons 3 years ago.
Updated ticket to not produce extra queries
10899.r16527.diff (6.6 KB) - added by prestontimmons 3 years ago.
Updated patch
10899-proposal.patch (7.8 KB) - added by aaugustin 11 months ago.

Download all attachments as: .zip

Change History (41)

Changed 5 years ago by tallfred

comment:1 Changed 5 years ago by tallfred

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

Changed 5 years ago by tallfred

comment:2 Changed 5 years ago by russellm

  • Resolution set to fixed
  • Status changed from new to closed

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

comment:3 Changed 5 years ago by russellm

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

comment:4 Changed 5 years ago by toxik

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

comment:5 Changed 5 years ago by SmileyChris

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.

comment:6 Changed 5 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Accepted

comment:7 Changed 4 years ago by chrischambers

  • Cc magma.chambers@… added

comment:8 Changed 4 years ago by prestontimmons

  • Needs tests unset
  • Patch needs improvement unset

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

Changed 4 years ago by prestontimmons

Patch with tests

comment:9 Changed 3 years ago by russellm

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.

comment:10 Changed 3 years ago by SmileyChris

  • Severity set to Normal
  • Type set to New feature

comment:11 Changed 3 years ago by mir

  • Easy pickings unset
  • Patch needs improvement set

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

Changed 3 years ago by prestontimmons

Updated patch against r16086

comment:12 Changed 3 years ago by prestontimmons

  • Needs documentation unset
  • Patch needs improvement unset

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.

comment:13 Changed 3 years ago by prestontimmons

  • Cc prestontimmons added

comment:14 Changed 3 years ago by mir

  • Patch needs improvement set

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

Changed 3 years ago by prestontimmons

Updated ticket to not produce extra queries

comment:15 Changed 3 years ago by prestontimmons

  • Patch needs improvement unset

Whoops. Let's try this again.

comment:16 Changed 3 years ago by prestontimmons

Thanks for reviewing that mirs.

comment:17 Changed 3 years ago by mir

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

comment:18 Changed 3 years ago by gennad

  • UI/UX unset

Any progress on this guys?

Changed 3 years ago by prestontimmons

Updated patch

comment:19 Changed 3 years ago by prestontimmons

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

comment:20 Changed 3 years ago by bmihelac

  • Cc bmihelac@… added

comment:21 Changed 3 years ago by ptone

  • Patch needs improvement set

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)

comment:22 Changed 2 years ago by mlowicki

  • Cc mlowicki added

comment:23 Changed 2 years ago by mlavin

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.

comment:24 Changed 18 months ago by sergeykolosov

  • Cc sergeykolosov added

comment:25 Changed 18 months ago by kitsunde

  • Cc kitsunde@… added

comment:26 Changed 13 months ago by aaugustin

  • Status changed from reopened to new

comment:27 Changed 11 months ago by deni

What's the progress on this?

comment:28 Changed 11 months ago by deni

  • Owner changed from nobody to deni
  • Status changed from new to assigned

comment:29 Changed 11 months ago by deni

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

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

Thanks prestontimmons for the patch. The 2 extra queries in the failing tests are from the read and
write of the session object to the database.

Last edited 11 months ago by deni (previous) (diff)

Changed 11 months ago by aaugustin

comment:30 Changed 11 months ago by aaugustin

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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!

comment:31 Changed 10 months ago by deni

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.

comment:32 Changed 10 months ago by deni

  • Cc deni@… added

comment:33 Changed 10 months ago by deni

  • Owner deni deleted
  • Status changed from assigned to new

comment:34 Changed 5 months ago by 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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from (none) to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.