gh-110525: Add tests for internal `set` CAPI #110630

sobolevn · 2023-10-10T13:05:08Z

Issue: Improve CAPI tests of set and frozenset #110525

Modules/_testinternalcapi/set.c

serhiy-storchaka · 2023-10-10T13:59:37Z

Modules/_testinternalcapi/set.c

+    NULLABLE(set);
+
+    rc = _PySet_NextEntry(set, &pos, &item, &hash);
+    if (rc == 1) {


What if it returns 2 or -2?

Suggested change

if (rc == 1) {

if (rc != 0 && rc != -1) {

Sorry, I don't understand this one. Right now it is defined as:

cpython/Objects/setobject.c

Lines 2332 to 2346 in 66a9b10

int

_PySet_NextEntry(PyObject *set, Py_ssize_t *pos, PyObject **key, Py_hash_t *hash)

{

setentry *entry;

if (!PyAnySet_Check(set)) {

PyErr_BadInternalCall();

return -1;

}

if (set_next((PySetObject *)set, pos, &entry) == 0)

return 0;

*key = entry->key;

*hash = entry->hash;

return 1;

}

It cannot return anything except [-1, 0, 1].
Do you mean that it can return something other than [-1, 0, 1] in the future?
I think that our test case must catch this change and be adapted if needed.

This function is not documented currently.

The purpose of the test is to verify our assumptions (that it cannot return anything except [-1, 0, 1]). Otherwise there would not be need of tests.

In future a new return can be added in the code, or refactoring can lead to returning non-initialized variable in rare case. The wrapper will successfully return None.

Got it, I used an assertion, which is more readable in my opinion 👍

Modules/_testinternalcapi/set.c

Lib/test/test_capi/test_set.py

miss-islington · 2023-10-10T16:00:42Z

Thanks @sobolevn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington · 2023-10-10T16:00:45Z

Sorry, @sobolevn and @serhiy-storchaka, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 9cfb4e0d1ebf2900c19ee07697818c621f46cc3d 3.12

sobolevn · 2023-10-10T16:29:27Z

Working on a backport

sobolevn · 2023-10-10T16:41:05Z

@serhiy-storchaka looks like #108787 never got backported, so we don't have the same file structure on 3.12

pythongh-110525: Add tests for internal set CAPI

f82ca46

sobolevn requested a review from serhiy-storchaka October 10, 2023 13:05

sobolevn requested a review from a team as a code owner October 10, 2023 13:05

bedevere-app bot added the awaiting review label Oct 10, 2023

bedevere-app bot mentioned this pull request Oct 10, 2023

Improve CAPI tests of set and frozenset #110525

Closed

3 tasks

sobolevn added the skip news label Oct 10, 2023

serhiy-storchaka reviewed Oct 10, 2023

View reviewed changes

Modules/_testinternalcapi/set.c Outdated Show resolved Hide resolved

Address review: simplify set_next_entry

338d340

serhiy-storchaka reviewed Oct 10, 2023

View reviewed changes

sobolevn added 2 commits October 10, 2023 18:00

Address review

21ca91f

Add assertion that rc cannot be anything else than [-1, 0, 1]

e4ff92b

serhiy-storchaka approved these changes Oct 10, 2023

View reviewed changes

bedevere-app bot added awaiting core review and removed awaiting review labels Oct 10, 2023

serhiy-storchaka merged commit 9cfb4e0 into python:main Oct 10, 2023

bedevere-app bot removed the awaiting core review label Oct 10, 2023

serhiy-storchaka added the needs backport to 3.12 only security fixes label Oct 10, 2023

miss-islington assigned serhiy-storchaka Oct 10, 2023

serhiy-storchaka removed the request for review from a team October 10, 2023 16:00

sobolevn mentioned this pull request Oct 10, 2023

gh-108777: Split _PyTime tests from _testinternalcapi.c #108787

Merged

serhiy-storchaka removed the needs backport to 3.12 only security fixes label Nov 10, 2023

serhiy-storchaka removed their assignment Nov 10, 2023

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024

pythongh-110525: Add tests for internal set CAPI (pythonGH-110630)

570e655

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-110525: Add tests for internal `set` CAPI #110630

gh-110525: Add tests for internal `set` CAPI #110630

sobolevn commented Oct 10, 2023 •

edited by bedevere-app bot

Loading

serhiy-storchaka Oct 10, 2023

sobolevn Oct 10, 2023

serhiy-storchaka Oct 10, 2023 •

edited

Loading

sobolevn Oct 10, 2023

miss-islington commented Oct 10, 2023

miss-islington commented Oct 10, 2023

sobolevn commented Oct 10, 2023

sobolevn commented Oct 10, 2023

	int
	_PySet_NextEntry(PyObject set, Py_ssize_t pos, PyObject *key, Py_hash_t hash)
	{
	setentry *entry;

	if (!PyAnySet_Check(set)) {
	PyErr_BadInternalCall();
	return -1;
	}
	if (set_next((PySetObject *)set, pos, &entry) == 0)
	return 0;
	*key = entry->key;
	*hash = entry->hash;
	return 1;
	}

gh-110525: Add tests for internal set CAPI #110630

gh-110525: Add tests for internal set CAPI #110630

Conversation

sobolevn commented Oct 10, 2023 • edited by bedevere-app bot Loading

serhiy-storchaka Oct 10, 2023

Choose a reason for hiding this comment

sobolevn Oct 10, 2023

Choose a reason for hiding this comment

serhiy-storchaka Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

sobolevn Oct 10, 2023

Choose a reason for hiding this comment

miss-islington commented Oct 10, 2023

miss-islington commented Oct 10, 2023

sobolevn commented Oct 10, 2023

sobolevn commented Oct 10, 2023

gh-110525: Add tests for internal `set` CAPI #110630

gh-110525: Add tests for internal `set` CAPI #110630

sobolevn commented Oct 10, 2023 •

edited by bedevere-app bot

Loading

serhiy-storchaka Oct 10, 2023 •

edited

Loading