Issue 16510: Using appropriate checks in tests

The proposed patch upgrades tests to use specialized checks added in 3.1 and 3.2 (assertIsNone(x) instead assertTrue(x is None), assertLess(a, b) instead assertTrue(a < b), etc).  This modern checks provide a more useful error message in case of a fail.

Replaced only those checks that are not related to the tested operators.  For example, assertTrue(a < b) preserved if the operator "<" is tested.

Thanks for the work, but we don't generally make bulk changes like this.  It generates churn in the codebase, and has the risk of inadvertently changing the meaning of the tests, to little actual benefit.  Instead we modernize tests when we touch them for other reasons and are in a position to confirm that the changes do not change the meaning of the tests.  (I realize that for most of your changes the meaning is trivially preserved...but when you make a lot of changes you are almost certain to make some mistakes...thus the resistance to doing bulk updates.)

I understand this.  I checked the patch few times, with long (more than a month) intervals between inspections.  If someone wants to modernize some tests, he can turn to this patch for reference.

@David is that policy documented in the devguide? If not, should it be?

If it isn't documented it should be, and you could open an issue for it.

Patch updated to sync with tip. It is 5% less than first version because some changes already applied in other commits.

I think there are two counterargument to leaving things alone.

1. The reason for the specialized checks is to change behavior when the test fails, to give more informative error messages. So the change is not purely cosmetic or stylistic. I believe there was a recent case where assertTrue(a == b), a and b strings, had to be changed to assertEqual(a, b) to find out why the assert failed. As I remember, nothing else in the test file obviously needed changing and it would not have been touched otherwise.

2. People adding tests may not review existing tests for modernization.

That said, it *is* possible to ignore 2. and not worry about 1. until a test fails. And I agree that the original patch is too much to review.

A third counterargument is:

3. Newer developers (and even seasoned ones) adding new tests may use existing tests as a model for best practices. If the existing tests model sub-optimal practices, then those practices will be perpetuated both in the codebase and in the minds of contributors.

Given that Serhiy has so diligently prepared and updated the patch, I'm inclined to say the codebase would be better off accepting the patch.

David, can you imagine a process by which a patch like this could be acceptable?

Here is synchronized to tip the patch which doesn't include patches for already separated children issues. It touches 129 files and modifies over 600 lines.

Jason: yes, if it is broken down into small enough pieces for the module maintainer to be comfortable with the review :).  I approved Serhiy's separate patch for test_email.  I still wasn't sure about backporting, but I really don't like the 'unless' construct in test_email, so I decided to say OK.

After chipping off yet some small separate issues, there are only 276 changed lines left in 80 files.

Can we follow up on this please as it's referenced from #9554 as well.

messages: + msg210566

messages: + msg203057

messages: + msg196459