test_minidom has many empty tests

[zware]

BPO	19683
Nosy	@bitdancer, @karlcow, @zware, @serhiy-storchaka
PRs	gh-63882: Adds tests for xml.dom.minidom #24152
Files	issue19683.patch issue19683.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Edit by TJR: The two patch files, by two different people deleted empty tests. Some version was merged to Python maintenance versions 2.7 and 3.3 (then on Mercurial) in Dec 2013: #63882 (comment). At that time, 3.4.0, released in March 2014, was tip. PR 24152 added docstrings to minidom.py, removed the empty tests, and revised existing tests. The author closed it due to lack of time when requested to split it into multiple PRs.

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2013-11-21.15:17:39.634>
labels = ['easy', 'type-feature', 'tests']
title = 'test_minidom has many empty tests'
updated_at = <Date 2021-01-07.13:48:32.590>
user = 'https://github.com/zware'

bugs.python.org fields:

activity = <Date 2021-01-07.13:48:32.590>
actor = 'karlcow'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Tests']
creation = <Date 2013-11-21.15:17:39.634>
creator = 'zach.ware'
dependencies = []
files = ['32901', '33258']
hgrepos = []
issue_num = 19683
keywords = ['patch', 'easy']
message_count = 21.0
messages = ['203637', '203642', '203643', '204781', '204806', '204834', '205274', '206637', '206639', '206641', '206642', '206873', '206939', '206979', '209897', '209899', '353793', '353835', '384561', '384575', '384576']
nosy_count = 7.0
nosy_names = ['r.david.murray', 'karlcow', 'python-dev', 'zach.ware', 'serhiy.storchaka', 'Julian.Gindi', 'ajitesh.gupta']
pr_nums = ['24152']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue19683'
versions = ['Python 3.4']

Linked PRs

• gh-128508: Add some docstrings to xml.dom.minidom #128477

[zware]

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.

[bitdancer]

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

[zware]

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

[ajiteshgupta]

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

[serhiy-storchaka]

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.

[bitdancer]

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

[JulianGindi]

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

[python-dev]

New changeset 2737c0e7ba71 by Zachary Ware in branch '2.7':
Issue bpo-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 bpo-19683: Removed empty tests from test_minidom. Patch by Ajitesh Gupta.
http://hg.python.org/cpython/rev/5e510117b71a

[zware]

Thanks for the removal patch, Ajitesh!

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

[JulianGindi]

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.

[zware]

Alright, sounds good.

[JulianGindi]

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.

[zware]

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

[JulianGindi]

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

[karlcow]

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

def testGetAttributeNode(self): pass

cpython/Lib/test/test_minidom.py

Line 412 in 3e04cd2

def testGetAttributeNode(self): pass

which refers to: GetAttributeNode

cpython/Lib/xml/dom/minidom.py

Lines 765 to 768 in 3e04cd2

	def getAttributeNode(self, attrname):
	if self._attrs is None:
	return None
	return self._attrs.get(attrname)

cpython/Lib/test/test_minidom.py

Lines 285 to 294 in 3e04cd2

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

# 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

cpython/Lib/test/test_minidom.py

Lines 383 to 384 in 3e04cd2

	def testGetAttrList(self):
	pass

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

cpython/Lib/test/test_minidom.py

Lines 285 to 294 in 3e04cd2

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

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.

cpython/Lib/test/test_minidom.py

Lines 296 to 305 in 3e04cd2

	def testRemoveAttr(self):
	dom = Document()
	child = dom.appendChild(dom.createElement("abc"))
	child.setAttribute("def", "ghi")
	self.confirm(len(child.attributes) == 1)
	self.assertRaises(xml.dom.NotFoundErr, child.removeAttribute, "foo")
	child.removeAttribute("def")
	self.confirm(len(child.attributes) == 0)
	dom.unlink()

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?

[karlcow]

err… Errata on my previous comment.

"""
Simple implementation of the Level 1 DOM.

Namespaces and other minor Level 2 features are also supported.
"""

cpython/Lib/xml/dom/minidom.py

Lines 1 to 3 in c65119d

	"""Simple implementation of the Level 1 DOM.
	Namespaces and other minor Level 2 features are also supported.

https://www.w3.org/TR/REC-DOM-Level-1/

[karlcow]

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

[karlcow]

These methods are not used anywhere in the code.
(EDIT by TJR: Untrue; see next comment)

cpython/Lib/xml/dom/minidom.py

Lines 71 to 80 in 5c30145

	def _get_childNodes(self):
	return self.childNodes
	def _get_firstChild(self):
	if self.childNodes:
	return self.childNodes[0]
	def _get_lastChild(self):
	if self.childNodes:
	return self.childNodes[-1]

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

73678da#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

[karlcow]

Ah no. They ARE used

through defproperty and minicompat.py

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

[terryjreedy]

This was initially about backporting tests to 2.7, then about removing empty tests in 3.3, and then about implementing empty tests in main (then 3.4). And then also about revising existing tests and adding docstrings to minidom.py itself. That has not happened and indeed is way too much for one PR and even one issue. I think the way forward is:

1. Add docstrings.
2. Comment out empty tests so not counted as successes and listed in verbose output.
3. Revise and unfactor existing tests - likely in multiple PRs.
4. Uncomment and implement missing tests in revised style - almost certainly in multiple PRs.

The existing PR does some of 1, 3, and 4 and is too much to review all at once and hence got bogged down and the works sits unused.

[srinivasreddy]

@terryjreedy I want to take this ticket to finishing line.
Do you accept the PRs like it was said in the above comment? Shall we keep this one issue, and raise multiple PRs? Or shall i create multiple issues too?

[terryjreedy]

Because the empty tests were neither deleted nor replaced in 3.4 or since, they are still in 3.14, resulting in bogus successes. I want to finish this issue with #2 above, a PR to comment out the empty tests with '##' added to the beginning of their lines. (PR #24152 deleted them, without replacement.)

I opened issue #128508 for minidom docstrings (#1 above) and repointed the docstring PR to that issue, and added a review.

Test revisions (#3 above) should be another new issue. In a new PR, MinidomTest should only have the module function tests. Tests for class methods should be put in other TestCase classes (one for each class with tests). It might be good to rearrange the tests first. I will say more later.