Code

Opened 14 months ago

Last modified 13 months ago

#20349 new Cleanup/optimization

Don't load django.test when not testing

Reported by: CollinAnderson Owned by: nobody
Component: Testing framework Version: 1.5
Severity: Normal Keywords:
Cc: cmawebsite@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I noticed that django/contrib/auth/hashers.py is loading the settings_changed signal from the test suite. This ends up also loading the whole test suite in addition.

All of the other code that handles the settings_changed signal is kept inside the test suite itself. I assume this code is kept out of the test suite because of the "core shouldn't be aware of contrib" rule. Seems to me it's more important to avoid unnecessarily loading the test suite on a production website.

Yes, I realize this is picky, but we're perfectionists, right?

Attachments (0)

Change History (8)

comment:1 Changed 14 months ago by CollinAnderson

  • Cc cmawebsite@… added
  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 14 months ago by CollinAnderson

comment:3 Changed 14 months ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 14 months ago by carljm

This pull request clearly can't work as written, since HASHERS and PREFERRED_HASHER don't exist in django.test.signals, so they can't be globaled there.

We need to decide if settings_changed is meant to be a generally-supported framework feature, or a test-only feature. If the former, then it should be moved out of django.test.signals. If the latter, then fixing this bug is complicated; we'd need a hook in the test runner to allow apps to contribute stuff (like settings_changed signal registrations) to setup_test_environment. Or an UNDER_TEST setting, so that code like this can make the import and signal registration conditional on UNDER_TEST. Or just live with the fact that django.test is imported in production.

comment:5 Changed 14 months ago by carljm

  • Needs tests set
  • Patch needs improvement set

comment:6 Changed 14 months ago by aaugustin

setting_changed is only targeted at tests for the time being.

Maybe we can put this code under django/contrib/auth/tests? If that doesn't work, let's just live with the status quo.

comment:7 Changed 13 months ago by claudep

The problem when moving the signal definition in django/contrib/auth/tests is that many test cases outside of auth tests are overriding the PASSWORD_HASHERS setting (see [8fad77da9597e0dd9fc]), and then the signal is not installed for those tests.

An argument for moving auth to core?

comment:8 Changed 13 months ago by CollinAnderson

  • Summary changed from Don't load test suite when not testing to Don't load django.test when not testing

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.