Opened 3 years ago

Closed 3 years ago

Last modified 22 months 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: master
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 3 years ago by Sephi

Owner: changed from nobody to Sephi
Status: newassigned

comment:2 Changed 3 years ago by Sephi

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

comment:3 Changed 3 years ago by Anssi Kääriäinen

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

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

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

Owner: Sephi deleted
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 Changed 3 years ago by Bouke Haarsma

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

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

comment:8 Changed 3 years ago by Bouke Haarsma

Owner: set to Bouke Haarsma
Status: newassigned

comment:9 Changed 3 years ago by Tim Graham <timograham@…>

In 3565efaa451db6eb735a085ea6aae3fe86e6d283:

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

comment:10 Changed 3 years ago by Bouke Haarsma

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

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

comment:11 Changed 3 years ago by Unai Zalakain

Owner: set to Unai Zalakain
Status: newassigned

comment:12 Changed 3 years ago by Unai Zalakain

Owner: Unai Zalakain deleted
Status: assignednew

comment:13 Changed 3 years ago by Tim Graham <timograham@…>

In 2688462f914960b568dce4cb529db77e982a5284:

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

comment:14 Changed 3 years ago by ANUBHAV JOSHI

Resolution: fixed
Status: newclosed

comment:15 Changed 3 years ago by Marc Tamlyn

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 Changed 3 years ago by Huu Nguyen

Owner: set to Huu Nguyen
Status: newassigned

comment:17 Changed 3 years ago by Huu Nguyen

Owner: Huu Nguyen deleted
Status: assignednew
Version 0, edited 3 years ago by Huu Nguyen (next)

comment:18 Changed 3 years ago by Huu Nguyen

Owner: set to Huu Nguyen
Status: newassigned

comment:19 Changed 3 years ago by Huu Nguyen

Owner: Huu Nguyen deleted
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 Changed 3 years ago by Tim Graham <timograham@…>

In b9bfcd82f0995b3bdd97a1c2cffa1cdd47ebc3c4:

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

comment:21 Changed 3 years ago by Tim Graham <timograham@…>

In f22177a9a3bf9d47695b69b1de06445f93f9d809:

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

Backport of b9bfcd82f0 from master

comment:22 Changed 3 years ago by Huu Nguyen

Owner: set to Huu Nguyen
Status: newassigned

comment:23 Changed 3 years ago by Huu Nguyen

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 Changed 3 years ago by Tim Graham <timograham@…>

In 949ee521fab106b44218c30577eb55f0097d39cd:

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

comment:25 Changed 3 years ago by Tim Graham

Resolution: fixed
Status: assignedclosed

comment:26 Changed 2 years ago by Tim Graham <timograham@…>

In b9feec959b9dfc08513607c9046fb38c5b8f7e8a:

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

This partially reverts commit 949ee521fab106b44218c30577eb55f0097d39cd
refs #21230.

comment:27 Changed 22 months ago by Tim Graham <timograham@…>

In 4444ff39a4d8c144f70fcdce08c6ca0abab67bb9:

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

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