Repository: metomi/isodatetime (language: python)

Issue:
get_local_time_zone() broken for time zones with minutes west of UT
The timezone functions are not working properly for non-standard timezones behind UTC that have either:
- zero hours but non-zero minutes (e.g. -00:30)
- minutes that aren't 0 or 30 (e.g. -02:15)

#### Steps to reproduce

(Note: the `TZ` env var +/- convention is backwards, so that Eastern Standard Time is written `TZ=EST+05:00` instead of UTC-05:00)

```
$ TZ=XXX+02:15 python
>>> from metomi.isodatetime import timezone
>>> timezone.get_local_time_zone()
(-2, 45)  # should be (-2, 15) or (-2, -15)
>>> timezone.get_local_time_zone_format()
'-0245'  # should be '-0215'
```

```
$ TZ=XXX+00:30 python
>>> from metomi.isodatetime import timezone
>>> timezone.get_local_time_zone()
(0, 30)  # should be (0, -30)
>>> timezone.get_local_time_zone_format()
'+0030'  # should be '-0030'
```

#### Additional context

If the timezone is east of UTC, it works ok

```
$ TZ=XXX-02:15 python
>>> from metomi.isodatetime import timezone
>>> timezone.get_local_time_zone()
(2, 15)
>>> timezone.get_local_time_zone_format()
'+0215'
```
get_local_time_zone() broken for time zones with minutes west of UT
The timezone functions are not working properly for non-standard timezones behind UTC that have either:
- zero hours but non-zero minutes (e.g. -00:30)
- minutes that aren't 0 or 30 (e.g. -02:15)

#### Steps to reproduce

(Note: the `TZ` env var +/- convention is backwards, so that Eastern Standard Time is written `TZ=EST+05:00` instead of UTC-05:00)

```
$ TZ=XXX+02:15 python
>>> from metomi.isodatetime import timezone
>>> timezone.get_local_time_zone()
(-2, 45)  # should be (-2, 15) or (-2, -15)
>>> timezone.get_local_time_zone_format()
'-0245'  # should be '-0215'
```

```
$ TZ=XXX+00:30 python
>>> from metomi.isodatetime import timezone
>>> timezone.get_local_time_zone()
(0, 30)  # should be (0, -30)
>>> timezone.get_local_time_zone_format()
'+0030'  # should be '-0030'
```

#### Additional context

If the timezone is east of UTC, it works ok

```
$ TZ=XXX-02:15 python
>>> from metomi.isodatetime import timezone
>>> timezone.get_local_time_zone()
(2, 15)
>>> timezone.get_local_time_zone_format()
'+0215'
```


Discussion / hints:

Fix patch:
diff --git a/CHANGES.md b/CHANGES.md
index 7ac8a7a..b8be4b3 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -13,7 +13,7 @@ ones in. -->
 
 ## isodatetime 2.1.0 (<span actions:bind='release-date'>Upcoming, 2020</span>)
 
-This is the 15th release of isodatetime. Requires Python 3.5+.
+This is the 15th release of isodatetime. **Requires Python 3.6+**.
 
 ### Noteworthy Changes
 
@@ -30,6 +30,10 @@ Replaced `TimePoint.get("attribute_name")` method with individual attributes
 `TimePoint.attribute_name`. Fixed a bug in rounding decimal properties of
 TimePoints.
 
+[#193](https://github.com/metomi/isodatetime/pull/193):
+Fixed a bug where the `timezone` functions would return incorrect results
+for certain non-standard/unusual system time zones.
+
 --------------------------------------------------------------------------------
 
 ## isodatetime 2.0.2 (Released 2020-07-01)
diff --git a/metomi/isodatetime/timezone.py b/metomi/isodatetime/timezone.py
index 95dab64..9301d4a 100644
--- a/metomi/isodatetime/timezone.py
+++ b/metomi/isodatetime/timezone.py
@@ -19,7 +19,7 @@
 """This provides utilities for extracting the local time zone."""
 
 import time
-import math
+from typing import Tuple
 
 
 class TimeZoneFormatMode(object):
@@ -27,33 +27,39 @@ class TimeZoneFormatMode(object):
     reduced = "reduced"
     extended = "extended"
 
+    templates = {
+        normal: '{sign}{hh:02d}{mm:02d}',
+        reduced: '{sign}{hh:02d}',
+        extended: '{sign}{hh:02d}:{mm:02d}'
+    }
 
-def get_local_time_zone():
+
+def get_local_time_zone() -> Tuple[int, int]:
     """Return the current local UTC offset in hours and minutes."""
     utc_offset_seconds = -time.timezone
     if time.localtime().tm_isdst == 1 and time.daylight:
         utc_offset_seconds = -time.altzone
-    utc_offset_minutes = (utc_offset_seconds // 60) % 60
-    utc_offset_hours = math.floor(utc_offset_seconds / float(3600)) if \
-        utc_offset_seconds > 0 else math.ceil(utc_offset_seconds / float(3600))
-    return int(utc_offset_hours), utc_offset_minutes
+    sign = -1 if utc_offset_seconds < 0 else 1
+    utc_offset_minutes = (utc_offset_seconds // 60) % (sign * 60)
+    utc_offset_hours = sign * ((sign * utc_offset_seconds) // 3600)
+    return utc_offset_hours, utc_offset_minutes
 
 
-def get_local_time_zone_format(tz_fmt_mode=TimeZoneFormatMode.normal):
+def get_local_time_zone_format(
+    tz_fmt_mode: str = TimeZoneFormatMode.normal
+) -> str:
     """Return a string denoting the current local UTC offset.
 
     :param tz_fmt_mode:
     :type tz_fmt_mode: TimeZoneFormat:
     """
     utc_offset_hours, utc_offset_minutes = get_local_time_zone()
-    if utc_offset_hours == 0 and utc_offset_minutes == 0:
+    if utc_offset_hours == utc_offset_minutes == 0:
         return "Z"
-    reduced_timezone_template = "%s%02d"
-    timezone_template = "%s%02d%02d"
-    if tz_fmt_mode == TimeZoneFormatMode.extended:
-        timezone_template = "%s%02d:%02d"
+    if tz_fmt_mode == TimeZoneFormatMode.reduced and utc_offset_minutes != 0:
+        tz_fmt_mode = TimeZoneFormatMode.normal
     sign = "-" if (utc_offset_hours < 0 or utc_offset_minutes < 0) else "+"
-    if tz_fmt_mode == TimeZoneFormatMode.reduced and utc_offset_minutes == 0:
-        return reduced_timezone_template % (sign, abs(utc_offset_hours))
-    return timezone_template % (
-        sign, abs(utc_offset_hours), abs(utc_offset_minutes))
+    timezone_template = TimeZoneFormatMode.templates[tz_fmt_mode]
+    return timezone_template.format(
+        sign=sign, hh=abs(utc_offset_hours), mm=abs(utc_offset_minutes)
+    )
diff --git a/setup.py b/setup.py
index 65175a8..8cf70e5 100644
--- a/setup.py
+++ b/setup.py
@@ -92,7 +92,7 @@ def run(self):
     install_requires=install_requires,
     tests_require=tests_require,
     extras_require=extras_require,
-    python_requires='>=3.5',
+    python_requires='>=3.6',
     classifiers=[
         "Development Status :: 5 - Production/Stable",
         "Environment :: Other Environment",
@@ -100,10 +100,10 @@ def run(self):
         ("License :: OSI Approved" +
          " :: GNU Lesser General Public License v3 (LGPLv3)"),
         "Operating System :: OS Independent",
-        "Programming Language :: Python :: 3.5",
         "Programming Language :: Python :: 3.6",
         "Programming Language :: Python :: 3.7",
         "Programming Language :: Python :: 3.8",
+        "Programming Language :: Python :: 3.9",
         "Programming Language :: Python :: 3 :: Only",
         "Topic :: Software Development :: Libraries :: Python Modules",
         "Topic :: Utilities"


Test patch:
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index fe6c7b6..76d4794 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -12,7 +12,7 @@ jobs:
     strategy:
       matrix:
         os: [ubuntu-latest]
-        python-version: ['3.5', '3.6', '3.7', '3.8', '3.9', '3.10']
+        python-version: ['3.6', '3.7', '3.8', '3.9', '3.10']
         coverage: [false]
         include:
           # Modify existing configurations:
@@ -50,7 +50,7 @@ jobs:
         env:
           TZ: ${{ matrix.tz }}
         run: |
-          PYTEST_ARGS=('-n' '2')
+          PYTEST_ARGS=(-n 2 -m 'slow or not slow')
           if ${{ matrix.coverage }}; then
             PYTEST_ARGS+=('--cov=metomi/isodatetime')
           fi
diff --git a/metomi/isodatetime/tests/conftest.py b/metomi/isodatetime/tests/conftest.py
new file mode 100644
index 0000000..3905a56
--- /dev/null
+++ b/metomi/isodatetime/tests/conftest.py
@@ -0,0 +1,41 @@
+# Copyright (C) British Crown (Met Office) & Contributors.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+import pytest
+import time
+from unittest.mock import Mock
+
+
+@pytest.fixture
+def mock_local_time_zone(monkeypatch: pytest.MonkeyPatch):
+    """A pytest fixture that simulates a given local system time zone.
+
+    Args:
+        seconds: The number of seconds UTC offset for the base time zone.
+        dst_seconds: The number of seconds UTC offset for the daylight savings
+            time zone.
+
+    Note: these args are positive for time zones East of the prime meridian
+    and negative for West.
+    """
+    def _mock_local_time_zone(seconds: int, dst_seconds: int = 0) -> None:
+        is_dst = 1 if dst_seconds else 0
+        mock_time = Mock(spec=time)
+        mock_time.timezone = -seconds
+        mock_time.altzone = -dst_seconds
+        mock_time.daylight = is_dst
+        mock_time.localtime.return_value = Mock(tm_isdst=is_dst)
+        monkeypatch.setattr('metomi.isodatetime.timezone.time', mock_time)
+    return _mock_local_time_zone
diff --git a/metomi/isodatetime/tests/test_00.py b/metomi/isodatetime/tests/test_00.py
index bfe796d..dab5637 100644
--- a/metomi/isodatetime/tests/test_00.py
+++ b/metomi/isodatetime/tests/test_00.py
@@ -1,5 +1,3 @@
-# -*- coding: utf-8 -*-
-# ----------------------------------------------------------------------------
 # Copyright (C) British Crown (Met Office) & Contributors.
 #
 # This program is free software: you can redistribute it and/or modify
@@ -14,7 +12,6 @@
 #
 # You should have received a copy of the GNU Lesser General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
-# ----------------------------------------------------------------------------
 
 """This tests the ISO 8601 parsing and data model functionality."""
 
@@ -23,13 +20,11 @@
 from itertools import chain
 import unittest
 import pytest
-from unittest.mock import patch, MagicMock, Mock
 
 from metomi.isodatetime import data
 from metomi.isodatetime import dumpers
 from metomi.isodatetime import parsers
 from metomi.isodatetime import parser_spec
-from metomi.isodatetime import timezone
 from metomi.isodatetime.exceptions import (
     ISO8601SyntaxError, TimePointDumperBoundsError, BadInputError)
 
@@ -1479,126 +1474,6 @@ def test_invalid_timerecurrence(self):
             with pytest.raises(BadInputError):
                 data.TimeRecurrence(**kwargs)
 
-    # data provider for the test test_get_local_time_zone_no_dst
-    # the format for the parameters is
-    # [tz_seconds, expected_hours, expected_minutes]]
-    get_local_time_zone_no_dst = [
-        [45900, 12, 45],  # pacific/chatham, +12:45
-        [20700, 5, 45],  # asia/kathmandu, +05:45
-        [3600, 1, 0],  # arctic/longyearbyen, +01:00
-        [0, 0, 0],  # UTC
-        [-10800, -3, 0],  # america/sao_paulo, -03:00
-        [-12600, -3, 30]  # america/st_johns, -03:30
-    ]
-
-    @patch('metomi.isodatetime.timezone.time')
-    def test_get_local_time_zone_no_dst(self, mock_time):
-        """Test that the hour/minute returned is correct.
-
-        Parts of the time module are mocked so that we can specify scenarios
-        without daylight saving time."""
-        for tz_seconds, expected_hours, expected_minutes in \
-                self.get_local_time_zone_no_dst:
-            # for a pre-defined timezone
-            mock_time.timezone.__neg__.return_value = tz_seconds
-            # time without dst
-            mock_time.daylight = False
-            # and localtime also without dst
-            mock_localtime = Mock()
-            mock_time.localtime.return_value = mock_localtime
-            mock_localtime.tm_isdst = 0
-            hours, minutes = timezone.get_local_time_zone()
-            self.assertEqual(expected_hours, hours)
-            self.assertEqual(expected_minutes, minutes)
-
-    # data provider for the test test_get_local_time_zone_with_dst
-    # the format for the parameters is
-    # [tz_seconds, tz_alt_seconds, expected_hours, expected_minutes]
-    get_local_time_zone_with_dst = [
-        [45900, 49500, 13, 45],  # pacific/chatham, +12:45 and +13:45
-        [43200, 46800, 13, 0],  # pacific/auckland, +12:00 and +13:00
-        [3600, 7200, 2, 0],  # arctic/longyearbyen, +01:00
-        [0, 0, 0, 0],  # UTC
-        [-10800, -7200, -2, 0],  # america/sao_paulo, -03:00 and -02:00,
-        [-12600, -9000, -2, 30]  # america/st_johns, -03:30 and -02:30
-    ]
-
-    @patch('metomi.isodatetime.timezone.time')
-    def test_get_local_time_zone_with_dst(self, mock_time):
-        """Test that the hour/minute returned is correct
-
-        Parts of the time module are mocked so that we can specify scenarios
-        with daylight saving time."""
-        for tz_seconds, tz_alt_seconds, expected_hours, expected_minutes in \
-                self.get_local_time_zone_with_dst:
-            # for a pre-defined timezone
-            mock_time.timezone.__neg__.return_value = tz_seconds
-            # time without dst
-            mock_time.daylight = True
-            # and localtime also without dst
-            mock_localtime = MagicMock()
-            mock_time.localtime.return_value = mock_localtime
-            mock_localtime.tm_isdst = 1
-            # and with the following alternative time for when dst is set
-            mock_time.altzone.__neg__.return_value = tz_alt_seconds
-            hours, minutes = timezone.get_local_time_zone()
-            self.assertEqual(expected_hours, hours)
-            self.assertEqual(expected_minutes, minutes)
-
-    # data provider for the test test_get_local_time_zone_format
-    # the format for the parameters is
-    # [tz_seconds, tz_format_mode, expected_format]
-    get_test_get_local_time_zone_format = [
-        # UTC, returns Z, flags are never used for UTC
-        [0, timezone.TimeZoneFormatMode.normal, "Z"],
-        # Positive values, some with minutes != 0
-        # asia/macao, +08:00
-        [28800, timezone.TimeZoneFormatMode.normal, "+0800"],
-        # asia/macao, +08:00
-        [28800, timezone.TimeZoneFormatMode.extended, "+08:00"],
-        # asia/macao, +08:00
-        [28800, timezone.TimeZoneFormatMode.reduced, "+08"],
-        # pacific/chatham, +12:45
-        [45900, timezone.TimeZoneFormatMode.normal, "+1245"],
-        # pacific/chatham, +12:45
-        [45900, timezone.TimeZoneFormatMode.extended, "+12:45"],
-        # pacific/chatham, +12:45
-        [45900, timezone.TimeZoneFormatMode.reduced, "+1245"],
-        # Negative values, some with minutes != 0
-        # america/buenos_aires, -03:00
-        [-10800, timezone.TimeZoneFormatMode.normal, "-0300"],
-        # america/buenos_aires, -03:00
-        [-10800, timezone.TimeZoneFormatMode.extended, "-03:00"],
-        # america/buenos_aires, -03:00
-        [-10800, timezone.TimeZoneFormatMode.reduced, "-03"],
-        # america/st_johns, -03:30
-        [-12600, timezone.TimeZoneFormatMode.normal, "-0330"],
-        # america/st_johns, -03:30
-        [-12600, timezone.TimeZoneFormatMode.extended, "-03:30"],
-        # america/st_johns, -03:30
-        [-12600, timezone.TimeZoneFormatMode.reduced, "-0330"]
-    ]
-
-    @patch('metomi.isodatetime.timezone.time')
-    def test_get_local_time_zone_format(self, mock_time):
-        """Test that the UTC offset string format is correct
-
-        Parts of the time module are mocked so that we can specify scenarios
-        with certain timezone seconds offsets. DST is not really
-        important for this test case"""
-        for tz_seconds, tz_format_mode, expected_format in \
-                self.get_test_get_local_time_zone_format:
-            # for a pre-defined timezone
-            mock_time.timezone.__neg__.return_value = tz_seconds
-            # time without dst
-            mock_time.daylight = False
-            # and localtime also without dst
-            mock_localtime = Mock()
-            mock_time.localtime.return_value = mock_localtime
-            mock_localtime.tm_isdst = 0
-            tz_format = timezone.get_local_time_zone_format(tz_format_mode)
-            self.assertEqual(expected_format, tz_format)
-
     def test_timepoint_dump_format(self):
         """Test the timepoint format dump when values are programmatically
         set to None"""
diff --git a/metomi/isodatetime/tests/test_timezone.py b/metomi/isodatetime/tests/test_timezone.py
new file mode 100644
index 0000000..1c29505
--- /dev/null
+++ b/metomi/isodatetime/tests/test_timezone.py
@@ -0,0 +1,155 @@
+# Copyright (C) British Crown (Met Office) & Contributors.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+"""This tests the timezone module."""
+
+from typing import Callable, Tuple
+import pytest
+
+from metomi.isodatetime import timezone
+
+
+@pytest.mark.parametrize('dst', [True, False])
+@pytest.mark.parametrize(
+    'tz_seconds, expected_no_dst, tz_dst_seconds, expected_with_dst',
+    [
+        pytest.param(
+            45900, (12, 45),
+            49500, (13, 45),
+            id="pacific/chatham, +12:45 and +13:45"
+        ),
+        pytest.param(
+            20700, (5, 45),
+            20700, (5, 45),
+            id="asia/kathmandu, +05:45"
+        ),
+        pytest.param(
+            3600, (1, 0),
+            7200, (2, 0),
+            id="europe/oslo, +01:00 and +02:00"
+        ),
+        pytest.param(
+            0, (0, 0),
+            0, (0, 0),
+            id="UTC"
+        ),
+        pytest.param(
+            0, (0, 0),
+            3600, (1, 0),
+            id="europe/london, +00:00 and +01:00"
+        ),
+        pytest.param(
+            -12600, (-3, -30),
+            -9000, (-2, -30),
+            id="america/st_johns, -03:30 and -02:30"
+        ),
+        pytest.param(
+            -5*3600, (-5, 0),
+            -4*3600, (-4, 0),
+            id="america/new_york, -05:00 and -04:00,"
+        ),
+        pytest.param(
+            -8100, (-2, -15),
+            -8100, (-2, -15),
+            id="hypothetical -02:15"
+        ),
+        pytest.param(
+            -1800, (0, -30),
+            -2400, (0, -40),
+            id="hypothetical -0:30 and -0:40"
+        ),
+    ]
+)
+def test_get_local_time_zone(
+    dst: bool,
+    tz_seconds: int,
+    expected_no_dst: Tuple[int, int],
+    tz_dst_seconds: int,
+    expected_with_dst: Tuple[int, int],
+    mock_local_time_zone: Callable
+):
+    """Test that the hour/minute returned is correct.
+
+    Params:
+        dst: Whether to simulate daylight savings time.
+        tz_seconds: Time zone UTC offset when not in DST.
+        expected_no_dst: Expected return value when in DST.
+        tz_dst_seconds: Time zone UTC offset when in DST.
+        expected_with_dst: Expected return value when not in DST.
+    """
+    mock_local_time_zone(
+        tz_seconds,
+        tz_dst_seconds if dst else 0
+    )
+    expected = expected_with_dst if dst else expected_no_dst
+    assert timezone.get_local_time_zone() == expected
+
+
+@pytest.mark.parametrize(
+    'tz_seconds, expected_formats',
+    [
+        pytest.param(
+            0, ('Z', 'Z', 'Z'),  # flags are never used for UTC
+            id="UTC"
+        ),
+        # Positive values, some with minutes != 0
+        pytest.param(
+            28800, ('+0800', '+08:00', '+08'),
+            id="asia/macao"
+        ),
+        pytest.param(
+            45900, ('+1245', '+12:45', '+1245'),
+            id="pacific/chatham"
+        ),
+        # Negative values, some with minutes != 0
+        pytest.param(
+            -3*3600, ('-0300', '-03:00', '-03'),
+            id="america/buenos_aires"
+        ),
+        pytest.param(
+            -12600, ('-0330', '-03:30', '-0330'),
+            id="america/st_johns"
+        ),
+        pytest.param(
+            -8100, ('-0215', '-02:15', '-0215'),
+            id="hypothetical -02:15"
+        ),
+        pytest.param(
+            -1800, ('-0030', '-00:30', '-0030'),
+            id="hypothetical -00:30"
+        ),
+    ]
+)
+def test_get_local_time_zone_format(
+    tz_seconds: int,
+    expected_formats: Tuple[int, int, int],
+    mock_local_time_zone: Callable
+):
+    """Test that the UTC offset string format is correct.
+
+    Params:
+        tz_seconds: The time zone's UTC offset.
+        expected_formats: The expected return values for normal, extended
+            and reduced formats, respectively.
+    """
+    # DST is not really important for this test
+    mock_local_time_zone(tz_seconds)
+    for i, tz_format_mode in enumerate([
+        timezone.TimeZoneFormatMode.normal,
+        timezone.TimeZoneFormatMode.extended,
+        timezone.TimeZoneFormatMode.reduced
+    ]):
+        expected = expected_formats[i]
+        assert timezone.get_local_time_zone_format(tz_format_mode) == expected
diff --git a/pytest.ini b/pytest.ini
index c11439f..887cbf9 100644
--- a/pytest.ini
+++ b/pytest.ini
@@ -1,5 +1,3 @@
-# -*- coding: utf-8 -*-
-# ----------------------------------------------------------------------------
 # Copyright (C) British Crown (Met Office) & Contributors.
 #
 # This program is free software: you can redistribute it and/or modify
@@ -14,10 +12,16 @@
 #
 # You should have received a copy of the GNU Lesser General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
-# ----------------------------------------------------------------------------
 
 [pytest]
-addopts = -v -s -ra --color=auto --doctest-glob='*.md'
-# Some tests fail when running in a different time zone (e.g. Pacific/Auckland)
-markers = slow
-testpaths = metomi/isodatetime/tests
+addopts =
+    -v
+    -s
+    -ra
+    --color=auto
+    --doctest-glob='*.md'
+    -m 'not slow'
+markers =
+    slow: mark a test as slow - it will be skipped by default (use '-m "slow or not slow"' to run all tests)
+testpaths =
+    metomi/isodatetime/tests