classification
Title: Use specific asserts in operator tests
Type: enhancement Stage: resolved
Components: Tests Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: BreamoreBoy, Vincentdavis, asvetlov, ezio.melotti, pitrou, python-dev, rbcollins, rhettinger, serhiy.storchaka, terry.reedy, vstinner
Priority: normal Keywords: easy, patch

Created on 2014-02-07 20:25 by serhiy.storchaka, last changed 2015-07-26 11:12 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
test_operator_asserts.patch serhiy.storchaka, 2014-02-07 20:25 review
Messages (19)
msg210540 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-07 20:25
The proposed patch makes the operator module tests use more specific asserts. This will provide more useful failure report.
msg211256 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2014-02-15 03:30
Rietveld's within-line diff highlighting really helps reviewing this sort of thing. LGTM.
msg211284 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2014-02-15 16:03
LGTM.
msg217758 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2014-05-02 16:46
LGTM. Ping?
msg217816 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2014-05-03 04:59
While this looks harmless, I seriously question whether it is an improvement.  

For example, how is this any better?

-        self.assertTrue(operator.setitem(a, 0, 2) is None)
+        self.assertIsNone(operator.setitem(a, 0, 2))

This error message for the first is already perfectly clear.
I don't see anything that warrants the code churn.

Also remember that changing tests is hazardous.
We don't have tests for the tests.  So, if a test
gets damaged, we won't know about it.   The particular
patch seems fine, but the whole exercise of going through
the test suite and altering the tests is a dubious.  The
odds of us getting ANY value out of this is vanishingly small.
msg217817 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2014-05-03 05:38
The generic error message for the first is 'False is not true'. Clear but useless. The error message for the second is the more specific '<representation of object)> is not None'. Since the expression was supposed to evaluate to None, but did not, it would be helpful to me, at least, to know what it did evaluate to.
msg217821 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2014-05-03 12:32
Ok. I agree with Teddy but Raymond's arguments make a value also.
Thus we need to make any decision and close the issue (and bunch of
similar issues).

On Sat, May 3, 2014 at 8:38 AM, Terry J. Reedy <report@bugs.python.org> wrote:
>
> Terry J. Reedy added the comment:
>
> The generic error message for the first is 'False is not true'. Clear but useless. The error message for the second is the more specific '<representation of object)> is not None'. Since the expression was supposed to evaluate to None, but did not, it would be helpful to me, at least, to know what it did evaluate to.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue20544>
> _______________________________________
msg218443 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-05-13 12:04
I had stopped committing patches for similar issues because there is an opposition against this. See discussion at http://comments.gmane.org/gmane.comp.python.devel/145535.
msg218445 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-05-13 12:12
I think these transformations are useful, because on failure they will give you what is the actual value of the compared operands (something which assertTrue is enable to extract).

Also they are clearer to read IMO.
msg218446 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-05-13 12:23
I agree with Antoine.
msg222300 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-07-04 18:14
There are 28 dependencies listed on #16510.  18 have already been closed, presumably with patches committed but I haven't checked them all.  10 are still open including this one.  Can we have a decision now as to whether we move forward with committing all of these changes, subject of course to formal review, or simply close the outstanding ones as "won't fix".
msg233344 - (view) Author: Vincent Davis (Vincentdavis) Date: 2015-01-03 03:52
Looks like this is ready to be applied and closed or just closed.
msg247379 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-25 19:14
Looks sane to me. Should go in 3.6 if we're going to do this or get closed to remove cognitive overhead in the issue tracker. No point backporting this to older releases.
msg247381 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-07-25 19:22
Backporting this to older releases is needed to help backporting future tests. We should keep tests consistent if possible.
msg247385 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2015-07-25 19:52
I don't think kind of shallow changes to the test suite should be backported.  They probably shouldn't have been done at all.  When you change code, the tests are a safety net.  When you change tests, you have almost no safety net at all.  If the original test was developed using test-driven-development or for a bug-fix, then it was demonstrated to have failed at one time.  But when pointlessly refactored, the revised tests have no longer been demonstrated to actually detect to original underlying bug.  

If the whole point of "specific asserts" is to have better error messages on a failure, then we should wait until there is an actual failure to make the change.  Otherwise, this reduced the reliability of the test suite with zero benefit.  Most of these tests will never become broken (I can't recall the last time any of them failed) and if they did, their existing message would suffice.

Another reason this should be done is Guido's notion of "holistic refactoring".  The idea is that someone shouldn't travel through modules making shallow changes.  That work should be deferred to the module maintainer to be done in the course of normal deeply thought out work on the module.

The last reason these kind of changes shouldn't be made is that undoes work by the person who originally wrote it, making the code less familiar to them when they come back.  For example, most of the tests for sets are written in a way that reflects how I was thinking about the problem when I wrote it.  If someone comes through and renames the variables, rewraps the lines, switches the choice of assertions, etc.  Then they make it more difficult to recover the original line of thinking by the designer.  Put another way, no one likes to have code they're written needlessly scrambled around.  If there is a real bug fix, then yes.  Otherwise, shallow wholesale search and replace missions should be approached reluctantly.
msg247387 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-25 19:59
So, is this specific patch ok to apply, or are we going to reject it? I don't particularly care either way, but having this issue open and stalled just adds cognitive load to working with the bug tracker.

FWIW I agree that it should not be backported.
msg247389 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-07-25 20:31
Everyone except Raymond seems to agree the patch is a good think, so it should probably be applied.
msg247403 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2015-07-26 02:41
Antoine, I'm surprised, usually you're the first to oppose backporting anything other than bug-fixes and were generally supportive of Guido's admonitions about holistic refactoring. 

It seems you feel strongly enough about the patch to warrant telling all the other participants to simply dismiss the concerns of another senior developer, so go ahead and apply it.  But, you should have strong reservations about backporting -- that seems completely unjustified.
msg247425 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-07-26 11:12
New changeset 7bf3d91f8d6c by Antoine Pitrou in branch 'default':
Closes #20544: use specific asserts in operator tests.
https://hg.python.org/cpython/rev/7bf3d91f8d6c
History
Date User Action Args
2015-07-26 11:12:06python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg247425

resolution: fixed
stage: commit review -> resolved
2015-07-26 02:41:39rhettingersetmessages: + msg247403
2015-07-25 20:31:27pitrousetmessages: + msg247389
2015-07-25 19:59:33rbcollinssetmessages: + msg247387
2015-07-25 19:52:47rhettingersetmessages: + msg247385
2015-07-25 19:22:20serhiy.storchakasetmessages: + msg247381
2015-07-25 19:14:07rbcollinssetnosy: + rbcollins

messages: + msg247379
versions: + Python 3.6, - Python 2.7, Python 3.4, Python 3.5
2015-01-16 08:41:04pitrousetassignee: serhiy.storchaka
2015-01-03 03:52:24Vincentdavissetnosy: + Vincentdavis
messages: + msg233344
2014-07-04 18:14:17BreamoreBoysetnosy: + BreamoreBoy

messages: + msg222300
versions: + Python 3.5, - Python 3.3
2014-05-13 12:23:58vstinnersetnosy: + vstinner
messages: + msg218446
2014-05-13 12:12:18pitrousetnosy: + pitrou
messages: + msg218445
2014-05-13 12:04:15serhiy.storchakasetmessages: + msg218443
2014-05-03 12:32:59asvetlovsetmessages: + msg217821
2014-05-03 05:38:46terry.reedysetmessages: + msg217817
2014-05-03 04:59:56rhettingersetnosy: + rhettinger
messages: + msg217816
2014-05-02 16:46:33asvetlovsetnosy: + asvetlov
messages: + msg217758
2014-02-15 16:03:52ezio.melottisetnosy: + ezio.melotti

messages: + msg211284
stage: patch review -> commit review
2014-02-15 03:30:09terry.reedysetnosy: + terry.reedy
messages: + msg211256
2014-02-07 20:29:32serhiy.storchakalinkissue16510 dependencies
2014-02-07 20:25:10serhiy.storchakacreate