[issue19683] test_minidom has many empty tests

[karl]

Change by karl :


--
pull_requests: +22980
stage: needs patch -> patch review
pull_request: https://github.com/python/cpython/pull/24152

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[karl]

karl  added the comment:

Ah no. They ARE used

through defproperty and minicompat.py

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[karl]

karl  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[karl]

karl  added the comment:

@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("")
el = dom.documentElement
el.setAttribute("spam", "jam2")
self.confirm(el.toxml() == '', "testAAA")


testAAA is not specifically helping. :)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[Mark Dickinson]

Change by Mark Dickinson :


--
nosy:  -mark.dickinson

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[karl]

karl  added the comment:

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/

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[karl]

karl  added the comment:

@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("")
>>> el = dom.documentElement
>>> el.setAttribute("def", "ghi")
>>> el.setAttribute("jkl", "mno")
>>> el.attributes

```

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?

--
nosy: +karlcow

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[Roundup Robot]

Roundup Robot added the comment:

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19683
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[Mark Dickinson]

Mark Dickinson added the comment:

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

--
nosy: +mark.dickinson

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19683
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[Zachary Ware]

Zachary Ware added the comment:

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19683
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[Julian Gindi]

Julian Gindi added the comment:

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19683
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[Julian Gindi]

Julian Gindi added the comment:

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.

--
Added file: http://bugs.python.org/file33258/issue19683.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19683
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[Roundup Robot]

Roundup Robot added the comment:

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

--
nosy: +python-dev

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19683
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[Zachary Ware]

Zachary Ware added the comment:

Thanks for the removal patch, Ajitesh!

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19683
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[Julian Gindi]

Julian Gindi added the comment:

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19683
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[Zachary Ware]

Zachary Ware added the comment:

Alright, sounds good.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19683
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[Zachary Ware]

Changes by Zachary Ware zachary.w...@gmail.com:


--
versions: +Python 3.4 -Python 2.7

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19683
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[Julian Gindi]

Julian Gindi added the comment:

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

--
nosy: +Julian.Gindi

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19683
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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.

--
nosy: +serhiy.storchaka

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19683
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[R. David Murray]

R. David Murray added the comment:

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19683
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[Ajitesh Gupta]

Ajitesh Gupta added the comment:

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

--
keywords: +patch
nosy: +ajitesh.gupta
Added file: http://bugs.python.org/file32901/issue19683.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19683
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[Zachary Ware]

New submission from Zachary Ware:

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.

--
components: Tests
keywords: easy
messages: 203637
nosy: zach.ware
priority: normal
severity: normal
stage: needs patch
status: open
title: test_minidom has many empty tests
type: enhancement
versions: Python 2.7

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19683
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[R. David Murray]

R. David Murray added the comment:

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

--
nosy: +r.david.murray

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19683
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19683] test_minidom has many empty tests

[Zachary Ware]

Zachary Ware added the comment:

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19683
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com