gh-109218: Refactor tests for the complex() constructor by serhiy-storchaka · Pull Request #119635 · python/cpython

serhiy-storchaka · 2024-05-27T20:40:46Z

Share common classes.
Use exactly representable floats and exact tests.
Check the sign of zero components.
Remove duplicated tests (mostly left after merging int and long).
Reorder tests in more consistent way.
Test more error messages.
Add tests for missed cases.

Issue: Invalid "equivalents" of the complex type constructor in docs #109218

* Share common classes. * Use exactly representable floats and exact tests. * Check the sign of zero components. * Remove duplicated tests (mostly left after merging int and long). * Reorder tests in more consistent way. * Test more error messages. * Add tests for missed cases.

skirpichev

LGTM, just few nitpicks. No test coverage regressions.

skirpichev · 2024-05-28T07:55:42Z

Lib/test/test_complex.py

+class MockIndex:
+    def __init__(self, value):
+        self.value = value
+    def __index__(self):
+        return self.value
+
+class MockFloat:
+    def __init__(self, value):
+        self.value = value
+    def __float__(self):
+        return self.value
+
+class ComplexSubclass(complex):
+    pass
+
+class MockComplex:
+    def __init__(self, value):
+        self.value = value
+    def __complex__(self):
+        return self.value
+


Note, that similar code could be shared also with test_long.py and test_float.py. That refactoring of the scope here, but maybe in this pr you could put these classes somewhere in the support module? // see #110956

I proposed this in #84310. But for some reasons this idea was not liked, so now we need to duplicate the code in many test files. cc @rhettinger

skirpichev · 2024-05-28T08:07:52Z

Lib/test/test_complex.py

+        self.assertFloatsAreIdentical(z1.imag, 0.0)
+        self.assertFloatsAreIdentical(z2.imag, -0.0)


This pattern is quite common. Maybe there should be helper method like:

def assertComplexesAreIdentical(self, x, y): self.assertFloatsAreIdentical(x.real, y.real) self.assertFloatsAreIdentical(x.imag, y.imag)

Yes, I seen this in your PR, and this looks helpful. There are also similar methods (with different names) in other test files. Perhaps we could move the best implementation in a common file and use it everywhere.

Lib/test/test_complex.py

mdickinson

Changes LGTM

mdickinson · 2024-05-29T17:31:01Z

Lib/test/test_complex.py

    (1, 0+0j),
 )

+class MockIndex:


Naming nitpick: the Mock prefix on these classes doesn't feel appropriate: nothing's being mocked here. I'd probably go with something like ImplementsIndex or HasIndex.

I once added common FakePath class which implements the __path__ method, and it is now used in many tests for the path protocol. I proposed to add similar FakeIndex, FakeFloat, etc, but Raymond rejected this idea. Here I use the Mock prefix which looks more neutral and test specific than Fake. Different tests currently use similar classes with different names. The most common prefix is perhaps My. Prefix Supports cannot be used because it can conflict with typing.SupportIndex, etc. Suffix Like can conflict with os.PathLike.

I see that several tests use the With prefix: WithStr, WithRepr, WithIterAnext, etc. What do you think about WithIndex?

WithIndex works. I'm also happy for this to be left as-is - it's not a strong objection, just a niggling feeling that the name isn't quite right.

mdickinson · 2024-05-29T17:33:48Z

Lib/test/test_complex.py

+        check(complex(4.25, imag=1.5), 4.25, 1.5)

        # check that the sign of a zero in the real or imaginary part
        # is preserved when constructing from two floats.  (These checks


We could drop the parenthetical here, given that we're now requiring IEEE 754 for CPython.

mdickinson · 2024-05-29T17:35:33Z

Lib/test/test_complex.py

+        check(complex(MockIndex(42)), 42.0, 0.0)
+        check(complex(MockIndex(42), 1.5), 42.0, 1.5)
+        check(complex(1.5, MockIndex(42)), 1.5, 42.0)
+        self.assertRaises(OverflowError, complex, MockIndex(2**2000))


Interesting - looks like these tests were already assuming IEEE 754 before.

skirpichev · 2024-05-30T03:02:14Z

Lib/test/test_complex.py

+        check(complex(complex0(1j)), 0.0, 42.0)
        with self.assertWarns(DeprecationWarning):
-            self.assertEqual(complex(complex1(1j)), 2j)
+            check(complex(complex1(1j)), 0.0, 2.0)


@serhiy-storchaka, maybe it's time to drop this? Ditto for float's.

Returning non-complex(float) values was deprecated in v3.7.

This is a different issue.

miss-islington-app · 2024-05-30T17:36:02Z

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

…GH-119635) * Share common classes. * Use exactly representable floats and exact tests. * Check the sign of zero components. * Remove duplicated tests (mostly left after merging int and long). * Reorder tests in more consistent way. * Test more error messages. * Add tests for missed cases. (cherry picked from commit bf098d4) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

bedevere-app · 2024-05-30T17:36:16Z

GH-119795 is a backport of this pull request to the 3.13 branch.

bedevere-app · 2024-05-30T17:36:20Z

GH-119796 is a backport of this pull request to the 3.12 branch.

…9635) (GH-119796) * Share common classes. * Use exactly representable floats and exact tests. * Check the sign of zero components. * Remove duplicated tests (mostly left after merging int and long). * Reorder tests in more consistent way. * Test more error messages. * Add tests for missed cases. (cherry picked from commit bf098d4) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

…GH-119635) * Share common classes. * Use exactly representable floats and exact tests. * Check the sign of zero components. * Remove duplicated tests (mostly left after merging int and long). * Reorder tests in more consistent way. * Test more error messages. * Add tests for missed cases.

…9635) (GH-119795) * Share common classes. * Use exactly representable floats and exact tests. * Check the sign of zero components. * Remove duplicated tests (mostly left after merging int and long). * Reorder tests in more consistent way. * Test more error messages. * Add tests for missed cases. (cherry picked from commit bf098d4) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

serhiy-storchaka added tests Tests in the Lib/test dir skip news needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels May 27, 2024

serhiy-storchaka requested a review from mdickinson May 27, 2024 20:40

bedevere-app bot added the awaiting core review label May 27, 2024

bedevere-app bot mentioned this pull request May 27, 2024

Invalid "equivalents" of the complex type constructor in docs #109218

Closed

Merge branch 'main' into complex-constructor-tests

607e2d5

skirpichev approved these changes May 28, 2024

View reviewed changes

Add more tests.

e8b6ba9

serhiy-storchaka mentioned this pull request May 28, 2024

gh-109218: Deprecate weird cases in the complex() constructor #119620

Merged

serhiy-storchaka commented May 28, 2024

View reviewed changes

Lib/test/test_complex.py Outdated Show resolved Hide resolved

Update Lib/test/test_complex.py

3c93ce2

mdickinson approved these changes May 29, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels May 29, 2024

serhiy-storchaka added 3 commits May 29, 2024 21:21

Rename some classes.

3cb4b62

Remove an outdated comment.

dbbd5ad

Merge branch 'main' into complex-constructor-tests

bdbc301

serhiy-storchaka enabled auto-merge (squash) May 29, 2024 18:25

skirpichev reviewed May 30, 2024

View reviewed changes

serhiy-storchaka added 2 commits May 30, 2024 12:42

Merge branch 'main' into complex-constructor-tests

54fbfde

Merge branch 'main' into complex-constructor-tests

8211f2d

serhiy-storchaka merged commit bf098d4 into python:main May 30, 2024

bedevere-app bot removed the awaiting merge label May 30, 2024

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label May 30, 2024

bedevere-app bot removed the needs backport to 3.12 only security fixes label May 30, 2024

		self.assertFloatsAreIdentical(z1.imag, 0.0)
		self.assertFloatsAreIdentical(z2.imag, -0.0)

Uh oh!

Conversation

serhiy-storchaka commented May 27, 2024 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skirpichev left a comment

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!

Uh oh!

mdickinson left a comment

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!

miss-islington-app bot commented May 30, 2024

Uh oh!

bedevere-app bot commented May 30, 2024

Uh oh!

bedevere-app bot commented May 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

serhiy-storchaka commented May 27, 2024 •

edited by bedevere-app bot

Loading