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.

Well, we generally don't backport tests unless we are fixing related bugs.  I think this should be left alone.

In that case, the empty tests should be removed, since they currently report success on doing nothing.

I have attached a patch. I deleted all the empty tests.

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.

On tip it would indeed be better to implement them.  The deletion is only for the released branches.

I agree that they should be implemented. I'll fill them in and submit a patch in a day or so.

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

Thanks for the removal patch, Ajitesh!

Julian, are you still working on implementing the tests on default?

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.

Alright, sounds good.

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.

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).

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 :)

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

Whoops; wrong issue number.  That commit message was for issue 19863.

@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?

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/

@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. :)

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

Ah no. They ARE used

through defproperty and minicompat.py

get = getattr(klass, ("_get_" + name))