Uh oh!
There was an error while loading. Please reload this page.
GH-94808: Cover PyOS_mystrnicmp and PyOS_mystricmp - #102469
Conversation
bedevere-bot
commented
Mar 6, 2023
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
artemmukhin
commented
Mar 10, 2023
I'm not sure who is best positioned to review this, but maybe @corona10 could take a look? |
corona10
commented
Mar 10, 2023
Okay, I will take a look by this weekend :) |
artemmukhin
commented
Mar 10, 2023
@corona10 Thank you! |
corona10
left a comment
There was a problem hiding this comment.
Would you like to separate the file into Modules/_testcapi/pyos.c
likewise https://github.com/python/cpython/blob/main/Modules/_testcapi/float.c ?
The test itself looks good.
bedevere-bot
commented
Mar 11, 2023
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I have made the requested changes; please review again. @corona10 Thank you for the review! I've separated the tests. Although I haven't managed to run the tests from |
bedevere-bot
commented
Mar 11, 2023
Thanks for making the requested changes! @corona10: please review the changes made to this pull request. |
artemmukhin
commented
Mar 11, 2023
I've found out that the tests were not executed because of the wrong naming.
cpython/Lib/test/test_capi/test_misc.py Lines 1422 to 1425 in ced13c9 It is mentioned in the C API Tests paragraph to some extent:
But at first reading, I got confused because in cpython/Modules/_testcapi/float.c Lines 84 to 88 in 534660f However, in contrast to my tests, these test methods are actually executed through cpython/Lib/test/test_float.py Lines 1516 to 1519 in 534660f I hope this investigation might help other first-time contributors. @corona10 If you think it would be helpful to clarify this point in the devguide, please let me know, and I'll be happy to open a corresponding PR. |
corona10
commented
Mar 12, 2023
AFAIK, those things are intended. |
corona10
left a comment
There was a problem hiding this comment.
LGTM
But I want to listen to the opinion of @erlend-aasland for this testing structure.
artemmukhin
commented
Mar 16, 2023
@erlend-aasland could you please take a look? |
artemmukhin
commented
Mar 22, 2023
Thank you! Cc @corona10 |
Uh oh!
There was an error while loading. Please reload this page.