-
-
Notifications
You must be signed in to change notification settings - Fork 31k
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-106368: Argument Clinic: Add tests for cloned functions with custom C base names #107977
gh-106368: Argument Clinic: Add tests for cloned functions with custom C base names #107977
Conversation
erlend-aasland
commented
Aug 15, 2023
•
edited by bedevere-bot
Loading
edited by bedevere-bot
- Issue: Increase Argument Clinic test coverage #106368
…ustom C base name
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.
There is still one case in this block that is untested:
cpython/Tools/clinic/clinic.py
Lines 4888 to 4890 in e28b0dc
if (is_legal_py_identifier(full_name) and | |
(not c_basename or is_legal_c_identifier(c_basename)) and | |
is_legal_py_identifier(existing)): |
What if is_legal_py_identifier(full_name)
evaluates to True
, not c_basename
evaluates to False
, is_legal_c_identifier(c_basename)
evaluates to False
and is_legal_py_identifier(existing)
evaluates to True
?
I.e., if I apply this diff to your PR branch, the assertion is never triggered:
- if (is_legal_py_identifier(full_name) and
- (not c_basename or is_legal_c_identifier(c_basename)) and
- is_legal_py_identifier(existing)):
+ if is_legal_py_identifier(full_name) and is_legal_py_identifier(existing):
+ if c_basename:
+ assert is_legal_c_identifier(c_basename)
...Could you also add a test with a cloned function with a custom C base name where the custom C base name is not a legal C identifier?
I'm getting warnings about the execution environment when I'm running Ran 243 tests in 0.620s
OK
Warning -- files was modified by test_clinic
Warning -- Before: []
Warning -- After: ['clinic/']
Warning -- files was modified by test_clinic
Warning -- Before: []
Warning -- After: ['clinic/']
test_clinic failed (env changed)
== Tests result: SUCCESS ==
1 test altered the execution environment:
test_clinic
Total duration: 1.2 sec
Tests result: SUCCESS |
Ah, and it looks like the CI is complaining about the same thing: https://github.com/python/cpython/actions/runs/5868181574/job/15910445072 |
Ah, yeah, I noticed earlier today, but got sidetracked. Since we're running a "expect success" test, we will actually generate clinic output in the added test (hence the sudden |
I wonder why we're not seeing complaints about |
... because they don't create output; either they're dumping to block or buffer, or they're testing some weird directive, or they're run with precomputed checksums in the input (hence no regenerated output). |
Ah, good call. On it! |
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.
Looks great, thank you!
Likewise! |