classification
Title: Add more pickling tests
Type: behavior Stage:
Components: Tests Versions: Python 3.1, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: alexandre.vassalotti, benjamin.peterson, collinwinter, pitrou, rhettinger
Priority: normal Keywords: needs review, patch

Created on 2009-04-02 04:45 by collinwinter, last changed 2009-04-16 03:19 by collinwinter. This issue is now closed.

Files
File name Uploaded Description Edit
pickle_tests.patch collinwinter, 2009-04-03 20:48 Patch against trunk, r71100
Messages (17)
msg85162 - (view) Author: Collin Winter (collinwinter) * (Python committer) Date: 2009-04-02 04:44
The attached patch adds more tests for pickling:
- Add tests for the module-level load() and dump() functions.
- Add tests for cPickle's internal data structures, stressing workloads
with many gets/puts.
- Add tests for the Pickler and Unpickler classes, in particular the
memo attribute.
- test_xpickle is extended to test backwards compatibility with Python
2.4, 2.5 and 2.6 by round-tripping pickled objects through a worker
process. 2.3 was too difficult to support.

I'll port to py3k after reviewed for trunk.
msg85291 - (view) Author: Alexandre Vassalotti (alexandre.vassalotti) * (Python committer) Date: 2009-04-03 03:46
Overall, the patch looks good. I haven't reviewed the patch thoroughly
yet, but there is a few things I am not sure about. First, why do you
bother supporting 2.4 (i.e. with a copy of run_with_locale) in a patch
aimed at 2.7? 

In test_many_puts_and_gets(), I believe this:

  for proto in [0, 1, 2]:
    ...

should be changed to:

  for proto in protocols:
    ...

Also, I think the tests in AbstractPicklerUnpicklerObjectTests could be
stronger if they used the pickletools module to decompile to check that
only PUT opcodes have changed or not. However, the extra complexity may
not worth it.

And one last thing, why AbstractCompatTests is hard-coded to use
cPickle? Shouldn't the standard pickle module be tested too?
msg85313 - (view) Author: Collin Winter (collinwinter) * (Python committer) Date: 2009-04-03 17:57
I've made test_xpickle support Python 2.4 because it uses Python 2.4,
2.5 and 2.6 to check that we haven't broken backwards compatibility with
those versions.

I made AbstractCompatTests use cPickle because that's what I've been
changing :) I can add tests for the pure-Python pickle module if you
want. At that point, though, test_xpickle will be very slow and we'll
want to add a regrtest resource to control how much time test_xpickle
spends running. Does xpickle work for the resource name (ie, regrtest.py
-u xpickle)?

I agree with your other points.
msg85314 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-04-03 18:00
How much is "very slow"? If it's less than 10-15 seconds I think it's ok.
msg85315 - (view) Author: Collin Winter (collinwinter) * (Python committer) Date: 2009-04-03 18:12
test_xpickle currently takes over three minutes on my MacBook Pro to
test compatibility with Python 2.4, 2.5 and 2.6, and adding tests for
the pickle module will at least double that, if not push it well above
10 minutes. That's why I recommend using the resource flag if we want to
add the same tests for the pickle module.
msg85316 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2009-04-03 18:18
We took a similar issue in test_decimal and came up with a better
approach to using the resource flag.  If the resource is not enabled,
the tester runs a random subset of the tests.  Over time, that means
that all the tests will get run even if testers are routinely not
enabling the resource.
msg85318 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-04-03 19:33
> test_xpickle currently takes over three minutes on my MacBook Pro to
> test compatibility with Python 2.4, 2.5 and 2.6, and adding tests for
> the pickle module will at least double that, if not push it well above
> 10 minutes.

Ouch! A couple of minutes is already too much for a single test IMHO. I
fully agree with the desire to test as deeply as possible, but if all
modules were to become as well tested, nobody would bother running the
regression suite anymore :-)
msg85320 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-04-03 19:50
Raymond, how would you reconcile your random approach with the fact that
buildbots only display errors after a second verification run?
msg85330 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2009-04-03 20:38
The buildbots should be running with the resource enabled (run the full
set of tests, every time).

If we need the reproducible results with the resource disabled, then a
random seed (different for each successful run) can be saved in a
logfile so that a particular pattern of tests can be re-run.

The idea to run some random tests when the resource flag is disabled
provides more coverage than not running any tests at all.  This was very
helpful during the early development of the decimal module.
msg85331 - (view) Author: Collin Winter (collinwinter) * (Python committer) Date: 2009-04-03 20:48
Updated the patch to include a regrtest xpickle resource, which
restricts if the backwards compat tests will be run. For normal
development, the existing tests should be fine; you only need these
tests if you're mucking with the pickle output.

Added backwards compat tests for the pure-Python pickle module. Total
run time with -uxpickle maxes out a little above five minutes on my
machine, with a lot of variation between runs.

Also addressed the comment about [0, 1, 2] -> protocols.
msg85783 - (view) Author: Collin Winter (collinwinter) * (Python committer) Date: 2009-04-08 18:55
If no-one has any objections to the xpickle resource included in the
latest version of the patch, I'd like to commit this soon so that we can
be more confident in the other changes I have queued up. If I no-one
objects, I'll commit this sometime early tomorrow morning PDT.
msg85787 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-04-08 23:12
> If no-one has any objections to the xpickle resource included in the
> latest version of the patch, I'd like to commit this soon so that we can
> be more confident in the other changes I have queued up. If I no-one
> objects, I'll commit this sometime early tomorrow morning PDT.

Ok if it doesn't take too long to run the tests (which may imply
implementing something like Raymond's suggestion of randomizing test
order, if you haven't already done so).

Thanks for your work,

cheers

Antoine.
msg85788 - (view) Author: Collin Winter (collinwinter) * (Python committer) Date: 2009-04-09 00:06
> Ok if it doesn't take too long to run the tests (which may imply
> implementing something like Raymond's suggestion of randomizing test
> order, if you haven't already done so).

I did something similar: if you don't pass the -uxpickle flag to
regrtest, only a small subset of the tests will be run, which takes less
than half a second on my machine; if you do pass -uxpickle, the full
battery of tests will run, which takes ~5 minutes.
msg85875 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2009-04-11 20:56
Collin, can you port these new tests to Py3k?
msg85885 - (view) Author: Collin Winter (collinwinter) * (Python committer) Date: 2009-04-12 04:37
Yes, I'm porting them. I got started, but got distracted by other things
since there were a lot of conflicts in the merge
msg85898 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2009-04-12 12:23
Ok. Thank you!
msg86012 - (view) Author: Collin Winter (collinwinter) * (Python committer) Date: 2009-04-16 03:18
Committed as r71408 (trunk) and r71638 (py3k).
History
Date User Action Args
2009-04-16 03:19:00collinwintersetstatus: open -> closed
resolution: fixed
messages: + msg86012
2009-04-12 12:23:38benjamin.petersonsetmessages: + msg85898
2009-04-12 04:37:56collinwintersetmessages: + msg85885
2009-04-11 20:56:42benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg85875
2009-04-09 00:06:26collinwintersetmessages: + msg85788
2009-04-08 23:12:40pitrousetmessages: + msg85787
2009-04-08 18:55:53collinwintersetmessages: + msg85783
2009-04-03 20:49:09collinwintersetfiles: - pickle_tests.patch
2009-04-03 20:48:58collinwintersetfiles: + pickle_tests.patch

messages: + msg85331
2009-04-03 20:38:47rhettingersetmessages: + msg85330
2009-04-03 19:50:03pitrousetmessages: + msg85320
2009-04-03 19:33:28pitrousetmessages: + msg85318
2009-04-03 18:18:33rhettingersetnosy: + rhettinger
messages: + msg85316
2009-04-03 18:12:03collinwintersetmessages: + msg85315
2009-04-03 18:00:12pitrousetnosy: + pitrou
messages: + msg85314
2009-04-03 17:57:05collinwintersetmessages: + msg85313
2009-04-03 03:46:25alexandre.vassalottisetmessages: + msg85291
2009-04-02 21:58:17pitrousetnosy: + alexandre.vassalotti
2009-04-02 04:45:47collinwintercreate