1212076 - Unit tests for timezone definition are not fully working

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[[:MakeMyDay]]

The current unit tests to check the completeness and correctness of zones.json are not fully working. Only every second zone/alias is tested.

Comment 1

[[:MakeMyDay]]

Attachment: AddUnitTestForTZCompleteness-V1.diff

This patch fixes both existing tests and corrects a wrong alias.

To test not just consistency within the current zones.json file but also to check for missing zones/aliases compared to previous versions, I have added another test.

If a tz is removed, that may end up in bugs like bug 1210723. Ones bug 1181304 is implemented, that should probably cover updating the test data as well.

Comment 2

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

Comment on attachment 8670477 [details] [diff] [review]
AddUnitTestForTZCompleteness-V1.diff

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

Reviewing from mobile therefore no per-line comments. A few whitespaces snuck in in the test. Also, if the file needs updating when zones.json is updated, can you change the update script to make the needed changes?

Comment 3

[[:MakeMyDay]]

Thanks for referencing, Geoff. I noticed to fixed the test bits already. I had a try run with a modified version of this patch after the latest tz update and there currently seem to be no other issues.

So, I'll wait with checking in the remaining completeness test to integrate this into the tz update script as requested in comment 2 instead.

The only open question here is why the alias America/Indiana/Indianapolis needs to be fixed manually after running the update script.

Comment 4

[[:MakeMyDay]]

Attachment: AddUnitTestForTZCompleteness-V2.diff

Updated patch with the bits removed, Geoff already fixed in the other bug.

This patch moves the test data to a separate file and adds a method to create that file to the update-zones.py script.

I've just thrown a try build to see whether there are still any issues with the automation, esp. the loading from the separate file.

Regarding the python stuff, I'd really appreciate some feedback. I would consider my skills very basic at best and the last time I wrote something in Python is ages ago...

Comment 5

[[:MakeMyDay]]

Attachment: AddUnitTestForTZCompleteness-V3.diff

Updated patch. The Python part is not changed (I won't touch this further until I got your feedback), but some adaptation in the test code, which should work, but in fact doesn't.

The test still fails to pick up the test data from the file. I've tried different locations (test/data/previous.json, test/unit/data/previous.json and test/unit/previous.json) but to no avail. According to MDN it should just work, but it seems to me the json file must be registered somewhere to be transfered the test machine. Do you have any idea on this?

Comment 6

[Geoff Lankow (:darktrojan)]

(In reply to MakeMyDay from comment #5)
> The test still fails to pick up the test data from the file. I've tried
> different locations (test/data/previous.json, test/unit/data/previous.json
> and test/unit/previous.json) but to no avail. According to MDN it should
> just work, but it seems to me the json file must be registered somewhere to
> be transfered the test machine. Do you have any idea on this?

Add this line to xpcshell-icaljs.ini and xpcshell-libical.ini (in the [DEFAULT] section):

> support-files = data/**

Comment 7

[Geoff Lankow (:darktrojan)]

Comment on attachment 8703457 [details] [diff] [review]
AddUnitTestForTZCompleteness-V3.diff

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

I've had a look through this. It seems good and the test passes. I've written a few things to think about until Philipp gets to it.

Also, be careful with your whitespace. I found Windows line-endings and some dodgy indentation. :-)

::: calendar/test/unit/head_consts.js
@@ +192,5 @@
> + */
> +function readJSONFile(aFile) {
> +    let stream = Cc["@mozilla.org/network/file-input-stream;1"].createInstance(Ci.nsIFileInputStream);
> +  try {
> +      stream.init(aFile, MODE_RDONLY, FileUtils.PERMS_FILE, 0);

I found I needed to import FileUtils.jsm for this, and use FileUtils.MODE_RDONLY.

::: calendar/test/unit/test_timezone_definition.js
@@ +33,5 @@
> + *                       1 - same version
> + *                       2 - first is older
> + *                       3 - second is older
> + */
> +function compare_tz_version(aFirst, aSecond) {

You could use Services.vc.compare here and avoid needing these two functions.

::: calendar/timezones/update-zones.py
@@ +222,5 @@
> +                test_aliases.append(alias_key)
> +    
> +            test_zones = []
> +            for zone_key in current_zones:
> +                test_zones.append(zone_key)

You could write this as test_zones = current_zones.keys()

@@ +243,4 @@
>      def run(self, zones_json_file, tzprops_file, vzic_path):
>          """Run the timezone updater, with a zones.json file and the path to vzic"""
> +
> +        self.create_test_data(zones_json_file)

You should make a comment about why this happens before everything else. I'd also move the function out of TimezoneUpdater and call it from main(), as it doesn't depend on anything in TimezoneUpdater.

Comment 8

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

Comment on attachment 8703457 [details] [diff] [review]
AddUnitTestForTZCompleteness-V3.diff

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

python code looks fine, just a few minor comments:

::: calendar/timezones/update-zones.py
@@ +201,5 @@
> +    
> +        current_data = {}
> +        current_version = ""
> +        current_zones = {}
> +        current_aliases = {}

You don't need to preinitialize variables here. The context manager created using |with| doesn't create a new execution scope.

@@ +209,5 @@
> +             current_version = current_data["version"]
> +             current_zones = current_data["zones"]
> +             current_aliases = current_data["aliases"]
> +        with open(previous_file, "r") as rpf:
> +             if rpf:

rpf should be defined here. If you are concerned that the file does not exist you'd need to test for it before, or catch the exception thrown by open().

@@ +218,5 @@
> +            """Extracting data from zones.json"""
> +    
> +            test_aliases = []
> +            for alias_key in current_aliases:
> +                test_aliases.append(alias_key)

+1 on Geoff's comment regarding .keys()

Comment 9

[[:MakeMyDay]]

Attachment: AddUnitTestForTZCompleteness-V4.diff

Updated patch with nearly all comments considered. I just decided to keep the function to verify timezone db version number as this is used at two places in the file.

I did a try push, the test passes now.

Regarding the last time required additional fix for America/Indiana/Indianapolis, I assume such happened because there was a manual fix checked into the tree and before running the update script the last revision of zones.json hasn't been pulled before - so this is something we can only avoid for sure by making tzupdate part of the build process. But for this we already have a separate bug.

Comment 10

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

Comment on attachment 8716798 [details] [diff] [review]
AddUnitTestForTZCompleteness-V4.diff

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

Since this is a test-only change, it doesn't break the build and there are no hidden failures, I don't think we need to backport this. r=philipp for comm-central though.

Comment 11

[:aceman]

https://hg.mozilla.org/comm-central/rev/4f3e70f3eec4