classification
Title: Eliminate buffer_tests.py
Type: enhancement Stage: resolved
Components: Tests Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: martin.panter Nosy List: ezio.melotti, martin.panter, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2016-02-01 10:49 by martin.panter, last changed 2016-04-09 00:38 by martin.panter. This issue is now closed.

Files
File name Uploaded Description Edit
buffer_tests.patch martin.panter, 2016-02-01 12:03 review
buffer_tests_2.patch serhiy.storchaka, 2016-03-19 09:40 review
buffer_tests_3.patch martin.panter, 2016-04-04 06:01 review
buffer_tests.py2.patch martin.panter, 2016-04-06 12:12 For 2.7 review
Messages (13)
msg259318 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-01 10:49
This sort of follows on from other cleanup of test_bytes.py in Issue 19587.

Currently buffer_tests defines one base test class, MixinBytesBufferCommonTests, which is used by test_bytes.py to test bytearray (but not bytes!). However there are other tests defined in test_bytes.py, or string_tests.py, which are run, or could be run, for both bytearray and bytes. I haven’t checking too closely, but I think all the methods in buffer_tests could be merged with other methods and collected into string_tests.BaseTest. Then they would get run for bytes, bytearray, str, and UserString.

The following methods are probably redundant with class string_tests.MixinStrUnicodeUserStringTest (only run for str and UserString). It looks like there is no bytes version of these tests:

* test_islower/upper/title/space/alpha/alnum/digit()
* test_title()
* test_splitlines()

test_capitalize() looks redundant with half of the method in string_tests.CommonTest (also only run for str and UserString). Again there doesn’t seem to be a bytes version of it. I think the common part could be moved into BaseTest, and the unicode-specific part left where it is.

The following are probably also redundant with class string_tests.CommonTest:

* test_ljust/rjust/center()
* test_swapcase()
* test_zfill()

For ljust(), rjust() and center(), there are also tests run for bytes and bytearray in test_bytes.BaseBytesTest that could also be merged at the same time. But swapcase() and zfill() don’t seem to be currently tested for bytes.
msg259321 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-01 12:03
I didn’t end up merging test_ljust/rjust/center() from test_bytes. Those tests are more specific to bytes and bytearray, so they are better left as-is.
msg261988 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-03-18 19:28
It looks to me that all tests from CommonTest except test_capitalize_nonascii can be merged into BaseTest.

A number of tests from MixinStrUnicodeUserStringTest could be applied to bytes and bytearray too.

I think we should left only two classes in string_tests: common tests for str, UserString, bytes and bytearray, and common tests for str and UserString that are not applicable to bytes and bytearray.
msg262028 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-03-19 08:54
Yes what you say about re-arranging the methods in string_tests.py makes sense to me. There might also be some factorizing possible between specific methods in test_bytes.py and the (currently) string-only test methods. But I would like to handle these as separate steps, to keep the patch size under control, ensure we don’t lose any important test cases in the process, etc.
msg262033 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-03-19 09:40
I meant that the patch can be smaller if move more tests from CommonTest to BaseTest without changing order. Here is smaller patch.
msg262844 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-04 06:01
I see what you meant about making the diff smaller. However in your patch you also moved three extra methods into BaseTest: test_additional_(r)split() and test_strip().

I prefer to handle these three methods in a separate patch that deals with test_bytes.BytesBaseTest, which also has split() and strip() tests. So this third patch moves those three methods back to CommonTest.
msg262845 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-04 07:48
Unfortunately Mercurial can't handle moving file fragments. The history of these methods will be lost. That is why I prefer the patch with minimal moving.

Are there any disadvantages of having test_additional_(r)split() and test_strip() in BaseTest?
msg262847 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-04 08:04
Okay you convinced me to leave those methods in the same relative position as in your patch. I can sympathize with keeping the Mercurial history small. :)

The only disadvantage is a couple extra microseconds wasted doing redundant tests. I will try and work on factoring out some of those tests next.
msg262849 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-04 08:21
And would be nice to backport these changes to 2.7. This will help backporting new future tests.
msg262943 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-06 12:12
I discovered a flaw in the bytearray tests: most of them don’t actually test bytearray objects! This is easy to fix in Python 3, and I added a test case to ensure that the arguments are converted to the expected type.

However porting this fix to Python 2 was trickier. A few of the bytearray methods do not accept bytearray arguments:

>>> bytearray(b"abc").ljust(10, bytearray(b"*"))
TypeError: ljust() argument 2 must be char, not bytearray
>>> bytearray(b"abc").rjust(10, bytearray(b"*"))
TypeError: rjust() argument 2 must be char, not bytearray
>>> bytearray(b"abc").center(10, bytearray(b"*"))
TypeError: center() argument 2 must be char, not bytearray

I adapted the tests from the deleted buffer_tests.py file to override the relevant tests from string_tests.py, so that we continue to test bytearray methods but with str a.k.a. bytes arguments.
msg262944 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-04-06 13:32
Nice.

> I discovered a flaw in the bytearray tests: most of them don’t actually test bytearray objects!

Good catch!

> I adapted the tests from the deleted buffer_tests.py file to override the relevant tests from string_tests.py, so that we continue to test bytearray methods but with str a.k.a. bytes arguments.

The patch LGTM. But may be instead of duplicating tests add fixfillchartype() (calling fixtype() by default)? If this don't complicate tests too much.
msg263006 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-04-08 05:32
New changeset 197e1f8b28b7 by Martin Panter in branch '3.5':
Issue #26257: Eliminate buffer_tests.py and fix ByteArrayAsStringTest
https://hg.python.org/cpython/rev/197e1f8b28b7

New changeset ea598d69b7d3 by Martin Panter in branch 'default':
Issue #26257: Merge buffer_tests cleanup from 3.5
https://hg.python.org/cpython/rev/ea598d69b7d3

New changeset 26f1543e806c by Martin Panter in branch '2.7':
Issue #26257: Eliminate buffer_tests.py and fix ByteArrayAsStringTest
https://hg.python.org/cpython/rev/26f1543e806c
msg263051 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-09 00:38
Thanks for you help with this Serhiy. Instead of your fixfillchartype(), I went with special “if self.type2test is bytearray” checks in the common tests.
History
Date User Action Args
2016-04-09 00:38:15martin.pantersetstatus: open -> closed
resolution: fixed
messages: + msg263051

stage: commit review -> resolved
2016-04-08 05:32:36python-devsetnosy: + python-dev
messages: + msg263006
2016-04-06 13:32:45serhiy.storchakasetmessages: + msg262944
2016-04-06 12:12:39martin.pantersetfiles: + buffer_tests.py2.patch

messages: + msg262943
versions: + Python 2.7
2016-04-04 08:21:56serhiy.storchakasetassignee: martin.panter
messages: + msg262849
stage: patch review -> commit review
2016-04-04 08:04:21martin.pantersetmessages: + msg262847
2016-04-04 07:48:35serhiy.storchakasetmessages: + msg262845
2016-04-04 06:01:35martin.pantersetfiles: + buffer_tests_3.patch

messages: + msg262844
2016-03-19 09:40:07serhiy.storchakasetfiles: + buffer_tests_2.patch

messages: + msg262033
2016-03-19 08:54:32martin.pantersetmessages: + msg262028
2016-03-18 19:28:55serhiy.storchakasetmessages: + msg261988
2016-03-18 18:20:07ezio.melottisetnosy: + ezio.melotti, serhiy.storchaka
2016-02-01 12:03:04martin.pantersetfiles: + buffer_tests.patch
keywords: + patch
messages: + msg259321

stage: needs patch -> patch review
2016-02-01 10:49:55martin.pantercreate