arrow-py / arrow

🏹 Better dates & times for Python
https://arrow.readthedocs.io
Apache License 2.0
8.71k stars 673 forks source link

CI failing on humanize test #890

Closed jadchaar closed 3 years ago

jadchaar commented 3 years ago

Issue Description

Current CRON CI build on master is failing. The test case tests/test_arrow.py::TestArrowHumanize::test_day is failing on the current master between 7:00-7:59pm EST. The tests pass when run at 8pm EST.

  __________________________ TestArrowHumanize.test_day __________________________

  self = <tests.test_arrow.TestArrowHumanize object at 0x7f851fcb8128>

      def test_day(self):

          later = self.now.shift(days=1)

          assert self.now.humanize(later) == "a day ago"
          assert later.humanize(self.now) == "in a day"

          # regression test for issue #697
          less_than_48_hours = self.now.shift(
              days=1, hours=23, seconds=59, microseconds=999999
          )
          assert self.now.humanize(less_than_48_hours) == "a day ago"
          assert less_than_48_hours.humanize(self.now) == "in a day"

          less_than_48_hours_date = less_than_48_hours._datetime.date()
          with pytest.raises(TypeError):
              # humanize other argument does not take raw datetime.date objects
              self.now.humanize(less_than_48_hours_date)

          # convert from date to arrow object
          less_than_48_hours_date = arrow.Arrow.fromdate(less_than_48_hours_date)
  >       assert self.now.humanize(less_than_48_hours_date) == "a day ago"
  E       AssertionError: assert '23 hours ago' == 'a day ago'
  E         - a day ago
  E         + 23 hours ago

  tests/test_arrow.py:1851: AssertionError

This test case reproduces it locally:

import arrow

now = arrow.Arrow(2020, 12, 1, 0, 55, 46, 798518)
print("now:", now)

less_than_48_hours = now.shift(
    days=1, hours=23, seconds=59, microseconds=999999
)
print("less_than_48_hours:", less_than_48_hours)

less_than_48_hours_date = arrow.Arrow.fromdate(less_than_48_hours._datetime.date())
print("less_than_48_hours_date:", less_than_48_hours_date)

humanized = now.humanize(less_than_48_hours_date)
print("humanized:", humanized)

# Should be "a day ago"
assert humanized == "a day ago"

System Info

jadchaar commented 3 years ago

Given our recent changes to the bounds, I personally do not see any point in us converting the shifted Arrow object's datetime object to a date. Instead, we should be doing this in the tests/test_arrow.py::TestArrowHumanize::test_day test case:

import arrow

now = arrow.Arrow(2020, 12, 1, 0, 55, 46, 798518)
print("now:", now)

less_than_48_hours = now.shift(
    days=1, hours=23, seconds=59, microseconds=999999
)
print("less_than_48_hours:", less_than_48_hours)

# Do not create an intermediary date
humanized = now.humanize(less_than_48_hours)
print("humanized:", humanized)

# Should be "a day ago"
assert humanized == "a day ago"

Thoughts @systemcatch?

systemcatch commented 3 years ago

Hey @jadchaar I agree with you about changing the tests to not use date.

But I don't get why this is failing 7-7:59, this is what I get as output from your example.

now: 2020-12-01T00:55:46.798518+00:00
less_than_48_hours: 2020-12-02T23:56:46.798517+00:00
less_than_48_hours_date: 2020-12-02T00:00:00+00:00
humanized: 23 hours ago

We expect '23 hours ago' right?

jadchaar commented 3 years ago

Yeah! I am not really getting it either... it seems to be 7EST because that is when UTC crosses over to the next day. 23 hours ago there is correct, but our test case is searching for 1 day ago since it is looking for less than a two day shift. I think the test case needs to just change.

krisfremen commented 3 years ago
  ________________________ TestArrowHumanize.test_months _________________________

  self = <tests.test_arrow.TestArrowHumanize object at 0x0000000003d52790>

      def test_months(self):

          later = self.now.shift(months=2)
          earlier = self.now.shift(months=-2)

          assert earlier.humanize(self.now) == "2 months ago"
  >       assert later.humanize(self.now) == "in 2 months"
  E       AssertionError: assert 'in a month' == 'in 2 months'
  E         - in 2 months
  E         ?    ^      -
  E         + in a month
  E         ?    ^

  tests/test_arrow.py:2160: AssertionError

Hmm, I guess another test failing also for some weird times, coming from https://github.com/arrow-py/arrow/pull/907/checks?check_run_id=1628813383

jadchaar commented 3 years ago
  ________________________ TestArrowHumanize.test_months _________________________

  self = <tests.test_arrow.TestArrowHumanize object at 0x0000000003d52790>

      def test_months(self):

          later = self.now.shift(months=2)
          earlier = self.now.shift(months=-2)

          assert earlier.humanize(self.now) == "2 months ago"
  >       assert later.humanize(self.now) == "in 2 months"
  E       AssertionError: assert 'in a month' == 'in 2 months'
  E         - in 2 months
  E         ?    ^      -
  E         + in a month
  E         ?    ^

  tests/test_arrow.py:2160: AssertionError

Hmm, I guess another test failing also for some weird times, coming from https://github.com/arrow-py/arrow/pull/907/checks?check_run_id=1628813383

Ugh good find. Might be related to https://github.com/arrow-py/arrow/issues/749

jadchaar commented 3 years ago

Seems to be a new years edge case. This test case reproduces it:

arrow.py::TestArrowHumanize::test_months

import arrow

now = arrow.Arrow(2020, 12, 31)
print("now:", now)

two_months_later = now.shift(months=2)
print("two_months_later:", two_months_later)

humanized = two_months_later.humanize(now)
print("humanized:", humanized)

# Should be "in 2 months"
assert humanized == "in 2 months"

Seems to be due to our averaging of the number of days in a month: https://github.com/arrow-py/arrow/blob/2eba8b97ea323c975fb2400b041f4c17c43c9709/arrow/arrow.py#L62

systemcatch commented 3 years ago

This was one of the problems I encountered in #868. Any value we pick for _SECS_PER_MONTH will result in something being wrong. To fix this properly the "month" duration has to be calculated at runtime imo.

The annoying thing is that this could break installation in some cases. I suggest we find a fix before 1.0.0 or mark the month test as xfail for now.