Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#7611 closed (wontfix)

auth app PasswordResetTest requires admin app to be installed

Reported by: andrewbadr Owned by: jason
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: UI/UX:

Description

The tests added in http://code.djangoproject.com/changeset/7808 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 6 years ago.
test_template_loader_r7967.diff (3.3 KB) - added by jason 6 years ago.
test_template_loader_r7967-2.diff (3.3 KB) - added by jason 6 years ago.
Previous patch missed a template change.
7611.diff (5.6 KB) - added by russellm 6 years ago.
Slightly different approach to that originally described.
7611_r7977.diff (2.0 KB) - added by jason 6 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 6 years ago by programmerq

  • Component changed from Uncategorized to Unit test system
  • milestone set to 1.0
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 6 years ago by russellm

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

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

Changed 6 years ago by jason

comment:4 Changed 6 years ago by jason

  • 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 6 years ago by jason

comment:5 Changed 6 years ago by jason

Updated patch for post-nfa merge to trunk.

comment:6 Changed 6 years ago by Simon Greenhill

#7756 was a duplicate.

Changed 6 years ago by jason

Previous patch missed a template change.

Changed 6 years ago by russellm

Slightly different approach to that originally described.

comment:7 Changed 6 years ago by russellm

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

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

comment:8 Changed 6 years ago by devin

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

comment:9 Changed 5 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 4 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 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.