Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Run unit tests on ical.js as well as libical

RESOLVED FIXED in 3.9

Status

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

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

(Blocks: 1 bug)

unspecified

Details

Attachments

(2 attachments)

(Assignee)

Description

3 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

3 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

3 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

3 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

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