Something wrong about AssertRaisesRegexp and assertFieldOutput

[Julien Phalip]

Hi,

I've found two similar (and potentially linked) weird problems in the
test framework while trying to understand why the perfectly looking
tests in #14608 were failing. Before I open some tickets I'd like to
hear if anybody has any clue about what's going on or if I'm missing
anything.

1) assertFieldOutput calls assertRaisesRegexp and passes the expected
error messages as a unicode string (in django/trunk/tests/
regressiontests/forms/localflavor/utils.py#L40):

self.assertRaisesRegexp(ValidationError, unicode(errors),
required.clean, input
)

... then further down the track in assertRaisesRegexp, that string is
compiled as a regular expression (in django/trunk/django/utils/
unittest/case.py#L991):

expected_regexp = re.compile(expected_regexp)

Now, the issue here is that the error message in the patch for #14608
contains some '-' characters, and that the errors list is cast to
unicode wrapping it with '[' and ']' characters:

class INLocalFlavorTests(LocalFlavorTestCase):
def test_INPhoneNumberField(self):
error_format = [u'Phone numbers must be in 02X-7X or
03X-6X or 04X-5X format.']
...

and:

unicode(errors) == u"[u'Phone numbers must be in 02X-7X or 03X-6X
or 04X-5X format.']"

Therefore, when assertRaisesRegexp compiles the unicode string it
interprets "X-7", "X-6" and "X-5" as ranges, and then throws a 'bad
character range' exception making the tests fail. If we remove those
'-' characters from the expected error message, then the tests will
pass.

I find it really weird that the error message is assumed to be a
compilable regular expression...

2) While debugging the problem above I've found another (possibly
linked) weirdness. Basically the error message that is passed to
assertRaisesRegexp is not always matched as you would expect. Here's a
simple test case I've made:

class AssertRaisesRegexp(TestCase):

def test_error_message_match(self):
def func():
raise ValidationError("No no no!")
self.assertRaisesRegexp(ValidationError, "No no no!", func) #
This passes - all good.
self.assertRaises(AssertionError, self.assertRaisesRegexp,
ValidationError, "Yes yes yes!", func) # This passes, as expected.
self.assertRaises(AssertionError, self.assertRaisesRegexp,
ValidationError, "[Yes yes yes]", func) # This fails, why?!

You'll notice that the only difference between the last two lines are
the use of '[' and ']' in the last line. So, when wrapping the error
message with '[' and ']', the error message is purely ignored and the
test just passes. This, by the way, means that pretty much all the
tests in 'regressiontests.forms.localflavor' using 'assertFieldOutput'
are currently passing without really being sure that the provided
error messages are correct.

I'm not sure how to properly fix this or where the core issue is. Am I
missing something or is it worth opening a ticket?

Thank you!

Julien

[Karen Tracey]

On Mon, Apr 11, 2011 at 10:17 PM, Julien Phalip <jph...@gmail.com> wrote:

I'm not sure how to properly fix this or where the core issue is. Am I
missing something or is it worth opening a ticket?

I think there's definitely a problem with that assertFieldOutput utility function. That 2nd parameter to assertRaisesRegexp is supposed to be a regular expression object or string containing a regular expression (http://docs.python.org/library/unittest.html#unittest.TestCase.assertRaisesRegexp). If we're going to be passing in arbitrary error messages, then re.escape() should be used on them to ensure that any special characters in those messages are not mistakenly interpreted as part of a regular expression. The unexpected results you mention are explained when you consider how the special characters (e.g. []) in the strings affect the resulting regexp.

Adding re.escape in that function does break some tests, from a quick look it seems mostly ones that are raising multiple errors but only one has been listed by the caller of that utility function. It isn't clear to me from the docstring of that utility function how it is supposed to work in this case...is the caller required to be passing in all errors or is the function supposed to find it acceptable if the raised errors include the one that is passed in? I don't know.

Karen

[Julien Phalip]

On Apr 12, 2:00 pm, Karen Tracey <kmtra...@gmail.com> wrote:
> Adding re.escape in that function does break some tests, from a quick look
> it seems mostly ones that are raising multiple errors but only one has been
> listed by the caller of that utility function. It isn't clear to me from the
> docstring of that utility function how it is supposed to work in this
> case...is the caller required to be passing in all errors or is the function
> supposed to find it acceptable if the raised errors include the one that is
> passed in? I don't know.

Thanks for looking into this, Karen. I agree that the API for
assertFieldOutput is quite confusing. Since assertRaisesRegexp does a
search (and not a match), I guess it means that providing one of the
errors is enough to pass the test. However, regardless of that, from
an end-user perspective it doesn't really make sense that it uses
regular expressions at all. I tend to think that assertFieldOutput
shouldn't use assertRaisesRegexp but either use the good old
assertRaises instead.

I think I may have found a fix for this and posted a patch to a new
ticket: http://code.djangoproject.com/ticket/15805

Thank you,

Julien