417065 - Add some tests for the more complex nsIAbCard functions

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Mark Banner (:standard8)]

Attachment: The fix

Looking at the review for bug 413260, I think it would help if we had at least some minimum tests for convertToBase64EncodedXML, convertToXMLPrintData and convertToEscapedVCard.

The attached tests basically just take a fully-filled in nsIAbCard and checks the results for them.

Yes, I did just copy and paste from what it currently does, but I don't think that's too much of a problem. We can expand them later and think about standards then, I'm just looking to get something in so that we can check we don't do major regressions.

Doing these functions also gives us the advantage is these functions should cover pretty much all the current properties that we use on ab cards.

Comment 1

[Mark Banner (:standard8)]

Attachment: The fix v2

This one has better comments at the top of it that I was meant to have saved before I generated the previous patch...

Comment 2

[[e-mail]]

Comment on attachment 302878 [details] [diff] [review]
The fix v2

>+    var testAB = do_get_file("mailnews/addrbook/test/unit/data/cardForEmail.mab");
This is an existing file, right?

>+    var rdf = Components.classes["@mozilla.org/rdf/rdf-service;1"]
>+                        .getService(Components.interfaces.nsIRDFService);
>+    do_check_true(rdf != null);
getService never returns null (it will throw on error)

>+    var AB = rdf.GetResource(kPABData.URI)
>+                .QueryInterface(Components.interfaces.nsIAbDirectory);
>+    do_check_true(AB != null);
As will QueryInterface. But you could do_check_true(AB instanceof ...)

>+      // If QI failes, just set tempCard to null.
Nit: fails

>+      try {
>+        tempCard = childCards.getNext().QueryInterface(Components.interfaces.nsIAbCard);
>+      }
>+      catch (ex) {
>+        tempCard = null;
>+      }
>+      // We want the one with this email...
>+      if (tempCard && tempCard.primaryEmail == "[e-mail]")
Again, easier to use tempCard instanceof nsIAbCard &&

>+    do_check_true(fullCard != childCards);
This makes no sense.

>+    do_check_eq(fullCard.convertToXMLPrintData(),
This isn't an ideal test, depending on what it is you're actually testing...

>+    do_check_eq(fullCard.convertToBase64EncodedXML(),
>+                "PD94bWwgdmVyc2lvbj0iMS4wIj8+Cjw/eG1sLXN0eWxlc2hlZXQgdHlwZT0idGV4dC9jc3MiIGhyZWY9ImNocm9tZTovL21lc3Nlbmdlci9jb250ZW50L2FkZHJlc3Nib29rL3ByaW50LmNzcyI/Pgo8ZGlyZWN0b3J5Pgo8dGl0bGUgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzE5OTkveGh0bWwiPkFkZHJlc3MgQm9vazwvdGl0bGU+CjxHZW5lcmF0ZWROYW1lPgpEaXNwbGF5TmFtZTE8L0dlbmVyYXRlZE5hbWU+Cjx0YWJsZT48dHI+PHRkPjxzZWN0aW9uPjxsYWJlbHJvdz48bGFiZWw+RGlzcGxheSBOYW1lOiA8L2xhYmVsPjxEaXNwbGF5TmFtZT5EaXNwbGF5TmFtZTE8L0Rpc3BsYXlOYW1lPjwvbGFiZWxyb3c+PGxhYmVscm93PjxsYWJlbD5OaWNrbmFtZTogPC9sYWJlbD48Tmlja05hbWU+Tmlja05hbWUxPC9OaWNrTmFtZT48L2xhYmVscm93PjxQcmltYXJ5RW1haWw+UHJpbWFyeUVtYWlsMUB0ZXN0LmludmFsaWQ8L1ByaW1hcnlFbWFpbD48U2Vjb25kRW1haWw+U2Vjb25kRW1haWwxQHRlc3QuaW52YWxpZDwvU2Vjb25kRW1haWw+PGxhYmVscm93PjxsYWJlbD5TY3JlZW4gTmFtZTogPC9sYWJlbD48X0FpbVNjcmVlbk5hbWU+U2NyZWVuTmFtZTE8L19BaW1TY3JlZW5OYW1lPjwvbGFiZWxyb3c+PC9zZWN0aW9uPjwvdGQ+PC90cj48dHI+PHRkPjxzZWN0aW9uPjxzZWN0aW9udGl0bGU+UGhvbmU8L3NlY3Rpb250aXRsZT48bGFiZWxyb3c+PGxhYmVsPldvcms6IDwvbGFiZWw+PFdvcmtQaG9uZT5Xb3JrUGhvbmUxPC9Xb3JrUGhvbmU+PC9sYWJlbHJvdz48bGFiZWxyb3c+PGxhYmVsPkhvbWU6IDwvbGFiZWw+PEhvbWVQaG9uZT5Ib21lUGhvbmUxPC9Ib21lUGhvbmU+PC9sYWJlbHJvdz48bGFiZWxyb3c+PGxhYmVsPkZheDogPC9sYWJlbD48RmF4TnVtYmVyPkZheE51bWJlcjE8L0ZheE51bWJlcj48L2xhYmVscm93PjxsYWJlbHJvdz48bGFiZWw+UGFnZXI6IDwvbGFiZWw+PFBhZ2VyTnVtYmVyPlBhZ2VyTnVtYmVyMTwvUGFnZXJOdW1iZXI+PC9sYWJlbHJvdz48bGFiZWxyb3c+PGxhYmVsPk1vYmlsZTogPC9sYWJlbD48Q2VsbHVsYXJOdW1iZXI+Q2VsbHVsYXJOdW1iZXIxPC9DZWxsdWxhck51bWJlcj48L2xhYmVscm93Pjwvc2VjdGlvbj48c2VjdGlvbj48c2VjdGlvbnRpdGxlPk90aGVyPC9zZWN0aW9udGl0bGU+PGxhYmVscm93PjxsYWJlbD5DdXN0b20gMTogPC9sYWJlbD48Q3VzdG9tMT5DdXN0b20xMTwvQ3VzdG9tMT48L2xhYmVscm93PjxsYWJlbHJvdz48bGFiZWw+Q3VzdG9tIDI6IDwvbGFiZWw+PEN1c3RvbTI+Q3VzdG9tMjE8L0N1c3RvbTI+PC9sYWJlbHJvdz48bGFiZWxyb3c+PGxhYmVsPkN1c3RvbSAzOiA8L2xhYmVsPjxDdXN0b20zPkN1c3RvbTMxPC9DdXN0b20zPjwvbGFiZWxyb3c+PGxhYmVscm93PjxsYWJlbD5DdXN0b20gNDogPC9sYWJlbD48Q3VzdG9tND5DdXN0b200MTwvQ3VzdG9tND48L2xhYmVscm93PjxOb3Rlcz5Ob3RlczE8L05vdGVzPjwvc2VjdGlvbj48L3RkPjx0ZD48c2VjdGlvbj48c2VjdGlvbnRpdGxlPkhvbWU8L3NlY3Rpb250aXRsZT48SG9tZUFkZHJlc3M+SG9tZUFkZHJlc3MxMTwvSG9tZUFkZHJlc3M+PEhvbWVBZGRyZXNzMj5Ib21lQWRkcmVzczIxPC9Ib21lQWRkcmVzczI+PEhvbWVDaXR5PkhvbWVDaXR5MTwvSG9tZUNpdHk+LCA8SG9tZVN0YXRlPkhvbWVTdGF0ZTE8L0hvbWVTdGF0ZT4gPEhvbWVaaXBDb2RlPkhvbWVaaXBDb2RlMTwvSG9tZVppcENvZGU+PEhvbWVDb3VudHJ5PkhvbWVDb3VudHJ5MTwvSG9tZUNvdW50cnk+PFdlYlBhZ2UyPmh0dHA6Ly9XZWJQYWdlMTE8L1dlYlBhZ2UyPjwvc2VjdGlvbj48c2VjdGlvbj48c2VjdGlvbnRpdGxlPldvcms8L3NlY3Rpb250aXRsZT48Sm9iVGl0bGU+Sm9iVGl0bGUxPC9Kb2JUaXRsZT48RGVwYXJ0bWVudD5EZXBhcnRtZW50MTwvRGVwYXJ0bWVudD48Q29tcGFueT5Pcmdhbml6YXRpb24xPC9Db21wYW55PjxXb3JrQWRkcmVzcz5Xb3JrQWRkcmVzczE8L1dvcmtBZGRyZXNzPjxXb3JrQWRkcmVzczI+V29ya0FkZHJlc3MyMTwvV29ya0FkZHJlc3MyPjxXb3JrQ2l0eT5Xb3JrQ2l0eTE8L1dvcmtDaXR5PiwgPFdvcmtTdGF0ZT5Xb3JrU3RhdGUxPC9Xb3JrU3RhdGU+IDxXb3JrWmlwQ29kZT5Xb3JrWmlwQ29kZTE8L1dvcmtaaXBDb2RlPjxXb3JrQ291bnRyeT5Xb3JrQ291bnRyeTE8L1dvcmtDb3VudHJ5PjxXZWJQYWdlMT5odHRwOi8vV2ViUGFnZTIxPC9XZWJQYWdlMT48L3NlY3Rpb24+PC90ZD48L3RyPjwvdGFibGU+PC9kaXJlY3Rvcnk+Cg==");
Is this simply the base64 of the last test? If it is, why not just compare the base64 encoded/decoded versions?

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #2)
> (From update of attachment 302878 [details] [diff] [review])
> >+    var testAB = do_get_file("mailnews/addrbook/test/unit/data/cardForEmail.mab");
> This is an existing file, right?

Yes

> >+    do_check_eq(fullCard.convertToXMLPrintData(),
> This isn't an ideal test, depending on what it is you're actually testing...

Here (and the two tests below it) I just wanted to check all the fields we currently support to ensure that when we're doing big changes we've got at least a basic test coverage. I think these functions need redoing anyway, but that's another bug.

> >+    do_check_eq(fullCard.convertToBase64EncodedXML(),
> >+                "PD94bWwgdmVyc2lvbj0iMS4wIj8+Cjw/eG1sLXN0eWxlc2hlZXQgdHlwZT0idGV4dC9jc3MiIGhyZWY9ImNocm9tZTovL21lc3Nlbmdlci9jb250ZW50L2FkZHJlc3Nib29rL3ByaW50LmNzcyI/Pgo8ZGlyZWN0b3J5Pgo8dGl0bGUgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzE5OTkveGh0bWwiPkFkZHJlc3MgQm9vazwvdGl0bGU+CjxHZW5lcmF0ZWROYW1lPgpEaXNwbGF5TmFtZTE8L0dlbmVyYXRlZE5hbWU+Cjx0YWJsZT48dHI+PHRkPjxzZWN0aW9uPjxsYWJlbHJvdz48bGFiZWw+RGlzcGxheSBOYW1lOiA8L2xhYmVsPjxEaXNwbGF5TmFtZT5EaXNwbGF5TmFtZTE8L0Rpc3BsYXlOYW1lPjwvbGFiZWxyb3c+PGxhYmVscm93PjxsYWJlbD5OaWNrbmFtZTogPC9sYWJlbD48Tmlja05hbWU+Tmlja05hbWUxPC9OaWNrTmFtZT48L2xhYmVscm93PjxQcmltYXJ5RW1haWw+UHJpbWFyeUVtYWlsMUB0ZXN0LmludmFsaWQ8L1ByaW1hcnlFbWFpbD48U2Vjb25kRW1haWw+U2Vjb25kRW1haWwxQHRlc3QuaW52YWxpZDwvU2Vjb25kRW1haWw+PGxhYmVscm93PjxsYWJlbD5TY3JlZW4gTmFtZTogPC9sYWJlbD48X0FpbVNjcmVlbk5hbWU+U2NyZWVuTmFtZTE8L19BaW1TY3JlZW5OYW1lPjwvbGFiZWxyb3c+PC9zZWN0aW9uPjwvdGQ+PC90cj48dHI+PHRkPjxzZWN0aW9uPjxzZWN0aW9udGl0bGU+UGhvbmU8L3NlY3Rpb250aXRsZT48bGFiZWxyb3c+PGxhYmVsPldvcms6IDwvbGFiZWw+PFdvcmtQaG9uZT5Xb3JrUGhvbmUxPC9Xb3JrUGhvbmU+PC9sYWJlbHJvdz48bGFiZWxyb3c+PGxhYmVsPkhvbWU6IDwvbGFiZWw+PEhvbWVQaG9uZT5Ib21lUGhvbmUxPC9Ib21lUGhvbmU+PC9sYWJlbHJvdz48bGFiZWxyb3c+PGxhYmVsPkZheDogPC9sYWJlbD48RmF4TnVtYmVyPkZheE51bWJlcjE8L0ZheE51bWJlcj48L2xhYmVscm93PjxsYWJlbHJvdz48bGFiZWw+UGFnZXI6IDwvbGFiZWw+PFBhZ2VyTnVtYmVyPlBhZ2VyTnVtYmVyMTwvUGFnZXJOdW1iZXI+PC9sYWJlbHJvdz48bGFiZWxyb3c+PGxhYmVsPk1vYmlsZTogPC9sYWJlbD48Q2VsbHVsYXJOdW1iZXI+Q2VsbHVsYXJOdW1iZXIxPC9DZWxsdWxhck51bWJlcj48L2xhYmVscm93Pjwvc2VjdGlvbj48c2VjdGlvbj48c2VjdGlvbnRpdGxlPk90aGVyPC9zZWN0aW9udGl0bGU+PGxhYmVscm93PjxsYWJlbD5DdXN0b20gMTogPC9sYWJlbD48Q3VzdG9tMT5DdXN0b20xMTwvQ3VzdG9tMT48L2xhYmVscm93PjxsYWJlbHJvdz48bGFiZWw+Q3VzdG9tIDI6IDwvbGFiZWw+PEN1c3RvbTI+Q3VzdG9tMjE8L0N1c3RvbTI+PC9sYWJlbHJvdz48bGFiZWxyb3c+PGxhYmVsPkN1c3RvbSAzOiA8L2xhYmVsPjxDdXN0b20zPkN1c3RvbTMxPC9DdXN0b20zPjwvbGFiZWxyb3c+PGxhYmVscm93PjxsYWJlbD5DdXN0b20gNDogPC9sYWJlbD48Q3VzdG9tND5DdXN0b200MTwvQ3VzdG9tND48L2xhYmVscm93PjxOb3Rlcz5Ob3RlczE8L05vdGVzPjwvc2VjdGlvbj48L3RkPjx0ZD48c2VjdGlvbj48c2VjdGlvbnRpdGxlPkhvbWU8L3NlY3Rpb250aXRsZT48SG9tZUFkZHJlc3M+SG9tZUFkZHJlc3MxMTwvSG9tZUFkZHJlc3M+PEhvbWVBZGRyZXNzMj5Ib21lQWRkcmVzczIxPC9Ib21lQWRkcmVzczI+PEhvbWVDaXR5PkhvbWVDaXR5MTwvSG9tZUNpdHk+LCA8SG9tZVN0YXRlPkhvbWVTdGF0ZTE8L0hvbWVTdGF0ZT4gPEhvbWVaaXBDb2RlPkhvbWVaaXBDb2RlMTwvSG9tZVppcENvZGU+PEhvbWVDb3VudHJ5PkhvbWVDb3VudHJ5MTwvSG9tZUNvdW50cnk+PFdlYlBhZ2UyPmh0dHA6Ly9XZWJQYWdlMTE8L1dlYlBhZ2UyPjwvc2VjdGlvbj48c2VjdGlvbj48c2VjdGlvbnRpdGxlPldvcms8L3NlY3Rpb250aXRsZT48Sm9iVGl0bGU+Sm9iVGl0bGUxPC9Kb2JUaXRsZT48RGVwYXJ0bWVudD5EZXBhcnRtZW50MTwvRGVwYXJ0bWVudD48Q29tcGFueT5Pcmdhbml6YXRpb24xPC9Db21wYW55PjxXb3JrQWRkcmVzcz5Xb3JrQWRkcmVzczE8L1dvcmtBZGRyZXNzPjxXb3JrQWRkcmVzczI+V29ya0FkZHJlc3MyMTwvV29ya0FkZHJlc3MyPjxXb3JrQ2l0eT5Xb3JrQ2l0eTE8L1dvcmtDaXR5PiwgPFdvcmtTdGF0ZT5Xb3JrU3RhdGUxPC9Xb3JrU3RhdGU+IDxXb3JrWmlwQ29kZT5Xb3JrWmlwQ29kZTE8L1dvcmtaaXBDb2RlPjxXb3JrQ291bnRyeT5Xb3JrQ291bnRyeTE8L1dvcmtDb3VudHJ5PjxXZWJQYWdlMT5odHRwOi8vV2ViUGFnZTIxPC9XZWJQYWdlMT48L3NlY3Rpb24+PC90ZD48L3RyPjwvdGFibGU+PC9kaXJlY3Rvcnk+Cg==");
> Is this simply the base64 of the last test? If it is, why not just compare the
> base64 encoded/decoded versions?

Suggestions please, because btoa doesn't work, neither does window.btoa in xpcshell tests :-(

Comment 4

[[e-mail]]

(In reply to comment #3)
>Suggestions please, because btoa doesn't work, neither does window.btoa in
>xpcshell tests :-(
Not to worry, but ask around on IRC perhaps just in case there's a way?

Comment 5

[Mark Banner (:standard8)]

Attachment: The fix v3

Updated version, no easy way found for btoa, other xpcshell tests have worked around it in this way as well unfortunately :-(

Comment 6

[[e-mail]]

I get that shutdown crash in this test too :-(

Comment 7

[Mark Banner (:standard8)]

Attachment: The fix v4

This should fix the windows crashes...

Comment 8

[[e-mail]]

Comment on attachment 308487 [details] [diff] [review]
The fix v4

I double-checked against test_collection.js and it calls gc before cleanup, which stops the crash here too. r+sr with that (and its comment) included.

Comment 9

[Mark Banner (:standard8)]

Patch checked in -> fixed.