Skip to content

gh-122102: Improve tests for descriptor-related tools in inspect #122144

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.

Sign up for GitHub

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

Open
zuo wants to merge 14 commits into
base: main
Choose a base branch
from
Open

gh-122102: Improve tests for descriptor-related tools in inspect #122144

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9944ef6
gh-122102: Improve tests of `inspect.is{data,method}descriptor()`
zuo Jul 22, 2024
40ce793
Update Lib/test/test_inspect/test_inspect.py
zuo Jul 23, 2024
9e5c475
Update Lib/test/test_inspect/test_inspect.py
zuo Jul 23, 2024
ca12cc2
Update Lib/test/test_inspect/test_inspect.py
zuo Jul 24, 2024
ebcb0dc
Update Lib/test/test_inspect/test_inspect.py
zuo Jul 25, 2024
9dab208
Remove Misc/NEWS.d/next/Tests/2024-07-22-01-32-00.gh-issue-122102.6sB…
zuo Aug 3, 2024
fc33fd5
Update Lib/test/test_inspect/test_inspect.py
zuo Aug 3, 2024
130d795
Merge branch 'main' into test-inspect-ismethoddescriptor-isdatadescri…
zuo Aug 3, 2024
ec79a04
Include also `functools.partial`-related cases
zuo Aug 3, 2024
8e2ea46
Merge branch 'main' into test-inspect-ismethoddescriptor-isdatadescri…
zuo Aug 5, 2024
bcec47a
Merge branch 'main' into test-inspect-ismethoddescriptor-isdatadescri…
zuo Aug 6, 2024
b249448
Merge branch 'main' into test-inspect-ismethoddescriptor-isdatadescri…
zuo Oct 2, 2024
17928e4
Merge branch 'main' into test-inspect-ismethoddescriptor-isdatadescri…
zuo Jul 18, 2026
288b883
Merge branch 'python:main' into test-inspect-ismethoddescriptor-isdat…
zuo Jul 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Update Lib/test/test_inspect/test_inspect.py 
(further minor tweaks/improvements...)
  • Loading branch information
@zuo
zuo committed Jul 23, 2024
commit 9e5c475e108ff07d23578354f3cd89c6d4656923
30 changes: 15 additions & 15 deletions Lib/test/test_inspect/test_inspect.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1832,10 +1832,10 @@ class MethodDescriptorSub(MethodDescriptor):
MethodDescriptorWithGetNone,
MethodDescriptorSub,
]:
with self.subTest(f"a descriptor {cls.__doc__}"):
with self.subTest(f"a descriptor {cls.__doc__}", cls=cls):
self.assertTrue(inspect.ismethoddescriptor(cls()))

@serhiy-storchaka serhiy-storchaka Jul 24, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Method descriptors and data descriptors are mutually exclusive. For every case where ismethoddescriptor() returns True we can test that isdatadescriptor() returns False and vice verse. Of course, there are objects for which both predicates return False.

@zuo zuo Jul 25, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Generally, both those functions – ismethoddescriptor() and isdatadescriptor() – are tested in similar ways, but separately: each in a dedicated test class (respectively: TestIsMethodDescriptor and TestIsDataDescriptor).

Do you suggest that those test classes should be merged into one class? That seems reasonable from the DRY point of view, and to emphasize the mutual exclusion you pointed to. Yet, on the other hand, I am afraid if that wouldn't result in tests whose essence would be harder to grasp (because of being less focused).

@zuo zuo Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PS Please note that the mutual exclusiveness in question is covered by each of these test classes.

with self.subTest(f"a descriptor class {cls.__doc__} "
f"(not a descriptor itself)"):
with self.subTest(f"a descriptor class (not a descriptor "
f"itself) {cls.__doc__} ", cls=cls):
self.assertFalse(inspect.ismethoddescriptor(cls))

# Custom data descriptors:
Expand Down Expand Up @@ -1908,10 +1908,10 @@ class DataDescriptorSub7(DataDescriptorWithSetOnly,
DataDescriptorSub6,
DataDescriptorSub7,
]:
with self.subTest(f"a descriptor {cls.__doc__}"):
with self.subTest(f"a descriptor {cls.__doc__}", cls=cls):
self.assertFalse(inspect.ismethoddescriptor(cls()))
with self.subTest(f"a descriptor class {cls.__doc__} "
f"(not a descriptor itself)"):
with self.subTest(f"a descriptor class (not a descriptor "
f"itself) {cls.__doc__} ", cls=cls):

@serhiy-storchaka serhiy-storchaka Jul 24, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Try to change assertFalse to assertTrue and look at the errors report. It may be only me, but I think that too long failure messages are difficult to read.

@zuo zuo Jul 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed, they become too verbose.

Now I've carefully prepared more concise messages. I hope you find them more readable.

self.assertFalse(inspect.ismethoddescriptor(cls))

def test_builtin_descriptors(self):
Expand Down Expand Up @@ -2025,10 +2025,10 @@ class MethodDescriptorSub(MethodDescriptor):
MethodDescriptorWithGetNone,
MethodDescriptorSub,
]:
with self.subTest(f"a descriptor {cls.__doc__}"):
with self.subTest(f"a descriptor {cls.__doc__}", cls=cls):
self.assertFalse(inspect.isdatadescriptor(cls()))
with self.subTest(f"a descriptor class {cls.__doc__} "
f"(not a descriptor itself)"):
with self.subTest(f"a descriptor class (not a descriptor "
f"itself) {cls.__doc__} ", cls=cls):
self.assertFalse(inspect.isdatadescriptor(cls))

# Custom data descriptors:
Expand Down Expand Up @@ -2129,10 +2129,10 @@ class DataDescriptorSub15(DataDescriptorWithDeleteOnly,
DataDescriptorSub14,
DataDescriptorSub15,
]:
with self.subTest(f"a descriptor {cls.__doc__}"):
with self.subTest(f"a descriptor {cls.__doc__}", cls=cls):
self.assertTrue(inspect.isdatadescriptor(cls()))
with self.subTest(f"a descriptor class {cls.__doc__} "
f"(not a descriptor itself)"):
with self.subTest(f"a descriptor class (not a descriptor "
f"itself) {cls.__doc__} ", cls=cls):
self.assertFalse(inspect.isdatadescriptor(cls))

def test_builtin_descriptors(self):
Expand All @@ -2155,15 +2155,15 @@ def function():

# Example builtin method descriptors (non-data descriptors):

self.assertFalse(
inspect.isdatadescriptor(builtin_slot_wrapper),
'a builtin slot wrapper is not a data descriptor')
self.assertFalse(
inspect.isdatadescriptor(Owner.__dict__['class_method']),
'a classmethod object is not a data descriptor')
self.assertFalse(
inspect.isdatadescriptor(Owner.__dict__['static_method']),
'a staticmethod object is not a data descriptor')
self.assertFalse(
inspect.isdatadescriptor(builtin_slot_wrapper),
'a builtin slot wrapper is not a data descriptor')

# ...so also `types.MethodType`/`types.FunctionType` instances
# (not only that they are not data descriptors, but also
Expand Down