This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: test_minidom has many empty tests
Type: enhancement Stage: patch review
Components: Tests Versions: Python 3.4
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Julian.Gindi, ajitesh.gupta, karlcow, 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 2022-04-11 14:57 by admin.

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
Pull Requests
URL Status Linked Edit
PR 24152 open karlcow, 2021-01-07 13:48
Messages (21)
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) (Python triager) 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) (Python triager) 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.
msg353793 - (view) Author: karl (karlcow) * Date: 2019-10-03 00:47
@zach.ware
@r.david.murray


So I was looking at that issue. There is a lot of work. 

I had a couple of questions, because there are different categories


# Empty tests for existing functions.

This seems to be straightforward as they would complete the module.

Example:
```python
def testGetAttributeNode(self): pass
```
https://github.com/python/cpython/blob/3e04cd268ee9a57f95dc78d8974b21a6fac3f666/Lib/test/test_minidom.py#L412

which refers to: `GetAttributeNode`
https://github.com/python/cpython/blob/3e04cd268ee9a57f95dc78d8974b21a6fac3f666/Lib/xml/dom/minidom.py#L765-L768
https://github.com/python/cpython/blob/3e04cd268ee9a57f95dc78d8974b21a6fac3f666/Lib/test/test_minidom.py#L285-L294



# Tests without any logical reference in the module.

This is puzzling because I'm not sure which DOM feature they should be testing.

For example:

```
    def testGetAttrList(self):
        pass
```

https://github.com/python/cpython/blob/3e04cd268ee9a57f95dc78d8974b21a6fac3f666/Lib/test/test_minidom.py#L383-L384


Or maybe this is just supposed to test Element.attributes returning a list of attributes, such as 
`NamedNodeMap [ def="ghi", jkl="mno"]` returned by a browser.


```
>>> import xml.dom.minidom
>>> from xml.dom.minidom import parse, Node, Document, parseString
>>> from xml.dom.minidom import getDOMImplementation
>>> dom = parseString("<abc/>")
>>> el = dom.documentElement
>>> el.setAttribute("def", "ghi")
>>> el.setAttribute("jkl", "mno")
>>> el.attributes
<xml.dom.minidom.NamedNodeMap object at 0x10b6d6780>
```

or is it supposed to test something like 

```
>>> el.attributes.items()
[('def', 'ghi'), ('jkl', 'mno')]
```

This is slightly confusing. And the missing docstrings are not making it easier.



# Tests which do not really test the module(?)

I think for example about this, which is testing that `del` is working, but it doesn't have anything to do with the DOM. 

```
    def testDeleteAttr(self):
        dom = Document()
        child = dom.appendChild(dom.createElement("abc"))

        self.confirm(len(child.attributes) == 0)
        child.setAttribute("def", "ghi")
        self.confirm(len(child.attributes) == 1)
        del child.attributes["def"]
        self.confirm(len(child.attributes) == 0)
        dom.unlink()
```

https://github.com/python/cpython/blob/3e04cd268ee9a57f95dc78d8974b21a6fac3f666/Lib/test/test_minidom.py#L285-L294

Specifically when there is a function for it: `removeAttribute`
https://www.w3.org/TR/2000/REC-DOM-Level-2-Core-20001113/core.html#ID-6D6AC0F9 

which is tested just below that test.
https://github.com/python/cpython/blob/3e04cd268ee9a57f95dc78d8974b21a6fac3f666/Lib/test/test_minidom.py#L296-L305

so I guess these should be removed or do I miss something in the testing logic?




# Missing docstrings.

Both the testing module and the module lack a lot of docstrings.
Would it be good to fix this too, probably in a separate commit.



# DOM Level 2

So the module intent is to implement DOM Level 2.
but does that make sense in the light of 
https://dom.spec.whatwg.org/

Should minidom tries to follow the current DOM spec?
msg353835 - (view) Author: karl (karlcow) * Date: 2019-10-03 09:29
err… Errata on my previous comment.

"""
Simple implementation of the Level 1 DOM.

Namespaces and other minor Level 2 features are also supported.
"""
https://github.com/python/cpython/blob/c65119d5bfded03f80a9805889391b66fa7bf551/Lib/xml/dom/minidom.py#L1-L3


https://www.w3.org/TR/REC-DOM-Level-1/
msg384561 - (view) Author: karl (karlcow) * Date: 2021-01-07 05:55
@zach.ware 
@r.david.murray

I'm going through the source currently. 

I see that the test file is using:

class MinidomTest(unittest.TestCase):
    def confirm(self, test, testname = "Test"):
        self.assertTrue(test, testname)


Is there a specific reason to use this form instead of just directly using self.assertEqual or similar forms for new tests or reorganizing some of the tests. 


I see that it is used for example for giving a testname but

    def testAAA(self):
        dom = parseString("<abc/>")
        el = dom.documentElement
        el.setAttribute("spam", "jam2")
        self.confirm(el.toxml() == '<abc spam="jam2"/>', "testAAA")


testAAA is not specifically helping. :)
msg384575 - (view) Author: karl (karlcow) * Date: 2021-01-07 11:08
These methods are not used anywhere in the code. 

https://github.com/python/cpython/blob/5c30145afb6053998e3518befff638d207047f00/Lib/xml/dom/minidom.py#L71-L80

What was the purpose when they were created… hmm maybe blame would give clue. Ah they were added a long time ago

https://github.com/python/cpython/commit/73678dac48e5858e40cba6d526970cba7e7c769c#diff-365c30899ded02b18a2d8f92de47af6ca213eefe7883064c8723598da600ea42R83-R88

but never used? or was it in the spirit to reserve the keyword for future use?
https://developer.mozilla.org/en-US/docs/Web/API/Node/firstChild
msg384576 - (view) Author: karl (karlcow) * Date: 2021-01-07 11:12
Ah no. They ARE used

through defproperty and minicompat.py

get = getattr(klass, ("_get_" + name))
History
Date User Action Args
2022-04-11 14:57:53adminsetgithub: 63882
2021-01-07 13:48:32karlcowsetstage: needs patch -> patch review
pull_requests: + pull_request22980
2021-01-07 11:12:55karlcowsetmessages: + msg384576
2021-01-07 11:08:12karlcowsetmessages: + msg384575
2021-01-07 05:55:30karlcowsetmessages: + msg384561
2020-02-03 21:37:42mark.dickinsonsetnosy: - mark.dickinson
2019-10-03 09:29:12karlcowsetmessages: + msg353835
2019-10-03 00:47:56karlcowsetnosy: + karlcow
messages: + msg353793
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