On Sat, Oct 10, 2009 at 1:16 AM, Christian Oudard
<[email protected]> wrote:
>
> First off, thank you all for responding and discussing this issue.
>
> Responding to Russell's comments on the quality of the patch, I agree
> that it needs better test coverage. I've uploaded an updated patch
> which covers all exit points of assertFormError. The better tests
> confirm that the original patch2 works as intended.
Apologies - in my haste I misread the way you were using the msg
argument. You are correct that it will pass on all exit paths of
assertFormError (although the extra tests certainly don't hurt :-).
> As far as whether this is worth doing, the point of this patch is not
> that the existing error messages are not descriptive enough. It is
> that they cannot know the circumstances under which they will be
> called in all cases. There may be very important bits of debugging
> information that will never even be passed to the assert methods, or
> be accessible from their scope.
I can completely accept this as an argument. However, what I can't
accept is that a reasonable solution is to replace the rich error
information that is currently returned (in the case of
assertFormError, this is four distinct messages, depending on the type
of failure) with a single generic "there was an error" message that
omits this detail.
> This is a test that looks through a list of urls and checks that there
> are no invalid template variables. Since the method assertNotContains
> doesn't (and shouldn't) parrot back the url for the response, you
> don't know what url failed, without using print statements or a
> debugger.
>
> class TemplateTestCase(ClientTestCase):
> def testTemplateError(self):
> urls = [
> '/',
> '/home/',
> '/admin/',
> # etcetera ...
> ]
> for url in urls:
> response = self.client.get(url, follow=True)
> self.assertNotContains(
> response,
> settings.TEMPLATE_STRING_IF_INVALID,
> msg='Found an invalid variable in url %s' % url)
This example reinforces my point. assertNotContains can fail for two
reasons - either the status code was bad, or the template context
didn't contain the variable. If your test returned the error message
"Found an invalid variable", you have no way of knowing if it was the
status code that caused the test failure.
One thought that has occurred to me is that rather than using msg as
the complete literal error string, we could use it as a prefix to the
literal.
i.e., to take line 124 that you used as an example:
> def assertFormError(self, response, form, field, errors, msg=None):
> ...
> self.fail(msg or "The field '%s' on form '%s' in context %d"
> " contains no errors" % (field, form, i))
would become:
prefix = msg and "%s: " % msg or ""
self.fail("%sThe field '%s' on form '%s' in context %d"
" contains no errors" % (prefix, field, form, i))
This preserves the best of both worlds - a rich failure message, plus
the ability to add user-specific context to help identify the source
of the problem in the test suite. This does differ from the behavior
of the assert* functions in the standard library, but hopefully in a
way that makes sense under the circumstances. Opinions?
Yours,
Russ Magee %-)
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---
