Message353793
@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? |
|
Date |
User |
Action |
Args |
2019-10-03 00:47:56 | karlcow | set | recipients:
+ karlcow, mark.dickinson, r.david.murray, python-dev, zach.ware, serhiy.storchaka, Julian.Gindi, ajitesh.gupta |
2019-10-03 00:47:56 | karlcow | set | messageid: <1570063676.44.0.221790770593.issue19683@roundup.psfhosted.org> |
2019-10-03 00:47:56 | karlcow | link | issue19683 messages |
2019-10-03 00:47:54 | karlcow | create | |
|