apjanke / octave-tablicious

Table (relational, tabular data) implementation for GNU Octave
https://apjanke.github.io/octave-tablicious/
GNU General Public License v3.0
29 stars 11 forks source link

datetime test failure: "Undefined TimeZone: America/New_York" #125

Closed mmuetzel closed 8 months ago

mmuetzel commented 8 months ago

pkg test is failing for @datetime/datetime.m with the following error:

  >>>>> processing /usr/share/octave/packages/tablicious-0.4.2/@datetime/datetime.m
  ***** test datetime ('2011-03-07 12:34:56', 'TimeZone', 'America/New_York');
  !!!!! test failed
  Undefined TimeZone: America/New_York
  ***** test
    d = datetime;
    d.TimeZone = 'America/New_York';
    d2 = d;
    d2.TimeZone = 'America/Chicago';
    assert (abs (datenum (d) - datenum (d2)), (1/24), .0001)
  !!!!! test failed
  Undefined TimeZone: America/New_York

Not sure what that means.

See the CI tests over at https://github.com/gnu-octave/packages/pull/401


Andrew's notes

Turns out this is what happens when you run on a Linux box that doesn't have tzdb installed (package tzdata on Ubuntu). We can at least provide a better error message here.

Handling that under separate ticket https://github.com/apjanke/octave-tablicious/issues/128.

apjanke commented 8 months ago

Undefined TimeZone: America/New_York

Hmm. That looks like it's not finding the time zone data in its tzdb, and producing a not-great error message. On Mac and Linux, Tablicious uses the system tzdb provided by the OS, not an internal database bundled with the package.

What OS and distro is this on? Maybe I'm making a bad assumption about where the tzdb data is stored on some Linux distros, and that's showing up as it not finding the particular tzdb file when it's looking at the wrong path, instead of it noticing the entire tzdb database is not located where expected.

mmuetzel commented 8 months ago

What OS and distro is this on?

Afaict, the tests on octave/packages are run on an Ubuntu based docker image built with the rules from https://github.com/gnu-octave/docker. Do they need to install some package for that database to be available?

Fwiw, I'm seeing the following with the tablicious package (currently still version 0.3.7) using the 1st release candidate of Octave 9 on Windows:

Testing functions in package 'tablicious':

Integrated test scripts:

                                                                                  [ CPU    /  CLOCK ]
  ...90\mingw64\share\octave\packages\tablicious-0.3.7\datetime.m  pass    2/4    [ 0.609s /  1.183s]
                                                                   FAIL    2
  ...90\mingw64\share\octave\packages\tablicious-0.3.7\duration.m  pass    3/3    [ 0.016s /  0.047s]

Fixed test scripts:

Failure Summary:

  ...90\mingw64\share\octave\packages\tablicious-0.3.7\datetime.m  pass    2/4
                                                                   FAIL    2

Summary:

  PASS                                5
  FAIL                                2

See the file C:\Users\Markus\fntests.log for additional details.

270 (of 272) .m files have no tests.

Please help improve Octave by contributing tests for these files
(see the list in the file C:\Users\Markus\fntests.log).

Respective part of fntests.log:

>>>>> processing C:\Program Files\GNU Octave\Octave-9.0.90\mingw64\share\octave\packages\tablicious-0.3.7\datetime.m
***** test datetime ('2011-03-07 12:34:56', 'TimeZone','America/New_York');
!!!!! test failed
POSIX zone rules are unimplemented
***** test
  d = datetime;
  d.TimeZone = 'America/New_York';
  d2 = d;
  d2.TimeZone = 'America/Chicago';
  assert (abs(d.dnums - d2.dnums), (1/24), .0001)
!!!!! test failed
ASSERT errors for:  assert (abs (d.dnums - d2.dnums),(1 / 24),.0001)

  Location  |  Observed  |  Expected  |  Reason
     ()           0         0.041667     Abs err 0.041667 exceeds tol 0.0001 by 0.04
>>>>> processing C:\Program Files\GNU Octave\Octave-9.0.90\mingw64\share\octave\packages\tablicious-0.3.7\duration.m

Maybe, it is still missing the same dependency? Or a different issue?

mmuetzel commented 8 months ago

Do they need to install some package for that database to be available?

To answer my own question: The test passes after installing the Ubuntu package tzdata.

apjanke commented 8 months ago

To answer my own question: The test passes after installing the Ubuntu package tzdata.

Ah, cool. That'll do it. On Ubuntu, those tzdb files are from package tzdata. On my Ubuntu, they're at the expected /usr/share/zoneinfo location

janke@ubix:/usr/share/zoneinfo$ cat /etc/lsb-release 
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=22.04
DISTRIB_CODENAME=jammy
DISTRIB_DESCRIPTION="Ubuntu 22.04.3 LTS"
janke@ubix:/usr/share/zoneinfo$ dpkg -S /usr/share/zoneinfo/America/New_York 
tzdata: /usr/share/zoneinfo/America/New_York
janke@ubix:/usr/share/zoneinfo$ 

I'm kinda surprised that the Ubuntu Docker image would have been missing the tzdata package in the first place. It's such a basic-seeming package that I would have expected it to be in even a minimal Linux image. But maybe minimal Linux images are really minimal, for use on stuff like embedded systems, or just super-lightweight containers.

I (and thus Tablicious) had assumed that tzdata would be present on any Linux system, and thus searches pretty unconditionally. That's not great, as we saw here.

A couple things I can do:

A) Do more sophisticated checking to see if the tzdb database as a whole is absent, as opposed to a specific timezone file being absent which indicates an invalid timezone identifier, and raise a better error message in that case. That would surely be good.

B) On Linux systems that are missing the tzdb db at the system level, Tablicious could fall back to using the bundled tzdb it ships with, like it does on Windows. That would get it to run in oddball locations like this. But I'm not sure if it's a great idea: it would then be using different code paths, and the tzdb it ships with may be stale, so that might lead to slightly different results, affecting test runners like this, and maybe oddball users who lack tzdb. Have to think about this one for a bit before doing it.

For option A, I'll put that together. This'll be a learning experience figuring out how to build an Ubuntu VM that doesn't have tzdata for testing it – even the ubuntu-minimal package requires tzdata, it seems.

janke@ubix:/usr/share/zoneinfo$ apt-cache rdepends --installed tzdata
tzdata
Reverse Depends:
  ubuntu-minimal
  python3-tz
  libmozjs-91-0
  ubuntu-minimal
  python3-tz
  python3-dateutil
  libtcl8.6
  libmozjs-91-0
  libical3

Guess I should just learn Docker and build off the version of your base image that lacked tzdata in the first place?

mmuetzel commented 8 months ago

Maybe, the Octave Docker image is really the odd ball out. No idea why it doesn't come with that package pre-installed. I don't know much about Docker either. So unfortunately, I can't help with setting it up.

Fwiw, I updated to the latest version of tablicious on Windows (using the release candidate Octave 9.0.90). All tests are passing. 🎉

>> pkg install -forge tablicious
>> pkg test tablicious
Testing functions in package 'tablicious':

Integrated test scripts:

                                                                                  [ CPU    /  CLOCK ]
  ..octave\api-v58\packages\tablicious-0.4.2\@datetime\datetime.m  pass    4/4    [ 0.875s /  1.173s]
  ..octave\api-v58\packages\tablicious-0.4.2\@duration\duration.m  pass    3/3    [ 0.047s /  0.040s]
  ..ing\octave\api-v58\packages\tablicious-0.4.2\@string\string.m  pass    6/6    [ 0.062s /  0.085s]
  ..ppData\Roaming\octave\api-v58\packages\tablicious-0.4.2\NaS.m  pass    1/1    [ 0.000s /  0.005s]
  ..ta\Roaming\octave\api-v58\packages\tablicious-0.4.2\missing.m  pass    2/2    [ 0.031s /  0.020s]

Fixed test scripts:

Summary:

  PASS                               16
  FAIL                                0

See the file C:\Users\Markus\fntests.log for additional details.

286 (of 291) .m files have no tests.

Please help improve Octave by contributing tests for these files
(see the list in the file C:\Users\Markus\fntests.log).

>>

I'd be ok with closing this as invalid if you agree.

apjanke commented 8 months ago

Yeah, let's close this. Seems like quite an unusual case, so not urgent to handle, and the immediate problem is fixed.

I'll look at a long-term improvement for this under separate low-priority ticket https://github.com/apjanke/octave-tablicious/issues/128, and close this one out to keep you from getting spammed with comment notifications you don't need to worry about.