Opened 14 years ago
Closed 13 years ago
#15805 closed Bug (fixed)
assertFieldOutput should not use assertRaisesRegexp
Reported by: | Julien Phalip | Owned by: | nobody |
---|---|---|---|
Component: | Testing framework | Version: | dev |
Severity: | Release blocker | Keywords: | |
Cc: | lawgon@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
The fact that assertFieldOutput
uses assertRaisesRegexp
causes some weird and inconsistent behaviours, which I describe in more detail in #14608 and in http://groups.google.com/group/django-developers/browse_thread/thread/96da81267aadade0
Not only does it make no sense to use regular expressions here, but also pretty much all the tests in regressiontests.forms.localflavor
using assertFieldOutput
are currently passing without being absolutely sure that the expected error messages are correct (To verify that, try modifying any error message in those tests -- the tests will still pass).
See attached patch fixing the behaviour and also straightening some tests that were incorrectly passing before.
Marking as Release Blocker since, in effect, the test suite is currently broken because of this behaviour.
Attachments (5)
Change History (12)
by , 14 years ago
Attachment: | 15805.assertFieldOutput.diff added |
---|
comment:1 by , 14 years ago
Needs tests: | set |
---|
by , 14 years ago
Attachment: | 15805.assertFieldOutput.2.diff added |
---|
Same patch with regression tests for assertFieldOutput itself
comment:2 by , 14 years ago
Needs tests: | unset |
---|
I've added the regression tests. I think the assertFieldOutput
is actually quite useful. Also it's quite generic and not really local flavor specific. So I'd suggest we eventually move it to the django.utils.unittest.TestCase
class and document it so that the world knows about it. I'll create a new ticket for that if/when this patch gets checked in.
comment:3 by , 14 years ago
Cc: | added |
---|
comment:4 by , 14 years ago
Easy pickings: | unset |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:5 by , 14 years ago
For the record, I've created #15838 to suggest moving assertFieldOutput
to the general TestCase
.
by , 14 years ago
Attachment: | 15805.assertFieldOutput.3.diff added |
---|
Updated patch to also fix broken tests for some recently added localflavors (HR and AU)
by , 14 years ago
Attachment: | 15805.assertFieldOutput.4.diff added |
---|
Had forgotten to add AssertFieldOutputTests to the list of tests to run
comment:6 by , 14 years ago
Triage Stage: | Accepted → Ready for checkin |
---|---|
Version: | 1.2 → SVN |
In the future, it would also be cool if the failing message would expose the exact value which is failing, instead of simply outputting the raw ValidationError. But this is already a nice improvement.
This actually needs some regression tests for
assertFieldOutput
itself. I'm working on it.