-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-111495: Add tests for PyNumber C API #111996
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
skirpichev
commented
Nov 12, 2023
•
edited by bedevere-app
bot
Loading
edited by bedevere-app
bot
- Issue: Add more C API tests #111495
This comment was marked as resolved.
This comment was marked as resolved.
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 wrapper code is similar for most 1- and 2-argumet functions. What do you think about using macros to generate them? It would be easier to review and extend.
dd182df
to
daf4b6f
Compare
daf4b6f
to
8cf4429
Compare
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.
For now LGTM. But please move changes in abstract.c to a separate PR. This PR should contain tests only.
Also, there is an existing PyNumber test in test_abstract. Please move it to other tests, and the corresponding wrapper too.
15f8a7b
to
2bc6bc7
Compare
That was the plan. But please could you first take look on these changes? This part seems to be finished and I would know, for example, if this can go in one PR or in several. For now, almost every line for PyNumber_* C API is covered. Exceptions are like this: Line 1593 in d4f83e1
Indeed. Should I remove number_check() from the abstract.c or from the number.c instead? Edit:
|
Lib/test/test_capi/test_number.py
Outdated
# Test PyNumber_Negative() | ||
negative = _testcapi.number_negative | ||
|
||
self.assertEqual(negative(42), -42) |
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.
For all tests currently with integers only, add also tests with floats and mixed.
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.
Did this with unary ops, then will extend to other tests as well.
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.
In fact, I think the old test layout was a bad idea. In the last commit I'm using just one method to test all four unary functions (PyNumber_Negative and friends).
This should be doable for binary and ternary functions as well, so I'll go that way, probably could do pr ready tomorrow. Special handling will be required for PyNumber_*Add/Multiply, which also utilize tp_as_sequence. Here almost all cases could be tested using built-in types. But not all. Thus, I think some C-coded test type must be used.
* Add test for [1, 2] and PY_SSIZE_T_MAX//2 + 1 (test_multiply) * test with floats for unary * test_float/index with strings * 42 is too large. What about 3? // tobase * test with float & string // tobase * complex test for absolute
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
Thanks @skirpichev for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
Thanks @skirpichev for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12. |
(cherry picked from commit 2f20f5a) Co-authored-by: Sergey B Kirpichev <[email protected]>
Sorry, @skirpichev and @vstinner, I could not cleanly backport this to
|
GH-123375 is a backport of this pull request to the 3.13 branch. |
(cherry picked from commit 2f20f5a) Co-authored-by: Sergey B Kirpichev <[email protected]>
GH-123376 is a backport of this pull request to the 3.12 branch. |