Opened 21 months ago

Closed 14 months ago

Last modified 5 months ago

#21230 closed Cleanup/optimization (fixed)

Remove usage of direct settings manipulation in Django's tests

Reported by: akaariai Owned by: whoshuu
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 21 months ago by Sephi

  • Owner changed from nobody to Sephi
  • Status changed from new to assigned

comment:2 Changed 21 months 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 21 months ago 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.

comment:4 Changed 21 months ago 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.

comment:5 Changed 21 months ago 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

comment:6 Changed 21 months ago by Sephi

  • Owner Sephi deleted
  • Status changed from assigned to 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 21 months ago by bouke

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

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

comment:8 Changed 21 months ago by bouke

  • Owner set to bouke
  • Status changed from new to assigned

comment:9 Changed 21 months ago by Tim Graham <timograham@…>

In 3565efaa451db6eb735a085ea6aae3fe86e6d283:

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

comment:10 Changed 20 months ago by bouke

  • Easy pickings unset
  • Owner bouke deleted
  • Status changed from assigned to new

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

comment:11 Changed 19 months ago by unaizalakain

  • Owner set to unaizalakain
  • Status changed from new to assigned

comment:12 Changed 19 months ago by unaizalakain

  • Owner unaizalakain deleted
  • Status changed from assigned to new

comment:13 Changed 19 months ago by Tim Graham <timograham@…>

In 2688462f914960b568dce4cb529db77e982a5284:

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

comment:14 Changed 16 months ago by anubhav9042

  • Resolution set to fixed
  • Status changed from new to closed

comment:15 Changed 16 months ago by mjtamlyn

  • Resolution fixed deleted
  • Status changed from closed to new

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

comment:16 Changed 15 months ago by whoshuu

  • Owner set to whoshuu
  • Status changed from new to assigned

comment:17 Changed 15 months ago by whoshuu

  • Owner whoshuu deleted
  • Status changed from assigned to new

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

Last edited 15 months ago by whoshuu (previous) (diff)

comment:18 Changed 15 months ago by whoshuu

  • Owner set to whoshuu
  • Status changed from new to assigned

comment:19 Changed 15 months ago by whoshuu

  • Owner whoshuu deleted
  • Status changed from assigned to 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:20 Changed 15 months ago by Tim Graham <timograham@…>

In b9bfcd82f0995b3bdd97a1c2cffa1cdd47ebc3c4:

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

comment:21 Changed 15 months 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 15 months ago by whoshuu

  • Owner set to whoshuu
  • Status changed from new to assigned

comment:23 Changed 14 months ago by whoshuu

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 14 months ago by Tim Graham <timograham@…>

In 949ee521fab106b44218c30577eb55f0097d39cd:

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

comment:25 Changed 14 months ago by timo

  • Resolution set to fixed
  • Status changed from assigned to closed

comment:26 Changed 6 months 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 5 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