gh-155503: Add more PyType C API tests - #155505
Conversation
Add tests on functions: * PyType_ClearCache() * PyType_GetFlags() * PyType_IsSubtype() * PyType_Ready() Move PyType limited C API tests from _testcapi to _testlimitedcapi. Add a new Modules/_testlimitedcapi/type.c file.
|
Keep PyType_GetSlot() test in _testcapi since it does access directly |
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Add tests for non-type arguments and NULL.
|
|
||
|
|
||
| static int | ||
| check_type_arg(PyObject *arg) |
There was a problem hiding this comment.
Absolutely not. You should not check the type of the argument. Use NULLABLE() instead.
If the tested API accepts non-type or NULL -- fine, you will add a test. If it crashes -- just add the commented out call to denote that such corner case was considered, but cannot be tested.
| { | ||
| PyTypeObject *type1, *type2; | ||
| if (!PyArg_ParseTuple(args, "O!O!", | ||
| &PyType_Type, &type1, |
There was a problem hiding this comment.
Do not enforce the type of arguments. Add tests for non-type arguments.
* Accept None: treat None as NULL * Fix typo in _PyTestLimitedCAPI_Init_Type(): return -1 on error. * Add test_type_modified(). * Move PyType_IsSubtype() result check to Python.
|
@serhiy-storchaka: I addressed your review. Please review the updated PR.
Ok, I modified the test wrappers to treat None as NULL. I added "CRASHES func(NULL)" comments: all tested PyType functions crash when NULL is passed.
The PyType C API is special: the first argument type is If you call a PyType C API with a |
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Remove type checks. If non-types are not supported, just do not pass non-types in tests. If they are supported, they could be tested. We should not accidentally test a TypeError raised by the wrapper.
For example, it looks to me that PyType_IsSubtype(a, b) works if b is a non-type or even NULL (the code only compares pointers).
I removed PyType_Check() checks.
I don't think that we should test this in test_capi. It looks like an implementation detail accident, not something that we want to test. |
|
We should test implementation detail accidents in test_capi, this is one of purposes of the tests. There is a large chance that there is a code that depends on this, even if by accident. If we change that detail in future, we should be aware what we do and properly document the change. You can gate it as CPython implementation detail. |
I disagree. As I wrote previously, if you pass a |
|
I added "CRASHES ...: argument must be a type" comments. |
Add tests on functions:
Move PyType limited C API tests from _testcapi to _testlimitedcapi. Add a new Modules/_testlimitedcapi/type.c file.