classification
Title: test_minidom has many empty tests
Type: enhancement Stage: needs patch
Components: Tests Versions: Python 3.4
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Julian.Gindi, ajitesh.gupta, mark.dickinson, python-dev, r.david.murray, serhiy.storchaka, zach.ware
Priority: normal Keywords: easy, patch

Created on 2013-11-21 15:17 by zach.ware, last changed 2014-02-01 16:34 by mark.dickinson.

Files
File name Uploaded Description Edit
issue19683.patch ajitesh.gupta, 2013-11-30 07:30 review
issue19683.patch Julian.Gindi, 2013-12-23 21:28 review
Messages (16)
msg203637 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-11-21 15:17
Many of the tests in test_minidom on 2.7 are empty, that is they are defined as "def testSomeFunction(self): pass".

I've marked this issue as "easy" since I suspect a lot of the tests can be either backported from 3.x, or removed if they don't exist in 3.x.
msg203642 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-11-21 15:41
Well, we generally don't backport tests unless we are fixing related bugs.  I think this should be left alone.
msg203643 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-11-21 15:43
In that case, the empty tests should be removed, since they currently report success on doing nothing.
msg204781 - (view) Author: Ajitesh Gupta (ajitesh.gupta) * Date: 2013-11-30 07:30
I have attached a patch. I deleted all the empty tests.
msg204806 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-11-30 12:24
Rather than removing these empty tests it will be better implement them. Otherwise we can accidentally break the code.

I see a lot of empty tests on 3.x.
msg204834 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-11-30 18:45
On tip it would indeed be better to implement them.  The deletion is only for the released branches.
msg205274 - (view) Author: Julian Gindi (Julian.Gindi) * Date: 2013-12-05 05:26
I agree that they should be implemented. I'll fill them in and submit a patch in a day or so.
msg206637 - (view) Author: Roundup Robot (python-dev) Date: 2013-12-19 19:47
New changeset 2737c0e7ba71 by Zachary Ware in branch '2.7':
Issue #19683: Removed empty tests from test_minidom.
http://hg.python.org/cpython/rev/2737c0e7ba71

New changeset 5e510117b71a by Zachary Ware in branch '3.3':
Issue #19683: Removed empty tests from test_minidom.  Patch by Ajitesh Gupta.
http://hg.python.org/cpython/rev/5e510117b71a
msg206639 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-12-19 19:55
Thanks for the removal patch, Ajitesh!

Julian, are you still working on implementing the tests on default?
msg206641 - (view) Author: Julian Gindi (Julian.Gindi) * Date: 2013-12-19 19:57
I have not started yet, wasn't completely sure of the status of this. I'll get going filling in those tests to the best of my ability.
msg206642 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-12-19 20:18
Alright, sounds good.
msg206873 - (view) Author: Julian Gindi (Julian.Gindi) * Date: 2013-12-23 21:28
So, it seems that there are many seemingly redundant tests and many tests whose intentions are unclear. I think this might be better suited for someone who has more experience with the xml minidom module. I have uploaded the work I have done but it is not complete.
msg206939 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-12-26 15:03
Some refactoring of the tests is certainly acceptable.  If there are some tests that can be merged together, it is fine to do so; also for removing ones that don't make any sense (it's not like they've ever tested anything anyway :)).  We don't have anyone listed as an expert on xml.dom.minidom (or the xml package as a whole), so we kind of have to just muddle along on our own with this.  Any tests you come up with to fill the empty ones will be better than no tests at all.  If someone with more experience with minidom comes along later and improves your tests, that will be great; but considering how long there have been this many empty tests in the file, I don't think that's terribly likely.

In fact, having looked at the test module in a bit more detail, it's in pretty sore need of an overall modernization.  The 'confirm' method is just a thin wrapper around assertTrue with an extremely unhelpful default message, and is used almost exclusively for all tests in the file.  So currently if anything breaks the tests will say "this failed, but I won't tell you why.  Good luck figuring it out!"  'confirm' should be removed, and all of the huge conditions passed into it throughout the file should be converted into individual assert*() calls.  It also looks like we could make use of setUp/tearDown to eliminate a lot of repetition (such as creating a base Document and subsequently removing it).
msg206979 - (view) Author: Julian Gindi (Julian.Gindi) * Date: 2013-12-27 04:04
Modernizing these tests will be a decent undertaking but seems important. I'll go a head and try to fix these up further. Thanks for the guidance and motivation on this one :)
msg209897 - (view) Author: Roundup Robot (python-dev) Date: 2014-02-01 16:32
New changeset fed468670866 by Mark Dickinson in branch '2.7':
Issue #19683: Add __closure__ and other missing attributes to function docs.
http://hg.python.org/cpython/rev/fed468670866
msg209899 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2014-02-01 16:34
Whoops; wrong issue number.  That commit message was for issue 19863.
History
Date User Action Args
2014-02-01 16:34:56mark.dickinsonsetnosy: + mark.dickinson
messages: + msg209899
2014-02-01 16:32:52python-devsetmessages: + msg209897
2013-12-27 04:04:29Julian.Gindisetmessages: + msg206979
2013-12-26 15:03:04zach.waresetmessages: + msg206939
2013-12-23 21:28:18Julian.Gindisetfiles: + issue19683.patch

messages: + msg206873
2013-12-19 20:19:07zach.waresetversions: + Python 3.4, - Python 2.7
2013-12-19 20:18:54zach.waresetmessages: + msg206642
2013-12-19 19:57:19Julian.Gindisetmessages: + msg206641
2013-12-19 19:55:31zach.waresetmessages: + msg206639
2013-12-19 19:47:43python-devsetnosy: + python-dev
messages: + msg206637
2013-12-05 05:26:54Julian.Gindisetnosy: + Julian.Gindi
messages: + msg205274
2013-11-30 18:45:24r.david.murraysetmessages: + msg204834
2013-11-30 12:24:28serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg204806
2013-11-30 07:30:44ajitesh.guptasetfiles: + issue19683.patch

nosy: + ajitesh.gupta
messages: + msg204781

keywords: + patch
2013-11-21 15:43:35zach.waresetmessages: + msg203643
2013-11-21 15:41:12r.david.murraysetnosy: + r.david.murray
messages: + msg203642
2013-11-21 15:17:39zach.warecreate