New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test cPickle with real files #61501
Comments
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: PEP-8 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. |
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). |
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.
This is just unnecessary. This is cPickle-only issue. Aman, can you please submit a contributor form? http://python.org/psf/contrib/contrib-form/ |
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': |
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. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: