345700 - Tests for Node.isEqualNode

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jeff Walden [:Waldo]]

Seeing as there exists a spiffy[0] testing framework for content testing, I figured I'd write some tests for the fairly new Node.isEqualNode support in Gecko, which I'll be attaching in a second.

While writing these tests, I've encountered a few issues:

- executing setAttribute("f","g") and setAttributeNS("a","f","g") in different
  orders affects equality, when according to bz the two should be equivalent;
  I speculate without proof that this is bug 312019
- normalization of an element containing only an empty text node doesn't remove
  that text node; I think this is previously unknown, so I filed bug 345666
- Node.isEqualNode(node) does a nullcheck on node and throws if node == null,
  which is arguably correct and incorrect; Safari returns false in this case,
  and I think that behavior's more reasonable, personally (but it would require
  changing some code)

Also, I assume that DOM implementations are not supposed to compare normalized versions of the relevant nodes for this method.  The DOM spec lists as one of the equality criteria:

  The childNodes NodeLists are equal. This is: they are both null, or they have
  the same length and contain equal nodes at the same index. Note that
  normalization can affect equality; to avoid this, nodes should be normalized
  before being compared.

I think this text is ambiguous about whether it is the user's job to normalize when needed or the implementation's job to normalize.  Amusingly enough, a Google search for "isequalnode normalization" returns the following URL as the first result:

http://lists.w3.org/Archives/Public/www-dom/2002JulSep/0062.html

The implementations I found via web searches (Safari appears to be the only browser that currently implements it, and I skimmed the patch[1] and they don't do any normalization) suggest that normalization is the user's job (and this seems to be the case with ours, as I found out the hard way after writing most of the normalization tests expecting otherwise), so the tests reflect that assumption.

The tests themselves range from targeted edge-case testing to basically random "test a bunch of stuff and see whether anything breaks" testing.  We probably need more namespace-related tests here, but I'm not familiar enough with namespace nuances to do a thorough job of testing them.  Suggestions for more tests here are appreciated.


0. Well, except for classinfo, whose absence in xpcshell makes the testing
   framework suck fairly hard
1. http://bugzilla.opendarwin.org/attachment.cgi?id=4485&action=diff

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Testcase and supporting files

I'd really like a sanity check on this, especially on the commented-out XXX tests that I believe are correct or arguably so but which current trunk code fails.

Also, feel free to forward this elsewhere if someone else is better reviewer here.

Comment 2

[Alex Vincent [:WeirdAl]]

Comment on attachment 230410 [details] [diff] [review]
Testcase and supporting files

>+  // the null namespace is equivalent to no namespace, according to doron

That's true in this case only because the null namespace is the default namespace for the element in question.  If there were a xmlns attribute on this element or any parent element, that would not be true.

>+  // XXX according to bz/doron, this test is correct -- we currently fail it
>+  //do_eq_nodes(foo1, foo2);

I'd say this test is valid, and you should file a bug for it.

This is great stuff!

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Updated with failing Node.normalize tests not commented out

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 230819 [details] [diff] [review]
Updated with failing Node.normalize tests not commented out

>Index: content/test/unit/test_isequalnode.js
>+ * Portions created by the Initial Developer are Copyright (C) 2006
>+ * the Initial Developer. All Rights Reserved.

You sure they're not (C) Mozilla Corporation?

>+function test_isEqualNode_setAttribute()
>+  // NOTE: 0, 2 are whitespace
>+  var test1 = doc.getElementById("test_setAttribute");
>+  var foo1 = test1.childNodes.item(1);
>+  var foo2 = test1.childNodes.item(3);

Same comments here as for equality_check_kids() below.  Perhaps we just need a function that returns the first two "real" nodes?

Also, it would be nice to use node1 and node2 instead of foo1 and foo2 for these tests.

>+  do_eq_nodes(foo1, foo2);

I think "check_eq_nodes" would make more sense...

Same for "check_neq_nodes".

>+  // the null namespace is equivalent to no namespace, according to doron

More to the point, this is the case according to section 1.3.3 (XML Namespaces) of DOM 3 Core.

Please ignore what Alex said about this -- he's incorrect.  Using null as the first argument to setAttributeNS means set the node in the null namespace, no matter what the default namespace of the Element involved.

>+  foo2.setAttribute("seamonkey", "rheet");
>+  // XXX according to bz/doron, this test is correct -- we currently fail it

Actually, we were wrong here.  setAttribute("seamonkey", "rheet") sets whatever attribute has the fully qualified name "seamonkey".  Since you didn't use any prefixes in the attribute names in your setAttributeNS calls, there is such an attribute, and its value gets set.  So this should, in fact, be failing as written.

>+function test_isEqualNode_variety()
>+  for (var i = 0; i < nodes.length; i++)
>+  {
>+    for (var j = 0; j < nodes.length; j++)

Since our node comparisons are already symmetric, why not make j start with j = i ?

>+      if (nodes[i] && nodes[j])

How would they be null?  I think you should be able to do_check_neq(nodes[i], null); and similar for nodes[j] here -- if they're null, the test failed.

>+function test_isEqualNode_normalization()
>+{
>+  var norm = doc.getElementById("test_normalization");
>+  var bar1 = norm.childNodes.item(1);
>+  var bar2 = norm.childNodes.item(3);

Again, node1 and node2, and again the same comments on the 1,3 thing.

>+  // Attr.appendChild() isn't implemented yet -- run this code when it is

Comment in the bug on this if there is one pointing to this test?  And also, cite the bug# in the comment in this file.  If there's no bug, file one?

>+  var b2text2a = doc.createTextNode("hydro");
>+  var b2text2b = doc.createTextNode("spanner");

These seem to be unused.

>+function test_isEqualNode_whitespace()

>+  equality_check_kids("test_text1", true);
>+  equality_check_kids("test_text2", false);
>+  equality_check_kids("test_text3", false);

I guess if equality_check_kids is fixed to look for non-whitespace nodes this part would need to be changed...

>+function test_isEqualNode_null()
>+  do_neq_nodes(root, null);

Why bother?  It's in getElementsByTagName("*")....

>+function n(node)  { return node ? node.QueryInterface(I.nsIDOMNode) : null; }

nsIDOMNode is already defined to be I.nsIDOMNode in head_content.js, fwiw.  Similar for nsIDOMElement and nsIDOMAttr.

>+function n3(node) { return node ? node.QueryInterface(I.nsIDOM3Node) : null; }

And we should just add nsIDOM3Node there.

>+// TESTING FUNCTIONS

>+ * Compares the first and third child nodes of the element (to allow whitespace)
>+ * referenced by parentId for isEqualNode equality or inequality based on the
>+ * value of areEqual.

Why not just compare the first two nodes that are not whitespace-only Text nodes?  That seems like it would be more robust.

If you do leave the 1,3 thing hardcoded, at least do checks on the nodes at 0 and 2 to ensure that they're whitespace nodes.

r+sr=bzbarsky with the nits picked.  Thanks a ton for doing this!  It's great to see more of these tests!

Comment 5

[Jeff Walden [:Waldo]]

(In reply to comment #4)
> You sure they're not (C) Mozilla Corporation?

Actually, now that you mention it, they probably are; I hadn't even been thinking about that.


> Same comments here as for equality_check_kids() below.  Perhaps we just need a
> function that returns the first two "real" nodes?

This would be nice, but given that using Text nodes for kids is something I'd like to happen, it's not really possible.


> I think "check_eq_nodes" would make more sense...

Sure; I was aiming for naming similarity with test harness functions, but to be honest, I don't really like the do_ convention anyway.


> Please ignore what Alex said about this -- he's incorrect.  Using null as the
> first argument to setAttributeNS means set the node in the null namespace, no
> matter what the default namespace of the Element involved.

I was pretty certain that was true, but thanks for clarifying anyway.


> Actually, we were wrong here.  setAttribute("seamonkey", "rheet") sets
> whatever attribute has the fully qualified name "seamonkey".  Since you didn't
> use any prefixes in the attribute names in your setAttributeNS calls, there is
> such an attribute, and its value gets set.  So this should, in fact, be
> failing as written.

Blargh  :-|


> >+      if (nodes[i] && nodes[j])
> 
> How would they be null?

Extra commas in array literals cause the corresponding array entries to be filled with |undefined|; I was using this to enable a trailing comma in the array literal.  It's mostly unnecessary, so I removed the trailing comma and the defined-check.


> >+  // Attr.appendChild() isn't implemented yet -- run this code when it is
> 
> Comment in the bug on this if there is one pointing to this test?  And also,
> cite the bug# in the comment in this file.  If there's no bug, file one?

It's bug 56758; comment added there and in the test.


> >+ * Compares the first and third child nodes of the element (to allow 
whitespace)
> >+ * referenced by parentId for isEqualNode equality or inequality based on the
> >+ * value of areEqual.
> 
> Why not just compare the first two nodes that are not whitespace-only Text
> nodes?  That seems like it would be more robust.

It's more robust, but I think the concern here is testing the edge cases, not aiming for robustness (which, incidentally, I think is an impossible goal if all the tests in the file must still somehow be run).  I ended up deciding not to look for non-whitespace nodes, because testing Text nodes, particularly empty ones, is difficult if not impossible.  To guard against accidental bugs, I added a note next to equality_check_kids and at the beginning of test_whitespace warning against careless edits which introduce whitespace, so that should be enough.


Anyway, patch with nits fixed checked into trunk.