Add test for _parseJID in XMPP

RESOLVED FIXED in Instantbird 42

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: abdelrahman, Assigned: abdelrahman)

Tracking

trunk
Instantbird 42

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Add test for _parseJID in XMPP
(Assignee)

Updated

4 years ago
Assignee: nobody → a.ahmed1026
Blocks: 735200
Status: NEW → ASSIGNED

Comment 1

4 years ago
The same test file could also test normalize and normalizeFullJid and check all three functions are consistent with each other.
(Assignee)

Comment 2

4 years ago
Attachment #8626752 - Flags: review?(aleth)
(Assignee)

Updated

4 years ago
Attachment #8626752 - Flags: review?(aleth)
(Assignee)

Comment 3

4 years ago
Attachment #8626752 - Attachment is obsolete: true
Attachment #8626871 - Flags: review?(aleth)

Updated

4 years ago
Attachment #8626871 - Flags: review?(aleth) → review?(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.)
Attachment #8626871 - Flags: review?(clokep) → review-

Comment 5

4 years ago
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.
(Assignee)

Comment 6

4 years ago
Attachment #8626871 - Attachment is obsolete: true
Attachment #8626938 - Flags: review?(clokep)

Comment 7

4 years ago
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

4 years ago
(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.
(Assignee)

Comment 9

4 years ago
(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).
(Assignee)

Updated

4 years ago
Attachment #8626938 - Flags: review?(clokep)
(Assignee)

Comment 10

4 years ago
Attachment #8626938 - Attachment is obsolete: true
Attachment #8629109 - Flags: review?(aleth)

Updated

4 years ago
Attachment #8629109 - Flags: review?(aleth) → review?(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.
Attachment #8629109 - Flags: review?(clokep) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/40330e8b3ee0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
Updating milestone from 1.6 to 42.
Target Milestone: 1.6 → Instantbird 42
You need to log in before you can comment on or make changes to this bug.