Opened 13 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)

15805.assertFieldOutput.diff (5.1 KB ) - added by Julien Phalip 13 years ago.
15805.assertFieldOutput.2.diff (7.1 KB ) - added by Julien Phalip 13 years ago.
Same patch with regression tests for assertFieldOutput itself
15805.assertFieldOutput.3.diff (8.0 KB ) - added by Julien Phalip 13 years ago.
Updated patch to also fix broken tests for some recently added localflavors (HR and AU)
15805.assertFieldOutput.4.diff (8.5 KB ) - added by Julien Phalip 13 years ago.
Had forgotten to add AssertFieldOutputTests to the list of tests to run
15805.assertFieldOutput.5.diff (8.6 KB ) - added by Julien Phalip 13 years ago.
Patch updated to latest trunk

Download all attachments as: .zip

Change History (12)

by Julien Phalip, 13 years ago

comment:1 by Julien Phalip, 13 years ago

Needs tests: set

This actually needs some regression tests for assertFieldOutput itself. I'm working on it.

by Julien Phalip, 13 years ago

Same patch with regression tests for assertFieldOutput itself

comment:2 by Julien Phalip, 13 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 Kenneth Gonsalves, 13 years ago

Cc: lawgon@… added

comment:4 by Łukasz Rekucki, 13 years ago

Easy pickings: unset
Triage Stage: UnreviewedAccepted

comment:5 by Julien Phalip, 13 years ago

For the record, I've created #15838 to suggest moving assertFieldOutput to the general TestCase.

by Julien Phalip, 13 years ago

Updated patch to also fix broken tests for some recently added localflavors (HR and AU)

by Julien Phalip, 13 years ago

Had forgotten to add AssertFieldOutputTests to the list of tests to run

comment:6 by Claude Paroz, 13 years ago

Triage Stage: AcceptedReady for checkin
Version: 1.2SVN

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.

by Julien Phalip, 13 years ago

Patch updated to latest trunk

comment:7 by Luke Plant, 13 years ago

Resolution: fixed
Status: newclosed

In [16303]:

Fixed #15805 - assertFieldOutput should not use assertRaisesRegexp

Thanks to julien for the report and patch.

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