-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-95065: Argument Clinic: Add functional tests of deprecated positionals #107768
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
gh-95065: Argument Clinic: Add functional tests of deprecated positionals #107768
Conversation
…ositionals Move the "deprecated positinal" tests from clinic.test.c to _testclinic.c. Mock PY_VERSION_HEX in order to prevent the generated compiler warnings/errors to trigger. Put clinic code for deprecated positionals in Modules/clinic/_testclinic_depr_star.c.h for easy inspection of the generated code.
The big diff is caused by moving the generated clinic code. |
@serhiy-storchaka, IMO this is better than visual inspection of the generated code. We want to prevent regressions because of future refactorings, features and bugfixes. |
The CI currently isn't running on this PR due to merge conflicts -- I tried building with it locally, but it causes build failures on Windows:
|
Does it still happen? |
output push | ||
destination deprstar new file '{dirname}/clinic/_testclinic_depr_star.c.h' | ||
output everything deprstar | ||
#output methoddef_ifndef buffer 1 |
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 output directive has a bug that prevents us from specifying the stack index. We should fix that in a separate PR.
Nope! Builds fine locally now :) |
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.
Would not be better to use version like 100.200 to avoid rewriting tests every year?
No need, we mock the version. |
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.
Do the test fail if change the stacklevel argument in PyErr_WarnEx()
from 1 to 2?
They do not. The same filename is produced for both stack levels. |
And wrong stacklevel is a common error. Even your original code did have it wrong initially. Therefore, I think it is important to check it using one of the methods I suggested above. |
Note: we will get merge conflicts from #107808. |
Co-authored-by: Alex Waygood <[email protected]>
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 agree with Serhiy that explicitly testing the stacklevel would be good. Other than that, this is looking much better now, thanks! The readabiliy is much improved :)
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'm happy if Serhiy's happy!
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.
Few nitpicks and LGTM.
Co-authored-by: Serhiy Storchaka <[email protected]>
Thank you Serhiy and Alex; this PR is in much better shape now! With this, we've got a nice framework for similar tests in place. I'll start working on what's left in clinic.test.c. |
Move the "deprecated positinal" tests from clinic.test.c to
_testclinic.c. Mock PY_VERSION_HEX in order to prevent the generated
compiler warnings/errors to trigger. Put clinic code for deprecated
positionals in Modules/clinic/_testclinic_depr_star.c.h for easy
inspection of the generated code.