Add some tests for the more complex nsIAbCard functions

RESOLVED FIXED

Status

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

(Blocks: 1 bug)

Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Created attachment 302872 [details] [diff] [review]
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.
Attachment #302872 - Flags: superreview?(neil)
Attachment #302872 - Flags: review?(neil)
Created attachment 302878 [details] [diff] [review]
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...
Attachment #302872 - Attachment is obsolete: true
Attachment #302878 - Flags: superreview?(neil)
Attachment #302878 - Flags: review?(neil)
Attachment #302872 - Flags: superreview?(neil)
Attachment #302872 - Flags: review?(neil)

Comment 2

11 years ago
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 == "PrimaryEmail1@test.invalid")
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?
Attachment #302878 - Flags: superreview?(neil)
Attachment #302878 - Flags: superreview-
Attachment #302878 - Flags: review?(neil)
(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

11 years ago
(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?
Created attachment 307091 [details] [diff] [review]
The fix v3

Updated version, no easy way found for btoa, other xpcshell tests have worked around it in this way as well unfortunately :-(
Attachment #302878 - Attachment is obsolete: true
Attachment #307091 - Flags: superreview?(neil)
Attachment #307091 - Flags: review?(neil)

Comment 6

11 years ago
I get that shutdown crash in this test too :-(
Created attachment 308487 [details] [diff] [review]
The fix v4

This should fix the windows crashes...
Attachment #307091 - Attachment is obsolete: true
Attachment #308487 - Flags: superreview?(neil)
Attachment #308487 - Flags: review?(neil)
Attachment #307091 - Flags: superreview?(neil)
Attachment #307091 - Flags: review?(neil)

Comment 8

11 years ago
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.
Attachment #308487 - Flags: superreview?(neil)
Attachment #308487 - Flags: superreview+
Attachment #308487 - Flags: review?(neil)
Attachment #308487 - Flags: review+
Patch checked in -> fixed.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.