Code

Opened 3 years ago

Closed 5 weeks ago

#16507 closed Bug (fixed)

Tests that alter ROOT_URLCONF should set DEBUG_PROPAGATE_EXCEPTION

Reported by: andymckay Owned by: nobody
Component: contrib.messages Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Contrib message unit tests check that certain requests pass or fail. They do this by setting up custom templates and overriding key parts such as ROOT_URLCONF. The tests except the templates to fail in a certain way.

However you might have a custom 500 page defined in your project. If you do then the request will try to render that 500 page. If that 500 page then depends upon urls defined in your site, they will fail.

An example of this hitting someone else: http://recursive-design.com/blog/2009/11/10/noreversematch-custom-registration-templates-break-django-unit-tests/

If a test is going to mess with base configuration like ROOT_URLCONF it shouldn't get into trying to render the nice 500 pages and the way to do that is to set DEBUG_PROPAGATE_EXCEPTION. Here's one I found in django.contrib.messages from Arecibo.

Attachments (9)

out.patch (1.0 KB) - added by andymckay 3 years ago.
16507.diff (1.5 KB) - added by aaugustin 3 years ago.
16507.2.diff (4.4 KB) - added by aaugustin 3 years ago.
16507.3.diff (2.0 KB) - added by claudep 3 years ago.
Refreshed patch against trunk
foo.txt (3.0 KB) - added by andymckay 3 years ago.
failure.py (2.5 KB) - added by andymckay 3 years ago.
Hopefully this demonstrates the problem
16507.4.diff (1.3 KB) - added by claudep 3 years ago.
Prevent any custom handler500 or 500.html template
16507.5.diff (2.2 KB) - added by claudep 3 years ago.
Add utility methods at SimpleTestCase level
16507.6.diff (679 bytes) - added by claudep 3 years ago.
Set DEBUG_PROPAGATE_EXCEPTIONS to True for specific test

Download all attachments as: .zip

Change History (29)

Changed 3 years ago by andymckay

comment:1 Changed 3 years ago by aaugustin

  • Component changed from Testing framework to contrib.messages
  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

I'm attaching a patch that uses @override_settings instead of emulating it.

Still, this tests uses three different mechanisms to change or restore the settings during the tests, which is kind of messy.

Changed 3 years ago by aaugustin

comment:2 Changed 3 years ago by aaugustin

I'm attaching a better patch that drastically simplifies the change / restore operations on the settings, and uses @override_settings everywhere.

Changed 3 years ago by aaugustin

comment:3 Changed 3 years ago by andymckay

Agreed, looks like a much better patch.

comment:4 Changed 3 years ago by claudep

#16574 has a similar patch, which uses override_settings. Probably worth of killing two birds with one stone.

Changed 3 years ago by claudep

Refreshed patch against trunk

comment:5 Changed 3 years ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

comment:6 Changed 3 years ago by julien

Thanks for this. The patch makes sense for SESSION_COOKIE_DOMAIN. However, I can't seem to reproduce any failures due to DEBUG_PROPAGATE_EXCEPTIONS being False. I've tried setting DEBUG to False and using a faulty 500.html template, but the contrib.messages test still pass without any problem.

Could you please explain the exact conditions to reproduce these failures?

comment:7 Changed 3 years ago by julien

In [17032]:

Made some contrib.messages tests use override_settings for clarity. Thanks to Claude Paroz for the patch. Refs #16507, #16574.

comment:8 Changed 3 years ago by julien

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

Neither claudep or I could reproduce the test failures originally reported here. They might have been fixed as a side effets of some prior refactoring. If you do think the problem isn't fixed, then please reopen with some clear instructions for reproducing.

comment:9 Changed 3 years ago by andymckay

  • Resolution fixed deleted
  • Status changed from closed to reopened

The error that occurs is a Caught NoReverseMatch while rendering: Reverse for ... with arguments ... and keyword arguments '{}' not found. To reproduce this: create a handler500 pointing to a view which renders a template, in that template do a URL reverse from your application. Because your application contains a URL reversal that is not in URLs (the test case removes them for you) you get an error. I'll attach a test case to demonstrate this and a test. I tested this on SVN head a few minutes ago.

Changed 3 years ago by andymckay

Changed 3 years ago by andymckay

Hopefully this demonstrates the problem

comment:10 Changed 3 years ago by claudep

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

When the error is in the handler itself, like you demonstrated in failure.py, I found no settings able to bypass the error. The only way to prevent the error was to explicitly redefine django.conf.urls.handler500 to the default view (then reset in tearDown). Thoughts?

Changed 3 years ago by claudep

Prevent any custom handler500 or 500.html template

comment:11 Changed 3 years ago by julien

I think the new patch makes good sense. I'm just wondering if this is an issue that any Django app's unit tests would suffer from, not just those in contrib.messages. If so then I'd like to find a robust and general solution.

comment:12 Changed 3 years ago by claudep

I'm relatively hesitant to automatically touch the handlers (or template dirs/loaders) for each test having custom urls. Explicit is better than implicit, isn't it :-) I'll attach a tentative patch to add utility methods at SimpleTestCase level.

Changed 3 years ago by claudep

Add utility methods at SimpleTestCase level

comment:13 Changed 3 years ago by carljm

At first glance, I think fixing this anywhere in the test suite or test utilities is at the wrong level. 404/403/500 handlers are defined in the root URLconf; if the root URLconf changes, the handlers from the new root URLconf should be used, and this should happen automatically in the urls code, not requiring any intervention from test code. IOW, the real problem is caching of state in the url resolver, and if possible I'd prefer to see it fixed there (otherwise similar issues could crop up for people using e.g. request.urlconf, I think).

comment:14 Changed 3 years ago by claudep

Thanks Carl for this precious insight.

Andy, did you imply by your failure.py example that the wrong handler was called (in your example, you redefined the handler in the test itself, which does not really happen in current messages tests), or was it only a mean to show the 500.html template problem?
If the latter is true, simply replacing TEMPLATE_DIRS = () by TEMPLATE_LOADERS = () should fix the issue and the remainder of the latest proposed patch can simply be dropped.

comment:15 follow-up: Changed 3 years ago by andymckay

  • I misspelled DEBUG_PROPAGATE_EXCEPTIONS in my code. If you add the S on the end, you can flip it and the test will flip between pass and fail, sorry about that.
  • The wrong handler is called, I was trying to boil this down to a one file test case. You can move the assignment to handler500 around it doesn't matter. The problem as Carl said is that we don't have a complete isolation in the test, we are altering some parts (the urls) but not others (the handlers). If you need a more comprehensive example I can point you at code in my projects that trigger this.

I would like to take it one step further though and suggest that DEBUG_PROPAGATE_EXCEPTIONS should be set to False for all tests. It can then be turned on when needed. It's only used in one place, core/handlers.py handle_uncaught_exception. That should have its own set of tests and have good coverage, with that flag turned on for those test. Perhaps there might be an integration test or two.

But, everywhere else does not need to be testing how well handle_uncaught_exception works, that is out of scope. In the given example we just need to know that a MessageFailure is raised. Nothing else. But perhaps that's a seperate bug.

comment:16 in reply to: ↑ 15 Changed 3 years ago by claudep

Replying to andymckay:
(...)

  • The wrong handler is called, I was trying to boil this down to a one file test case. You can move the assignment to handler500 around it doesn't matter. The problem as Carl said is that we don't have a complete isolation in the test, we are altering some parts (the urls) but not others (the handlers). If you need a more comprehensive example I can point you at code in my projects that trigger this.

I've separated the issue about wrong handler being called in ticket #17113. (The test I uploaded in this ticket is currently passing)

comment:17 Changed 3 years ago by claudep

  • Patch needs improvement unset

I found that to reproduce this error, just create django/contrib/contenttype/templates/500.html containing {% url foo %} (or in any other installed app). I'm not sure I follow all content of Andy's latest comment. Anyway, I'm uploading a new patch which should fix the 500.html template issue.

Changed 3 years ago by claudep

Set DEBUG_PROPAGATE_EXCEPTIONS to True for specific test

comment:18 Changed 3 years ago by carljm

Based on #17113, I think that my comment above was bogus, just a misunderstanding of what was happening here based on looking at the failing-handler case. Thanks for confirming that, Claude - sorry for the misdirection.

Andy, if you can write a test case demonstrating a situation where the wrong handler is actually used, please upload to #17113 and re-open.

It seems to me that Claude is right that this bug is a simple case where both urlconfs use the default 500 handler and template, but that template tries to reverse a URL that doesn't exist in the temporary URLconf. It's tempting to say we should set DEBUG_PROPAGATE_EXCEPTIONS to True in the base TestCase class whenever a custom URLconf is used (or just set it for all test runs, as Andy suggests), but I think this is overly implicit and non-obvious behavior, and not backwards-compatible; it could cause changes in the behavior of existing tests. (And is there any other precedent where we force the default value of a setting implicitly, just for running tests? Other than DATABASES, I guess).

Really this is not a general issue in the test suite, only in the contrib tests which might be run from someone's project that has a custom 500 template. And if someone triggers it in their own tests with their own custom 500 template, they can deal with that. So in the end I think Claude's latest approach of fixing the specific test is probably the best option?

comment:19 Changed 16 months ago by aaugustin

  • Status changed from reopened to new

comment:20 Changed 5 weeks ago by aaugustin

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

With the new test runner introduced in Django 1.6, tests of contrib apps aren't run anymore when you run your projects' tests.

So this issue shouldn't manifest itself anymore. It was just a particularly insidious example of the lack of isolation of contrib apps tests.

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.