Re: [Django] #10899: easier manipulation of sessions by test client
Django
---------------------------------------+-----------------------------------
Reporter: tallfred | Owner: nobody
Type: New feature | Status: reopened
Milestone: | Component: Testing framework
Version: SVN | Severity: Normal
Resolution: | Keywords:
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------+-----------------------------------
Changes (by gennad):
* ui_ux: => 0
Comment:
Any progress on this guys?
--
Ticket URL: <https://code.djangoproject.com/ticket/10899#comment:18>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
---------------------------------------+-----------------------------------
Reporter: tallfred | Owner: nobody
Type: New feature | Status: reopened
Milestone: | Component: Testing framework
Version: SVN | Severity: Normal
Resolution: | Keywords:
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------+-----------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/10899#comment:19>
Django
---------------------------------------+-----------------------------------
Reporter: tallfred | Owner: nobody
Type: New feature | Status: reopened
Milestone: | Component: Testing framework
Version: SVN | Severity: Normal
Resolution: | Keywords:
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------+-----------------------------------
* cc: bmihelac@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/10899#comment:20>
Django
Type: New feature | Status: reopened
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by ptone):
* needs_better_patch: 0 => 1
Comment:
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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/10899#comment:21>
Django
-----------------------------------+------------------------------------
Reporter: tallfred | Owner: nobody
Type: New feature | Status: reopened
Component: Testing framework | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
* cc: mlowicki (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/10899#comment:22>
Django
-----------------------------------+------------------------------------
Reporter: tallfred | Owner: nobody
Type: New feature | Status: reopened
Component: Testing framework | Version: SVN
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/10899#comment:23>
Django
-----------------------------------+------------------------------------
Reporter: tallfred | Owner: nobody
Type: New feature | Status: reopened
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
* cc: sergeykolosov (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/10899#comment:24>
Django
-----------------------------------+------------------------------------
Reporter: tallfred | Owner: nobody
Type: New feature | Status: reopened
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
* cc: kitsunde@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/10899#comment:25>
Django
-----------------------------------+------------------------------------
Reporter: tallfred | Owner: nobody
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
Comment (by deni):
What's the progress on this?
--
Ticket URL: <https://code.djangoproject.com/ticket/10899#comment:27>
Django
-----------------------------------+------------------------------------
Type: New feature | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
* owner: nobody => deni
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/10899#comment:28>
Django
Reporter: tallfred | Owner: deni
Type: New feature | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by deni):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
Comment:
Fixed in: https://github.com/django/django/pull/1167
--
Ticket URL: <https://code.djangoproject.com/ticket/10899#comment:29>
Django
Reporter: tallfred | Owner: deni
Type: New feature | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/10899#comment:30>
Django
-----------------------------------+------------------------------------
Reporter: tallfred | Owner: deni
Type: New feature | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/10899#comment:31>
Django
-----------------------------------+------------------------------------
Reporter: tallfred | Owner: deni
Type: New feature | Status: assigned
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
* cc: deni@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/10899#comment:32>
Django
-----------------------------------+------------------------------------
Reporter: tallfred | Owner:
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
Changes (by deni):
* status: assigned => new
* owner: deni =>
--
Ticket URL: <https://code.djangoproject.com/ticket/10899#comment:33>
Django
-----------------------------------+------------------------------------
Reporter: tallfred | Owner:
Type: New feature | Status: new
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
Comment (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
--
Ticket URL: <https://code.djangoproject.com/ticket/10899#comment:34>
Django
-----------------------------------+------------------------------------
Reporter: tallfred | Owner:
Type: New feature | Status: new
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
Comment (by prestontimmons):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/10899#comment:35>
Django
-----------------------------------+------------------------------------
Reporter: tallfred | Owner:
Type: New feature | Status: new
Component: Testing framework | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
Comment (by adamzap):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/10899#comment:36>
Django
-----------------------------------+------------------------------------
Reporter: tallfred | Owner:
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
* status: new => closed
* resolution: => wontfix
--
Ticket URL: <https://code.djangoproject.com/ticket/10899#comment:37>