Opened 16 years ago

Closed 16 years ago

Last modified 12 years ago

#8523 closed (wontfix)

contrib.auth's ChangePasswordTest tests have invalid setUp/tearDown methods

Reported by: mtrichardson Owned by: cammacrae
Component: contrib.auth Version: dev
Severity: Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This was introduced in #8497 by my patch - we shouldn't be changing the template directory at all on that since current behavior is that the tests should fail if these templates aren't present. However, ChangePasswordTest changes the TEMPLATE_DIRS setting, which we were using to make sure that our tests would run properly - this isn't actually relevant at all.

Attachments (3)

8523.remove_unneeded_setupteardown_in_auth_tests.diff (1.2 KB ) - added by mtrichardson 16 years ago.
Remove improper setUp/tearDown methods on ChangePasswordTest
8523.prepend_test_templates_to_settings_templates.patch (824 bytes ) - added by cammacrae 16 years ago.
This patch prepends the test template directory to settings.template_dirs and as a bonus ensures the login method honors settings.LOGIN_REDIRECT_URL
8523.prepend_test_templates_to_settings_templates2.diff (467 bytes ) - added by cammacrae 16 years ago.
This patch prepends the test template directory to settings.template_dirs ONLY

Download all attachments as: .zip

Change History (12)

by mtrichardson, 16 years ago

Remove improper setUp/tearDown methods on ChangePasswordTest

comment:1 by mtrichardson, 16 years ago

Additionally django/contrib/auth/tests/templates should be removed as well.

comment:2 by Jacob, 16 years ago

The ChangePasswordTest.test_password_change_succeeds test case depends on that template being there, so I'm going to leave it.

comment:3 by Jacob, 16 years ago

Resolution: wontfix
Status: newclosed

comment:4 by cammacrae, 16 years ago

Resolution: wontfix
Status: closedreopened

Thumping the template_dirs means that anyone who uses django.contrib.auth without django.contrib.admin has a broken build - unless they move their templates into an app directory.

by cammacrae, 16 years ago

This patch prepends the test template directory to settings.template_dirs and as a bonus ensures the login method honors settings.LOGIN_REDIRECT_URL

comment:5 by cammacrae, 16 years ago

See also here: http://groups.google.com/group/django-developers/browse_thread/thread/ac888ea8567ba466 for Russell's argument that the tests should fail in the absence of the templates. I actually think that given the test is now using it's own template_dir, the case could be made that contrib.auth tests should not depend on the contrib.admin templates at all.

comment:6 by cammacrae, 16 years ago

Owner: changed from nobody to cammacrae
Status: reopenednew

just noticed that my patch fixes also #8552 which has a patch of its own. will new patch for this issue only to follow.

by cammacrae, 16 years ago

This patch prepends the test template directory to settings.template_dirs ONLY

comment:7 by Malcolm Tredinnick, 16 years ago

Resolution: wontfix
Status: newclosed

@cammacrae: your patches are not what this ticket was about. The issue in this ticket has been resolved as "wontfix". Russell's post on the mailing list overlooked the problem that we need to be able to test things internally and making it impossible to run such tests as a way of catching errors in user-application configuration is certainly not the intention. It's a trade-off.

I'm reclosing this ticket with the original resolution. If you want to make these other changes, please open a new ticket, addressing the specific issue. As soon as you find yourself writing "as a bonus", you know you're trying to do two things at once (one of which, it turns out, is already addressed in another ticket). Sorry to be a curmudgeon on this, but it is very useful for us to be able to have one issue per ticket and not have a ticket side-tracked by something in the same area but not directly related to the original problem report.

Finally, from a quick read of your patch, if you want to go down that path, you'll almost certainly want the user configured template directories to be read before the standard ones (so that they override the standard ones). It doesn't look like that is going to happen.

comment:8 by cammacrae, 16 years ago

Malcolm, you're really splitting hairs to say the patch doesn't address the ticket - contrib.auth's ChangePasswordTest tests do actually have an invalid setUp/tearDown methods. The first patch from mtrichardson is probably correct, but was rejected because the login.html template is required for tests to pass - neatly overlooking the fact that other templates are likewise required, but not included in the test template dir presumably for the reasons outlined in Russell's post. They are included, by seemingly happy accident, in contrib.admin. My patch addresses the broken setup method by ensuring that the tests can still pass in the absence of contrib.admin.

On the issue of the order of template directory resolution I'd agree with you if the templates used in the test were in fact standard templates for contrib.auth, but they're not: they're specific test templates (broken as that is).

As for the bonus, you're right & I knew you were right the second I uploaded the patch.

comment:9 by Jacob, 12 years ago

milestone: 1.0

Milestone 1.0 deleted

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