Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
Remove raw asserts in test_typing.py#104951
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Lib/test/test_typing.py Outdated
| self.assertEqual(Point2Dor3D.__required_keys__,frozenset(['x', 'y'])) | ||
| self.assertEqual(Point2Dor3D.__optional_keys__,frozenset(['z'])) |
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.
The changes in this PR look good. But it looks like we have a lot of tests in this test class that do things likeself.assertEqual(some_frozenset, frozenset(['x', 'y'])). Thatlooks like it's checking thatsome_frozenset is a frozenset, but it's actually not, because frozensets and sets with the same members compare equal. So I wonder if we should be doing this:
| self.assertEqual(Point2Dor3D.__required_keys__,frozenset(['x','y'])) | |
| self.assertEqual(Point2Dor3D.__optional_keys__,frozenset(['z'])) | |
| self.assertEqual(Point2Dor3D.__required_keys__, {'x','y'}) | |
| self.assertIsInstance(Point2Dor3D.__required_keys__,frozenset) | |
| self.assertEqual(Point2Dor3D.__optional_keys__, {'z'}) | |
| self.assertIsInstance(Point2Dor3D.__optional_keys__,frozenset) |
But maybe we don't need to assert the type of__required_keys__ every time we want to check the contents of the set.
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.
Anyway, we don't need to change that in this PR; the changes here are definitely good.
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.
Might as well do that in this PR too.
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.
Pushed a few new asserts checking that it's really a frozenset
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
Thanks@JelleZijlstra for the PR, and@AlexWaygood for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12. |
(cherry picked from commit2cb4456)Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
bedevere-bot commentedMay 26, 2023
GH-104978 is a backport of this pull request to the3.12 branch. |
bedevere-bot commentedMay 26, 2023
GH-104979 is a backport of this pull request to the3.11 branch. |
(cherry picked from commit2cb4456)Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
No description provided.