Opened 11 years ago

Closed 10 years ago

Last modified 9 years ago

#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 by Sylvain Fankhauser, 11 years ago

Owner: changed from nobody to Sylvain Fankhauser
Status: newassigned

comment:2 by Sylvain Fankhauser, 11 years ago

Looks like it's not as easy as it seems: the usage of override_settings in some tests is problematic (see #21263).

comment:3 by Anssi Kääriäinen, 11 years ago

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 by Bouke Haarsma, 11 years ago

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 by Bouke Haarsma, 11 years ago

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 by Sylvain Fankhauser, 11 years ago

Owner: Sylvain Fankhauser removed
Status: assignednew

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 by Bouke Haarsma, 11 years ago

Ah yes, I probably didn't pay attention, sorry!

Please see my updated PR, containing 75% reduction in direct settings manipulation.

comment:8 by Bouke Haarsma, 11 years ago

Owner: set to Bouke Haarsma
Status: newassigned

comment:9 by Tim Graham <timograham@…>, 11 years ago

In 3565efaa451db6eb735a085ea6aae3fe86e6d283:

Removed some direct settings manipulations in tests; refs #21230.

comment:10 by Bouke Haarsma, 11 years ago

Easy pickings: unset
Owner: Bouke Haarsma removed
Status: assignednew

There are still some direct settings manipulations left, but require a bit more puzzling.

comment:11 by Unai Zalakain, 11 years ago

Owner: set to Unai Zalakain
Status: newassigned

comment:12 by Unai Zalakain, 11 years ago

Owner: Unai Zalakain removed
Status: assignednew

comment:13 by Tim Graham <timograham@…>, 11 years ago

In 2688462f914960b568dce4cb529db77e982a5284:

Refs #21230 -- removed direct settings manipulation from template tests

comment:14 by ANUBHAV JOSHI, 10 years ago

Resolution: fixed
Status: newclosed

comment:15 by Marc Tamlyn, 10 years ago

Resolution: fixed
Status: closednew

By my reckoning, there is still direct manipulation in signed_cookies_tests/tests.py and staticfiles_tests/tests.py

comment:16 by Huu Nguyen, 10 years ago

Owner: set to Huu Nguyen
Status: newassigned

comment:17 by Huu Nguyen, 10 years ago

Owner: Huu Nguyen removed
Status: assignednew

PR sent: https://github.com/django/django/pull/2525
Addresses direct manipulation of settings in staticfiles_tests/tests.py

Last edited 10 years ago by Huu Nguyen (previous) (diff)

comment:18 by Huu Nguyen, 10 years ago

Owner: set to Huu Nguyen
Status: newassigned

comment:19 by Huu Nguyen, 10 years ago

Owner: Huu Nguyen removed
Status: assignednew

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:20 by Tim Graham <timograham@…>, 10 years ago

In b9bfcd82f0995b3bdd97a1c2cffa1cdd47ebc3c4:

Refs #21230 -- removed direct settings manipulation from signed cookies tests

comment:21 by Tim Graham <timograham@…>, 10 years ago

In f22177a9a3bf9d47695b69b1de06445f93f9d809:

[1.7.x] Refs #21230 -- removed direct settings manipulation from signed cookies tests

Backport of b9bfcd82f0 from master

comment:22 by Huu Nguyen, 10 years ago

Owner: set to Huu Nguyen
Status: newassigned

comment:23 by Huu Nguyen, 10 years ago

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:24 by Tim Graham <timograham@…>, 10 years ago

In 949ee521fab106b44218c30577eb55f0097d39cd:

Refs #21230 -- removed direct settings manipulation from staticfile tests

comment:25 by Tim Graham, 10 years ago

Resolution: fixed
Status: assignedclosed

comment:26 by Tim Graham <timograham@…>, 9 years ago

In b9feec959b9dfc08513607c9046fb38c5b8f7e8a:

Fixed #23700 -- Fixed non-deterministic static files test failures on Windows.

This partially reverts commit 949ee521fab106b44218c30577eb55f0097d39cd
refs #21230.

comment:27 by Tim Graham <timograham@…>, 9 years ago

In 4444ff39a4d8c144f70fcdce08c6ca0abab67bb9:

Removed direct manipulation of settings in auth tests; refs #21230.

Note: See TracTickets for help on using tickets.
Back to Top