[Django] #21230: Remove usage of direct settings manipulation in Django's tests
Django
------------------------------------------------+------------------------
Reporter: akaariai | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Severity: Normal | Keywords:
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
------------------------------------------------+------------------------
A number of tests use direct settings manipulation. They should instead
use django.test.utils.override_settings(). Direct manipulation doesn't
send setting_changed signal, and is also error-prone as it is somewhat
easy to miss changing the setting back to its original value.
I am marking this as easy picking. The problematic tests can be found with
`git grep 'settings\.\w* ='` in tests/ directory (another directory is
django/contrib/gis). Pick one file, change instances of
settings.SOME_SETTING = someval to with
override_settings(SOME_SETTING=someval) and reindent as needed. Run tests,
commit, send pull request and repeat for some other file.
--
Ticket URL: <https://code.djangoproject.com/ticket/21230>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: akaariai | Owner: Sephi
Type: Cleanup/optimization | Status: assigned
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Sephi):
* owner: nobody => Sephi
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:1>
Django
Reporter: akaariai | Owner: Sephi
Type: Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Sephi):
Looks like it's not as easy as it seems: the usage of override_settings in
some tests is problematic (see #21263).
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:2>
Django
Reporter: akaariai | Owner: Sephi
Type: Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by akaariai):
You don't have to tackle all of the manual settings changes in one patch.
The problematic cases can be dealt separately. There are also some easy
cases, and getting rid of those would be a good start.
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:3>
Django
Reporter: akaariai | Owner: Sephi
Type: Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by bouke):
The listed `git grep` didn't work for me (Git 1.8.2.2 on OS X), but `git
grep 'settings\.[a-zA-Z_]* ='` provides a starting point.
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:4>
Django
Reporter: akaariai | Owner: Sephi
Type: Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by bouke):
I started off with some changes (mostly low hanging fruits), as for some
cases the manipulations are very concise. See the PR:
https://github.com/django/django/pull/1739
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:5>
Django
Reporter: akaariai | Owner:
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Sephi):
* status: assigned => new
* owner: Sephi =>
Comment:
Please don't forget to check the owner field before starting to work on a
ticket (see https://docs.djangoproject.com/en/1.5/internals/contributing
/writing-code/submitting-patches/#claiming-tickets). I'm unassigning it,
feel free to reassign it to yourself.
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:6>
Django
Reporter: akaariai | Owner:
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by bouke):
Ah yes, I probably didn't pay attention, sorry!
Please see my updated PR, containing 75% reduction in direct settings
manipulation.
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:7>
Django
Reporter: akaariai | Owner: bouke
Type: Cleanup/optimization | Status: assigned
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by bouke):
* status: new => assigned
* owner: => bouke
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:8>
Django
Reporter: akaariai | Owner: bouke
Type: Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"3565efaa451db6eb735a085ea6aae3fe86e6d283"]:
{{{
#!CommitTicketReference repository=""
revision="3565efaa451db6eb735a085ea6aae3fe86e6d283"
Removed some direct settings manipulations in tests; refs #21230.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:9>
Django
Reporter: akaariai | Owner:
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Changes (by bouke):
* status: assigned => new
* owner: bouke =>
* easy: 1 => 0
Comment:
There are still some direct settings manipulations left, but require a bit
more puzzling.
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:10>
Django
Reporter: akaariai | Owner:
Type: | unaizalakain
Cleanup/optimization | Status: assigned
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by unaizalakain):
* status: new => assigned
* owner: => unaizalakain
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:11>
Django
Reporter: akaariai | Owner:
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Changes (by unaizalakain):
* status: assigned => new
* owner: unaizalakain =>
Comment:
PR sent: https://github.com/django/django/pull/2003
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:12>
Django
Reporter: akaariai | Owner:
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"2688462f914960b568dce4cb529db77e982a5284"]:
{{{
#!CommitTicketReference repository=""
revision="2688462f914960b568dce4cb529db77e982a5284"
Refs #21230 -- removed direct settings manipulation from template tests
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:13>
Django
Reporter: akaariai | Owner:
Type: Cleanup/optimization | Status: closed
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
* status: new => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:14>
Django
Reporter: akaariai | Owner:
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
* status: closed => new
* resolution: fixed =>
Comment:
By my reckoning, there is still direct manipulation in
`signed_cookies_tests/tests.py` and `staticfiles_tests/tests.py`
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:15>
Django
Reporter: akaariai | Owner: whoshuu
Type: Cleanup/optimization | Status: assigned
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
* owner: => whoshuu
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:16>
Django
Reporter: akaariai | Owner:
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Changes (by whoshuu):
* status: assigned => new
* owner: whoshuu =>
Comment:
PR sent: [https://github.com/django/django/pull/2525]
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:17>
Django
Reporter: akaariai | Owner: whoshuu
Type: Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Changes (by whoshuu):
* status: new => assigned
* owner: => whoshuu
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:18>
Django
Reporter: akaariai | Owner:
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Changes (by whoshuu):
* status: assigned => new
* owner: whoshuu =>
Comment:
PR sent: [https://github.com/django/django/pull/2530]
Addresses direct manipulation of settings in signed_cookies_tests/tests.py
This might be the last of the direct settings manipulations, so this
ticket may be ready to close
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:19>
Django
Reporter: akaariai | Owner:
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"b9bfcd82f0995b3bdd97a1c2cffa1cdd47ebc3c4"]:
{{{
#!CommitTicketReference repository=""
revision="b9bfcd82f0995b3bdd97a1c2cffa1cdd47ebc3c4"
Refs #21230 -- removed direct settings manipulation from signed cookies
tests
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:20>
Django
Reporter: akaariai | Owner:
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"f22177a9a3bf9d47695b69b1de06445f93f9d809"]:
{{{
#!CommitTicketReference repository=""
revision="f22177a9a3bf9d47695b69b1de06445f93f9d809"
[1.7.x] Refs #21230 -- removed direct settings manipulation from signed
cookies tests
Backport of b9bfcd82f0 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:21>
Django
Reporter: akaariai | Owner: whoshuu
Type: Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Changes (by whoshuu):
* status: new => assigned
* owner: => whoshuu
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:22>
Django
Reporter: akaariai | Owner: whoshuu
Type: Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Comment (by whoshuu):
There is still a pull request that references this ticket
[https://github.com/django/django/pull/2525 here]. The outstanding issue
is whether `staticfiles_tests/project/site_media/static/testfile.txt` is
deleted after running the affected test code. Reviewing the history of
that file, it seems to be vestigial and can safely be removed from source
control. The pull request currently doesn't remove it itself, but it can.
It would be helpful if someone can take a look at this and decide if the
refactor is worth taking this additional step. Otherwise, I can close the
PR and unassign this ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:23>
Django
Reporter: akaariai | Owner: whoshuu
Type: Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"949ee521fab106b44218c30577eb55f0097d39cd"]:
{{{
#!CommitTicketReference repository=""
revision="949ee521fab106b44218c30577eb55f0097d39cd"
Refs #21230 -- removed direct settings manipulation from staticfile tests
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:24>
Django
Reporter: akaariai | Owner: whoshuu
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
* status: assigned => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:25>
Django
--------------------------------------+------------------------------------
Reporter: akaariai | Owner: whoshuu
Type: Cleanup/optimization | Status: closed
Component: Testing framework | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"b9feec959b9dfc08513607c9046fb38c5b8f7e8a"]:
{{{
#!CommitTicketReference repository=""
revision="b9feec959b9dfc08513607c9046fb38c5b8f7e8a"
Fixed #23700 -- Fixed non-deterministic static files test failures on
Windows.
This partially reverts commit 949ee521fab106b44218c30577eb55f0097d39cd
refs #21230.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:26>
Django
--------------------------------------+------------------------------------
Reporter: akaariai | Owner: whoshuu
Type: Cleanup/optimization | Status: closed
Component: Testing framework | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"4444ff39a4d8c144f70fcdce08c6ca0abab67bb9"]:
{{{
#!CommitTicketReference repository=""
revision="4444ff39a4d8c144f70fcdce08c6ca0abab67bb9"
Removed direct manipulation of settings in auth tests; refs #21230.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21230#comment:27>