classification
Title: Use specific asserts in bigmem tests
Type: enhancement Stage: resolved
Components: Tests Versions: Python 3.3, Python 3.4, Python 2.7
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: ezio.melotti, rhettinger, serhiy.storchaka, terry.reedy
Priority: normal Keywords: easy, patch

Created on 2014-02-07 20:25 by serhiy.storchaka, last changed 2017-04-05 05:43 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
test_bigmem_asserts.patch serhiy.storchaka, 2014-02-07 20:25 review
test_bigmem_asserts_2.patch serhiy.storchaka, 2014-02-14 12:01 review
test_bigmem_asserts_3.patch serhiy.storchaka, 2017-04-05 05:42 review
Pull Requests
URL Status Linked Edit
PR 791 closed serhiy.storchaka, 2017-03-23 17:21
Messages (6)
msg210544 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-07 20:25
The proposed patch makes bigmem tests use more specific asserts. This will provide more useful failure report.
msg211197 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2014-02-14 02:15
These should be backported.

And it probably shouldn't be done at all unless there is an actual failure with an uninformative error message.  Otherwise, you're just destabilizing the test suite and creating unnecessary code churn.

In the case of the collections tests, I used test-driven-development for parts of it and am very confident in the test as they stand.  If you start  switching the test methods, I become less confident in those tests (i.e. I haven't seen the new ones fail in the absence of the code they were meant to test).

Additionally, the "more specific tests" introduce some additional opacity that is harmful for knowing what methods and operators are specifically used internally in test method.  For end users of Python, they don't have to worry much about this, but we as developers of core types really care whether self.assertLessThan(x, y) really does x < y, or x.__lt__(y), or "not y >= x", etc.

IOW, I am of the strong opinion that your patches are not a good idea.  The "more specific tests" can be used in new tests or in tests that are failing, but going back and making blanket sweeps of the test suite isn't a good practice.

Please lookup Guido's comments on "holistic refactoring" being preferred to these kind of "sweeps".
msg211216 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-14 12:01
> Additionally, the "more specific tests" introduce some additional opacity
> that is harmful for knowing what methods and operators are specifically
> used internally in test method.  For end users of Python, they don't have
> to worry much about this, but we as developers of core types really care
> whether self.assertLessThan(x, y) really does x < y, or x.__lt__(y), or
> "not y >= x", etc.

If the test explicitly designed to test relation or boolean operations, I 
leave it as is. I try to be very careful.

After more careful reviewing this patch, I have found than some tests 
shouldn't be changed, because they produce utterly large error message in case 
of a failure (even if resulted message is truncated, as in case of 
assertEqual, large intermediate strings are created). Here is corrected patch.
msg211218 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2014-02-14 12:29
> I try to be very careful.

It's great to be careful, but it is a smarter move to not change the test suite at all (except in cases where there is a known problem).  

There is almost zero benefit to the patch (i.e. the tests are currently not failing and have been stable for a long time).  You risk making a mistake, leaving an undetected hole in the tests, and increasing chances of future regression during normal maintenance.

Further, this patch churns the code away from what the original test case authors were thinking about when they were deeply engaged with the code.  That is why Guido wants only "holistic" refactorings.

Another issue is that if you make the extensive test suite changes, there is no case for backporting those changes (especially for Python 2.7).  But then, if the versions don't line up, it makes cross-version maintenance more difficult if an actual bug arises (i.e. the patches won't apply cleanly).

In short, I recommend that you please don't do this.  It isn't good for the project.   Thank you.
msg211257 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2014-02-15 03:58
Patch2 looks correct, given that the replacement is the right thing to do. The assertTrue error message 'False is not true' is definitely not helpful. But I see Raymond's point. I think these patches should be discussed on pydev.
msg211260 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2014-02-15 06:37
> For end users of Python, they don't have to worry much about this, but
> we as developers of core types really care whether self.assertLessThan(x, y)
> really does x < y, or x.__lt__(y), or "not y >= x", etc.

FWIW the assert methods should guarantee that the corresponding operator is used (e.g. < for assertLess), and I think this is already the case.

> After more careful reviewing this patch, I have found than some tests
> shouldn't be changed, because they produce utterly large error 
> message in case of a failure (even if resulted message is truncated, 
> as in case of assertEqual, large intermediate strings are created).

Some of these cases are already fixed, for others there are still open issues.  If you find cases that are not tracked you should report them.
History
Date User Action Args
2017-04-05 05:43:00serhiy.storchakasetstatus: open -> closed
files: + test_bigmem_asserts_3.patch
resolution: rejected
stage: patch review -> resolved
2017-03-23 17:21:30serhiy.storchakasetpull_requests: + pull_request696
2014-02-15 06:37:38ezio.melottisetnosy: + ezio.melotti
messages: + msg211260
2014-02-15 03:58:20terry.reedysetnosy: + terry.reedy
messages: + msg211257
2014-02-14 12:29:17rhettingersetmessages: + msg211218
2014-02-14 12:01:11serhiy.storchakasetfiles: + test_bigmem_asserts_2.patch

messages: + msg211216
2014-02-14 02:16:01rhettingersetnosy: + rhettinger
messages: + msg211197
2014-02-07 20:29:32serhiy.storchakalinkissue16510 dependencies
2014-02-07 20:25:56serhiy.storchakacreate