1283139 - Add tests for xmpp-xml

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Comment 1

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v1 - testXMLNode

Comment 2

[Patrick Cloke [:clokep]]

Comment on attachment 8766393 [details] [diff] [review]
v1 - testXMLNode

Review of attachment 8766393 [details] [diff] [review]:
-----------------------------------------------------------------

As an aside...while writing tests for some of these methods, it likely makes sense to add some comments to xmpp-xml.jsm!

::: chat/protocols/xmpp/xmpp-xml.jsm
@@ +127,5 @@
>      let n = new XMLNode(null, aNs, aName, aName, null);
>  
>      if (aAttr) {
> +      for (let at in aAttr) {
> +        if (aAttr[at])

When would this change occur?

Comment 3

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to Patrick Cloke [:clokep] from comment #2)
> Comment on attachment 8766393 [details] [diff] [review]
> v1 - testXMLNode
> 
> Review of attachment 8766393 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As an aside...while writing tests for some of these methods, it likely makes
> sense to add some comments to xmpp-xml.jsm!
> 
> ::: chat/protocols/xmpp/xmpp-xml.jsm
> @@ +127,5 @@
> >      let n = new XMLNode(null, aNs, aName, aName, null);
> >  
> >      if (aAttr) {
> > +      for (let at in aAttr) {
> > +        if (aAttr[at])
> 
> When would this change occur?

this example
> let x = Stanza.node("x", Stanza.NS..., { jid: aRoomJid, password: aPassword });
the aPassword may have null value, so we do not want to add attributes that do not have values.

Comment 4

[aleth [:aleth]]

Comment on attachment 8766393 [details] [diff] [review]
v1 - testXMLNode

Review of attachment 8766393 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/xmpp/test/test_xmppXml.js
@@ +16,5 @@
> +        type: null
> +      },
> +      data: []
> +    },
> +    output: '<message xmlns="jabber:client" jid="user@domain"/>'

When I said you didn't need to add tests for everything at once, I didn't mean just one testcase ;)

Some more examples for testXMLNode would be nice. Some standard ones and whatever edge cases you can think of (missing namespace, undefined name, ...).

Add a string to each test case explaining what that test case is checking (eg. for this one "Ignore attribute with null value") and use it as the third parameter in equal().

@@ +25,5 @@
> +  for (let current of TEST_DATA) {
> +    let result =
> +      xmppXml.Stanza.node(current.input.name, current.input.namespace,
> +                          current.input.attributes, current.input.data);
> +    equal(result.convertToString().trim(), current.output);

Why the trim()?

Why convertToString and not getXML? I'm sure sendStanza uses getXML.

Would it be easy to test both, or (better) to rewrite the code in xmpp-xml so they both call a generalized helper function, so that we can be sure that if getXML works so does convertToString?

::: chat/protocols/xmpp/xmpp-xml.jsm
@@ +123,5 @@
>    },
>  
>    /* Create a XML node */
>    node: function(aName, aNs, aAttr, aData) {
>      let n = new XMLNode(null, aNs, aName, aName, null);

let node =

@@ +126,5 @@
>    node: function(aName, aNs, aAttr, aData) {
>      let n = new XMLNode(null, aNs, aName, aName, null);
>  
>      if (aAttr) {
> +      for (let at in aAttr) {

While changing this, improve the variable names a bit ;)
e.g. at -> attributeName

@@ +127,5 @@
>      let n = new XMLNode(null, aNs, aName, aName, null);
>  
>      if (aAttr) {
> +      for (let at in aAttr) {
> +        if (aAttr[at])

add a comment

Comment 5

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v2 - testXMLNode

Comment 6

[aleth [:aleth]]

Comment on attachment 8766412 [details] [diff] [review]
v2 - testXMLNode

Review of attachment 8766412 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/xmpp/xmpp-xml.jsm
@@ +126,5 @@
>    node: function(aName, aNs, aAttr, aData) {
>      let n = new XMLNode(null, aNs, aName, aName, null);
>  
>      if (aAttr) {
> +      for (let at in aAttr) {

Actually, this code is weird. Can you see a reason why aAttr is not passed as a parameter in new XMLNode?

Comment 7

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #6)
> Comment on attachment 8766412 [details] [diff] [review]
> v2 - testXMLNode
> 
> Review of attachment 8766412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: chat/protocols/xmpp/xmpp-xml.jsm
> @@ +126,5 @@
> >    node: function(aName, aNs, aAttr, aData) {
> >      let n = new XMLNode(null, aNs, aName, aName, null);
> >  
> >      if (aAttr) {
> > +      for (let at in aAttr) {
> 
> Actually, this code is weird. Can you see a reason why aAttr is not passed
> as a parameter in new XMLNode?

I mean, there may be a reason, but in that case a comment would be great ;)

Comment 8

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v3 - testXMLNode

(In reply to aleth [:aleth] from comment #4)
> Why the trim()?
>
> Why convertToString and not getXML? I'm sure sendStanza uses getXML.
>
> Would it be easy to test both, or (better) to rewrite the code in xmpp-xml
> so they both call a generalized helper function, so that we can be sure that
> if getXML works so does convertToString?

Yes, you are right. I tested both as I think the flow of the generalized helper will be determined according to parameters and we will need to test also both (check convertToString and getXML methods of TextNode)

(In reply to aleth [:aleth] from comment #6)
> Actually, this code is weird. Can you see a reason why aAttr is not passed
> as a parameter in new XMLNode?

Clarified in this patch.

Comment 9

[aleth [:aleth]]

Comment on attachment 8767257 [details] [diff] [review]
v3 - testXMLNode

Review of attachment 8767257 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/xmpp/test/test_xmppXml.js
@@ +55,5 @@
> +      attributes: {},
> +      data: []
> +    },
> +    XmlOutput: '',
> +    stringOutput: '<message>\n test message\n</message>\n',

This isn't used ?

@@ +80,5 @@
> +      xmppXml.Stanza.node(current.input.name, current.input.namespace,
> +                          current.input.attributes, current.input.data);
> +    equal(result.getXML(), current.XmlOutput, current.description);
> +    equal(result.convertToString(), current.stringOutput, current.description);
> +    equal(current.isError, false);

indent

::: chat/protocols/xmpp/xmpp-xml.jsm
@@ +172,5 @@
> +  // Each element has a type, identified by name.
> +  if (!aLocalName)
> +    throw "aLocalName must have value";
> +  if (!aQName)
> +    throw "aQName must have value";

You can use aLocalName as the default value for aQName?

@@ +182,5 @@
>    this.attributes = {};
>    this.children = [];
>  
> +  if (!aAttr)
> +    return;

Just give it a default value {}

Comment 10

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v4 - testXMLNode

(In reply to aleth [:aleth] from comment #9) 
> This isn't used ?

Yes, sorry for that. 

> You can use aLocalName as the default value for aQName?

Yes, since aLocalName is a part of aQName.

Comment 11

[aleth [:aleth]]

Comment on attachment 8767286 [details] [diff] [review]
v4 - testXMLNode

Review of attachment 8767286 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/xmpp/xmpp-xml.jsm
@@ +172,5 @@
> +  // Each element has a type, identified by name.
> +  if (!aLocalName)
> +    throw "aLocalName must have value";
> +  if (!aQName)
> +    aQName = aLocalName;

Move it to the function definition

@@ +182,5 @@
>    this.attributes = {};
>    this.children = [];
>  
> +  if (!aAttr)
> +    aAttr = {};

Move it to the function definition

Comment 12

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v5 - testXMLNode

Comment 13

[Abdelrahman Ahmed [:abdelrahman]]

https://hg.mozilla.org/comm-central/rev/3e4a05ed0bdb