405251 - Unit tests for memory and storage providers

Status: VERIFIED FIXED

(Calendar :: General, defect)

Description

[Sebastian Schwieger]

Attachment: provider unit tests

These unit tests should address issues from bug 333363.
Some items are added to memory/storage calendar and using getItems() it is checked whether the expected amount of items is returned.
Additionally I added a test for getItem() that checks that a bunch of properties are roundtripped. deleteItem() and modifyItem() should follow later on.

Comment 1

[Sebastian Schwieger]

Attachment: added some test todos

I added some test todos to reflect bug 405459. Also changed a print() statement to actually print the correct test number.

Comment 2

[Daniel Boelzle [:dbo]]

Comment on attachment 290749 [details] [diff] [review]
added some test todos

Some nits while I am using that unit test...

>+function getProps(aItem, aProp) {
...
>+    switch (aProp) {
>+        case "start":
>+            var value = aItem.startDate || aItem.entryDate || null;
please declare the variable up front, not in every case block.

>+        case "calendar":
>+            var value = aItem.calendar;
calendar.id would be reliable (if you would set one), you never know about js wrappers...

>+function compareItems(aLeftItem, aRightItem, aPropArray) {
...
>+                      "recurrenceInfo", "recurrenceStartDate", "DUE"];
Comparing calIRecurrenceInfo reference makes no sense since it will be cloned when returned on item via onOperationComplete/onGetResult (or might be a wrapper); doesn't work for me. A feasible way would be to collect all recurrence items, sorting and concatenating their ical strings.

Comment 3

[Daniel Boelzle [:dbo]]

Attachment: added some test todos

debitrotted; removed recurrenceInfo from property list.

BTW: Great tests, I'd appreciate to see those land soon!

Comment 4

[Daniel Boelzle [:dbo]]

I need to manually remove /tmp/test_storage.sqlite in case the test fails.

Comment 5

[Martin Schröder [:mschroeder]]

Part 1 (head_consts.js):
My comments are mostly style nits and in addition to Daniel's. Please see http://wiki.mozilla.org/Calendar:Style_Guide for some guidelines and examples.

Add yourself to the contributors.

[...]

+function createEventFromIcalString(ics) {
+    var eventClass = Cc["@mozilla.org/calendar/event;1"];
+    var eventIID = Ci.calIEvent;
+    var event = eventClass.createInstance(eventIID);
+    event.icalString = ics;
+    return event;
+}

Better:
function createEventFromIcalString(icalString) {
    var event = Cc["@mozilla.org/calendar/event;1"]
                .createInstance(Ci.calIEvent);
    event.icalString = icalString;
    return event;
}

+function createTodoFromIcalString(ics) {
+    var todoClass = Cc["@mozilla.org/calendar/todo;1"];
+    var todoIID = Ci.calITodo;
+    var todo = todoClass.createInstance(todoIID);
+    todo.icalString = ics;
+    return todo;
+}

Better:
function createTodoFromIcalString(icalString) {
    var todo = Cc["@mozilla.org/calendar/todo;1"]
               .createInstance(Ci.calITodo);
    todo.icalString = icalString;
    return todo;
}

+function getMemoryCal() {
+    var cal = Cc["@mozilla.org/calendar/calendar;1?type=memory"]
+                .createInstance(Components.interfaces.calICalendar);

Better:
var cal = Cc["@mozilla.org/calendar/calendar;1?type=memory"]
          .createInstance(Ci.calICalendar);

+    var calendar = cal.QueryInterface(Components.interfaces.calICalendarProvider);

Use Ci instead of Components.interfaces.

[...]

+function getStorageCal() {
+    var dirSvc = Cc["@mozilla.org/file/directory_service;1"].
+               getService(Ci.nsIProperties);

Align Cc with .getService(...).

+    var db = dirSvc.get("TmpD", Ci.nsIFile);
+    db.append("test_storage.sqlite");
+
+    //create URI
+    var ioSvc = Components.classes["@mozilla.org/network/io-service;1"].
+                       getService(Components.interfaces.nsIIOService);

Use Cc and correct alignment.

+    var uri = ioSvc.newFileURI(db);
+
+    //create storage calendar
+    var cal = Cc["@mozilla.org/calendar/calendar;1?type=storage"]
+                 .createInstance(Components.interfaces.calICalendar);

Correct alignment.

+    cal.uri = uri;
+    // remove existing items
+    var calendar = cal.QueryInterface(Components.interfaces.calICalendarProvider);

Use Ci.

+    try {
+     calendar.deleteCalendar(calendar, null);
+    } catch (e) {
+     print("*** error purging calendar: " + e);
+    }
+    return cal;
+}

Use 4 space indentation in this try-catch block.

[...]

+function compareItems(aLeftItem, aRightItem, aPropArray) {
+    if (!aPropArray) {
+        // left out:  id", "calendar", "lastModifiedTime" as these are expected to change

Missing double quote in comment. ;)

+        aPropArray = ["start", "end", "duration", "generation",
+                      "title", "priority", "privacy", "creationDate",
+                      "stampTime", "status",
+                      "alarmOffset", "alarmRelated", "alarmLastAck",
+                      "recurrenceInfo", "recurrenceStartDate", "DUE"];
+    }

Do we need "DUE" if we have "end" that gets dueDate?

+    for (var i = 0; i < aPropArray.length; i++) {
+        do_check_eq(getProps(aLeftItem, aPropArray[i]), getProps(aRightItem, aPropArray[i]));
+        //print(aPropArray[i] + ": " + getProps(aLeftItem, aPropArray[i]));
+    }
+}

Remove commented out print statement.

Comment 6

[Martin Schröder [:mschroeder]]

Part 2 (test_providers.js):

[...]

+ * The Original Code is mozilla calendar tests code.
+ *
+ * The Initial Developer of the Original Code is
+ *      Sebastian Schwieger <[e-mail]>

No indentation.

+ * Portions created by the Initial Developer are Copyright (C) 2007
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+*
+ *

Remove the line under "Contributor(s):".

[...]

+var icalStringArray = [
+                // 1: one-hour event
+                "BEGIN:VEVENT\n" + 
+                "DTSTART:20020402T114500Z\n" +
+                "DTEND:20020402T124500Z\n" +
+                "END:VEVENT\n",

[...]

+                // 29: todo, that has the status "COMPLETED" but no completion time. See bug 405459
+                "BEGIN:VTODO\n" +
+                "SUMMARY:Todo\n" +
+                "STATUS:COMPLETED\n" +
+                "END:VTODO\n",];

Close the array at a new line. Is the trailing comma correct syntax?
Unify the testcase descriptions. Including a reference to the RFC or a bug# would be perfect. ;)

+function getIcalString(aNumber) {
+    return icalStringArray[aNumber];
+}

We don't need this wrapper function. Use the array directly.

+function run_test() {
+
+    // first entry is the test number (starting with 1), second item is expected Result for testGetItems()
+    var wantedArray = [[ 1, 1],  // 1: one-hour event in the range

[...]

+                       [ 29, 1]]; // 29: todo, that has the status "COMPLETED" but no completion time. See bug 405459

Should we really duplicate the descriptions here? I try to think of a better way to group and describe the testcases.
Remove the empty line at the beginning of the function.

+    for (var i = 0; i < wantedArray.length; i++) {
+        var itemArray = wantedArray[i];
+        // correct for 1 to stay in synch with the Test numbers
+        var icalitem = getIcalString(itemArray[0] - 1);

Use camelcase capitalization: icalItem.

+        if (icalitem.search(/VEVENT/) != -1) {
+            var item = createEventFromIcalString(icalitem);
+        } else if (icalitem.search(/VTODO/) != -1) {
+            var item = createTodoFromIcalString(icalitem);
+        }
+
+        print("Test " + wantedArray[i][0]);
+        testGetItems(item, itemArray[1]);

Should we call this function testGetItemsInRange(...)? testGetItem and testGetItems are too similar in my opinion.

+        testGetItem(item);
+    }

This print statement is a good idea.
You should add comments to both test funcions to describe what the check, maybe describe the workflow of the calls and the listener.

+    function testGetItems(aItem, aResult) {
+        // construct range
+        var rangeStart = createDate(2002, 03, 02); // 03 means April
+        var rangeEnd = rangeStart.clone();
+        rangeEnd.day += 1;
+
+        // filter options
+        var filter = Ci.calICalendar.ITEM_FILTER_TYPE_ALL | 
+                     Ci.calICalendar.ITEM_FILTER_CLASS_OCCURRENCES |
+                     Ci.calICalendar.ITEM_FILTER_COMPLETED_ALL;
+
+        // get the calendars
+        var calArray = [];
+        calArray.push(getStorageCal());
+        calArray.push(getMemoryCal());
+        for each (cal in calArray) {
+            // implement Listener
+            var count = 0;
+            var listener = {
+                onOperationComplete: function(aCalendar, aStatus,
+                                                aOperationType, aId, aDetail) {

One argument per line.

+                    do_check_eq(aStatus, 0);
+                    if (aOperationType == Ci.calIOperationListener.ADD) {
+                        //perform getItems() on the calendar
+                        aCalendar.getItems(filter, 0, rangeStart, rangeEnd, listener);
+                    } else if (aOperationType == Ci.calIOperationListener.GET) {
+                        print("Testing getItems()");
+                        do_check_eq(count, aResult);
+                    }
+                },

Remove the print statement.

+                onGetResult: function(aCalendar, aStatus, aItemType,
+                                    aDetail, aCount, aItems) {

One argument per line.

+                    if (aCount) {
+                        count += aCount;
+                        for (var i = 0; i < aCount; i++) {
+                            var start = aItems[i].startDate || aItems[i].entryDate || null;
+                            if (start) {
+                                do_check_eq(isdate, start.isDate);
+                            }
+                        }
+                    }
+                }
+            };
+            //check if isDate is roundtripped
+            var isdate = false;
+            var itemStart = aItem.startDate || aItem.entryDate || null;
+            if (itemStart && itemStart.isDate) {
+                isdate = true;
+            }

Does it make sense to do it also for the end/due date?

+            // add item to calendar
+            cal.addItem(aItem, listener);
+        }
+    }
+
+    function testGetItem(aItem) {
+
+        // get the calendars
+        var calArray = [];
+        calArray.push(getStorageCal());
+        calArray.push(getMemoryCal());
+        for each (cal in calArray) {
+            // implement Listener
+            var count = 0;
+            var returnedItem = null;
+            var listener = {
+                onOperationComplete: function(aCalendar, aStatus,
+                                                aOperationType, aId, aDetail) {

One argument per line.
Remove the empty line at the beginning of the function.

+                    do_check_eq(aStatus, 0);
+                    if (aOperationType == Ci.calIOperationListener.ADD) {
+                        compareItems(aDetail, aItem);
+                        //perform getItem() on the calendar
+                        aCalendar.getItem(aId, listener);
+                    } else if (aOperationType == Ci.calIOperationListener.GET) {
+                        print("Testing getItem()");
+                        do_check_eq(count, 1);
+                        compareItems(returnedItem, aItem);
+                    }

Remove the print statement.

+                },
+                onGetResult: function(aCalendar, aStatus, aItemType,
+                                      aDetail, aCount, aItems) {

One argument per line.

+                    if (aCount) {
+                        count += aCount;
+                        returnedItem = aItems[0];
+                    }
+                }
+            };
+            // add item to calendar
+            cal.addItem(aItem, listener);
+        }
+    }
+}

Comment 7

[Martin Schröder [:mschroeder]]

Comment on attachment 290749 [details] [diff] [review]
added some test todos

r- the patch. There are some style nits and other things. A new iteration of patch was planned anyway. ;)

Comment 8

[Sebastian Schwieger]

Attachment: provider unit tests v3

Comments follow

Comment 9

[Sebastian Schwieger]

(In reply to comment #4)
> I need to manually remove /tmp/test_storage.sqlite in case the test fails.
> 

Do you get an error purging the storage calendar? The database should be purged before every call, removing the file should not be necessary.

(In reply to comment #5)
[...]
> Do we need "DUE" if we have "end" that gets dueDate?
removed. I originally had it in there to test the generic method with getProperty

(In reply to comment #6)
[...]
> Close the array at a new line. Is the trailing comma correct syntax?
> Unify the testcase descriptions. Including a reference to the RFC or a bug#
> would be perfect. ;)
I changed some descriptions and added tests for the duration property. Not every test has a reference bug. Most of them are pretty trivial and should just work.

[...]
> Should we really duplicate the descriptions here? I try to think of a better
> way to group and describe the testcases.
I removed them for the time being.

[...]
> Should we call this function testGetItemsInRange(...)? testGetItem and
> testGetItems are too similar in my opinion.
I would like to stay with the function names that correspond to the function names in calICalendar.idl

[...]
> Does it make sense to do it also for the end/due date?
I changed this to use the compareItems() function. This checks for start and end.

To the best of my knowledge I addressed all other comments.

Comment 10

[Sebastian Schwieger]

Attachment: unit provider tests v4

Forgot to update the function description of testGetItems().

Comment 11

[Martin Schröder [:mschroeder]]

Comment on attachment 293380 [details] [diff] [review]
unit provider tests v4

I tweaked some comments, and will check the patch in.

r=mschroeder

Comment 12

[Martin Schröder [:mschroeder]]

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED