#21230 closed Cleanup/optimization (fixed)
Remove usage of direct settings manipulation in Django's tests
Reported by: | Anssi Kääriäinen | Owned by: | Huu Nguyen |
---|---|---|---|
Component: | Testing framework | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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.
Change History (27)
comment:1 Changed 10 years ago by
Owner: | changed from nobody to Sylvain Fankhauser |
---|---|
Status: | new → assigned |
comment:2 Changed 10 years ago by
comment:3 Changed 10 years ago by
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.
comment:4 Changed 10 years ago by
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.
comment:5 Changed 10 years ago by
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
comment:6 Changed 10 years ago by
Owner: | Sylvain Fankhauser deleted |
---|---|
Status: | assigned → new |
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.
comment:7 Changed 10 years ago by
Ah yes, I probably didn't pay attention, sorry!
Please see my updated PR, containing 75% reduction in direct settings manipulation.
comment:8 Changed 10 years ago by
Owner: | set to Bouke Haarsma |
---|---|
Status: | new → assigned |
comment:10 Changed 10 years ago by
Easy pickings: | unset |
---|---|
Owner: | Bouke Haarsma deleted |
Status: | assigned → new |
There are still some direct settings manipulations left, but require a bit more puzzling.
comment:11 Changed 10 years ago by
Owner: | set to Unai Zalakain |
---|---|
Status: | new → assigned |
comment:12 Changed 10 years ago by
Owner: | Unai Zalakain deleted |
---|---|
Status: | assigned → new |
comment:14 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:15 Changed 10 years ago by
Resolution: | fixed |
---|---|
Status: | closed → new |
By my reckoning, there is still direct manipulation in signed_cookies_tests/tests.py
and staticfiles_tests/tests.py
comment:16 Changed 10 years ago by
Owner: | set to Huu Nguyen |
---|---|
Status: | new → assigned |
comment:17 Changed 10 years ago by
Owner: | Huu Nguyen deleted |
---|---|
Status: | assigned → new |
PR sent: https://github.com/django/django/pull/2525
Addresses direct manipulation of settings in staticfiles_tests/tests.py
comment:18 Changed 10 years ago by
Owner: | set to Huu Nguyen |
---|---|
Status: | new → assigned |
comment:19 Changed 10 years ago by
Owner: | Huu Nguyen deleted |
---|---|
Status: | assigned → new |
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
comment:22 Changed 10 years ago by
Owner: | set to Huu Nguyen |
---|---|
Status: | new → assigned |
comment:23 Changed 10 years ago by
There is still a pull request that references this ticket 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.
comment:25 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Looks like it's not as easy as it seems: the usage of override_settings in some tests is problematic (see #21263).