classification
Title: Using appropriate checks in tests
Type: enhancement Stage: resolved
Components: Tests Versions: Python 3.4
process
Status: closed Resolution: rejected
Dependencies: 19589 19590 19591 19592 19593 19594 19600 19601 19602 19603 19604 19605 19606 19607 20544 20545 20546 20547 20548 20549 20550 20551 20552 20553 20554 20555 20556 20557 Superseder:
Assigned To: Nosy List: ezio.melotti, jason.coombs, jcea, michael.foord, pitrou, r.david.murray, serhiy.storchaka, terry.reedy, tshepang
Priority: normal Keywords: patch

Created on 2012-11-19 10:44 by serhiy.storchaka, last changed 2017-04-05 05:44 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
tests_asserts.patch serhiy.storchaka, 2012-11-19 10:43 review
tests_asserts_2.patch serhiy.storchaka, 2013-08-29 13:47 review
tests_asserts_3.patch serhiy.storchaka, 2013-11-16 16:03 review
tests_asserts_4.patch serhiy.storchaka, 2014-02-07 21:42 review
tests_asserts_7.patch serhiy.storchaka, 2017-04-05 05:44
Pull Requests
URL Status Linked Edit
PR 792 closed serhiy.storchaka, 2017-03-23 17:33
Messages (12)
msg175953 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-19 10:43
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.
msg175956 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-11-19 11:54
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.)
msg175960 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-19 13:58
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.
msg186713 - (view) Author: Tshepang Lekhonkhobe (tshepang) * Date: 2013-04-13 12:42
@David is that policy documented in the devguide? If not, should it be?
msg186743 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-04-13 16:11
If it isn't documented it should be, and you could open an issue for it.
msg196459 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-08-29 13:47
Patch updated to sync with tip. It is 5% less than first version because some changes already applied in other commits.
msg202909 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-11-14 22:38
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.
msg203053 - (view) Author: Jason R. Coombs (jason.coombs) * (Python committer) Date: 2013-11-16 15:44
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?
msg203057 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-11-16 16:03
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.
msg203070 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-11-16 18:15
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.
msg210566 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-07 21:42
After chipping off yet some small separate issues, there are only 276 changed lines left in 80 files.
msg221904 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-06-29 21:35
Can we follow up on this please as it's referenced from #9554 as well.
History
Date User Action Args
2017-04-05 05:44:23serhiy.storchakasetstatus: open -> closed
files: + tests_asserts_7.patch
resolution: rejected
stage: patch review -> resolved
2017-03-23 18:37:04BreamoreBoysetnosy: - BreamoreBoy
2017-03-23 17:33:46serhiy.storchakasetpull_requests: + pull_request697
2014-06-29 21:35:49BreamoreBoysetnosy: + BreamoreBoy
messages: + msg221904
2014-02-07 21:42:55serhiy.storchakasetfiles: + tests_asserts_4.patch

messages: + msg210566
2014-02-07 21:36:36serhiy.storchakasetdependencies: + Use specific asserts in optparse test, Use specific asserts in urllib related tests, Use specific asserts in threading tests, Use specific asserts in io tests
2014-02-07 20:30:27serhiy.storchakasetdependencies: + Use specific asserts in collections tests, Use specific asserts in decimal tests, Use specific asserts in bytes tests, Use specific asserts in ipaddress tests
2014-02-07 20:29:32serhiy.storchakasetdependencies: + Use specific asserts in operator tests, Use specific asserts in unicode tests, Use specific asserts in int tests, Use specific asserts in bigmem tests, Use specific asserts in warnings and exceptions tests, Use specific asserts in mailbox, smtplib and poplib tests
2013-11-16 18:15:33r.david.murraysetmessages: + msg203070
2013-11-16 16:03:24serhiy.storchakasetfiles: + tests_asserts_3.patch

messages: + msg203057
2013-11-16 15:44:18jason.coombssetnosy: + jason.coombs
messages: + msg203053
2013-11-15 09:57:58serhiy.storchakasetdependencies: + Use specific asserts in array tests, Use specific asserts in datetime tests, Use specific asserts in http.cookiejar tests, Use specific asserts in weakref tests
2013-11-15 08:14:58serhiy.storchakasetdependencies: + Use specific asserts in distutils tests, Use specific asserts in sqlite3 tests, Use specific asserts in tkinter tests, Use specific asserts in test_decr
2013-11-14 22:38:59terry.reedysetmessages: + msg202909
2013-11-14 21:02:18serhiy.storchakasetdependencies: + Use specific asserts in ctype tests, Use specific asserts in lib2to3 tests, Use specific asserts in importlib tests, Use specific asserts in unittest tests
2013-11-14 20:46:27serhiy.storchakasetdependencies: + Use specific asserts in test_email
2013-11-14 20:41:40serhiy.storchakasetdependencies: + Use specific asserts in test_asyncio
2013-08-29 13:47:48serhiy.storchakasetfiles: + tests_asserts_2.patch

messages: + msg196459
2013-04-13 16:11:55r.david.murraysetmessages: + msg186743
2013-04-13 12:42:05tshepangsetmessages: + msg186713
2013-04-13 12:39:26tshepangsetnosy: + tshepang
2012-11-23 20:13:02terry.reedysetnosy: + terry.reedy
2012-11-22 01:54:58jceasetnosy: + jcea
2012-11-19 13:58:54serhiy.storchakasetmessages: + msg175960
2012-11-19 11:54:13r.david.murraysetnosy: + r.david.murray
messages: + msg175956
2012-11-19 10:44:18serhiy.storchakacreate