Closed Bug 1283139 Opened 4 years ago Closed 4 years ago

Add tests for xmpp-xml

Categories

(Chat Core :: XMPP, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Instantbird 50

People

(Reporter: abdelrahman, Assigned: abdelrahman)

References

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
Attached patch v1 - testXMLNode (obsolete) — Splinter Review
Attachment #8766393 - Flags: review?(aleth)
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?
(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 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
Attachment #8766393 - Flags: review?(aleth) → review-
Attached patch v2 - testXMLNode (obsolete) — Splinter Review
Attachment #8766393 - Attachment is obsolete: true
Attachment #8766412 - Flags: review?(aleth)
Attachment #8766412 - Flags: review?(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?
(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 ;)
Attached patch v3 - testXMLNode (obsolete) — Splinter Review
(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.
Attachment #8766412 - Attachment is obsolete: true
Attachment #8767257 - Flags: review?(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 {}
Attached patch v4 - testXMLNode (obsolete) — Splinter Review
(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.
Attachment #8767257 - Attachment is obsolete: true
Attachment #8767257 - Flags: review?(aleth)
Attachment #8767286 - Flags: review?(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
Attached patch v5 - testXMLNodeSplinter Review
Attachment #8767286 - Attachment is obsolete: true
Attachment #8767286 - Flags: review?(aleth)
Attachment #8767293 - Flags: review?(aleth)
Attachment #8767293 - Flags: review?(aleth) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/3e4a05ed0bdb
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 50
You need to log in before you can comment on or make changes to this bug.