Last Comment Bug 1117456 - Run unit tests on ical.js as well as libical
: Run unit tests on ical.js as well as libical
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: ICAL.js Integration (show other bugs)
: unspecified
: All All
-- normal (vote)
: 3.9
Assigned To: Geoff Lankow (:darktrojan)
:
:
Mentors:
Depends on:
Blocks: icaljs
  Show dependency treegraph
 
Reported: 2015-01-03 19:30 PST by Geoff Lankow (:darktrojan)
Modified: 2015-01-04 17:21 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
1117456-1.diff (2.45 KB, patch)
2015-01-04 00:35 PST, Geoff Lankow (:darktrojan)
philipp: review+
Details | Diff | Splinter Review
1117456-fixes-1.diff (7.15 KB, patch)
2015-01-04 15:16 PST, Geoff Lankow (:darktrojan)
philipp: review+
Details | Diff | Splinter Review

Description User image Geoff Lankow (:darktrojan) 2015-01-03 19:30:25 PST
I have a patch to make the XPCShell tests run for both libraries, although there's currently a number of failures when using ical.js.

Mostly they are because ical.js returns YYYY-MM-DDThh:mm:ssZ and the test expects YYYYMMDDThhmmssZ. Not sure if the test should just be adapted to accommodate.

There are a few other failures I haven't looked into further.
Comment 1 User image Geoff Lankow (:darktrojan) 2015-01-04 00:35:06 PST
Created attachment 8543593 [details] [diff] [review]
1117456-1.diff
Comment 2 User image Philipp Kewisch [:Fallen] 2015-01-04 03:18:53 PST
Comment on attachment 8543593 [details] [diff] [review]
1117456-1.diff

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

(In reply to Geoff Lankow (:darktrojan) from comment #0)
> I have a patch to make the XPCShell tests run for both libraries, although
> there's currently a number of failures when using ical.js.
Wow this is a slick solution, I really like it. I always wanted the tests to run, but never though it would be so easy.

> 
> Mostly they are because ical.js returns YYYY-MM-DDThh:mm:ssZ and the test
> expects YYYYMMDDThhmmssZ. Not sure if the test should just be adapted to
> accommodate.
The wrapper should be adjusted, afaik. ical.js rightfully returns YYYY-MM-DDThh:mm:ssZ because in jCal we decided this format is more commonly used on the internets. libical, and therefore the rest of Lightning, uses the pure iCalendar format YYYYMMDDThhmmssZ.

> 
> There are a few other failures I haven't looked into further.
We should definitely fix them before pushing this patch. There was a time when all tests passed, so we might need some more specific tests that pinpoint issues a little better. I have the feeling our tests are really hard to debug sometimes because they are so broad.

::: calendar/test/unit/xpcshell-icaljs.ini
@@ +1,5 @@
> +[DEFAULT]
> +head = head_icaljs.js head_consts.js
> +tail =
> +run-sequentially = Avoid bustage.
> +dupe-manifest =

What is dupe-manifest?

::: calendar/test/unit/xpcshell-libical.ini
@@ +1,4 @@
> +[DEFAULT]
> +head = head_consts.js
> +tail =
> +run-sequentially = Avoid bustage.

In another bug we should see if we can remove this line and run the tests in parallel again.
Comment 3 User image Geoff Lankow (:darktrojan) 2015-01-04 15:16:58 PST
Created attachment 8543696 [details] [diff] [review]
1117456-fixes-1.diff

This patch makes some minor adjustments to the tests and the wrapper so that the tests pass. I've also uplifted some changes from ical.js that haven't yet landed on c-c.
Comment 4 User image Philipp Kewisch [:Fallen] 2015-01-04 15:39:50 PST
Comment on attachment 8543696 [details] [diff] [review]
1117456-fixes-1.diff

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

Could you also do a try-run with this patch before pushing?

::: calendar/base/backend/icaljs/calICSService.js
@@ +30,5 @@
>      get parent() this.innerObject.parent,
>      toString: function() this.innerObject.toICAL(),
>  
> +    get value() {
> +        if (this.innerObject.type == "text") {

I re-read the comment in calIICSService, and it seems it should be unescaped not only for text types but also for x- properties. Is this really the case or is that an obsolete assumption?

Maybe you could improve the IDL documentation for value and valueAsIcalString so its easier to understand and also add a short inline comment while you are at it?
Comment 5 User image Geoff Lankow (:darktrojan) 2015-01-04 17:10:15 PST
(In reply to Philipp Kewisch [:Fallen] from comment #4)
> I re-read the comment in calIICSService, and it seems it should be unescaped
> not only for text types but also for x- properties. Is this really the case
> or is that an obsolete assumption?

I just did a quick check, and innerObject.type is "text" for x- properties. So this works as expected.

Note You need to log in before you can comment on or make changes to this bug.