GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-109218: Refactor tests for the complex() constructor by serhiy-storchaka · Pull Request #119635 · python/cpython

Conversation

@serhiy-storchaka

@serhiy-storchaka serhiy-storchaka commented May 27, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@serhiy-storchaka
pythongh-109218: Refactor tests for the complex() constructor
c35779b
* 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.
@serhiy-storchaka 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 only security fixes labels May 27, 2024
@serhiy-storchaka
serhiy-storchaka requested a review from mdickinson May 27, 2024 20:40
@bedevere-app bedevere-app Bot added the awaiting core review label May 27, 2024
@bedevere-app bedevere-app Bot mentioned this pull request May 27, 2024
Invalid "equivalents" of the complex type constructor in docs #109218
Closed
@serhiy-storchaka
Merge branch 'main' into complex-constructor-tests
607e2d5
skirpichev
skirpichev approved these changes May 28, 2024
View reviewed changes

@skirpichev skirpichev left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just few nitpicks. No test coverage regressions.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_complex.py Outdated
Comment on lines +24 to +44
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

@skirpichev skirpichev May 28, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@serhiy-storchaka serhiy-storchaka May 28, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_complex.py
Comment on lines +692 to +693
self.assertFloatsAreIdentical(z1.imag, 0.0)
self.assertFloatsAreIdentical(z2.imag, -0.0)

@skirpichev skirpichev May 28, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@serhiy-storchaka serhiy-storchaka May 28, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@serhiy-storchaka
Add more tests.
e8b6ba9
@serhiy-storchaka serhiy-storchaka mentioned this pull request May 28, 2024
gh-109218: Deprecate weird cases in the complex() constructor #119620
Merged
serhiy-storchaka
serhiy-storchaka commented May 28, 2024
View reviewed changes
Comment thread Lib/test/test_complex.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka
Update Lib/test/test_complex.py
3c93ce2
mdickinson
mdickinson approved these changes May 29, 2024
View reviewed changes

@mdickinson mdickinson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_complex.py Outdated
(1, 0+0j),
)

class MockIndex:

@mdickinson mdickinson May 29, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@serhiy-storchaka serhiy-storchaka May 29, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@mdickinson mdickinson May 29, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_complex.py Outdated
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

@mdickinson mdickinson May 29, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_complex.py Outdated
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))

@mdickinson mdickinson May 29, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app 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
@serhiy-storchaka
Rename some classes.
3cb4b62
@serhiy-storchaka
Remove an outdated comment.
dbbd5ad
@serhiy-storchaka
Merge branch 'main' into complex-constructor-tests
bdbc301
@serhiy-storchaka
serhiy-storchaka enabled auto-merge (squash) May 29, 2024 18:25
skirpichev
skirpichev reviewed May 30, 2024
View reviewed changes
Comment thread 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)

@skirpichev skirpichev May 30, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@serhiy-storchaka serhiy-storchaka May 30, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a different issue.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
serhiy-storchaka added 2 commits May 30, 2024 12:42
@serhiy-storchaka
Merge branch 'main' into complex-constructor-tests
54fbfde
@serhiy-storchaka
Merge branch 'main' into complex-constructor-tests
8211f2d
@serhiy-storchaka
serhiy-storchaka merged commit bf098d4 into python:main May 30, 2024
@miss-islington-app

miss-islington-app Bot commented May 30, 2024

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the awaiting merge label May 30, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 30, 2024
@serhiy-storchaka @miss-islington
pythongh-109218: Refactor tests for the complex() constructor (python…
354f797
…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 <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 30, 2024
@serhiy-storchaka @miss-islington
pythongh-109218: Refactor tests for the complex() constructor (python…
b7379b9
…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 <[email protected]>
@bedevere-app

bedevere-app Bot commented May 30, 2024

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label May 30, 2024
@bedevere-app

bedevere-app Bot commented May 30, 2024

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label May 30, 2024
serhiy-storchaka added a commit that referenced this pull request May 30, 2024
@miss-islington @serhiy-storchaka
[3.12] gh-109218: Refactor tests for the complex() constructor (GH-11…
2e31e56
…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 <[email protected]>
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@serhiy-storchaka @estyxx
pythongh-109218: Refactor tests for the complex() constructor (python…
23d4599
…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.
serhiy-storchaka added a commit that referenced this pull request Aug 9, 2024
@miss-islington @serhiy-storchaka
[3.13] gh-109218: Refactor tests for the complex() constructor (GH-11…
013752c
…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 <[email protected]>
@serhiy-storchaka
serhiy-storchaka deleted the complex-constructor-tests branch July 1, 2026 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@skirpichev skirpichev skirpichev approved these changes
+1 more reviewer
@mdickinson mdickinson mdickinson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@serhiy-storchaka @mdickinson @skirpichev
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.