bpo-30822: Fix testing of datetime module. #2530

musically-ut · 2017-07-01T23:44:35Z

On master and in versions 3.6 and 3.5, there is a bug in how certain tests are executed differently for the C extension and python extension. The execution of the tests (in datetimetester.py) depends on the following condition (or the negation thereof):

if '_Fast' in str(self):
    # ...

The cls.__name__ is modified in the test_datetime.py to be:

    cls.__name__ = name + suffix   # suffix in ['_Fast', '_Pure']

However, str(self) to derives the class name from cls.__qualname__ and not cls.__name__. This meant that _Fast was never in str(self) and, hence, some of the tests were being unnecessarily skipped (notably test_name_cleanup, which was failing).

This PR fixes the problem by clearly setting a string value on the class (_test_type) and then conditioning tests based on that value. Also, instead of editing the __name__ of the class, the __qualname__ is edited so that the tests show the correct variant of the test being run in verbose mode, which helps reduce confusion [1].

Also, on master and in version 3.6, there was another bug which prevented all tests from running because of an accidental resetting of the global variable test_classes inside a loop. That is fixed in a separate commit.

#1493 depends on this PR.

Backport to 3.6
Backport to 3.5

[1]: Example on core-mentorship archives (accessible only to members).

mention-bot · 2017-07-01T23:44:38Z

@musically-ut, thanks for your PR! By analyzing the history of the files in this pull request, we identified @abalkin, @serhiy-storchaka and @birkenfeld to be potential reviewers.

serhiy-storchaka · 2017-07-02T03:04:46Z

Lib/test/datetimetester.py


    def test_name_cleanup(self):
-        if '_Fast' not in str(self):
+        if self._test_type == 'Pure':


Maybe just test '_Fast' not in self.__class__.__name__?

I'd rather have one property serve one purpose: __qualname__ to show correct class names in verbose mode (cosmetic) and _test_type for determining what kind of test it is (functional). This would, hopefully, prevent future cosmetic changes from interfering with the functional parts.

However, if you think "_Fast" in self.__class__.__qualname__ will minimize changes, I'll be happy to change it.

PS: I believe at some point in time, unittest used the cls.__name__ in verbose mode instead of cls.__qualname__. This would explain the change in the outputs which surprised @Haypo. It would be ideal if such cosmetic changes do not break other parts of the code.

In the past __qualname__ didn't exist. repr(self) contained class' __name__. After introducing __qualname__ that tests were not updated.

I think it would be better to modify both __name__ and __qualname__ (they should be consistent). _test_type is redundant since required information can be extracted from __name__.

serhiy-storchaka · 2017-07-02T03:14:42Z

Lib/test/datetimetester.py

    def test_name_cleanup(self):
-        if '_Fast' not in str(self):
+        if self._test_type == 'Pure':
            return


Please replace return with self.skipTest(...). It is better if the skipped test reported as skipped rather than passed.

serhiy-storchaka · 2017-07-02T10:06:00Z

Lib/test/test_datetime.py

            test_classes.extend(type(test) for test in suit)
    for cls in test_classes:
-        cls.__name__ = name + suffix
+        cls.__qualname__ += suffix


Add the suffix also to __name__.

serhiy-storchaka · 2017-07-02T10:13:36Z

Lib/test/datetimetester.py


    def test_name_cleanup(self):
-        if '_Fast' not in str(self):
+        if self._test_type == 'Pure':


In the past __qualname__ didn't exist. repr(self) contained class' __name__. After introducing __qualname__ that tests were not updated.

I think it would be better to modify both __name__ and __qualname__ (they should be consistent). _test_type is redundant since required information can be extracted from __name__.

serhiy-storchaka · 2017-07-02T11:06:24Z

Lib/test/datetimetester.py

-        if '_Fast' not in str(self):
-            return
+        if '_Pure' in self.__class__.__name__:
+            return self.skipTest('Only run for Fast C implementation')


return is redundant. skipTest() doesn't return, it raises an exception.

Oh, I didn't realize that. Fixed.

Only C implementation was tested.. (cherry picked from commit 98b6bc3)

bedevere-bot · 2017-07-03T19:37:30Z

GH-2550 is a backport of this pull request to the 3.5 branch.

Only C implementation was tested. (cherry picked from commit 98b6bc3)

bedevere-bot · 2017-07-03T19:55:34Z

GH-2551 is a backport of this pull request to the 3.6 branch.

This reverts commit 98b6bc3.

* Revert "bpo-30854: Fix compile error when --without-threads (#2581)" This reverts commit 0c31163. * Revert "NEWS for 30777 (#2576)" This reverts commit aaa917f. * Revert "bpo-21624: IDLE -- minor htest fixes (#2575)" This reverts commit 2000150. * Revert "bpo-30777: IDLE: configdialog - add docstrings and improve comments (#2440)" This reverts commit 7eb5883. * Revert "bpo-30319: socket.close() now ignores ECONNRESET (#2565)" This reverts commit 67e1478. * Revert "bpo-30789: Use a single memory block for co_extra. (#2555)" This reverts commit 378ebb6. * Revert "bpo-30845: Enhance test_concurrent_futures cleanup (#2564)" This reverts commit 3df9dec. * Revert "bpo-29293: multiprocessing.Condition.notify() lacks parameter `n` (#2480)" This reverts commit 4835041. * Revert "Remove outdated FOX from GUI FAQ (GH-2538)" This reverts commit d3ed287. * Revert "bpo-6691: Pyclbr now reports nested classes and functions. (#2503)" This reverts commit 246ff3b. * Revert "bpo-29464: Rename METH_FASTCALL to METH_FASTCALL|METH_KEYWORDS and make (#1955)" This reverts commit 6969eaf. * Revert "bpo-30832: Remove own implementation for thread-local storage (#2537)" This reverts commit aa0aa04. * Revert "bpo-30764: Fix regrtest --fail-env-changed --forever (#2536)" This reverts commit 5e87592. * Revert "bpo-30822: Deduplicate ZoneInfoTest classes in test_datetime. (#2534)" This reverts commit 34b5487. * Revert "bpo-30822: Fix testing of datetime module. (#2530)" This reverts commit 98b6bc3.

Only C implementation was tested.

…honGH-2783) Only C implementation was tested.. (cherry picked from commit 287c559)

…honGH-2783) Only C implementation was tested. (cherry picked from commit 287c559)

…honGH-2783) Only C implementation was tested.. (cherry picked from commit 287c559)

…2816) * [3.6] bpo-30822: Fix testing of datetime module. (GH-2530) (GH-2783) Only C implementation was tested. (cherry picked from commit 287c559) * [3.6] bpo-30822: Fix testing of datetime module. (GH-2530) (GH-2783) Only C implementation was tested.. (cherry picked from commit 287c559)

musically-ut added 2 commits July 1, 2017 14:56

Test both _Pure and _Fast impl of datetime.

a55cc6d

Fix name modification and variant saving of tests.

c4b6b32

the-knights-who-say-ni added the CLA signed label Jul 1, 2017

Add name to ACKS file.

7f52bab

serhiy-storchaka reviewed Jul 2, 2017

View reviewed changes

serhiy-storchaka requested a review from abalkin July 2, 2017 03:25

Report skipped tests.

a67964b

serhiy-storchaka reviewed Jul 2, 2017

View reviewed changes

Drop testing _test_type in favor of cls.__name__.

db40f96

serhiy-storchaka approved these changes Jul 2, 2017

View reviewed changes

Remove redundant returns.

61c240d

serhiy-storchaka merged commit 98b6bc3 into python:master Jul 2, 2017

serhiy-storchaka added needs backport to 3.5 tests Tests in the Lib/test dir labels Jul 2, 2017

musically-ut added a commit to musically-ut/cpython that referenced this pull request Jul 3, 2017

[3.5] bpo-30822: Fix testing of datetime module. (pythonGH-2530)

e62922c

Only C implementation was tested.. (cherry picked from commit 98b6bc3)

bedevere-bot removed the needs backport to 3.5 label Jul 3, 2017

musically-ut added a commit to musically-ut/cpython that referenced this pull request Jul 3, 2017

[3.6] bpo-30822: Fix testing of datetime module. (pythonGH-2530)

f4753c7

Only C implementation was tested. (cherry picked from commit 98b6bc3)

bedevere-bot removed the needs backport to 3.6 label Jul 3, 2017

vstinner added a commit that referenced this pull request Jul 5, 2017

Revert "bpo-30822: Fix testing of datetime module. (#2530)"

f7149af

This reverts commit 98b6bc3.

vstinner mentioned this pull request Jul 5, 2017

Revert "bpo-30822: Fix testing of datetime module." #2588

Merged

musically-ut added a commit to musically-ut/cpython that referenced this pull request Jul 20, 2017

bpo-30822: Fix testing of datetime module. (python#2530)

ae86838

Only C implementation was tested.

vstinner pushed a commit that referenced this pull request Jul 21, 2017

bpo-30822: Fix testing of datetime module. (#2530) (#2783)

287c559

Only C implementation was tested.

vstinner mentioned this pull request Jul 21, 2017

bpo-30822: Fix testing of datetime module. #2783

Merged

musically-ut added a commit to musically-ut/cpython that referenced this pull request Jul 22, 2017

[3.5] bpo-30822: Fix testing of datetime module. (pythonGH-2530) (pyt…

3ecb28a

…honGH-2783) Only C implementation was tested.. (cherry picked from commit 287c559)

musically-ut added a commit to musically-ut/cpython that referenced this pull request Jul 22, 2017

[3.6] bpo-30822: Fix testing of datetime module. (pythonGH-2530) (pyt…

ee2799c

…honGH-2783) Only C implementation was tested. (cherry picked from commit 287c559)

musically-ut added a commit to musically-ut/cpython that referenced this pull request Jul 22, 2017

[3.6] bpo-30822: Fix testing of datetime module. (pythonGH-2530) (pyt…

b187f0b

…honGH-2783) Only C implementation was tested.. (cherry picked from commit 287c559)

ammaraskar mentioned this pull request May 16, 2018

bpo-29097: Forego fold detection on windows for low timestamp values #2385

Merged

Uh oh!

bpo-30822: Fix testing of datetime module. #2530

bpo-30822: Fix testing of datetime module. #2530

Uh oh!

Conversation

musically-ut commented Jul 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Jul 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jul 3, 2017

Uh oh!

bedevere-bot commented Jul 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

musically-ut commented Jul 1, 2017 •

edited

Loading