Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-109218: Refactor tests for the complex() constructor#119635
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
gh-109218: Refactor tests for the complex() constructor#119635
Uh oh!
There was an error while loading.Please reload this page.
Conversation
* 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.
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.
Lib/test/test_complex.py Outdated
| 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 | ||
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
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
| self.assertFloatsAreIdentical(z1.imag,0.0) | ||
| self.assertFloatsAreIdentical(z2.imag, -0.0) |
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:
defassertComplexesAreIdentical(self,x,y):self.assertFloatsAreIdentical(x.real,y.real)self.assertFloatsAreIdentical(x.imag,y.imag)
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.
Uh oh!
There was an error while loading.Please reload this page.
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
Lib/test/test_complex.py Outdated
| @@ -21,6 +21,27 @@ | |||
| (1, 0+0j), | |||
| ) | |||
| class MockIndex: | |||
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: theMock prefix on these classes doesn't feel appropriate: nothing's being mocked here. I'd probably go with something likeImplementsIndex orHasIndex.
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 commonFakePath 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 theMock prefix which looks more neutral and test specific thanFake. Different tests currently use similar classes with different names. The most common prefix is perhapsMy. PrefixSupports cannot be used because it can conflict withtyping.SupportIndex, etc. SuffixLike can conflict withos.PathLike.
I see that several tests use theWith prefix:WithStr,WithRepr,WithIterAnext, etc. What do you think aboutWithIndex?
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.
Lib/test/test_complex.py Outdated
| check(complex(real=4.25+1.5j), 4.25, 1.5) | ||
| check(complex(imag=1.5), 0.0, 1.5) | ||
| check(complex(real=4.25, imag=1.5), 4.25, 1.5) | ||
| 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 |
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.
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)) |
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.
| with self.assertWarns(DeprecationWarning): | ||
| self.assertEqual(complex(complex1(1j)),2j) | ||
| check(complex(complex1(1j)),0.0, 2.0) |
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.
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.
bf098d4 intopython:mainUh oh!
There was an error while loading.Please reload this page.
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 commitbf098d4)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.(cherry picked from commitbf098d4)Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
GH-119795 is a backport of this pull request to the3.13 branch. |
GH-119796 is a backport of this pull request to the3.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 commitbf098d4)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.
…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 commitbf098d4)Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Uh oh!
There was an error while loading.Please reload this page.