Opened 7 years ago

Closed 6 weeks ago

#10899 closed New feature (wontfix)

easier manipulation of sessions by test client

Reported by: Rick Dean Owned by:
Component: Testing framework Version: master
Severity: Normal Keywords:
Cc: magma.chambers@…, Preston Timmons, bmihelac@…, Michał Łowicki, Sergey Kolosov, 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 Rick Dean 7 years ago.
easy_session_manipulation6.diff (5.5 KB) - added by Rick Dean 7 years ago.
easy_session_manipulation_against_14099_with_tests.diff (7.1 KB) - added by Preston Timmons 6 years ago.
Patch with tests
10899_at_r16086.diff (6.0 KB) - added by Preston Timmons 5 years ago.
Updated patch against r16086
10899_at_r16097.diff (6.5 KB) - added by Preston Timmons 5 years ago.
Updated ticket to not produce extra queries
10899.r16527.diff (6.6 KB) - added by Preston Timmons 5 years ago.
Updated patch
10899-proposal.patch (7.8 KB) - added by Aymeric Augustin 3 years ago.

Download all attachments as: .zip

Change History (44)

Changed 7 years ago by Rick Dean

comment:1 Changed 7 years ago by Rick Dean

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 7 years ago by Rick Dean

comment:2 Changed 7 years ago by Russell Keith-Magee

Resolution: fixed
Status: newclosed

(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 7 years ago by 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.

comment:4 Changed 7 years ago by Ludvig Ericson

Needs documentation: set
Needs tests: set
Patch needs improvement: set
Resolution: fixed
Status: closedreopened

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 7 years ago by 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.

comment:6 Changed 7 years ago by Chris Beaven

Triage Stage: UnreviewedAccepted

comment:7 Changed 6 years ago by Chris Chambers

Cc: magma.chambers@… added

comment:8 Changed 6 years ago by Preston Timmons

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 6 years ago by Preston Timmons

Patch with tests

comment:9 Changed 6 years ago by 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.

comment:10 Changed 5 years ago by Chris Beaven

Severity: Normal
Type: New feature

comment:11 Changed 5 years ago by Michael Radziej

Easy pickings: unset
Patch needs improvement: set

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

Changed 5 years ago by Preston Timmons

Attachment: 10899_at_r16086.diff added

Updated patch against r16086

comment:12 Changed 5 years ago by Preston Timmons

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 5 years ago by Preston Timmons

Cc: Preston Timmons added

comment:14 Changed 5 years ago by Michael Radziej

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 5 years ago by Preston Timmons

Attachment: 10899_at_r16097.diff added

Updated ticket to not produce extra queries

comment:15 Changed 5 years ago by Preston Timmons

Patch needs improvement: unset

Whoops. Let's try this again.

comment:16 Changed 5 years ago by Preston Timmons

Thanks for reviewing that mirs.

comment:17 Changed 5 years ago by Michael Radziej

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

comment:18 Changed 5 years ago by gennad

UI/UX: unset

Any progress on this guys?

Changed 5 years ago by Preston Timmons

Attachment: 10899.r16527.diff added

Updated patch

comment:19 Changed 5 years ago by 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.

comment:20 Changed 5 years ago by bmihelac

Cc: bmihelac@… added

comment:21 Changed 5 years ago by Preston Holmes

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 5 years ago by Michał Łowicki

Cc: Michał Łowicki added

comment:23 Changed 5 years ago by 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.

comment:24 Changed 4 years ago by Sergey Kolosov

Cc: Sergey Kolosov added

comment:25 Changed 4 years ago by Kit Sunde

Cc: kitsunde@… added

comment:26 Changed 4 years ago by Aymeric Augustin

Status: reopenednew

comment:27 Changed 3 years ago by Deni Bertovic

What's the progress on this?

comment:28 Changed 3 years ago by Deni Bertovic

Owner: changed from nobody to Deni Bertovic
Status: newassigned

comment:29 Changed 3 years ago by Deni Bertovic

Patch needs improvement: unset
Triage Stage: AcceptedReady 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 3 years ago by Deni Bertovic (previous) (diff)

Changed 3 years ago by Aymeric Augustin

Attachment: 10899-proposal.patch added

comment:30 Changed 3 years ago by Aymeric Augustin

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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 3 years ago by 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.

comment:32 Changed 3 years ago by Deni Bertovic

Cc: deni@… added

comment:33 Changed 3 years ago by Deni Bertovic

Owner: Deni Bertovic deleted
Status: assignednew

comment:34 Changed 3 years 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

comment:35 Changed 2 years ago by 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.
  1. 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.

comment:36 Changed 6 weeks ago by 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?

comment:37 Changed 6 weeks ago by Tim Graham

Resolution: wontfix
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top