Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more exception related assertions to unittest #62254

Open
ncoghlan opened this issue May 25, 2013 · 13 comments
Open

Add more exception related assertions to unittest #62254

ncoghlan opened this issue May 25, 2013 · 13 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@ncoghlan
Copy link
Contributor

BPO 18054
Nosy @rhettinger, @ncoghlan, @pitrou, @rbtcollins, @ezio-melotti, @merwok, @voidspace, @Julian, @serhiy-storchaka

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2013-05-25.06:55:59.211>
labels = ['type-feature']
title = 'Add more exception related assertions to unittest'
updated_at = <Date 2013-11-29.16:48:46.954>
user = 'https://github.com/ncoghlan'

bugs.python.org fields:

activity = <Date 2013-11-29.16:48:46.954>
actor = 'pitrou'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = []
creation = <Date 2013-05-25.06:55:59.211>
creator = 'ncoghlan'
dependencies = []
files = []
hgrepos = []
issue_num = 18054
keywords = []
message_count = 13.0
messages = ['189945', '190183', '190188', '190190', '190570', '190581', '190587', '190592', '190601', '190602', '190843', '190849', '204739']
nosy_count = 11.0
nosy_names = ['rhettinger', 'ncoghlan', 'pitrou', 'rbcollins', 'vila', 'ezio.melotti', 'eric.araujo', 'michael.foord', 'Julian', 'aliles', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'needs patch'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue18054'
versions = ['Python 3.4']

@ncoghlan
Copy link
Contributor Author

When creating the error handling tests for the new ipaddress module, one of the things I added was an "assertCleanError" method on the test cases [1].

This is a wrapper around assertRaisesRegex which ensures that either no exception context is set, or if there is one set, ensures that the display is suppressed by default.

While I don't think assertCleanError itself is suitable for adoption (there are two many use case specific details), I think it *would* be useful to provide two related helper assertions along the lines of the following (the failure message in the first case could likely be improved).

Firstly, ensuring that the context has been *suppressed* when it should have been:

    def assertCleanTraceback(self, exc, msg=None):
        exc_context = exc.__context__
        if exc_context is not None and not exc.__suppress_context__:
            if msg is None:
                fmt = "{} has context set: {}"
                msg = fmt.format(type(exc), type(exc_context))
            self.fail(msg)
        return exc_context

and secondly ensuring that the cause has been *set* when it should have been:

    def assertRaisedFrom(self, exc, expected_cause, expected_regex, msg=None):
        exc_cause = exc.__cause__
        # Use the guts of assertRaises to compare the actual cause
        # and the expected cause and raise an appropriate failure
        # if they don't match
        return exc_cause

Both proposed helpers return the actual context/cause so they can be used to test longer deliberate exception chaings.

[1] http://hg.python.org/cpython/file/04ca3f1515cf/Lib/test/test_ipaddress.py#l37

@ncoghlan ncoghlan added the type-feature A feature request or enhancement label May 25, 2013
@voidspace
Copy link
Contributor

I'm slightly torn.

TestCase has a wide (waaaay too wide) API and adding methods makes it wider. The use cases for these methods are relatively niche, so my instinct would be that these don't meet the bar for inclusion on TestCase.

On the other hand, despite their brevity they're hard to get right, so having them in the standard library is helpful from that point of view.

I might be more sympathetic to having some helper mixin classes, so that we can provide esoteric-but-difficult assert methods without having to dump them all on TestCase.

@serhiy-storchaka
Copy link
Member

I'm sure not all user of the TestCase class even know about all it's existing methods.

@ncoghlan
Copy link
Contributor Author

I like the idea of a separate mixin class like TracebackMixin or TracebackHelper.

@ezio-melotti
Copy link
Member

What about adding a "recipes" section in the docs with sample implementation of specific assertMethods?

@Julian
Copy link
Mannequin

Julian mannequin commented Jun 4, 2013

Can I throw in, and hopefully not in a way that's too out of place, that I think that unittest might want to rethink it's strategy re: assertion methods? The fact that the object that has all the assertions and the object that has the logic for running a test, and the thing that holds all the tests on it, makes things difficult. For a concrete example, subclasses of unittest.TestCase like e.g. trial.unittest.TestCase do not have easy ways of allowing pluggable assertions, by which I mean you can't show up and say "Here I have a thing with some assertions, use this". So, for trial, if we want to support unittest2 assertions (which I'd personally like), we have to put code in trial to do so.

It would seem to me that it'd be nicer to (obviously keep what we have for backwards compatibility) but rather than adding a bunch of mixins, have an API that decouples things holding assertion methods from a TestCase, and mix those in by calling a method on the TestCase rather than mixing in a bunch of assertion mixins.

(also, in a slightly lighter note, I might like an assertEqualAndAlsoNotUnequal method :D)

@rhettinger
Copy link
Contributor

+1 to what Michael said. The current API is way too big, but these proposed methods aren't trivially easy to get right. It would be nice to have them done once and done well.

@ncoghlan
Copy link
Contributor Author

ncoghlan commented Jun 4, 2013

I like the idea of using the strategy pattern to at least decouple the assertion API from the main testcase API. I also like it better than the mixin suggested earlier.

We would obviously have to keep the existing methods for backwards compatibility, but could avoid adding new ones.

I'm not sure what such an API would look like, though :(

@Julian
Copy link
Mannequin

Julian mannequin commented Jun 4, 2013

I don't either really off the top of my head, though I've toyed with some things.

If there's support for some exploration I wouldn't mind attempting a POC externally though.

@voidspace
Copy link
Contributor

There's always (almost) support for exploration...

@rbtcollins
Copy link
Member

Hey, feel free to +nosy me on unittest things :). Julian pinged me on IRC about this - Jml and I would be delight to prepare a patchset for assertThat for unittest and as much of the core of Matchers as makes sense. The bare core can come in without any of the things that Michael is concerned about vis-a-vis unneeded complexity in testtools [e.g. nothing about arbitrary attachments is needed].

We can state from experience - both ours and users that have adopted it - that the decoupling assertThat brings is very effective at ending the forced-hierarchy-or-mixin mess that self-homed assertions brings. A while back we rewrote the entire core of testtools own assertions to be Matcher based :).

https://testtools.readthedocs.org/en/latest/for-test-authors.html#matchers has the documentation on the Matcher protocol and what it looks like for users.

Structurally, we decouple the raising of AssertionError from the act of determining whether a particular condition is met - this frees one from needing to have a reference to a test case to honour failureException, and from needing to subclass to share condition logic - unrelated test classes can how share condition logic by sharing a matcher definition.

It also lends itself rather naturally to higher order definitions of matchers, as matchers are now standalone objects with no required connection to a test case : you can curry and compose matchers easily.

We make matchers have a nice __str__, so that their structure can be printed out by assertThat when an error has occurred even if they have been curried or compose or defined much earlier.

So - is there some interest in this? If so, I can put together a patchset with just the core, and let you see what it looks like.

@ncoghlan
Copy link
Contributor Author

ncoghlan commented Jun 9, 2013

I think something like "assertThat" could address the problem nicely. Probably best to propose it as a separate issue, then we can make this one depend on that one if we decide to go that way.

@pitrou
Copy link
Member

pitrou commented Nov 29, 2013

Also, see issue bpo-19645.

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@iritkatriel iritkatriel added the stdlib Python modules in the Lib dir label Nov 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement
Projects
Status: No status
Development

No branches or pull requests

8 participants