Opened 10 years ago

Closed 10 years ago

Last modified 7 years ago

#7611 closed (wontfix)

auth app PasswordResetTest requires admin app to be installed

Reported by: Andrew Badr Owned by: Jason Yan
Component: Testing framework Version: master
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


The tests added in fail when the admin app is not installed. Tests for one app should not depend on the presence of another. The error is: TemplateDoesNotExist: registration/password_reset_form.html

Attachments (5)

test_template_loader_r7958.diff (3.3 KB) - added by Jason Yan 10 years ago.
test_template_loader_r7967.diff (3.3 KB) - added by Jason Yan 10 years ago.
test_template_loader_r7967-2.diff (3.3 KB) - added by Jason Yan 10 years ago.
Previous patch missed a template change.
7611.diff (5.6 KB) - added by Russell Keith-Magee 10 years ago.
Slightly different approach to that originally described.
7611_r7977.diff (2.0 KB) - added by Jason Yan 10 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 10 years ago by Jeff Anderson

Component: UncategorizedUnit test system
milestone: 1.0
Triage Stage: UnreviewedAccepted

comment:2 Changed 10 years ago by Russell Keith-Magee

The problem isn't that admin is required - it's that the test requires templates that may not be available. Admin just happens to provide those templates.

I suspect what is needed here is a 'templates' attribute on TestCase, similar to the newly added 'urls' attribute:

class MyTestCase(TestCase):
    fixtures = ['testdata.json']
    urls = 'test.urls'
    templates = {
        'registration/password_reset_form.html': ' ...template content here...'
    def test_stuff(self):
        # do test here

The TestCase would then install a simple template loader to extract the templates from the text.

comment:3 Changed 10 years ago by Jason Yan

Owner: changed from nobody to Jason Yan
Status: newassigned

Changed 10 years ago by Jason Yan

comment:4 Changed 10 years ago by Jason Yan

Has patch: set

Attached patch adds a test template loader (resides in the test package right now, I wasn't sure if it would be useful enough to live in template/loaders). This currently uses the TEMPLATE_DIRS setting to pass the dictionary to the loader.

Changed 10 years ago by Jason Yan

comment:5 Changed 10 years ago by Jason Yan

Updated patch for post-nfa merge to trunk.

comment:6 Changed 10 years ago by Simon Greenhill

#7756 was a duplicate.

Changed 10 years ago by Jason Yan

Previous patch missed a template change.

Changed 10 years ago by Russell Keith-Magee

Attachment: 7611.diff added

Slightly different approach to that originally described.

comment:7 Changed 10 years ago by Russell Keith-Magee

Resolution: wontfix
Status: assignedclosed

I've provided a slightly patch updated to post-newforms-admin merge, with a slightly modified approach - the implementation of the template loader convinced me that a dictionary syntax probably isn't the right thing to do here, and munging TEMPLATE_DIRS to be a dictionary could have all sorts of consequences.

However, after working on this patch and thinking about it a bit more, I'm not convinced that this is the right thing to do. I'm now of the opinion that the reported problem is in fact a correct test failure.

In the reported circumstances (auth deployed, but not admin), the auth test is failing because certain templates are not found.

When auth is tested as part of the Django system tests, admin is also deployed, so the templates are found.

The tests would pass for the reporter if the user provided a 'registration/password_reset_form.html' template in their own project.

If the reporter _had_ deployed contrib.admin, but _didn't_ use the app_directories template loader, the tests would have failed, because the ability to load the admin templates is dependent on the use of the app_directories template loader.

Essentially, as currently defined, the auth tests are validating that the auth application will work _in your current project_. If you're not providing a way to load a template called 'registration/password_reset_form.html', then the auth views will fail in your project, and the failure is correctly reported. It doesn't matter if you are providing that template yourself, or if it is loaded from admin, but it must be provided.

This test also serves as a check that when a template that _is_ provided, it is correct - in this case, that it renders the form, and displays the right error message. It's not enough that you have a template - you have to have a meaningful template, too. You don't have to use the admin password_reset_form.html template, but whatever you use, it has to render the form, and display the errors on the email field.

This does raise the slight problem that the 'missing' templates are required, even if you aren't using the auth views in your project. However, I feel that this is probably an edge case that we can live with.

If anyone disagrees with this analysis, feel free to raise it on django-dev, but for the moment, I'm closing wontfix. Apologies to Jason who worked on this ticket after my initial advice. Hopefully you gained some experience with the test and template systems, even if that experience isn't going to be put to use for the moment.

Changed 10 years ago by Jason Yan

Attachment: 7611_r7977.diff added

comment:8 Changed 10 years ago by devin

I've covered this in the patch to #4788.

comment:9 Changed 10 years ago by siddhi

I'm not sure wontfix is the right solution. Many people use the models defined in the auth app without using the views (and hence not providing the templates). The tests will fail in this case. Either the tests have to be deleted or dummy templates provided or the admin app added, none of which is a particularly nice solution.

comment:10 Changed 9 years ago by zbyte64

Seems strange that we allow a testcase to locally override urls on the assumption that a project may have its own set of urls, but not allow a testcase to locally override templates even though a project is likely to have its own set of templates. Doing one and not the other actually raises its own bit of problems (which I have come across). An example would be a login template that has a url tag referencing another app. This url tag will not resolve as the test case has its own set of urls, thus a failure occurs. Either way I am looking forward to this being resolved, I have two test failures lingering due to this bug ;)

comment:11 Changed 7 years ago by Jacob

milestone: 1.0

Milestone 1.0 deleted

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