Skip to content
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

Improve CAPI tests of set and frozenset #110525

Closed
3 tasks done
sobolevn opened this issue Oct 8, 2023 · 0 comments
Closed
3 tasks done

Improve CAPI tests of set and frozenset #110525

sobolevn opened this issue Oct 8, 2023 · 0 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) tests Tests in the Lib/test dir topic-C-API type-feature A feature request or enhancement

Comments

@sobolevn
Copy link
Member

sobolevn commented Oct 8, 2023

Feature or enhancement

Right now CAPI tests of set and frozenset are quite outdated. They are defined as .test_c_api method on set objects, when Py_DEBUG is set:

cpython/Objects/setobject.c

Lines 2371 to 2511 in 7e30821

#ifdef Py_DEBUG
/* Test code to be called with any three element set.
Returns True and original set is restored. */
#define assertRaises(call_return_value, exception) \
do { \
assert(call_return_value); \
assert(PyErr_ExceptionMatches(exception)); \
PyErr_Clear(); \
} while(0)
static PyObject *
test_c_api(PySetObject *so, PyObject *Py_UNUSED(ignored))
{
Py_ssize_t count;
const char *s;
Py_ssize_t i;
PyObject *elem=NULL, *dup=NULL, *t, *f, *dup2, *x=NULL;
PyObject *ob = (PyObject *)so;
Py_hash_t hash;
PyObject *str;
/* Verify preconditions */
assert(PyAnySet_Check(ob));
assert(PyAnySet_CheckExact(ob));
assert(!PyFrozenSet_CheckExact(ob));
/* so.clear(); so |= set("abc"); */
str = PyUnicode_FromString("abc");
if (str == NULL)
return NULL;
set_clear_internal(so);
if (set_update_internal(so, str)) {
Py_DECREF(str);
return NULL;
}
Py_DECREF(str);
/* Exercise type/size checks */
assert(PySet_Size(ob) == 3);
assert(PySet_GET_SIZE(ob) == 3);
/* Raise TypeError for non-iterable constructor arguments */
assertRaises(PySet_New(Py_None) == NULL, PyExc_TypeError);
assertRaises(PyFrozenSet_New(Py_None) == NULL, PyExc_TypeError);
/* Raise TypeError for unhashable key */
dup = PySet_New(ob);
assertRaises(PySet_Discard(ob, dup) == -1, PyExc_TypeError);
assertRaises(PySet_Contains(ob, dup) == -1, PyExc_TypeError);
assertRaises(PySet_Add(ob, dup) == -1, PyExc_TypeError);
/* Exercise successful pop, contains, add, and discard */
elem = PySet_Pop(ob);
assert(PySet_Contains(ob, elem) == 0);
assert(PySet_GET_SIZE(ob) == 2);
assert(PySet_Add(ob, elem) == 0);
assert(PySet_Contains(ob, elem) == 1);
assert(PySet_GET_SIZE(ob) == 3);
assert(PySet_Discard(ob, elem) == 1);
assert(PySet_GET_SIZE(ob) == 2);
assert(PySet_Discard(ob, elem) == 0);
assert(PySet_GET_SIZE(ob) == 2);
/* Exercise clear */
dup2 = PySet_New(dup);
assert(PySet_Clear(dup2) == 0);
assert(PySet_Size(dup2) == 0);
Py_DECREF(dup2);
/* Raise SystemError on clear or update of frozen set */
f = PyFrozenSet_New(dup);
assertRaises(PySet_Clear(f) == -1, PyExc_SystemError);
assertRaises(_PySet_Update(f, dup) == -1, PyExc_SystemError);
assert(PySet_Add(f, elem) == 0);
Py_INCREF(f);
assertRaises(PySet_Add(f, elem) == -1, PyExc_SystemError);
Py_DECREF(f);
Py_DECREF(f);
/* Exercise direct iteration */
i = 0, count = 0;
while (_PySet_NextEntry((PyObject *)dup, &i, &x, &hash)) {
s = PyUnicode_AsUTF8(x);
assert(s && (s[0] == 'a' || s[0] == 'b' || s[0] == 'c'));
count++;
}
assert(count == 3);
/* Exercise updates */
dup2 = PySet_New(NULL);
assert(_PySet_Update(dup2, dup) == 0);
assert(PySet_Size(dup2) == 3);
assert(_PySet_Update(dup2, dup) == 0);
assert(PySet_Size(dup2) == 3);
Py_DECREF(dup2);
/* Raise SystemError when self argument is not a set or frozenset. */
t = PyTuple_New(0);
assertRaises(PySet_Size(t) == -1, PyExc_SystemError);
assertRaises(PySet_Contains(t, elem) == -1, PyExc_SystemError);
Py_DECREF(t);
/* Raise SystemError when self argument is not a set. */
f = PyFrozenSet_New(dup);
assert(PySet_Size(f) == 3);
assert(PyFrozenSet_CheckExact(f));
assertRaises(PySet_Discard(f, elem) == -1, PyExc_SystemError);
assertRaises(PySet_Pop(f) == NULL, PyExc_SystemError);
Py_DECREF(f);
/* Raise KeyError when popping from an empty set */
assert(PyNumber_InPlaceSubtract(ob, ob) == ob);
Py_DECREF(ob);
assert(PySet_GET_SIZE(ob) == 0);
assertRaises(PySet_Pop(ob) == NULL, PyExc_KeyError);
/* Restore the set from the copy using the PyNumber API */
assert(PyNumber_InPlaceOr(ob, dup) == ob);
Py_DECREF(ob);
/* Verify constructors accept NULL arguments */
f = PySet_New(NULL);
assert(f != NULL);
assert(PySet_GET_SIZE(f) == 0);
Py_DECREF(f);
f = PyFrozenSet_New(NULL);
assert(f != NULL);
assert(PyFrozenSet_CheckExact(f));
assert(PySet_GET_SIZE(f) == 0);
Py_DECREF(f);
Py_DECREF(elem);
Py_DECREF(dup);
Py_RETURN_TRUE;
}
#undef assertRaises
#endif

There are several things that I can see as problematic:

  • It is stored together with the production code
  • These tests are mixing CAPI (PySet_Check), internal CAPI (_PySet_Update), and internal function calls (set_clear_internal)
  • These tests are hard to parametrize since they are called as

@unittest.skipUnless(hasattr(set, "test_c_api"),
'C API test only available in a debug build')
def test_c_api(self):
self.assertEqual(set().test_c_api(), True)

  • Multiple things are not tested: like set and frozenset subclasses
  • Test failures are not quite informative, because using C assert
  • This is a single huge test
  • This test is only run under debug builds and not under "release" builds

I propose a plan to improve it:

  • Move CAPI tests to Modules/_testcapi/set.c and add Lib/test/test_capi/test_set.py
  • Move internal CAPI tests Modules/_testinternalcapi/set.c and make sure that they are executed in test_capi.py
  • Delete test_c_api from setobject.c

I plan to work on this and already have the PR for 1. :)

Linked PRs

@sobolevn sobolevn added type-feature A feature request or enhancement tests Tests in the Lib/test dir interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-C-API labels Oct 8, 2023
@sobolevn sobolevn self-assigned this Oct 8, 2023
sobolevn added a commit to sobolevn/cpython that referenced this issue Oct 8, 2023
sobolevn added a commit to sobolevn/cpython that referenced this issue Oct 8, 2023
sobolevn added a commit to sobolevn/cpython that referenced this issue Oct 9, 2023
sobolevn added a commit to sobolevn/cpython that referenced this issue Oct 9, 2023
…ythonGH-110526).

(cherry picked from commit c49edd7)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Oct 9, 2023
…ythonGH-110544)

(cherry picked from commit ea39c87)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
serhiy-storchaka pushed a commit that referenced this issue Oct 9, 2023
…H-110544) (GH-110554)

(cherry picked from commit ea39c87)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
sobolevn added a commit to sobolevn/cpython that referenced this issue Oct 10, 2023
sobolevn added a commit to sobolevn/cpython that referenced this issue Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) tests Tests in the Lib/test dir topic-C-API type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

1 participant