Movatterモバイル変換

bedevere-appbot mentioned this pull request

Improve CAPI tests ofset andfrozenset#110525

Closed

3 tasks

Fix Win builds

314a7b3

sobolevn requested a review froma team as acode owner

October 8, 2023 20:33

sobolevn added the skip news label

serhiy-storchaka reviewed

Copy link

Member

serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Nice.

Please test also with non-hashable items, non-set as the first argument, NULLs for any argument. If some call crashes, add a comment.

Lib/test/test_capi/test_set.py OutdatedShow resolvedHide resolved

vstinner reviewed

Lib/test/test_capi/test_set.py

Copy link

Member

vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I was never comfortable with _testcapi having fun with changing C API names. We cannot simply use PySet_New() name in _testcapi and in the test? I would make it more obvious that the test is on... PySet_New().


		class TestSetCAPI(unittest.TestCase):
		def assertImmutable(self, action, *args):
		self.assertRaises(SystemError, action, frozenset(), *args)

Copy link

Member

vstinnerOct 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

SystemError? That's a surprising error. Usually, it's used when the C API is misused, like passing NULL or the wrong type. frozenset is a "wrong type" here?

Copy link

MemberAuthor

sobolevnOct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, immutable frozenset is a wrong type for mutation-based functions :)

Lib/test/test_capi/test_set.py OutdatedShow resolvedHide resolved

Copy link

MemberAuthor

sobolevn commentedOct 9, 2023

I was never comfortable with _testcapi having fun with changing C API names. We cannot simply use PySet_New() name in _testcapi and in the test?

I think that this is intentional, because we don't really callPySet_Check, we callset_check intermediate function which changes some behavior compared toPySet_Check. For example, it convertsNone toNULL (so we can test passingNULL easier).

So, I would prefer to keep this naming convention.

Address review

9feda28

sobolevn force-pushed theissue-110525 branch from1ac40e4 to9feda28Compare

October 9, 2023 06:52

serhiy-storchaka reviewed

Lib/test/test_capi/test_set.py OutdatedShow resolvedHide resolved

Lib/test/test_capi/test_set.pyShow resolvedHide resolved

Lib/test/test_capi/test_set.py OutdatedShow resolvedHide resolved

Address review

9b8f7a5

serhiy-storchaka approved these changes

Copy link

Member

serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM. Thank you@sobolevn for your PR.

bedevere-appbot added awaiting core review and removed awaiting review labels

serhiy-storchaka added the needs backport to 3.12only security fixes label

serhiy-storchaka merged commitc49edd7 intopython:main

Copy link

Contributor

miss-islington commentedOct 9, 2023

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

bedevere-appbot removed the awaiting core review label

Copy link

Contributor

miss-islington commentedOct 9, 2023

Sorry,@sobolevn and@serhiy-storchaka, I could not cleanly backport this to3.12 due to a conflict.
Please backport usingcherry_picker on command line.

cherry_picker c49edd7d9c5395a6a6696a4846f56bc8b2b22792 3.12

miss-islington assignedserhiy-storchaka

Copy link

MemberAuthor

sobolevn commentedOct 9, 2023•
edited
Loading

Should I do a manual backport?

Copy link

Member

serhiy-storchaka commentedOct 9, 2023

It would be nice. Otherwise I will do it.

sobolevn added a commit to sobolevn/cpython that referenced this pull request

[3.12]pythongh-110525: Add CAPI tests for set and frozenset objects (p…

90c2b08

…ythonGH-110526).(cherry picked from commitc49edd7)Co-authored-by: Nikita Sobolev <mail@sobolevn.me>

Copy link

bedevere-appbot commentedOct 9, 2023

GH-110547 is a backport of this pull request to the3.12 branch.

bedevere-appbot removed the needs backport to 3.12only security fixes label

serhiy-storchaka pushed a commit that referenced this pull request