tests fail in March

[brianmay]

======================================================================
FAIL: testFloat (tests.TestDelta.test)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/brian/tree/debian/python-modules/parsedatetime/tests/TestDelta.py", line 64, in testFloat
    self.cal.parse('1.4 months ago', self.source), months=-1.4)
  File "/home/brian/tree/debian/python-modules/parsedatetime/tests/TestDelta.py", line 37, in assertDelta
    self.assertTrue(diff < 0.05, '%s is not less than 0.05' % diff)
AssertionError: 0.0666666666667 is not less than 0.05

----------------------------------------------------------------------

I suspect this is because February only has 28 days, not the 30 days assumed by the test.

[idpaterson]

Thank you for reporting, I am personally grateful for this observation since it identifies a few issues that I need to fix in the new test suite for v3.0 (#199).

The assertion checks that there is not more than a 5% variation between the targeted time and the output, in this case your test shows 6.7%. Over the span of 1.4 months, that represents a difference of 2.8 days.

I believe that this is a bug in the test case rather than in parsedatetime. The test assertion uses the following logic to calculate months:

        if months:
            delta += datetime.timedelta(days=30 * months)

Whereas Calendar.inc(), which I believe is used in these deltas, calculates month deltas based on the calendar (1 month before March 10 is always Feb 10 regardless of leap years), then adds the partial months.

The 5% rule for deltas is problematic, that can represent a huge variation from expectation when the range is months or years. Fortunately the new test suite uses different logic for deltas and as a result this test passes (once I fixed the few minor issues that this case exposed).

To help determine whether there is actually a bug in parsedatetime, here is a comparison between 2016 and 2017:

>>> cal.parse('1.4 months ago', sourceTime=datetime.datetime(2016,3,10,9,24,0))
(time.struct_time(tm_year=2016, tm_mon=1, tm_mday=29, tm_hour=19, tm_min=0, tm_sec=0, tm_wday=4, tm_yday=29, tm_isdst=-1), 1)
>>> cal.parse('1.4 months ago', sourceTime=datetime.datetime(2017,3,10,9,24,0))
(time.struct_time(tm_year=2017, tm_mon=1, tm_mday=30, tm_hour=4, tm_min=36, tm_sec=0, tm_wday=0, tm_yday=30, tm_isdst=-1), 1)

[bmw]

Is there a way a fix for this can be prioritized? I work at EFF where I'm a developer of Certbot. Certbot depends on parsedatetime and this issue is causing some trouble in Debian packaging. In the worst case, if this bug isn't fixed parsedatetime may be removed from the next version of Debian which will also cause problems with Certbot's packaging.

When can we expect a fix for this to be available to the Debian developers so they can continue to include parsedatetime?

[idpaterson]

The assertion can be made more lax but I will have to defer to @bear regarding publishing a release with that change. Is that something you could push out soon, Mike?

[bear]

yes, we can push one a release out now if needed

[bear]

@idpaterson any reason we don't use Calendar.inc() to calculate that instead of the hardcoded 30 ?

[brianmay]

I personally don't like the test as is, it tests something different depending on the current time. The tests should test the same thing every time it is run. If you want it to test from multiple times, that should be allowed for in the tests. Instead of requiring you to run the tests multiple times throughout the year. I would suggest the fix is to hard code the source time, to something that is known to work, for example (I picked 2017-01-01 but this is arbitrary):

diff --git a/tests/TestDelta.py b/tests/TestDelta.py
index c628d5e..054eabe 100644
--- a/tests/TestDelta.py
+++ b/tests/TestDelta.py
@@ -3,7 +3,6 @@
 Test time delta
 """
 
-import time
 import datetime
 import unittest
 import parsedatetime as pdt
@@ -13,9 +12,9 @@ class test(unittest.TestCase):
 
     def setUp(self):
         self.cal = pdt.Calendar(version=pdt.VERSION_CONTEXT_STYLE)
-        self.source = (self.yr, self.mth, self.dy,
-                       self.hr, self.mn, self.sec,
-                       self.wd, self.yd, self.isdst) = time.localtime()
+        self.source = (2017, 1, 1,
+                       7, 1, 2,
+                       6, 1, 1)
 
     def assertDelta(self, ts, years=None, months=None, **deltakw):
         ts = ts[0]

[bear]

I think that test may be one of the more older of them, so is very likely broken in the sense that it's not following our current style.

@idpaterson I'm all for making the change @brianmay suggests

[idpaterson]

@bear I agree that using Calendar.inc() here is best. It is consistent with how the new test suite handles deltas, is already tested elsewhere, and is much more accurate than the < 5% assertion. I don't think I have time for that tonight but could work on it tomorrow if you're busy.

@brianmay the new tests for v3.0 do exactly that, every test is automatically run against a consistent set of source times. Modifying or adding to those dates is trivial, though with too many values the tests could take too long to run. I selected three dates so far:

DEFAULT_SOURCE_TIMES = (
    datetime(2016, 2, 29, 3, 4, 5),
    datetime(2015, 2, 28, 23, 22, 21),
    datetime(1945, 12, 31, 3, 4, 5),
)

[bear]

fixed in release v2.3 which i'm pushing out now - we can add more tests and switch to .inc() later

[bmw]

Thanks so much for fixing this unbelievably quickly.

[bear]

I know how OS packagers can be (used to be one way back in the day) and it needed fixing...

long way of saying your welcome! :)

[exarkun]

This issue is still open but the comments suggest that the test was fixed. What's the current status?