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.

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?

Created a small patch for python 2.7 using file test_pickle.py .

Fixed the patch by removing TESTFN from tearDown.

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.

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?

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).

David, yes, this is 2.7 only issue. The code was broken recently (see msg182979 in issue13555) due to insufficient testing.

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??

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/

Version 3.

LGTM. You forgot to remove close() from some classes, I'll do it myself before committing.

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

I'm a little polished the patch before committing. Thank you for the patch, Aman Shah.

I think this broke the 2.7 Windows bots. Please unbreak.

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.

I think you want to open the files in binary mode, not text mode.

Oh, yes.

Benjamin has fixed this in the changeset 6aab72424063.