Python implementation of datetime module is not being tested correctly.

a24690b2-760c-4b16-8d68-d9a44e29742c commented 7 years ago

BPO	30822
Nosy	@abalkin, @vstinner, @ned-deily, @serhiy-storchaka, @musically-ut
PRs	python/cpython#2530 python/cpython#2534 python/cpython#2550 python/cpython#2551 python/cpython#2588 python/cpython#2775 python/cpython#2777 python/cpython#2781 python/cpython#2782 python/cpython#2783 python/cpython#2788 python/cpython#2815 python/cpython#2816 python/cpython#3405

^{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: ```python assignee = 'https://github.com/serhiy-storchaka' closed_at = created_at = labels = ['3.7', 'type-bug', 'tests'] title = 'Python implementation of datetime module is not being tested correctly.' updated_at = user = 'https://github.com/musically-ut' ``` bugs.python.org fields: ```python activity = actor = 'vstinner' assignee = 'serhiy.storchaka' closed = True closed_date = closer = 'vstinner' components = ['Tests'] creation = creator = 'musically_ut' dependencies = [] files = [] hgrepos = [] issue_num = 30822 keywords = [] message_count = 48.0 messages = ['297455', '297470', '297515', '297517', '297518', '297519', '297520', '297522', '297523', '297524', '297525', '297530', '297546', '297548', '297554', '297555', '297557', '297559', '297562', '297571', '297573', '297612', '297734', '297736', '297743', '297752', '297753', '297760', '297762', '297763', '298032', '298033', '298059', '298664', '298717', '298721', '298725', '298726', '298729', '298732', '298755', '298756', '298786', '299221', '299222', '299223', '299224', '301749'] nosy_count = 5.0 nosy_names = ['belopolsky', 'vstinner', 'ned.deily', 'serhiy.storchaka', 'musically_ut'] pr_nums = ['2530', '2534', '2550', '2551', '2588', '2775', '2777', '2781', '2782', '2783', '2788', '2815', '2816', '3405'] priority = 'normal' resolution = 'fixed' stage = 'resolved' status = 'closed' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue30822' versions = ['Python 3.6', 'Python 3.7'] ```

a24690b2-760c-4b16-8d68-d9a44e29742c commented 7 years ago

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.

serhiy-storchaka commented 7 years ago

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 commented 7 years ago

New changeset 98b6bc3bf72532b784a1c1fa76eaa6026a663e44 by Serhiy Storchaka (Utkarsh Upadhyay) in branch 'master': bpo-30822: Fix testing of datetime module. (bpo-2530) https://github.com/python/cpython/commit/98b6bc3bf72532b784a1c1fa76eaa6026a663e44

a24690b2-760c-4b16-8d68-d9a44e29742c commented 7 years ago

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 bpo-30822 in their titles.

Sounds about right?

~ ut

ned-deily commented 7 years ago

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

serhiy-storchaka commented 7 years ago

Oh, test.datetimetester.ZoneInfoTest[Europe/Andorra]_Pure_Pure_Pure -- something looks wrong.

a24690b2-760c-4b16-8d68-d9a44e29742c commented 7 years ago

I can reproduce the names with _Pure_Pure_Pure if I run the tests with -utzdata. Investigating.

~ ut

serhiy-storchaka commented 7 years ago

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.

serhiy-storchaka commented 7 years ago

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 bpo-30822 in their titles.

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.

serhiy-storchaka commented 7 years ago

New changeset 34b54873b51a1ebee2a3c57b7205537b4f33128d by Serhiy Storchaka in branch 'master': bpo-30822: Deduplicate ZoneInfoTest classes in test_datetime. (bpo-2534) https://github.com/python/cpython/commit/34b54873b51a1ebee2a3c57b7205537b4f33128d

a24690b2-760c-4b16-8d68-d9a44e29742c commented 7 years ago

So the problem is occurring because a single Test class is being instantiated with three different tests here in datetimetester.py:

            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 Test class which is being dynamically created is being instantiated with the test methods: test_folds, test_gaps, test_system_transitions and is being appended to tests. This makes that class to appear thrice in test_classes in test_datetime.py:

    elif issubclass(cls, unittest.TestSuite):
        suit = cls()
        test_classes.extend(type(test) for test in suit)

here: https://github.com/python/cpython/blob/master/Lib/test/test_datetime.py#L34

Hence, the class gets _Pure and _Fast appended to its name thrice and gets executed thrice, making the tests take 3 times as long to run.

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.

~ ut

serhiy-storchaka commented 7 years ago

This is confirmed by changing the code to the following:

Yes, this is other way to fix test_datetime. But this breaks running tests via datetimetester:

./python -m test -vuall datetimetester

a24690b2-760c-4b16-8d68-d9a44e29742c commented 7 years ago

Hmm, I see; I did not know that.

I had just assumed that ./python -m test test_* was the standard command for running tests.

However, if one does run tests using standalone datetimetester (which only imports the _Fast C extension module), then I'll have to rewrite some of the self.skipTest conditions such that they are more defensive, i.e.:

    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.

~ ut

serhiy-storchaka commented 7 years ago

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.

vstinner commented 7 years ago

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_PurePure) ... Timeout (0:15:00)! Thread 0x00003fffa9aa5a20 (most recent call first): File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/datetime.py", line 454 in \_new File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/datetime.py", line 1870 in __sub File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/datetime.py", line 1519 in local File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/datetime.py", line 1532 in _mktime File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/datetime.py", line 1550 in timestamp File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/test/datetimetester.py", line 4836 in test_systemtransitions File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/unittest/case.py", line 615 in run File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/unittest/case.py", line 663 in \_call File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/unittest/suite.py", line 122 in run File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/unittest/suite.py", line 84 in __call File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/unittest/suite.py", line 122 in run File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/unittest/suite.py", line 84 in __call File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/unittest/runner.py", line 176 in run File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/test/support/init.py", line 1896 in _run_suite File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/test/support/init.py", line 1940 in run_unittest File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/test/test_datetime.py", line 53 in test_main File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/test/libregrtest/runtest.py", line 172 in runtest_inner File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/test/libregrtest/runtest.py", line 140 in runtest File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/test/libregrtest/main.py", line 290 in rerun_failed_tests File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/test/libregrtest/main.py", line 540 in _main File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/test/libregrtest/main.py", line 510 in main File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/test/libregrtest/main.py", line 585 in main File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/test/main__.py", line 2 in \<module> File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/runpy.py", line 85 in _run_code File "/home/shager/cpython-buildarea/3.x.edelsohn-fedora-ppc64/build/Lib/runpy.py", line 193 in _run_module_as_main make: *** [buildbottest] Error 1 program finished with exit code 2 elapsedTime=2290.631264

vstinner commented 7 years ago

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 test_system_transitions (test.datetimetester.ZoneInfoTest[Europe/Riga]_Pure_Pure_Pure) ... ok test_folds (test.datetimetester.ZoneInfoTest[Europe/Riga]_Pure_Pure_Pure) ... ok test_gaps (test.datetimetester.ZoneInfoTest[Europe/Riga]_Pure_Pure_Pure) ... ok test_system_transitions (test.datetimetester.ZoneInfoTest[Europe/Riga]_Pure_Pure_Pure) ... ok test_folds (test.datetimetester.ZoneInfoTest[Europe/Riga]_Pure_Pure_Pure) ... Timeout (0:15:00)! Thread 0x400c9110 (most recent call first): File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/datetime.py", line 263 in _check_int_field File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/datetime.py", line 281 in _check_datefields File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/datetime.py", line 1376 in \_new File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/datetime.py", line 1600 in replace File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/datetimetester.py", line 4776 in testfolds File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/unittest/case.py", line 615 in run File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/unittest/case.py", line 663 in \_call File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/unittest/suite.py", line 122 in run File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/unittest/suite.py", line 84 in __call File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/unittest/suite.py", line 122 in run File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/unittest/suite.py", line 84 in __call File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/unittest/runner.py", line 176 in run File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/support/init.py", line 1896 in _run_suite File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/support/init.py", line 1940 in run_unittest File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/test_datetime.py", line 53 in test_main File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/libregrtest/runtest.py", line 172 in runtest_inner File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/libregrtest/runtest.py", line 140 in runtest File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/libregrtest/main.py", line 290 in rerun_failed_tests File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/libregrtest/main.py", line 540 in _main File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/libregrtest/main.py", line 510 in main File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/libregrtest/main.py", line 585 in main File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/main.py", line 2 in \<module> File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/runpy.py", line 85 in _run_code File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/runpy.py", line 193 in _run_module_as_main make: *** [buildbottest] Error 1 program finished with exit code 2 elapsedTime=2225.001566

vstinner commented 7 years ago

Oh, I didn't read the issue. In fact, it seems like the commit 34b54873b51a1ebee2a3c57b7205537b4f33128d already repaired buildbots (x86 Tiger 3.x at least).

But... test_datetime became the *slowest* test of x86 Tiger 3.x:

10 slowest tests:

test_datetime: 19 min 6 sec
test_tools: 7 min 20 sec
test_multiprocessing_spawn: 6 min 54 sec
...

Wait, 19 minutes to test datetime??? What's wrong with test_datetime?

a24690b2-760c-4b16-8d68-d9a44e29742c commented 7 years ago

Previously, test_datetime was running only _Fast tests (i.e. testing the C module). Then (because of an erring commit from me), testing for _Pure implementation started but all tests per time-zone started running thrice instead of once. Hence, the timeouts while testing were triggered, breaking the buildbots. (Err ... sorry about that.)

However, the latest fix by Serihy now runs each test only once. The _Pure tests, nevertheless, are significantly slower than _Fast tests and, hence, the slowdown.

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.

~ ut

vstinner commented 7 years ago

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?

a24690b2-760c-4b16-8d68-d9a44e29742c commented 7 years ago

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:

Iran had a sub-minute UTC offset before 1946.

in https://github.com/python/cpython/blob/master/Lib/test/datetimetester.py#L4865

Perhaps @belopolsky can suggest something? (Already in Nosy List).

~ ut

serhiy-storchaka commented 7 years ago

Let discuss too long time of testing datetime time zones in separate issue.

vstinner commented 7 years ago

I would prefer to make test_datetime faster in master before backporting.

vstinner commented 7 years ago

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) running: test_datetime (832 sec) running: test_datetime (862 sec) running: test_datetime (892 sec) 0:15:02 load avg: 2.30 [406/406/1] testdatetime crashed (Exit code 1) Timeout (0:15:00)! Thread 0x400b4110 (most recent call first): File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/datetime.py", line 394 in \_new File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/datetime.py", line 1519 in local File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/datetime.py", line 1522 in _mktime File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/datetime.py", line 1550 in timestamp File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/datetimetester.py", line 4836 in test_systemtransitions File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/unittest/case.py", line 615 in run File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/unittest/case.py", line 663 in \_call File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/unittest/suite.py", line 122 in run File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/unittest/suite.py", line 84 in __call File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/unittest/suite.py", line 122 in run File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/unittest/suite.py", line 84 in __call File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/unittest/runner.py", line 176 in run File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/support/init.py", line 1896 in _run_suite File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/support/init.py", line 1940 in run_unittest File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/test_datetime.py", line 54 in test_main File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/libregrtest/runtest.py", line 172 in runtest_inner File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/libregrtest/runtest.py", line 130 in runtest File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/libregrtest/runtest_mp.py", line 71 in run_tests_slave File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/libregrtest/main.py", line 519 in _main File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/libregrtest/main.py", line 512 in main File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/libregrtest/main.py", line 587 in main File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/regrtest.py", line 46 in _main File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/test/regrtest.py", line 50 in \<module> File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/runpy.py", line 85 in _run_code File "/ssd/buildbot/buildarea/3.x.gps-ubuntu-exynos5-armv7l/build/Lib/runpy.py", line 193 in _run_module_as_main

serhiy-storchaka commented 7 years ago

Another solution -- disable "cpu" and "tzdata" resources on slow buildbots (see bpo-30417).

vstinner commented 7 years ago

New changeset 8207c17486baece8ed0ac42d9f8d69ecec4ba7e4 by Victor Stinner in branch 'master': Revert "bpo-30822: Fix testing of datetime module." (bpo-2588) https://github.com/python/cpython/commit/8207c17486baece8ed0ac42d9f8d69ecec4ba7e4

vstinner commented 7 years ago

I reverted the change to repair buildbots and get more time to find a proper fix: https://github.com/python/cpython/pull/2588#issuecomment-313092304

vstinner commented 7 years ago

Another solution -- disable "cpu" and "tzdata" resources on slow buildbots (see bpo-30417).

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?

abalkin commented 7 years ago

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.

vstinner commented 7 years ago

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 run all these tests on all branches for each Python commit?

Maybe we should disable these tests on all CI, and maybe setup a buildbot which runs these tests?

Instead of being exhaustive, would it be possible to pick a few significant tests?

serhiy-storchaka commented 7 years ago

Two timezones (America/New_York and Asia/Tehran) are picked for testing independently from the -utzdata flag.

vstinner commented 7 years ago

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 :-(

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.

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.

vstinner commented 7 years ago

Two timezones (America/New_York and Asia/Tehran) are picked for testing independently from the -utzdata flag.

By the way, if someone is aware of other interesting timezones, we may also test more timezones (without tzdata)?

serhiy-storchaka commented 7 years ago

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.

a24690b2-760c-4b16-8d68-d9a44e29742c commented 7 years ago

So it seems that running the experiments without -utzdata would be an acceptable fix (assuming that there are no other interesting time-zones worthy of explicit testing)?

Can I help in the resolution of this issue, since resolution of http://bugs.python.org/issue30302 is tangentially conditioned on it. (-:

vstinner commented 7 years ago

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: https://github.com/python/cpython/pull/2775

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.

vstinner commented 7 years ago

New changeset 5b392bbaeb9d9b1db961ecfc7315d8c8662c27f6 by Victor Stinner in branch 'master': bpo-30822: Exclude tzdata from regrtest --all (bpo-2775) https://github.com/python/cpython/commit/5b392bbaeb9d9b1db961ecfc7315d8c8662c27f6

vstinner commented 7 years ago

New changeset 96ef06f3979023fd69ed8796d0b1d88885d3ae4d by Victor Stinner in branch '3.6': bpo-30822: Exclude tzdata from regrtest --all (bpo-2775) (bpo-2777) https://github.com/python/cpython/commit/96ef06f3979023fd69ed8796d0b1d88885d3ae4d

vstinner commented 7 years ago

New changeset 645e503ba58086c7f51eda4d025743f2afc05a2a by Victor Stinner in branch '3.5': bpo-30822: Exclude tzdata from regrtest --all (bpo-2775) (bpo-2781) https://github.com/python/cpython/commit/645e503ba58086c7f51eda4d025743f2afc05a2a

vstinner commented 7 years ago

(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.)

vstinner commented 7 years ago

New changeset bf3a1e973b49236e6f267cca135d36290568e6a4 by Victor Stinner in branch '3.5': bpo-30822: regrtest: remove tzdata (bpo-2782) https://github.com/python/cpython/commit/bf3a1e973b49236e6f267cca135d36290568e6a4

vstinner commented 7 years ago

New changeset 80ebc438ed377f8c6491e8887425526c6787c0e7 by Victor Stinner in branch '2.7': bpo-30822: regrtest: fix -u extralargefile (bpo-2788) https://github.com/python/cpython/commit/80ebc438ed377f8c6491e8887425526c6787c0e7

vstinner commented 7 years ago

New changeset 287c5594edc1ca08db64d1f4739cc36bfe75ae75 by Victor Stinner (Utkarsh Upadhyay) in branch 'master': bpo-30822: Fix testing of datetime module. (bpo-2530) (bpo-2783) https://github.com/python/cpython/commit/287c5594edc1ca08db64d1f4739cc36bfe75ae75

vstinner commented 7 years ago

Utkarsh comment on the PR:

I'm afraid I don't really know how to read the waterfall chart at http://buildbot.python.org/all/waterfall.

Yeah, sorry about this awful view :-) We now have a mailing list getting notifications when a buildbot fails: https://mail.python.org/mm3/mailman3/lists/buildbot-status.python.org/

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).

vstinner commented 7 years ago

New changeset c52cea49544621b612c7f17f45a0c2b8b61a6c67 by Victor Stinner (Utkarsh Upadhyay) in branch '3.6': [3.6] bpo-30822: Fix testing of datetime module. (GH-2530) (GH-2783) (bpo-2816) https://github.com/python/cpython/commit/c52cea49544621b612c7f17f45a0c2b8b61a6c67

vstinner commented 7 years ago

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: https://github.com/python/cpython/pull/2815

Thanks Utkarsh Upadhyay for your work on datetime, not only this issue, but also repr(timedelta) (and maybe also timedelta constructor doc later? ;-)).

a24690b2-760c-4b16-8d68-d9a44e29742c commented 7 years ago

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. :)

~ ut

vstinner commented 7 years ago

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

Reverting is a new policy that we decided last June: https://mail.python.org/pipermail/python-committers/2017-June/004588.html

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.

vstinner commented 7 years ago

New changeset 3892799668dbf2b123a52780fd1d78f8880fdeb7 by Victor Stinner (Miss Islington (bot)) in branch '3.6': [3.6] bpo-30822: Deduplicate ZoneInfoTest classes in test_datetime. (GH-2534) (bpo-3405) https://github.com/python/cpython/commit/3892799668dbf2b123a52780fd1d78f8880fdeb7

python / cpython

Python implementation of datetime module is not being tested correctly. #75005

Iran had a sub-minute UTC offset before 1946.