dstandish commented on a change in pull request #20508:
URL: https://github.com/apache/airflow/pull/20508#discussion_r775653054
##########
File path: tests/utils/test_dates.py
##########
@@ -29,8 +29,10 @@
class TestDates(unittest.TestCase):
def test_days_ago(self):
+ from airflow.settings import TIMEZONE
Review comment:
your change to this test, it passes in the current codebase i.e. without
your change, because by default TIMEZONE is UTC
i think you need to patch the configuration with `from
tests.test_utils.config import conf_vars`. see other usages.
may want to parameterize with 2 timezones, one utc one not
##########
File path: tests/utils/test_dates.py
##########
@@ -29,8 +29,10 @@
class TestDates(unittest.TestCase):
def test_days_ago(self):
+ from airflow.settings import TIMEZONE
+
today = pendulum.today()
- today_midnight =
pendulum.instance(datetime.fromordinal(today.date().toordinal()))
+ today_midnight =
pendulum.instance(datetime.fromordinal(today.date().toordinal()), tz=TIMEZONE)
assert dates.days_ago(0) == today_midnight
assert dates.days_ago(100) == today_midnight + timedelta(days=-100)
Review comment:
with the code in `days_ago` this will actually fail if you cross DST /
PST boundary, because timedelta gives 24-hour period so it will be off by 1
hour. so e.g. it should fail right now for timezone `America/Los_Angeles`.
but i think if we make this change, we should change the code to subtract
calendar days instead of timedelta
##########
File path: tests/utils/test_dates.py
##########
@@ -29,8 +29,10 @@
class TestDates(unittest.TestCase):
def test_days_ago(self):
+ from airflow.settings import TIMEZONE
Review comment:
hmm i tried patching default timezone and have not had luck (see
https://github.com/apache/airflow/pull/20529)
but you should be able to do so like this (in a local import for example):
```
from airflow import settings
settings.TIMEZONE = pendulum.tz.timezone('America/Los_Angeles')
```
##########
File path: tests/utils/test_dates.py
##########
@@ -29,8 +29,10 @@
class TestDates(unittest.TestCase):
Review comment:
i think these tests should pass
```python
class TestDates:
@pytest.mark.parametrize('timezone', ['UTC', 'America/Los_Angeles'])
def test_days_ago(self, timezone):
from airflow import settings
settings.TIMEZONE = pendulum.tz.timezone(timezone)
from airflow.utils import dates
today_midnight = pendulum.today(settings.TIMEZONE)
assert today_midnight.tzinfo == dates.days_ago(2).tzinfo ==
pendulum.tz.timezone(timezone)
assert dates.days_ago(0) == today_midnight
assert dates.days_ago(100) == today_midnight.add(days=-100)
assert dates.days_ago(0, hour=3) == today_midnight +
timedelta(hours=3)
assert dates.days_ago(0, minute=3) == today_midnight +
timedelta(minutes=3)
assert dates.days_ago(0, second=3) == today_midnight +
timedelta(seconds=3)
assert dates.days_ago(0, microsecond=3) == today_midnight +
timedelta(microseconds=3)
```
you will need to add more logic to `days_ago` though.
additionally, i think rather than using moving time `pendulum.today` we
should actually use a fixed time such as 2021-12-28 so that way we ensure
that days_ago(100) actually does cross a DST boundary. i would also add a
comment inline above the days_ago(100) assert that explains why we need it
(namely to test behavior across DST boundary)
##########
File path: tests/utils/test_dates.py
##########
@@ -29,8 +29,10 @@
class TestDates(unittest.TestCase):
def test_days_ago(self):
+ from airflow.settings import TIMEZONE
+
today = pendulum.today()
- today_midnight =
pendulum.instance(datetime.fromordinal(today.date().toordinal()))
+ today_midnight =
pendulum.instance(datetime.fromordinal(today.date().toordinal()), tz=TIMEZONE)
Review comment:
can you not simply do
```
today_midnight = pendulum.today(TIMEZONE)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
