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

Clean up/refactor/make discoverable test_decimal #64207

Open
zware opened this issue Dec 17, 2013 · 6 comments
Open

Clean up/refactor/make discoverable test_decimal #64207

zware opened this issue Dec 17, 2013 · 6 comments
Labels
tests Tests in the Lib/test dir type-feature A feature request or enhancement

Comments

@zware
Copy link
Member

zware commented Dec 17, 2013

BPO 20008
Nosy @rhettinger, @facundobatista, @mdickinson, @ezio-melotti, @skrah, @zware, @serhiy-storchaka
Files
  • test_decimal_cleanup.diff
  • 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 = None
    created_at = <Date 2013-12-17.21:24:31.840>
    labels = ['type-feature', 'tests']
    title = 'Clean up/refactor/make discoverable test_decimal'
    updated_at = <Date 2016-01-02.14:08:02.924>
    user = 'https://github.com/zware'

    bugs.python.org fields:

    activity = <Date 2016-01-02.14:08:02.924>
    actor = 'serhiy.storchaka'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Tests']
    creation = <Date 2013-12-17.21:24:31.840>
    creator = 'zach.ware'
    dependencies = []
    files = ['33182']
    hgrepos = []
    issue_num = 20008
    keywords = ['patch', 'needs review']
    message_count = 6.0
    messages = ['206482', '206507', '206546', '247469', '257328', '257346']
    nosy_count = 7.0
    nosy_names = ['rhettinger', 'facundobatista', 'mark.dickinson', 'ezio.melotti', 'skrah', 'zach.ware', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue20008'
    versions = ['Python 3.3', 'Python 3.4']

    @zware
    Copy link
    Member Author

    zware commented Dec 17, 2013

    This patch makes extensive changes to test_decimal, with the ultimate goal of making python -m unittest discover [Lib/test/](https://github.com/python/cpython/blob/main/Lib/test) "test_*.py" not choke on test_decimal (see bpo-16748). Trying to do so uncovered a few other issues, such as some tests not properly cleaning up the context.

    Here's a (non-exhaustive) list of what the patch will do:

    • Clean up imports, including a repeated import of warnings
    • Create a new hierarchy of TestCase subclasses
      • BaseTestCase is an empty subclass of unittest.TestCase to serve
        as a base class for all tests that are meant to test both
        implementations. This makes it easy to find such tests and
        create the implementation-specific test cases programmatically.
      • DecimalTest defines some methods for all tests:
        • setUp and tearDown, which ensure that the context is set up
          properly and cleaned up properly. A test that changes the
          context is marked as a failure in tearDown. These take the
          place of the toplevel init(module) function.
        • assertSignals, formerly toplevel assert_signals. It has
          also been enhanced to provide the current context if no
          context is given, and to accept strings as signals (which
          are then looked up on the current decimal module to get the
          real exception)
        • assertAndClearFlags, which is a shortcut for
          assertSignals('flags' ...) and clears the flags when done.
          Several tests made no changes to the context except for
          flags, and this was a quick, easy, and convenient way to
          confirm behavior and clean up.
      • CDecimalTest and PyDecimalTest, subclasses of DecimalTest which
        can be inherited from directly by tests that are meant for a
        single implementation (such as C/PyWhitebox, C/PyFunctionality),
        and are inherited by the generated subclasses of the base classes
        (IBMTestCases, FormatTest, etc.)
    • Do away with the 'skip_expected' global, use a decorator to skip
      IBMTestCases if the test data can't be found.
    • Clean up other toplevel setup code a bit.
    • Make vertical spacing more consistent throughout the module.
    • Move a couple of tests into with localcontext() blocks to avoid
      context pollution.
    • Decorate all of CFunctionality with @requires_extra_functionality
      instead of each individual test
    • Remove the explicit listing of test classes.
    • Remove test_main().
    • Add a Doctests base class which runs the doctests via
      doctest.testmod() and expects a certain number of tests to have been
      run. Attempts to use doctest.DocTestSuite were stymied by the
      two-headed nature of decimal and _decimal, and would have required a
      load_tests function to load them anyway.
    • Add tearDownModule, which restores the original contexts and checks
      to make sure sys.modules['decimal'] is as expected.
    • Convert if __name__ == '__main__' argument handling from optparse
      to argparse.
      • Allow arguments to be passed through to unittest.main().
      • The old method of specifying IBM test case names now requires a
        '--test' or '-t' switch before each set of names (more than one
        -t switch can be present, but each must have at least one name
        following).
      • Add an '--extended'/'-e' switch for switching
        'EXTENDEDERRORTEST', which formerly required editing the file.
    • Use unittest.main() for running the script directly.

    With the patch test_decimal can be run successfully: directly, by regrtest, or by unittest discovery in Lib/test/; with any acceptable combination of arguments when run directly; with or without _decimal; with or without -OO. I have not yet been able to test -DEXTRA_FUNCTIONALITY, --without-docstrings, or on any platform but Windows, but I don't expect any issues (of course :).

    The patch is against default; 3.3 requires the removal of a usage of subTest and a tweaking of the Doctest expected results.

    Any review will be very much appreciated!

    Thanks,

    Zach

    @zware zware added tests Tests in the Lib/test dir type-feature A feature request or enhancement labels Dec 17, 2013
    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Dec 18, 2013

    Assigning to myself, since my private 100% coverage test suite
    would have to be updated as well.

    I have just glanced at the patch and only have some superficial
    remarks:

    In any case, I would prefer a patch without stylistic changes. Elimination of "return" is a personal preference, so are the
    whitespace changes.

    Listing the tests explicitly is sometimes helpful, e.g. one can
    comment out tests when tracking down a refleak.

    Curiously enough, testing with a "polluted" context is also useful,
    since all functions must handle contexts with flags that are already
    set.

    Finally, I *suspect* no one is using the command line arguments any
    more. They were probably used heavily during the design phase of
    decimal.py.

    @skrah skrah mannequin self-assigned this Dec 18, 2013
    @zware
    Copy link
    Member Author

    zware commented Dec 18, 2013

    Stefan Krah wrote:

    In any case, I would prefer a patch without stylistic changes.
    Elimination of "return" is a personal preference, so are the
    whitespace changes.

    The returns were removed in bpo-19572, to reduce false positives in searching for silently skipped tests. I have no strong opinion on them beyond that; if you want them back I can put them back :)

    The whitespace removals are just to make the test module consistent with itself, and I personally think it looks better with the removals. I can put the blank lines back too if need be. I don't think there are any stylistic changes that aren't also code changes beyond the blank lines removed.

    Listing the tests explicitly is sometimes helpful, e.g. one can
    comment out tests when tracking down a refleak.

    The same can be accomplished by either deleting the test case after it is created (which wouldn't take much more editing of the file than commenting it out), or by using e.g. python -m test.test_decimal CFormatTest CWhitebox PyDoctests to run a particular set of tests without having to edit the file at all. It would also be simple enough to add another command line switch to only run the tests on the specified implementation.

    Curiously enough, testing with a "polluted" context is also useful,
    since all functions must handle contexts with flags that are already
    set.

    True, but the real problem I ran into was tests changing the precision of the context, not changing it back, and the next test expecting the original precision (or a test expects a changed precision but gets the original). I'm fine with losing all of the new 'assertAndClearFlags' calls and the checking of flags (and even traps) in tearDown, but the precision changes cause headaches dependent upon test execution order. In fact, the only reason the tests pass right now is because of their ordering; if you random.shuffle(test_classes) before run_unittest(*test_classes), a few different failures pop up.

    Another option would be to add another command line option to set random flags before each test.

    (Note that I'm not terribly attached to either of the new command line options I've suggested in this message. They're just ideas I've had while thinking about your message, and have an idea how either could be implemented pretty easily if anyone else thought it was worthwhile since the command line parsing structure is already there. I don't plan to do anything with them without agreement from others.)

    Finally, I *suspect* no one is using the command line arguments any
    more. They were probably used heavily during the design phase of
    decimal.py.

    That may well be. It doesn't hurt anything to keep them, but it would make the patch quite a bit simpler to remove them. It might be useful in future though, and doesn't add that much more complexity to the module.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Jul 27, 2015

    Unassigning, since I've no time for reviewing (I think the patch
    needs review though, either by Mark or Raymond).

    @skrah skrah mannequin removed their assignment Jul 27, 2015
    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Jan 2, 2016

    Is this a political nosying?

    @serhiy-storchaka
    Copy link
    Member

    serhiy-storchaka commented Jan 2, 2016

    Added comments on Rietveld. The patch is slightly outdated, so I can't test it.

    @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-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants