Last Comment Bug 1192883 - Fix timezone definition test
: Fix timezone definition test
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: Internal Components (show other bugs)
: Trunk
: Unspecified Unspecified
-- normal (vote)
: 4.5
Assigned To: Philipp Kewisch [:Fallen]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2015-08-10 09:15 PDT by Philipp Kewisch [:Fallen]
Modified: 2015-08-25 12:55 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix - v1 (4.53 KB, patch)
2015-08-10 09:15 PDT, Philipp Kewisch [:Fallen]
makemyday: review+
Details | Diff | Splinter Review
Fix - v2 (4.76 KB, patch)
2015-08-20 02:28 PDT, Philipp Kewisch [:Fallen]
philipp: review+
Details | Diff | Splinter Review

Description User image Philipp Kewisch [:Fallen] 2015-08-10 09:15:12 PDT
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 User image Philipp Kewisch [:Fallen] 2015-08-10 09:15:56 PDT
Created attachment 8645763 [details] [diff] [review]
Fix - v1
Comment 2 User image [:MakeMyDay] 2015-08-16 11:33:04 PDT
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 User image Philipp Kewisch [:Fallen] 2015-08-19 14:06:14 PDT
(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 User image [:MakeMyDay] 2015-08-19 23:38:50 PDT
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 User image Philipp Kewisch [:Fallen] 2015-08-20 02:28:08 PDT
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.
Comment 6 User image Stefan Sitter 2015-08-20 10:32:27 PDT
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 User image [:MakeMyDay] 2015-08-20 13:18:16 PDT
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 User image Philipp Kewisch [:Fallen] 2015-08-25 02:07:07 PDT
Pushed to comm-central changeset 045d1e8fe4fc

Note You need to log in before you can comment on or make changes to this bug.