test(alerter): fix time and timezone issues in alerter tests #1598

wyardley · 2025-02-08T19:07:35Z

Some tests wouldn't pass locally if TZ was unset, or not set to to, e.g., UTC.

One issue was the fallback to "local" if TZ was unset - this is a valid value internally within simplemonitor, but I don't think valid as a time zone.

Also, having many tests use the local timezone creates some unpredictability in cases where there are hardcoded offsets, or where the test depends on the local timezone having a specific offset from UTC.

Mock TZ as UTC or MST explicitly, and adjust existing tests based on offsets from that where needed.

Fix / unskip some skipped tests

A few comments:

~~I didn't fix all the skipped tests, but hopefully this might help identify fixes for them?~~ They're all unskipped now.
These pass for me locally now, but I'm not 100% sure that all the logic is correct, so please review carefully
Should confirm that these work in your environment, and that there's no remaining logic that would create tests that are flaky depending on the time of day or other factors.

wyardley · 2025-02-08T19:10:07Z

tests/test_alerter.py

+# compare the offset between UTC and local time. For simplicity and
+# predictability, use a time zone that doesn't have daylight savings.
+LOCAL_TZ = "MST"
+LOCAL_TZ_OFFSET = -7


I just defined these as top level constants vs. in the classes, mostly so that it would be easier to use the @freeze_time decorator in more places.

wyardley · 2025-02-08T19:10:52Z

tests/test_alerter.py

    def test_should_alert_limit(self):
        config = {
            "times_type": "only",
            "time_lower": "10:00",
            "time_upper": "11:00",
            "limit": 2,
+            "times_tz": "+07:00",


I'm not 100% sure if this is the right / desired logic. I think since we're just testing limits, as long as times_tz is set, and the offset is right, that's sufficient to test the behavior you're trying to test here?

wyardley · 2025-02-08T19:11:33Z

tests/test_alerter.py

@@ -416,12 +419,9 @@ class TestMessageBuilding(unittest.TestCase):
    def setUp(self):
        self.test_alerter = alerter.Alerter()
        self.freeze_time_value = "2020-03-10 09:00"
-        self.tz = os.environ.get("TZ", "local")


I don't think the fallback ("local") was working as expected here and in other spots. I also don't think it's necessary if we force MST for these.

jamesoff · 2025-02-08T21:19:29Z

This looks amazing, thanks! I've been finding trying to straighten these tests out a bit mind-bending. I'm on holiday at the moment but will evaluate properly when I'm back based on your notes :)

wyardley · 2025-02-08T22:18:42Z

I squashed my changes, but check out 22ed7ee if you want to see the changes / evolution.

I think the latest changes should pass in CI.

In particular, I'm not sure the skipped ones where I update times_tz is the right fix.

wyardley · 2025-02-08T23:42:14Z

tests/test_alerter.py

-        self.utcoffset = a.utcoffset()
+        # Force these ones to UTC to simplify; see also TestMessageBuildingTZ class
+        # Note: For Reasons, the decorator doesn't work well with this.
+        with mock.patch.dict(os.environ, {"TZ": "UTC"}, clear=True):


With this one only, I could induce failures by doing, e.g.,

$ TZ=EST5EDT poetry run pytest -vvv

somehow, simplemonitor/Alerters/alerter.py still would be reading os.env.TZ from the system in this case. I first tried seeing if I needed to use importlib to reload the libraries being tested, because maybe they were evaluating os.getenv() first, or overriding the decorator for setup, or that it was something related to using unittest test cases within pytest (which I'm guessing is it?); in the end, using a with in the setup routine here seemed to resolve it.

https://adamj.eu/tech/2020/10/13/how-to-mock-environment-variables-with-pythons-unittest/
https://adamj.eu/tech/2020/10/13/how-to-mock-environment-variables-with-pytest/

Fixes jamesoff#1330 Some tests wouldn't pass locally if `TZ` was unset, or not set to to, e.g., UTC. One issue was the fallback to `"local"` if `TZ` was unset - this is a valid value internally within simplemonitor, but I don't think valid as a time zone. Also, having many tests use the local timezone creates some unpredictability in cases where there are hardcoded offsets, or where the test depends on the local timezone having a specific offset from UTC. Mock `TZ` as `UTC` or `MST` explicitly, and adjust existing tests based on offsets from that where needed. Fix / unskip some skipped tests

wyardley · 2025-02-09T03:16:55Z

tests/test_alerter.py

    def test_should_alert_no_catchup(self):
+        """
+        Here, `support_catchup` is unset, so catchup notifications shouldn't be
+        sent.


I would check the docstring and behavior of both test_should_alert_no_catchup() and test_should_alert_catchup(). I think I got it right, but please check some of these to make sure they're not evergreen.

wyardley · 2025-02-09T03:17:29Z

tests/test_alerter.py

+
+# Default to UTC, then override for tests where we need to have timezone
+# specific logic.
+@mock.patch.dict(os.environ, {"TZ": TZ_UTC}, clear=True)


I originally had this as MST mostly, but decided it was simpler to use UTC in most cases, except where we're explicitly testing TZ specific logic.

wyardley · 2025-02-10T19:39:05Z

tests/test_alerter.py

            self.assertFalse(a._allowed_time())

+    # Influence time_tz indirectly, though we could also set it directly in
+    # alerter.Alerter() below.
+    @mock.patch.dict(os.environ, {"TZ": TZ_LOCAL}, clear=True)


not sure if I should leave this comment, and also whether you prefer directly manipulating time_tz in the test or patching os.environ["TZ"], or add tests for both variants?

I'd originally found the static offset being hard-coded into specific tests a little confusing, since it wasn't clear how it related to any of the timezones in use (it didn't at all), but open to suggestion here.

I have no real preference — the original approach was brute force of me trying to make it work and understand how to do testing around it simultaneously, so happy with this for now including the comment :)

jamesoff · 2025-02-15T14:49:25Z

Really appreciate you tackling this stuff :)

wyardley commented Feb 8, 2025

View reviewed changes

wyardley mentioned this pull request Feb 8, 2025

feat(monitors): add gmirror monitor #1566

Merged

wyardley force-pushed the wyardley/tz branch from 68bab82 to 7eafee7 Compare February 8, 2025 21:10

This comment was marked as outdated.

Sign in to view

wyardley force-pushed the wyardley/tz branch from 22ed7ee to 0cca4f1 Compare February 8, 2025 22:18

wyardley force-pushed the wyardley/tz branch from 0cca4f1 to 9306d8d Compare February 8, 2025 23:38

wyardley commented Feb 8, 2025

View reviewed changes

wyardley force-pushed the wyardley/tz branch 2 times, most recently from 275ce5d to 69e1306 Compare February 8, 2025 23:47

wyardley mentioned this pull request Feb 8, 2025

Unit test execution #1330

Closed

wyardley changed the title ~~test(alerter): update timezones in tests~~ test(alerter): fix time and timezone issues in alerter tests Feb 9, 2025

wyardley force-pushed the wyardley/tz branch 4 times, most recently from d427dd0 to b91074b Compare February 9, 2025 06:43

wyardley force-pushed the wyardley/tz branch from b91074b to 7dd6352 Compare February 9, 2025 07:12

wyardley commented Feb 10, 2025

View reviewed changes

jamesoff merged commit 5a27794 into jamesoff:develop Feb 15, 2025
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(alerter): fix time and timezone issues in alerter tests #1598

test(alerter): fix time and timezone issues in alerter tests #1598

wyardley commented Feb 8, 2025 •

edited

Loading

wyardley Feb 8, 2025

wyardley Feb 8, 2025

wyardley Feb 8, 2025

jamesoff commented Feb 8, 2025

This comment was marked as outdated.

wyardley commented Feb 8, 2025 •

edited

Loading

wyardley Feb 8, 2025

wyardley Feb 9, 2025

wyardley Feb 9, 2025

wyardley Feb 10, 2025

jamesoff Feb 15, 2025

jamesoff commented Feb 15, 2025

test(alerter): fix time and timezone issues in alerter tests #1598

test(alerter): fix time and timezone issues in alerter tests #1598

Conversation

wyardley commented Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesoff commented Feb 8, 2025

This comment was marked as outdated.

wyardley commented Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesoff commented Feb 15, 2025

wyardley commented Feb 8, 2025 •

edited

Loading

wyardley commented Feb 8, 2025 •

edited

Loading