classification
Title: Rewrite ElementTree tests in a cleaner and safer way
Type: behavior Stage: resolved
Components: Tests Versions: Python 3.3, Python 3.4, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: eli.bendersky Nosy List: Arfrever, asvetlov, danielsh, eli.bendersky, ezio.melotti, flox, ncoghlan, python-dev, serhiy.storchaka, tshepang
Priority: normal Keywords: patch

Created on 2012-06-16 03:52 by eli.bendersky, last changed 2017-04-02 14:26 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
test_xml_etree.patch serhiy.storchaka, 2013-02-25 11:17 review
Pull Requests
URL Status Linked Edit
PR 906 merged serhiy.storchaka, 2017-03-30 14:49
Messages (14)
msg162952 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2012-06-16 03:52
As #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.
msg179801 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2013-01-12 14:39
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.
msg179815 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-01-12 15:45
New changeset f9d1d120c19e by Eli Bendersky in branch '3.3':
Issues #15083 and #16992: port find.* method tests to unittest
http://hg.python.org/cpython/rev/f9d1d120c19e

New changeset 18b16104166c by Eli Bendersky in branch 'default':
Issues #15083 and #16992: port find.* method tests to unittest
http://hg.python.org/cpython/rev/18b16104166c
msg182929 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-25 11:17
Here is a patch which converts all etree doctests to unittests.
msg182937 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2013-02-25 13:59
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?
msg182942 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-25 14:11
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.
msg182944 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2013-02-25 14:26
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.
msg182947 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-02-25 15:23
New changeset af570205b978 by Serhiy Storchaka in branch '3.3':
Issue #15083: Convert ElementTree doctests to unittests.
http://hg.python.org/cpython/rev/af570205b978

New changeset 5eefc85b8be8 by Serhiy Storchaka in branch 'default':
Issue #15083: Convert ElementTree doctests to unittests.
http://hg.python.org/cpython/rev/5eefc85b8be8
msg183058 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2013-02-26 14:04
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.
msg183110 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2013-02-27 04:08
> 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?
msg183111 - (view) Author: Eli Bendersky (eli.bendersky) * (Python committer) Date: 2013-02-27 04:40
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 #15083 and AFAIR Eric's and Brett's
proposals move these modules away from the global namespace.
msg183112 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2013-02-27 05:12
> 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.
msg290844 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-30 14:55
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.
msg291037 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-04-02 13:55
New changeset 68903b656d4e1011525a46cbd1338c6cbab83d6d by Serhiy Storchaka in branch '2.7':
bpo-15083: Convert ElementTree doctests to unittests. (#906)
https://github.com/python/cpython/commit/68903b656d4e1011525a46cbd1338c6cbab83d6d
History
Date User Action Args
2017-04-02 14:26:36serhiy.storchakasetstatus: open -> closed
stage: patch review -> resolved
resolution: fixed
versions: + Python 2.7
2017-04-02 13:55:45serhiy.storchakasetmessages: + msg291037
2017-03-30 15:03:26serhiy.storchakalinkissue29948 dependencies
2017-03-30 14:56:47serhiy.storchakalinkissue27863 dependencies
2017-03-30 14:55:08serhiy.storchakasetmessages: + msg290844
2017-03-30 14:49:14serhiy.storchakasetpull_requests: + pull_request806
2013-02-27 05:12:30ezio.melottisetmessages: + msg183112
2013-02-27 04:40:59eli.benderskysetmessages: + msg183111
2013-02-27 04:08:52ezio.melottisetmessages: + msg183110
2013-02-26 14:04:06eli.benderskysetmessages: + msg183058
2013-02-25 15:23:11python-devsetmessages: + msg182947
2013-02-25 14:26:52eli.benderskysetversions: + Python 3.3
2013-02-25 14:26:46eli.benderskysetmessages: + msg182944
2013-02-25 14:11:16serhiy.storchakasetmessages: + msg182942
2013-02-25 13:59:09eli.benderskysetmessages: + msg182937
2013-02-25 11:17:38serhiy.storchakasetfiles: + test_xml_etree.patch

nosy: + serhiy.storchaka
messages: + msg182929

keywords: + patch
stage: needs patch -> patch review
2013-01-12 15:45:10python-devsetnosy: + python-dev
messages: + msg179815
2013-01-12 14:39:23eli.benderskysetmessages: + msg179801
2012-12-30 17:05:38asvetlovsetnosy: + asvetlov
2012-12-30 00:22:24Arfreversetnosy: + Arfrever
2012-12-29 16:20:13danielshsetnosy: + danielsh
2012-07-15 03:45:42eli.benderskysetversions: - Python 3.3
2012-06-22 18:28:12tshepangsetnosy: + tshepang
2012-06-22 16:09:46ezio.melottisetnosy: + ezio.melotti
2012-06-16 03:53:16eli.benderskylinkissue15075 superseder
2012-06-16 03:52:39eli.benderskycreate