1192883 - Fix timezone definition test

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

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

The test passes, but only because the do_test_pending/do_test_finished is missing :-) This patch should fix it, and also adds the missing idl.

I needed to change the .ics attribute in the test, this is not exposed directly. Please let me know if the test still does what you planned with it.

Comment 1

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

Attachment: Fix - v1

Comment 2

[[:MakeMyDay]]

Comment on attachment 8645763 [details] [diff] [review]
Fix - v1

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

Thanks for looking into this. r=me with the comments below considered.

::: calendar/base/public/calITimezoneProvider.idl
@@ +8,5 @@
>  interface calITimezone;
>  
>  [scriptable, uuid(0E502BF5-4FD3-4090-9122-F1EC3CA701BB)]
>  interface calITimezoneProvider : nsISupports
>  {

You need a new guid here when changing the interface.

::: calendar/test/unit/test_timezone_definition.js
@@ +24,5 @@
> +        ok(tz.icalComponent.serializeToICS().startsWith("BEGIN:VTIMEZONE"),
> +                 "ics property contains VTIMEZONE for " + aZoneId);
> +        notEqual(tz.latitude.match(/^[+-]\d{7}$/), null, "Correct latitude on " + aZoneId);
> +        notEqual(tz.longitude.match(/^[+-]\d{7}$/), null, "Correct longitude on " + aZoneId);
> +    }

The checks would fail with a type error if tz.latitude or tz.longitude is null. Can we include a check for null here?

Comment 3

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

(In reply to MakeMyDay from comment #2)
> You need a new guid here when changing the interface.
Good catch!


> The checks would fail with a type error if tz.latitude or tz.longitude is
> null. Can we include a check for null here?

I believe all timezones must have a latitude/longitude, right? If so, then I believe this will be fine. If a zone does not have lat/lng, it is an error and the test will fail with "tz.latitude is undefined". Saves an extra line and has the same effect as if I had also used ok(!!tz.latitude).

Comment 4

[[:MakeMyDay]]

I was thinking of something like ok(tz.longitude && tz.longitude.match(/^[+-]\d{7}$/), "...") instead, but it's up to you to change this.

Comment 5

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

Attachment: Fix - v2

(In reply to MakeMyDay from comment #4)
> I was thinking of something like ok(tz.longitude &&
> tz.longitude.match(/^[+-]\d{7}$/), "...") instead, but it's up to you to
> change this.

Ok, sounds fine to me.

Comment 6

[Stefan Sitter [:ssitter]]

Out of curiosity, how did you get latitude and longitude on timezone? I checked some timezones exported from Lightning and none of them has those properties. I find those names only as part of the GEO property in RFC 5545 but it seems this property is only allowed on VEVENT and VTODO.

Comment 7

[[:MakeMyDay]]

This is returned by cal.getTimezone(), but is not exposed as a property of the vtimezone component:

http://mxr.mozilla.org/comm-central/source/calendar/base/src/calTimezoneService.js#182

Comment 8

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

Pushed to comm-central changeset 045d1e8fe4fc