Fix timezone definition test

RESOLVED FIXED in 4.5

Status

Calendar
Internal Components
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

Trunk

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

2 years ago
Created attachment 8645763 [details] [diff] [review]
Fix - v1
Attachment #8645763 - Flags: review?(makemyday)

Comment 2

2 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+
(Assignee)

Comment 3

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

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

Comment 5

2 years ago
Created attachment 8650358 [details] [diff] [review]
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.
Attachment #8645763 - Attachment is obsolete: true
Attachment #8650358 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 6

2 years ago
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

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

Comment 8

2 years ago
Pushed to comm-central changeset 045d1e8fe4fc
Status: ASSIGNED → RESOLVED
Last Resolved: 2 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.