Run unit tests on ical.js as well as libical

RESOLVED FIXED in 3.9

Status

Calendar
ICAL.js Integration
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

(Blocks: 1 bug)

unspecified

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8543593 [details] [diff] [review]
1117456-1.diff
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attachment #8543593 - Flags: review?(philipp)
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.
Attachment #8543593 - Flags: review?(philipp) → review+
(Assignee)

Comment 3

2 years ago
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.
Attachment #8543696 - Flags: review?(philipp)
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?
Attachment #8543696 - Flags: review?(philipp) → review+
(Assignee)

Comment 5

2 years ago
(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.
(Assignee)

Comment 6

2 years ago
https://hg.mozilla.org/comm-central/rev/6b286c943119
https://hg.mozilla.org/comm-central/rev/c92b8904d33e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.9
You need to log in before you can comment on or make changes to this bug.