Python implementation of datetime module is not being tested correctly. #75005
Comments
|
While investigating http://bugs.python.org/issue30302 it was discovered that the Python implementation of the datetime module was not being tested correctly. The datetimetester.py was supposed to execute tests twice, once for the _Fast implementation (i.e. C extension code in _datetimemodule.c) and once for the _Pure implementation (i.e the Python code). The fix is contained in the following two commits: The bug does not effect Python 2.7 as the C version of the datetime module had not been introduced back then. However, as fas as I can tell, the fix needs to be applied to all Python 3.x branches. |
musically-ut
mannequin
added
type-feature
|
Please provide a separate PR for this. After merging it with the master branch it should be backported to 3.6 and 3.5 (3.4 and 3.3 are for security only fixes). Add your name in Misc/ACKS. Changes in Misc/NEWS.d/ are not required. |
serhiy-storchaka
added
type-bug
|
Are there any guides re: backporting commits? Just to confirm verify that I'm going about it right; I’m planning on cherry-picking relevant commits back to branch 3.5, 3.6, resolving the merge conflicts, and then opening two separate PRs for them with Sounds about right? ~ |
|
Serhiy, it appears that the added tests are taking a long time to run and thus causing many buildbot failures, for example: http://buildbot.python.org/all/builders/ARMv7%20Ubuntu%203.x/builds/1013 |
|
Oh, test.datetimetester.ZoneInfoTest[Europe/Andorra]_Pure_Pure_Pure -- something looks wrong. |
|
I can reproduce the names with ~ |
|
The quick solution -- just deduplicate test classes. But the code needs rewriting. Current code is tricky and fragile. It doesn't work with --list-cases (and I suppose it doesn't work well with other advanced unittest features). This would be not easy. |
All right. See https://docs.python.org/devguide/committing.html#backporting-changes-to-an-older-version . You can cherry-pick manually or using cherry_picker.py which adds correct prefix to commit message and helps to create a pull request. |
|
So the problem is occurring because a single for name in ZoneInfo.zonenames():
Test = type('ZoneInfoTest[%s]' % name, (ZoneInfoTest,), {})
Test.zonename = name
for method in dir(Test):
if method.startswith('test_'):
tests.append(Test(method))here: https://github.com/python/cpython/blob/master/Lib/test/datetimetester.py#L4853 The here: https://github.com/python/cpython/blob/master/Lib/test/test_datetime.py#L34 Hence, the class gets This is confirmed by changing the code to the following: for name in ZoneInfo.zonenames():
Test = type('ZoneInfoTest[%s]' % name, (ZoneInfoTest,), {})
Test.zonename = name
tests.append(Test())
# for method in dir(Test):
# if method.startswith('test_'):
# tests.append(Test(method))However, I'm not sure what implications this has w.r.t. unittests and the advanced cases. The other way to fix it is to create a set out of the classes, as suggested in PR bpo-2534. ~ |
Yes, this is other way to fix test_datetime. But this breaks running tests via datetimetester: ./python -m test -vuall datetimetester |
|
Hmm, I see; I did not know that. I had just assumed that However, if one does run tests using standalone if '_Pure' not in self.__class__.__name__:
self.skipTest('...')instead of: if '_Fast' in self.__class__.__name__:
self.skipTest('...')because some (one?) of the tests fail otherwise. Shall I make that change? Also, here is an alternate fix which is a tiny bit less murky than straight up de-duplication of test-classes: https://github.com/musically-ut/cpython/pull/1/files I'm not sure whether this is any clearer or less fragile, though. ~ |
|
Good point! But datetimetester uses the implementation depending on the availability of the _datetime extension module. If it is not available, the pure Python implementation is used, and other tests are failed. I think it would be better to fix first the issues with test_datetime, since this is the common way of running tests, and try to restore running tests with datetimetester in separate issue. Just backport your PR 2530 to 3.5 and 3.6. You can include the change from PR 2534 or it can be backported separately. |
|
Regression: http://buildbot.python.org/all/builders/PPC64%20Fedora%203.x/builds/1000/steps/test/logs/stdio test_system_transitions (test.datetimetester.ZoneInfoTest[America/Juneau]Pure_Pure_Pure) ... Timeout (0:15:00)! |
|
http://buildbot.python.org/all/builders/ARMv7%20Ubuntu%203.x/builds/1013/steps/test/logs/stdio test_gaps (test.datetimetester.ZoneInfoTest[Europe/Riga]Pure_Pure_Pure) ... ok |
|
Oh, I didn't read the issue. In fact, it seems like the commit 34b5487 already repaired buildbots (x86 Tiger 3.x at least). But... test_datetime became the *slowest* test of x86 Tiger 3.x: 10 slowest tests:
Wait, 19 minutes to test datetime??? What's wrong with test_datetime? |
|
Previously, test_datetime was running only However, the latest fix by Serihy now runs each test only once. The Running the tests take ~ 10 minutes on my system with -utzdata enabled; 19 minutes does sound like a bit much but I'm not aware of the specs of the buildbot machines. ~ |
I don't consider that 10 minutes to test the small datetime module is a reasonable duration. What does take such more time? Do we really need to test all timezones around the world? |
|
Hmm, I don't know if testing every zone is necessary. However, I would not be comfortable picking out the zones one ought to test, considering: if ('Riyadh8' in self.zonename or
# From tzdata NEWS file:
# The files solar87, solar88, and solar89 are no longer distributed.
# They were a negative experiment - that is, a demonstration that
# tz data can represent solar time only with some difficulty and error.
# Their presence in the distribution caused confusion, as Riyadh
# civil time was generally not solar time in those years.
self.zonename.startswith('right/')):and:
in https://github.com/python/cpython/blob/master/Lib/test/datetimetester.py#L4865 Perhaps @belopolsky can suggest something? (Already in Nosy List). ~ |
|
Let discuss too long time of testing datetime time zones in separate issue. |
|
I would prefer to make test_datetime faster in master before backporting. |
|
It's not just a matter of making test_datetime "faster". I see it as a regression, since now it fails randomly on slow buildbots. Maybe we should revert the change until a solution is found. http://buildbot.python.org/all/builders/ARMv7%20Ubuntu%203.x/builds/1025/steps/test/logs/stdio running: test_datetime (802 sec) |
|
Another solution -- disable "cpu" and "tzdata" resources on slow buildbots (see bpo-30417). |
|
I reverted the change to repair buildbots and get more time to find a proper fix: |
I didn't read test_datetime. How test_datetime can spend 20 minutes to test timestamps? Does it spawn subprocesses? Why is it so slow? |
The tests enabled by "-utzdata" check UTC to local and back conversions at several points around *every* time transition in *every* timezone. On systems with a complete installation of IANA tzdata, this is a lot of test points. These tests were supposed to be exhaustive and I did not expect them to be run by default. that's why I introduced the -utzdata flag in the first place. |
Buildbots use "-u all" and so enables tzdata tests. Is it useful to Maybe we should disable these tests on all CI, and maybe setup a Instead of being exhaustive, would it be possible to pick a few |
|
Two timezones (America/New_York and Asia/Tehran) are picked for testing independently from the -utzdata flag. |
|
I reverted the commit which fixes test_datetime to also test Lib/datetime.py, to repair buildbots. But since I reverted the change, nothing was done so datetime.py is still not tested :-(
Alexander: yeah, having an opt-in option to test all timezones makes sense. It's likely to trigger bugs in some corner cases. But the question is really having our CI. Alexander, Serhiy: would you be ok to disable tzdata resource on all our CI (Travis CI, AppVeyor, all buildbots)? *Maybe* we might enable tzdata on selected (fast) buildbots where test_datetime takes less than 20 minutes. But I would prefer to keep the option as an *opt-in*, rather than always running all tests on all CI. |
By the way, if someone is aware of other interesting timezones, we may also test more timezones (without tzdata)? |
|
If testing with -u tzdata is such expensive, tzdata can be excluded from all resources. There is a precedence with the extralargefile resource (see test_zipfile64). Actually this means that these test will never run until you request this specially. |
|
So it seems that running the experiments without Can I help in the resolution of this issue, since resolution of http://bugs.python.org/issue30302 is tangentially conditioned on it. (-: |
|
Serhiy Storchaka: "If testing with -u tzdata is such expensive, tzdata can be excluded from all resources." I agree, I wrote a PR for that: I like using "-u all" without having to remind that a specific resource (tzdata) is too slow to be enabled by default. I suggest to backport this change to all branches. |
|
(Oops, I gone too fast in my 3.5 backport. tzdata was only introduced in Python 3.6, for me it was something older. I wrote a new fix for 3.5, to only add extralargefile, but don't add tzdata.) |
|
Utkarsh comment on the PR:
Yeah, sorry about this awful view :-) We now have a mailing list getting notifications when a buildbot fails: We got two new failures since yesterday on Windows buildbots, but it's related to IDLE and so unrelated to your change. So a quick check says that test_datetime doesn't time out anymore! Previously, the test failed on a few buildbots. I checked the test output to look if test_datetime became the new slowest test and ... no. For example, on PPC64 Fedora 3.x, test_datetime is not even listed in the "10 slowest tests", which means that it took 36 sec or less, whereas the test took longer than 15 minutes when tzdata was used. IMHO you can already cook a first backport for 3.6, but I would prefer to wait another day just to make sure to everything is fine (to wait for new buildbot builds). |
|
Ok, I merged the change in the 3.6 branch. I decided to not merge the 3.5 change, since it's now too late: 3.5 slowly enters a new security-only fixes: Thanks Utkarsh Upadhyay for your work on datetime, not only this issue, but also repr(timedelta) (and maybe also timedelta constructor doc later? ;-)). |
|
Thanks Victor! \o/ Secretly, I'm just happy that my legacy will not be a commit which broke all (?) the build-bots and had to be reverted. :P W.r.t. the docs; in retrospect, that'll probably have a larger impact on the end-users and is less likely to cause disagreement. I probably should have led with that. :) ~ |
Reverting is a new policy that we decided last June: Sorry that your commit was the first guilty, but honestly, it was a wise decision since it allowed to discuss how to handle tzdata without the pressure of broken buildbots. Nowadays, Python relies much more on CI for pull requests than before the migration to GitHub. Well, read the thread if you want to know the full rationale. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: