The proposed patch makes the operator module tests use more specific asserts. This will provide more useful failure report.

Rietveld's within-line diff highlighting really helps reviewing this sort of thing. LGTM.

LGTM.

LGTM. Ping?

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.

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.

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>
> _______________________________________

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.

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.

I agree with Antoine.

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".

Looks like this is ready to be applied and closed or just closed.

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.

Backporting this to older releases is needed to help backporting future tests. We should keep tests consistent if possible.

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.

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.

Everyone except Raymond seems to agree the patch is a good think, so it should probably be applied.

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.

New changeset 7bf3d91f8d6c by Antoine Pitrou in branch 'default':
Closes #20544: use specific asserts in operator tests.
https://hg.python.org/cpython/rev/7bf3d91f8d6c