Skip to content
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

Add more pickling tests #49915

Closed
collinwinter mannequin opened this issue Apr 2, 2009 · 17 comments
Closed

Add more pickling tests #49915

collinwinter mannequin opened this issue Apr 2, 2009 · 17 comments
Labels
tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error

Comments

@collinwinter
Copy link
Mannequin

collinwinter mannequin commented Apr 2, 2009

BPO 5665
Nosy @rhettinger, @pitrou, @avassalotti, @benjaminp
Files
  • pickle_tests.patch: Patch against trunk, r71100
  • 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:

    assignee = None
    closed_at = <Date 2009-04-16.03:19:00.440>
    created_at = <Date 2009-04-02.04:45:46.462>
    labels = ['type-bug', 'tests']
    title = 'Add more pickling tests'
    updated_at = <Date 2009-04-16.03:19:00.354>
    user = 'https://bugs.python.org/collinwinter'

    bugs.python.org fields:

    activity = <Date 2009-04-16.03:19:00.354>
    actor = 'collinwinter'
    assignee = 'none'
    closed = True
    closed_date = <Date 2009-04-16.03:19:00.440>
    closer = 'collinwinter'
    components = ['Tests']
    creation = <Date 2009-04-02.04:45:46.462>
    creator = 'collinwinter'
    dependencies = []
    files = ['13599']
    hgrepos = []
    issue_num = 5665
    keywords = ['patch', 'needs review']
    message_count = 17.0
    messages = ['85162', '85291', '85313', '85314', '85315', '85316', '85318', '85320', '85330', '85331', '85783', '85787', '85788', '85875', '85885', '85898', '86012']
    nosy_count = 5.0
    nosy_names = ['collinwinter', 'rhettinger', 'pitrou', 'alexandre.vassalotti', 'benjamin.peterson']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue5665'
    versions = ['Python 3.1', 'Python 2.7']

    @collinwinter
    Copy link
    Mannequin Author

    collinwinter mannequin commented Apr 2, 2009

    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.

    @collinwinter collinwinter mannequin added tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error labels Apr 2, 2009
    @avassalotti
    Copy link
    Member

    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?

    @collinwinter
    Copy link
    Mannequin Author

    collinwinter mannequin commented Apr 3, 2009

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Apr 3, 2009

    How much is "very slow"? If it's less than 10-15 seconds I think it's ok.

    @collinwinter
    Copy link
    Mannequin Author

    collinwinter mannequin commented Apr 3, 2009

    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.

    @rhettinger
    Copy link
    Contributor

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Apr 3, 2009

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

    @pitrou
    Copy link
    Member

    pitrou commented Apr 3, 2009

    Raymond, how would you reconcile your random approach with the fact that
    buildbots only display errors after a second verification run?

    @rhettinger
    Copy link
    Contributor

    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.

    @collinwinter
    Copy link
    Mannequin Author

    collinwinter mannequin commented Apr 3, 2009

    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.

    @collinwinter
    Copy link
    Mannequin Author

    collinwinter mannequin commented Apr 8, 2009

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Apr 8, 2009

    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.

    @collinwinter
    Copy link
    Mannequin Author

    collinwinter mannequin commented Apr 9, 2009

    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.

    @benjaminp
    Copy link
    Contributor

    Collin, can you port these new tests to Py3k?

    @collinwinter
    Copy link
    Mannequin Author

    collinwinter mannequin commented Apr 12, 2009

    Yes, I'm porting them. I got started, but got distracted by other things
    since there were a lot of conflicts in the merge

    @benjaminp
    Copy link
    Contributor

    Ok. Thank you!

    @collinwinter
    Copy link
    Mannequin Author

    collinwinter mannequin commented Apr 16, 2009

    Committed as r71408 (trunk) and r71638 (py3k).

    @collinwinter collinwinter mannequin closed this as completed Apr 16, 2009
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants