classification
Title: Test cPickle with real files
Type: enhancement Stage: resolved
Components: Tests Versions: Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Aman.Shah, Arfrever, alexandre.vassalotti, benjamin.peterson, pitrou, python-dev, r.david.murray, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2013-02-26 08:39 by serhiy.storchaka, last changed 2013-03-18 11:24 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
patch Aman.Shah, 2013-02-27 16:18
patch Aman.Shah, 2013-02-27 17:44 review
test_cpickle_patch Aman.Shah, 2013-03-09 12:44 Updated to accomodate corrections in review review
issue17299v3.patch Aman.Shah, 2013-03-11 20:17 review
Messages (19)
msg183028 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-26 08:39
Currently cPickle module tested only with cStringIO.StringIO. However cPickle uses different code for cStringIO.StringIO, for file objects, and for general IO streams (i.e. io.BytesIO). Last two cases are not covered by tests.
msg183143 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-02-27 12:23
Serhiy, in Python3 the corresponding test uses io.BytesIO.  That means the additional tests are needed on Python3 as well, just a slightly different set, right?
msg183158 - (view) Author: Aman Shah (Aman.Shah) * Date: 2013-02-27 16:18
Created a small patch for python 2.7 using file test_pickle.py .
msg183166 - (view) Author: Aman Shah (Aman.Shah) * Date: 2013-02-27 17:44
Fixed the patch by removing TESTFN from tearDown.
msg183238 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-02-28 22:11
Aman: another nit: PEP8 calls for no unneeded parentheses around logical expressions, so things like:

   if(self.f):

should be written:

   if self.f:

in order to adhere to our coding style.

Also, it's not obvious to me that there is any reason to rename the base classes (PicklerTests->AbstractIOPicklerTests, and same for Persistent test).

Finally, rereading Serhiy's note, Python3 only has "general IO streams", both non-IO files and cStringIO.StringIO are gone.  So this really is a 2.7-only issue, and only worth doing because 2.7 is a long term maintenance release.
msg183243 - (view) Author: Aman Shah (Aman.Shah) * Date: 2013-03-01 07:27
I had put in the renaming because "ioclass" is not defined in the class itself, only in the subclasses. So, instantiating it and using dumps/loads would be meaningless and would raise an error. Which is similar to the behavior of an abstract class.
Also, the "if(self.f):" part can be improved as you said and I looked at the patch for other similar mistakes. But, it's an isolated mistake (at lines 33 and 74) and so maybe it can be fixed during the merge itself?
msg183435 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-03-04 12:14
I have added comments on Rietveld.

Perhaps it will be worth to create mixings for cStringIO.StringIO, BytesIO and file object and then mix them to other tests.

Note that there is no sense to change pure Python pickle tests. Python implementation uses the same code for all streams. It's C implementation needs specialized tests (in Lib/test/test_cpickle.py).
msg183436 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-03-04 12:20
David, yes, this is 2.7 only issue. The code was broken recently (see msg182979 in issue13555) due to insufficient testing.
msg183813 - (view) Author: Aman Shah (Aman.Shah) * Date: 2013-03-09 12:44
I have updated the patch for test_cpickle.py . Also, I would like to help out in creating mixins for the 3 but, it would be helpful if you can explain it in a bit more detail. What is the problem in using the existing pickletester.py??
msg183958 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-03-11 13:22
DRY (do not repeat yourself). Do not implement the same method three times. Definitely the common code should be extracted into separate mixin classes: cStringIOMixin, BytesIOMixin, FileIOMixin.

> What is the problem in using the existing pickletester.py??

This is just unnecessary. This is cPickle-only issue.


Aman, can you please submit a contributor form?

http://python.org/psf/contrib/contrib-form/
http://python.org/psf/contrib/
msg183990 - (view) Author: Aman Shah (Aman.Shah) * Date: 2013-03-11 20:17
Version 3.
msg183991 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-03-11 20:23
LGTM. You forgot to remove close() from some classes, I'll do it myself before committing.
msg184179 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-03-14 18:59
New changeset 8a0b5c9f04c2 by Serhiy Storchaka in branch '2.7':
Issue #17299: Add test coverage for cPickle with file objects and general IO
http://hg.python.org/cpython/rev/8a0b5c9f04c2
msg184181 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-03-14 19:38
I'm a little polished the patch before committing. Thank you for the patch, Aman Shah.
msg184359 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-03-16 22:33
I think this broke the 2.7 Windows bots. Please unbreak.
msg184400 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-03-17 21:25
I'm not sure what is wrong and can't check on Windows, but it is possible that this patch fixes tests. Please check it if you can.
msg184402 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-03-17 22:08
I think you want to open the files in binary mode, not text mode.
msg184442 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-03-18 11:05
Oh, yes.
msg184447 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-03-18 11:24
Benjamin has fixed this in the changeset 6aab72424063.
History
Date User Action Args
2013-03-18 11:24:27serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg184447
2013-03-18 11:05:53serhiy.storchakasetfiles: - test_cpickle_fileio.patch
2013-03-18 11:05:27serhiy.storchakasetmessages: + msg184442
2013-03-17 22:08:43pitrousetmessages: + msg184402
2013-03-17 21:25:12serhiy.storchakasetfiles: + test_cpickle_fileio.patch

messages: + msg184400
2013-03-17 20:56:59serhiy.storchakasetstatus: closed -> open
resolution: fixed -> (no value)
2013-03-16 22:33:42benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg184359
2013-03-14 19:38:51serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg184181

stage: commit review -> resolved
2013-03-14 18:59:47python-devsetnosy: + python-dev
messages: + msg184179
2013-03-11 20:23:31serhiy.storchakasetassignee: serhiy.storchaka
messages: + msg183991
stage: needs patch -> commit review
2013-03-11 20:17:03Aman.Shahsetfiles: + issue17299v3.patch
keywords: + patch
messages: + msg183990
2013-03-11 13:22:36serhiy.storchakasetmessages: + msg183958
2013-03-09 12:44:11Aman.Shahsetfiles: + test_cpickle_patch

messages: + msg183813
2013-03-04 12:20:42serhiy.storchakasetmessages: + msg183436
2013-03-04 12:14:18serhiy.storchakasetmessages: + msg183435
2013-03-01 07:27:09Aman.Shahsetmessages: + msg183243
2013-02-28 22:11:41r.david.murraysetmessages: + msg183238
2013-02-27 17:44:12Aman.Shahsetfiles: + patch

messages: + msg183166
2013-02-27 16:18:09Aman.Shahsetfiles: + patch
nosy: + Aman.Shah
messages: + msg183158

2013-02-27 12:23:49r.david.murraysetnosy: + r.david.murray
messages: + msg183143
2013-02-26 18:00:15Arfreversetnosy: + Arfrever
2013-02-26 08:39:39serhiy.storchakacreate