classification
Title: Adding an assertClose() method to unittest.TestCase
Type: enhancement Stage:
Components: Library (Lib) Versions: Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: ChrisBarker, Pam.McANulty, berker.peksag, ezio.melotti, mark.dickinson, michael.foord, r.david.murray, rbcollins, rhettinger
Priority: normal Keywords: patch

Created on 2016-06-03 18:48 by ChrisBarker, last changed 2016-06-24 16:44 by ChrisBarker.

Files
File name Uploaded Description Edit
assertClose.patch ChrisBarker, 2016-06-03 18:48 patch against recent default branch review
assertClose.patch ChrisBarker, 2016-06-03 19:57 updated with equation in the docs review
Messages (19)
msg267133 - (view) Author: Chris Barker (ChrisBarker) * Date: 2016-06-03 18:48
In py3.5, the math.isclose() function was added to the standard library. It can be used to compare floating point numbers to see if they are close to each other, rather than exactly equal. It's not a lot of code, but there are nuances that not every python user should need to understand.

One of the major use cases for isclose() is test code, so it would be good to make it easily available in unitest.TestCase.

TestCase.assertAlmostEqual is NOT the same thing, and can only be used properly for values near 1.

Enclosed is a patch that adds  assertClose and assertNotClose to unittest, as well as tests and additions to the docs.

Still pending: should this support complex numbers, there is a cmath.isclose(). If so, but switching on type?

Also -- should this be back-ported to 3.5 as well -- math.isclose() was introduced there.

Done in the pyCon sprints.
msg267139 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-06-03 19:15
The word description of the meaning of the tolerance attributes told me absolutely nothing.  The equation from the math.islcose docs makes it prefectly clear.  So I think the formula should be included in the documentation.

assertAlomstEqual has a delta attribute now, which is equivalent to abs_tol.  Perhaps instead we should add rel_tol to assertAlmostEqual?
msg267150 - (view) Author: Chris Barker (ChrisBarker) * Date: 2016-06-03 19:48
Thanks,

I'll add the equation to the docstring and docs.

As for adding a rel_tol to assertAlmostEqual -- I think that's a bad idea -- it's a pretty different concept -- overloading the same method would be more confusing than anything else.

in isclose(), abs_tol is mostly there to support comparison to zero. delta, on the other hand, is consistent with assertAlmost eual in the sense that you need to know the order of magnitude of your values apriori.
msg267153 - (view) Author: Chris Barker (ChrisBarker) * Date: 2016-06-03 19:57
updated patch with the equation in the docs.
msg267160 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2016-06-03 20:41
Thanks for proposing this. I really don't want to add new assertions to unittest, and I certainly don't want to add narrow usage ones like this, nor ones that are confusingly named (this has nothing to do with files, but 'close' is a verb for files, just like equal is a verb for objects.

Instead, I suggest a regular function that will raise AssertionError on failure. The only thing you need _formatMessage for is handling long messages, and I don't think thats useful or relevant for a binary numeric test like this.
msg267168 - (view) Author: Chris Barker (ChrisBarker) * Date: 2016-06-03 21:51
I'm not sure it's confusing --what would "close" mean for an assertion for a file? "assertClosed" would be confusing -- and an even more trivial assert :-). But we can bikeshed the name if we decide to put this in.

"""
I certainly don't want to add narrow usage ones like this
"""

well, I can't say I like the unittest API either -- but "this would be just as well served with a function" applies to almost all the asserts in TestCase -- assertTrue? really? So unless we're going to deprecate that API, or at least document that you should be using simpler constructs:

assert something

rather than:

self.assertTrue(something)

then we might as well go with it and have a well supported API.

""" I suggest a regular function that will raise AssertionError on failure"""

where would that go? and what would it be called? and how would it be any better than:

assert math.isclose(a,b)

??

The primary reason I think it should be there is discoverability. Folks will be looking through the testCase docs looking for the asserts they need. And they will find assertAlmostEqual -- and it will often be the wrong solution to their problem. Having the right solution be a first-class part of the API is "a good thing"

And it does add a bit of useful information to the message -- knowing the tolerances used for the failing test really helps you know if it's a bug or too strict a test.
msg267169 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-06-03 21:59
On the name, I assumed it had something to do with files when I saw the issue title.  I'm not sure that's enough of an argument against it, though.

There's an issue where Serhiy is proposing a mixin with additional assert methods.  If that has any traction then this could possibly go there.
msg267172 - (view) Author: Chris Barker (ChrisBarker) * Date: 2016-06-03 22:08
thanks, that's Issue27152 if anyone's curious.

Though I have no idea why you'd want it in a mixin, rather than just there.

But sure, this could be "bundled" in with that.

Perhaps it's time for a broader discussion / consensus about the future of the unittest API?
msg267176 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-06-03 22:14
I raised the same point on that issue, and the answer as I understand it is that we want to change the API by making the assert methods independent of the rest of unit test, so I think Serhiy was seeing the mixin as a move in that general direction.
msg267178 - (view) Author: Chris Barker (ChrisBarker) * Date: 2016-06-03 22:17
Would that make folks more amenable to adding more "specialized" asserts? If so, then sure.

I don't know that it takes a PEP (I hope not) but it would be good to have some guidance as to the direction we want unittest to take written down somewhere.
msg267187 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2016-06-03 23:30
Future direction: hamcrest style matchers. You can read more about them in the context of unittest https://rbtcollins.wordpress.com/2010/05/10/maintainable-pyunit-test-suites/ and http://testtools.readthedocs.io/en/latest/for-test-authors.html#matchers - sorry about the formatting in the blog post, wordpress changed theme details some time after I wrote the post and it now renders horribly :(.

w.r.t. error messages, a regular function that raises AssertionError with a nice message will be precisely as usable.

def assert_math_isclose(first, second, rel_tol=1e-09, abs_tol=0.0, msg=None):
    if math.isclose(first, second, rel_tol=rel_tol, abs_tol=abs_tol):
        return
    standardMsg = ('%s != %s with relative tolerance of %.3g'
                   ' and absolute tolerance of %.3g' % (safe_repr(first),
                                                        safe_repr(second),
                                                        rel_tol,
                                                        abs_tol))
    if msg:
        raise AssertionError("{} : {}".format(standardMsg, msg))
    else:
        raise AssertionError(standardMsg)

The reason I'm pushing back on adding methods to TestCase is that every one we add collides with some number of subclasses out in the wild: the TestCase class has multiple distinct APIs in the same namespace - and thats very poor for maintaining compatibility.

Long term I want to have entirely separate interfaces for these things, but thats even more radical an overhaul than adding matchers as a stock facility, and I don't currently have a planned timeline for starting on that.

All of that said, I see that this isn't the same as assertAlmostEqual in mathematical terms - but in /user/ terms I think it is. So why can't we make assertAlmostEqual be this? Just add the extra parameter needed with a default value and change the implementation?
msg267215 - (view) Author: Chris Barker (ChrisBarker) * Date: 2016-06-04 01:14
"""w.r.t. error messages, a regular function that raises AssertionError with a nice message will be precisely as usable."""

sure -- I totally agree -- but that's not the current unittest API :-( where would you put it? How would people find it and know to use it?

"""The reason I'm pushing back on adding methods to TestCase is that every one we add collides with some number of subclasses out in the wild"""

yup -- but again, the existing API :-(

"""
Long term I want to have entirely separate interfaces for these things, but thats even more radical an overhaul than adding matchers as a stock facility, and I don't currently have a planned timeline for starting on that.
"""
that i like -- but yeah, not this week :-). I also think the matchers thing is a nice idea, but still pretty heavy-weight, and it'll be along time before casual users and newbies will latch onto it..

"""
So why can't we make assertAlmostEqual be this? Just add the extra parameter needed with a default value and change the implementation?
"""

That almost sound like your suggesting we actually change the assertAlmostEqual implementation -- that could be disastrous. On the other hand, you could add a relative_tolerance parameter, and if the user sets that, they are indicating that they want a relative test.

I don't like that API: setting different optional parameters to select an algorithm, when you really just want a different function. On the other hand, it does keep us from polluting the namespace, and allows the documentation for them all to be in the same place, increasing the likelihood that it will be found when it should be. 

And if you ask me -- assertAlmostEqual should have had an algorithm like isclose() in the first place ;-) 

It will present an additional documentation challenge. (and a bit of ugly switching code.

Now to find some time to do it -- my sprinting time is done :-(
msg267224 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-06-04 03:10
FWIW, I find assertClose easy to misinterpret.  At first, it looks like an assertion that a file is closed.  And once you mentally pronounce it like "close something that is open", it is harder to make the jump to "close as in nearby".

Also, I feed there is very little need for this. that we don't need another way to do it, and that the menagerie of specific asserts is getting harder to learn and remember (i.e. is easier to make up new assert methods than it is to remember what someone else wanted to name it).
msg267445 - (view) Author: Chris Barker (ChrisBarker) * Date: 2016-06-05 18:00
Thanks Raymond.

Damn! I wrote a nice comprehensive note, and my browser lost it somehow :-(.

Here's a shorter version:

"FWIW, I find assertClose easy to misinterpret.  At first, it looks like an assertion that a file is closed."

sure -- we can find a new name -- I only used that because it's called "isclose" in the math module -- but no one's likely to think a math module function is about files...

"we don't need another way to do it"

Yes, we certainly do -- it was added to the math module (even though a large fraction of use cases would be testing), and something like this is in numpy, Boost, etc, and as the long debate about the PEP indicates -- it's not obvious how to do it -- I'd argue this is more necessary in unitest than most of the other asserts....

(and, sample size 1: a relatively new user offered to proofread the patch for me, and immediately said "hey this is great -- I've been needing this!"

Frankly, the objections to adding new aseert methods are really a critique of the unittest API -- it is clearly DESIGNED to have specialized asserts for many common use cases. So I think we should either:

Embrace the API and add useful asserts like this one.

or

Make a concerted effort (Primarily through documentation) to move toward a different API (or a different way to use the current one, anyway). Robert's posts about "matches" are a good start.

In the meantime, maybe the way to go with this is to add it to assertAlmostEqual -- it gives folks the functionality, makes it discoverable, and doesn't add a new name.

Any objections to that before I take the time to code that up?

-Chris
msg268878 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-06-20 02:28
> Embrace the API and add useful asserts like this one.

This kind of philosophy is going to lead to egregious API expansion, making Python harder to learn and remember.  You're suggesting that we have a nearly zero resistance to adding new assert variants.  

Please keep in mind that once something is added to the standard library, it is very painful to remove it later.  As far as I can tell, there has been zero usability testing of this method with actual users and there have been no user requests for it ever.  I don't see any analog for it in the unittest modules for other languages.

I don't like the method name at all and think we will regret this method if later an assertIsClosed() method is added for making sure objects have had a close() method called.

If recall correctly, Kent Beck himself opposed this kind of expansion of unittest modules, "Some of the implementations have gotten a little complicated for my taste."  He believed that the tool itself should be minimal so that it could be easily learned and mastered while letting "tests be expressed in ordinary source code".

> it is clearly DESIGNED to have specialized asserts for 
> many common use cases.

Actually, is wasn't at all.  When unittest was added, there were no specialized asserts (see https://docs.python.org/2.1/lib/testcase-objects.html ).  It was a translation of the successful and well respected JUnit module.  Its design goal was to provide user extendability rather than throwing in everything including the kitchen sink.

Python's unittest module was around for a very long time before the zoo of specialized asserts was added (courtesy of code donated by Google).
msg268881 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-06-20 03:13
I agree with Raymond. I doubt most of the unittest users would ever need a builtin assertClose() method in unittest. I suggest closing this as 'rejected'.
msg269034 - (view) Author: Chris Barker (ChrisBarker) * Date: 2016-06-21 23:55
Did my comments not get posted, or are they not being read? Anyway:

Could we keep the issues separate here?

1) If you don't like the name, propose another name -- no none has defended this name since an objection was first raised. I"m sure we can find one that would work, if we want to add it.

2) regardless of the original intent, the current API:
 - requires fairly substantial effort to write a new assertion, whether as a method or a function. Thus it's a good idea to provide generally useful ones
 - has a bunch of assert methods, many of which are no more commonly useful than the proposed method.

So adding this is very much in keeping with the current API.

However, it seems there is much resistance to adding new asserts to the base TestCase. Fine. I have trouble defending that as I don't like the API anyway (and yes, of course, it comes from Java -- that's actually the source of the problem :-) )

But: this is a generally useful and non-trivial assert (indeed, as way too many people are confused about floating point, I think it's pretty critical to provide appropriate tools -- and assertAlmostEqual is NOT the appropriate tool in many cases.

So how to provide this? Options:

1) Robert suggested a stand alone function -- sure, but where to put it?

2) Apparently there is a movement afoot to add a mixin with extra stuff -- sure, that would be fine, but only if there actually is such a mixin.

3) add the functionality to assertAlmostEqual, with a new optional parameter -- I suggested this a few comments back, and have had no feedback. (that at least avoids bike shedding the name...)

My key points:

1) This is generally useful and non-trivial -- so would be good to add to the stdlib.

2) it should be discoverable -- as pointed out, folks have not been clamoring for it -- which means they won't know to go digging for it -- they should find it easily (as many folks will use assertAlmostEqual, when they would be better served by this)

which leaves us with adding to a library of functions or a mixin, only if such a thing will actually be build and documented, with more than one function it it :-)

or adding to assertAlmostEqual.

Feedback on this?

(note: adding assertClose as is now off the table, no more need to argue about that)
msg269036 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2016-06-22 00:34
Chris, I suggested altering assertAlmostEqual in http://bugs.python.org/issue27198#msg267187 :) - I took your agreement with that as a good thing and didn't reply because I had nothing more to add.

IMO the status of this issue is as you indicated: you needed time to code up the changes, so that its an extension to assertAlmostEquals, rather than a new assertion.

When thats done I'll happily review it and commit it.
msg269192 - (view) Author: Chris Barker (ChrisBarker) * Date: 2016-06-24 16:44
Thanks Robert.

I'll try to find time to re-do the patch soon. 

There was enough resistance to the whole idea that I wanted some confirmation that is was worth my time to do that!

Stay tuned.
History
Date User Action Args
2016-06-24 16:44:00ChrisBarkersetmessages: + msg269192
2016-06-22 00:34:37rbcollinssetmessages: + msg269036
2016-06-21 23:55:26ChrisBarkersetmessages: + msg269034
2016-06-20 03:13:53berker.peksagsetnosy: + berker.peksag
messages: + msg268881
2016-06-20 02:29:01rhettingersetmessages: + msg268878
2016-06-19 22:43:37Pam.McANultysetnosy: + Pam.McANulty
2016-06-05 18:00:03ChrisBarkersetmessages: + msg267445
2016-06-04 03:10:39rhettingersetnosy: + rhettinger
messages: + msg267224
2016-06-04 01:14:15ChrisBarkersetmessages: + msg267215
2016-06-03 23:30:13rbcollinssetmessages: + msg267187
2016-06-03 22:17:53ChrisBarkersetmessages: + msg267178
2016-06-03 22:14:00r.david.murraysetmessages: + msg267176
2016-06-03 22:08:17ChrisBarkersetmessages: + msg267172
2016-06-03 21:59:48r.david.murraysetmessages: + msg267169
2016-06-03 21:51:06ChrisBarkersetmessages: + msg267168
2016-06-03 20:41:26rbcollinssetmessages: + msg267160
2016-06-03 19:57:58ChrisBarkersetfiles: + assertClose.patch

messages: + msg267153
2016-06-03 19:53:40r.david.murraysetnosy: + mark.dickinson
2016-06-03 19:48:37ChrisBarkersetmessages: + msg267150
2016-06-03 19:15:01r.david.murraysetnosy: + r.david.murray
messages: + msg267139
2016-06-03 18:48:32ChrisBarkercreate