GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-109218: Refactor tests for the complex() constructor by serhiy-storchaka · Pull Request #119635 · python/cpython Conversation
|
| 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.
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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
| self.assertFloatsAreIdentical(z1.imag, 0.0) | ||
| self.assertFloatsAreIdentical(z2.imag, -0.0) |
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| (1, 0+0j), | ||
| ) | ||
|
|
||
| class MockIndex: |
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
| 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.
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.
There was an error while loading. Please reload this page.
| 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.
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.
There was an error while loading. Please reload this page.
| 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) |
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…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]>
…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]>
|
GH-119795 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
GH-119796 is a backport of this pull request to the 3.12 branch. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…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]>
…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 <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.