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

Refactor test_class to use unittest lib #44638

Closed
jyzude mannequin opened this issue Feb 28, 2007 · 12 comments
Closed

Refactor test_class to use unittest lib #44638

jyzude mannequin opened this issue Feb 28, 2007 · 12 comments
Assignees
Labels
tests Tests in the Lib/test dir

Comments

@jyzude
Copy link
Mannequin

jyzude mannequin commented Feb 28, 2007

BPO 1671298
Nosy @birkenfeld
Files
  • test_class.patch: Patch to convert test_class.py to use unittest library
  • test_class.patch: Updated patch with modifications recommended by Collin Winter
  • test_class.patch: Removed unnecessary global statement
  • test_class.py.diff: Tweaks by Collin Winter
  • test_class.patch: Way more thorough replacement for test_class.py
  • 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 = 'https://github.com/birkenfeld'
    closed_at = <Date 2007-08-24.19:34:25.595>
    created_at = <Date 2007-02-28.22:38:44.000>
    labels = ['tests']
    title = 'Refactor test_class to use unittest lib'
    updated_at = <Date 2007-08-24.19:34:25.594>
    user = 'https://bugs.python.org/jyzude'

    bugs.python.org fields:

    activity = <Date 2007-08-24.19:34:25.594>
    actor = 'georg.brandl'
    assignee = 'georg.brandl'
    closed = True
    closed_date = <Date 2007-08-24.19:34:25.595>
    closer = 'georg.brandl'
    components = ['Tests']
    creation = <Date 2007-02-28.22:38:44.000>
    creator = 'jyzude'
    dependencies = []
    files = ['7809', '7810', '7811', '7812', '7813']
    hgrepos = []
    issue_num = 1671298
    keywords = ['patch']
    message_count = 12.0
    messages = ['52010', '52011', '52012', '52013', '52014', '52015', '52016', '52017', '52018', '52019', '55270', '55271']
    nosy_count = 5.0
    nosy_names = ['nnorwitz', 'georg.brandl', 'collinwinter', 'jerry.seutter', 'jyzude']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue1671298'
    versions = ['Python 2.6']

    @jyzude
    Copy link
    Mannequin Author

    jyzude mannequin commented Feb 28, 2007

    Refactored Lib/test/test_class.py to use unittest library instead of icky output comparison tests. Also have to delete output/test_class after adding this patch.

    @jyzude jyzude mannequin assigned collinwinter Feb 28, 2007
    @jyzude jyzude mannequin added the tests Tests in the Lib/test dir label Feb 28, 2007
    @jyzude jyzude mannequin assigned collinwinter Feb 28, 2007
    @jyzude jyzude mannequin added the tests Tests in the Lib/test dir label Feb 28, 2007
    @collinwinter
    Copy link
    Mannequin

    collinwinter mannequin commented Mar 6, 2007

    Thanks for your effort!

    This generally looks good. A few minor things:

    • testMixIntsAndLongs doesn't have any assertions in it.
    • "assert" statements should probably be changed to use the failUnlessEqual/assertEqual methods.
    • I'm wary of your assertLastCallWas system; I'd feel more comfortable if you were making assertions about the entire call chain, not just its last item. Also, something like callLst feels strange as a global. Feel free to contact me to discuss this off-list.

    @jyzude
    Copy link
    Mannequin Author

    jyzude mannequin commented Mar 6, 2007

    Hi collin,

    • I improved testMixIntsAndLongs. It now asserts things
    • assert is banished, replaced by the correct calls
    • the reason why callLst is global is because I have to track calls to __getitem__ in some cases. Because of this, if I put callLst on the object I end up with horrible recursive loops, or at the very least the last call on the stack will always be __getitem__ when I get the list to inspect.
    • assertLastCall only checks the last thing on the list because generally the thing called before that is always __getitem__ or associated magic. I don't want my tests to be bound to the internals of __getitem__. All I care about is that ultimately the right function was called. That said, I modified assertLastCallWas to erase the callLst to prevent any possible bleed-over from the previous test.

    Let me know if you have further suggestions.
    File Added: test_class.patch

    @birkenfeld
    Copy link
    Member

    Note that you don't need the global statement for callLst as you aren't rebinding it in the function.

    @jyzude
    Copy link
    Mannequin Author

    jyzude mannequin commented Mar 7, 2007

    Removed unnecessary global statement in trackCall.

    Anything else? :-)
    File Added: test_class.patch

    @collinwinter
    Copy link
    Mannequin

    collinwinter mannequin commented Mar 11, 2007

    Mike, I've tweaked your refactoring some: strengthened the assertions in testMixIntsAndLongs and testDel; code cleanup in testBadTypeReturned; removed Jython-related code; reduced line lengths to < 80; changed a few print statements to self.fail() calls. Look over the version I've uploaded and tell me what you think.
    File Added: test_class.py.diff

    @jyzude
    Copy link
    Mannequin Author

    jyzude mannequin commented Mar 19, 2007

    Hi Collin,

    Sorry for the delay. Your tweaks look very good. I don't see any problems.

    @jerryseutter
    Copy link
    Mannequin

    jerryseutter mannequin commented Apr 18, 2007

    This patch looks good to me.

    @nnorwitz
    Copy link
    Mannequin

    nnorwitz mannequin commented Apr 19, 2007

    There should be a test_main method otherwise this won't work when called from regrtest.

    Have you tried to run this with regrtest -R :: ? I think it will work, but just wanted to be sure.

    One thing that might be nice to add to this test is to verify the change in the length of the callLst since assertLastCallWas() was called. Typically there should be only one method call from what I saw in the test. However, if we screw something up and there are two method calls, this test could catch that. It would be an enhancement (ie new feature) over the existing test.

    @jyzude
    Copy link
    Mannequin Author

    jyzude mannequin commented Apr 24, 2007

    Hi all,

    Good catch Neal, I needed a test_main method.

    I also finally got around to tightening up the tests so that at all times the entire call stack is tested. It's a bit messier than before and somewhat brittle, but it's thorough and checks every ugly implicit call to __coerce__ just like the old tests did.

    Only problem is that now the tests fail. The test of the statement "1 == testme" on line 419 generates a call stack that seems very strange and I can't figure out what it means. It might be a bug in the python interpreter... or a feature... or just a mistake in the test. At this point I can't figure it out but I'll post my patch so far. If anyone can figure out what's going on please let me know!
    File Added: test_class.patch

    @birkenfeld
    Copy link
    Member

    I've figured out the problem with your call stack: the comparison of
    callLst with the expected calls adds more calls to callLst, leading to a
    failing comparison.

    I've fixed this, but the new test still fails with regrtest -R:: -- will
    investigate.

    @birkenfeld
    Copy link
    Member

    Argh, the test modified the state of one of its classes.

    Fixed that and committed now as rev. 57409.

    @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
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant