Closed Bug 1192883 Opened 5 years ago Closed 5 years ago

Fix timezone definition test

Categories

(Calendar :: Internal Components, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Fallen, Assigned: Fallen)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Fix - v1 (obsolete) β€” β€” Splinter Review
Attachment #8645763 - Flags: review?(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?
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).
I was thinking of something like ok(tz.longitude && tz.longitude.match(/^[+-]\d{7}$/), "...") instead, but it's up to you to change this.
Attached 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+
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.
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
Closed: 5 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.