Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 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: master
Severity: Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

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 6 years ago.
Remove improper setUp/tearDown methods on ChangePasswordTest
8523.prepend_test_templates_to_settings_templates.patch (824 bytes) - added by cammacrae 6 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 6 years ago.
This patch prepends the test template directory to settings.template_dirs ONLY

Download all attachments as: .zip

Change History (12)

Changed 6 years ago by mtrichardson

Remove improper setUp/tearDown methods on ChangePasswordTest

comment:1 Changed 6 years ago by mtrichardson

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 Changed 6 years ago by jacob

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

comment:3 Changed 6 years ago by jacob

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

comment:4 Changed 6 years ago by cammacrae

  • Resolution wontfix deleted
  • Status changed from closed to reopened

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.

Changed 6 years ago by cammacrae

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 Changed 6 years ago by cammacrae

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 Changed 6 years ago by cammacrae

  • Owner changed from nobody to cammacrae
  • Status changed from reopened to new

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

Changed 6 years ago by cammacrae

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

comment:7 Changed 6 years ago by mtredinnick

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

@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 Changed 6 years ago by cammacrae

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

  • milestone 1.0 deleted

Milestone 1.0 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.