1117456 - Run unit tests on ical.js as well as libical

Status: RESOLVED FIXED

(Calendar :: ICAL.js Integration, defect)

Description

[Geoff Lankow (:darktrojan)]

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

[Geoff Lankow (:darktrojan)]

Attachment: 1117456-1.diff

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

[Geoff Lankow (:darktrojan)]

Attachment: 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

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

[Geoff Lankow (:darktrojan)]

(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.

Comment 6

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/comm-central/rev/6b286c943119
https://hg.mozilla.org/comm-central/rev/c92b8904d33e