1177902 - Add test for _parseJID in XMPP

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Description

[Abdelrahman Ahmed [:abdelrahman]]

Add test for _parseJID in XMPP

Comment 1

[aleth [:aleth]]

The same test file could also test normalize and normalizeFullJid and check all three functions are consistent with each other.

Comment 2

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 1 - add test for _parseJID

Comment 3

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 2 - add test for _parseJID and normalization methods

Comment 4

[Patrick Cloke [:clokep]]

Comment on attachment 8626871 [details] [diff] [review]
rev 2 - add test for _parseJID and normalization methods

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

This looks pretty good! A few other comments:
* Please name the file "test_parseJidAndNormalization.js" (i.e. a capital "J").
* Please add a comment above _parseJid that explains what it's doing. I know you didn't write this, but the other two of these three similar functions have comments and I'd like to see one there too.

::: chat/protocols/xmpp/test/test_parsejidAndNormalization.js
@@ +6,5 @@
> +let xmpp = {};
> +Services.scriptloader.loadSubScript("resource:///components/xmpp.js", xmpp);
> +
> +function testParseJID() {
> +	const testData = {

Nit: Use 2 space indentation instead of tabs.

@@ +51,5 @@
> +	};
> +
> +	for (let currentJID in testData) {
> +		equal(xmpp.XMPPAccount.prototype.normalize(currentJID),
> +					testData[currentJID]);

Nit: Line up testData with xmpp on the line above it. (This applies to a few places in this file.)

Comment 5

[aleth [:aleth]]

Comment on attachment 8626871 [details] [diff] [review]
rev 2 - add test for _parseJID and normalization methods

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

To avoid duplication and to make adding additional test cases easy in the future, you could use the same test data for all three tests.

Comment 6

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 3 - add test for _parseJID and normalization methods

Comment 7

[aleth [:aleth]]

Doing a quick search for jid test cases I discovered these links which might be helpful:

http://stackoverflow.com/questions/3514342/validating-an-xmpp-jid-with-python
https://github.com/otalk/xmpp-jid

I haven't fully read them but it seems likely the current implementations of parseJid/normalize/normalizeFullJid don't always match RFC 6122, which is surprisingly complicated.

Probably worth checking
* that parseJid doesn't fail for more unusual jids (e.g. with unicode characters)
* whether normalize and normalizeFullJid need rewriting in order to really ensure the resulting normalized jids can be used as unique identifiers for comparisons.

The second link does seem to include some tests, which should be worth a look.

Comment 8

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #7)
> http://stackoverflow.com/questions/3514342/validating-an-xmpp-jid-with-python
> https://github.com/otalk/xmpp-jid

Note I'm not suggesting we use that code (or even need to), just that it might be helpful in figuring out if we are spec compliant.

Comment 9

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #7)
> Probably worth checking
> * that parseJid doesn't fail for more unusual jids (e.g. with unicode
> characters)
> * whether normalize and normalizeFullJid need rewriting in order to really
> ensure the resulting normalized jids can be used as unique identifiers for
> comparisons.

I will extend test cases in next patch to be sure of everything (parseJid, normalize and normalizeFullJid).

Comment 10

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 4 - add test for _parseJID and normalization methods

Comment 11

[Patrick Cloke [:clokep]]

Comment on attachment 8629109 [details] [diff] [review]
rev 4 - add test for _parseJID and normalization methods

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

Looks pretty reasonable to me.

Minor nit: change `testData` to `TEST_DATA` since it's a constant. r+ with that change!

Also, if any of the elements in that test data are derived from a specific bug or issue, it might make sense to add a comment about that above that specific test-case.

Comment 12

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 5 - add test for _parseJID and normalization methods

Comment 13

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/40330e8b3ee0

Comment 14

[Patrick Cloke [:clokep]]

Updating milestone from 1.6 to 42.