Fix timezone definition test

RESOLVED FIXED in 4.5

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

Trunk

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Posted patch Fix - v1 (obsolete) β€” β€” Splinter Review
Attachment #8645763 - Flags: review?(makemyday)

Comment 2

4 years ago
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?
Attachment #8645763 - Flags: review?(makemyday) → review+
(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

4 years ago
I was thinking of something like ok(tz.longitude && tz.longitude.match(/^[+-]\d{7}$/), "...") instead, but it's up to you to change this.
Posted patch Fix - v2 β€” β€” Splinter Review
(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.
Attachment #8645763 - Attachment is obsolete: true
Attachment #8650358 - Flags: review+
Keywords: checkin-needed
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

4 years ago
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
Pushed to comm-central changeset 045d1e8fe4fc
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 4.4
Target Milestone: 4.4 → 4.5
You need to log in before you can comment on or make changes to this bug.