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

Jump to bottom

Rewrite ElementTree tests in a cleaner and safer way #59288

Closed
elibendersky mannequin opened this issue Jun 16, 2012 · 14 comments
Closed

Rewrite ElementTree tests in a cleaner and safer way #59288

elibendersky mannequin opened this issue Jun 16, 2012 · 14 comments
Labels
tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error

Comments

@elibendersky
Copy link
Mannequin

elibendersky mannequin commented Jun 16, 2012

BPO 15083
Nosy @ncoghlan, @ezio-melotti, @asvetlov, @florentx, @serhiy-storchaka
PRs
  • [2.7] bpo-15083: Convert ElementTree doctests to unittests. #906
    		•
    Files
  • test_xml_etree.patch
    		•

    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 2017-04-02.14:26:36.301>
created_at = <Date 2012-06-16.03:52:39.227>
labels = ['type-bug', 'tests']
title = 'Rewrite ElementTree tests in a cleaner and safer way'
updated_at = <Date 2017-04-02.14:26:36.300>
user = 'https://bugs.python.org/elibendersky'

    bugs.python.org fields:

    activity = <Date 2017-04-02.14:26:36.300>
actor = 'serhiy.storchaka'
assignee = 'eli.bendersky'
closed = True
closed_date = <Date 2017-04-02.14:26:36.301>
closer = 'serhiy.storchaka'
components = ['Tests']
creation = <Date 2012-06-16.03:52:39.227>
creator = 'eli.bendersky'
dependencies = []
files = ['29230']
hgrepos = []
issue_num = 15083
keywords = ['patch']
message_count = 14.0
messages = ['162952', '179801', '179815', '182929', '182937', '182942', '182944', '182947', '183058', '183110', '183111', '183112', '290844', '291037']
nosy_count = 10.0
nosy_names = ['ncoghlan', 'ezio.melotti', 'Arfrever', 'eli.bendersky', 'asvetlov', 'flox', 'tshepang', 'python-dev', 'serhiy.storchaka', 'danielsh']
pr_nums = ['906']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue15083'
versions = ['Python 2.7', 'Python 3.3', 'Python 3.4']
    @elibendersky
    Copy link
    Mannequin Author

    elibendersky mannequin commented Jun 16, 2012

    As bpo-15075 demonstrated, the ET tests are sensitive to execution order because of the way they operate.

    Two sets of tests (one for the C module and one for the pure Python module) operate from the same test code, monkey-patching the imported module. This sometimes breaks. A more robust solution is needed that is completely deterministic and does not rely on import artifacts.

    @elibendersky elibendersky mannequin self-assigned this Jun 16, 2012
    @elibendersky elibendersky mannequin added tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error labels Jun 16, 2012
    @elibendersky
    Copy link
    Mannequin Author

    elibendersky mannequin commented Jan 12, 2013

    It should be noted that the doctests complicate things considerably, and should be rewritten to be unittest, which are easier to manipulate in terms of modules used.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 12, 2013

    New changeset f9d1d120c19e by Eli Bendersky in branch '3.3':
    Issues bpo-15083 and bpo-16992: port find.* method tests to unittest
    http://hg.python.org/cpython/rev/f9d1d120c19e

    New changeset 18b16104166c by Eli Bendersky in branch 'default':
    Issues bpo-15083 and bpo-16992: port find.* method tests to unittest
    http://hg.python.org/cpython/rev/18b16104166c

    @serhiy-storchaka
    Copy link
    Member

    Here is a patch which converts all etree doctests to unittests.

    @elibendersky
    Copy link
    Mannequin Author

    elibendersky mannequin commented Feb 25, 2013

    Serhiy, thanks for working on this! I didn't read the whole patch yet - will tinker with it a bit more when applying. Did you prepare the patch vs. 3.3 or default? The two are still synced and I'd be happy to apply it to both branches.

    Now, the real reason I wanted to get rid of doctests is to make the global environment clean in test_xml_etree. The next logical step is to make all test classes in test_xml_etree accept the ET module in some way and store it, using it to get classes & function. I.e. no more global "ET" and "pyET" at all. Would you like to add this to your patch?

    @serhiy-storchaka
    Copy link
    Member

    This patch is for default branch. It applied to 3.3 too.

    The next logical step is to make all test classes in test_xml_etree accept the ET module in some way and store it, using it to get classes & function. I.e. no more global "ET" and "pyET" at all. Would you like to add this to your patch?

    I was going to do this on the next step, but if you prefer I can prepare a single patch. But it seems to me that more simple patches are easier to review.

    @elibendersky
    Copy link
    Mannequin Author

    elibendersky mannequin commented Feb 25, 2013

    No problems. You can go ahead and commit this patch to 3.3 and default, then. I will review it post-commit, since I wanted to tweak some things around anyway.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 25, 2013

    New changeset af570205b978 by Serhiy Storchaka in branch '3.3':
    Issue bpo-15083: Convert ElementTree doctests to unittests.
    http://hg.python.org/cpython/rev/af570205b978

    New changeset 5eefc85b8be8 by Serhiy Storchaka in branch 'default':
    Issue bpo-15083: Convert ElementTree doctests to unittests.
    http://hg.python.org/cpython/rev/5eefc85b8be8

    @elibendersky
    Copy link
    Mannequin Author

    elibendersky mannequin commented Feb 26, 2013

    Great, thanks.

    Now looking forward to the patch getting rid of the module-level globals. One idea is explicitly pass the module into each testing class. The classes should not rely on anything global in this respect.

    @ezio-melotti
    Copy link
    Member

    The next logical step is to make all test classes in test_xml_etree
    accept the ET module in some way and store it, using it to get classes
    & function. I.e. no more global "ET" and "pyET" at all.

    The idiom suggested by PEP-399 has the two modules (cmod and pymod) as globals, and then simply sets them as class attributes. Is there any reason why this should be avoided in this case?

    @elibendersky
    Copy link
    Mannequin Author

    elibendersky mannequin commented Feb 27, 2013

    On Tue, Feb 26, 2013 at 8:08 PM, Ezio Melotti <report@bugs.python.org>wrote:

    Ezio Melotti added the comment:

    > The next logical step is to make all test classes in test_xml_etree
    > accept the ET module in some way and store it, using it to get classes
    > & function. I.e. no more global "ET" and "pyET" at all.

    The idiom suggested by PEP-399 has the two modules (cmod and pymod) as
    globals, and then simply sets them as class attributes. Is there any
    reason why this should be avoided in this case?

    I'm just not sure how the globals help except for saving 2-3 lines of code.
    I do see how they can cause problems because even when I just want to run
    the pure Python code, the C module gets imported. Why should it be? I
    really don't want it to, I want to isolate things as much as possible
    (after all, testing the pure Python module actually tests a scenario where
    there is no C module). Pickle is one concrete place that can cause problems
    with this.

    We talked about related things in Issue bpo-15083 and AFAIR Eric's and Brett's
    proposals move these modules away from the global namespace.

    @ezio-melotti
    Copy link
    Member

    I'm just not sure how the globals help

    It doesn't, but if it works fine there's probably no reason to complicate (if it's complicated at all) things to change this.

    even when I just want to run the pure Python code, the C module gets
    imported. Why should it be?

    Doesn't the test always run both? (assuming the C module is available -- if it's not it won't be imported anyway)

    Pickle is one concrete place that can cause problems with this.

    This might be an actual reason to avoid globals, but I don't know the details.

    @serhiy-storchaka
    Copy link
    Member

    It is hard to backport bugfixes to 2.7 since tests are too different. Due to this I backported some bugfixes without tests and omitted backporting other bugfixes.

    PR 906 converts doctests in 2.7 to unittests. This will help backporting bugfixes too much.

    Actually I have backported tests from 3.5 and checked that all old tests are present in new tests. Perhaps I found a bug in ElementTree in 2.7. Will open an issue after merging tests.

    @serhiy-storchaka
    Copy link
    Member

    New changeset 68903b6 by Serhiy Storchaka in branch '2.7':
    bpo-15083: Convert ElementTree doctests to unittests. (#906)
    68903b6

    @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
    Assignees
    No one assigned
    Labels
    tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @serhiy-storchaka @ezio-melotti