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

[zuo]

Improve (and refactor) the tests of ismethoddescriptor() (test.test_inspect.test_inspect.TestIsMethodDescriptor) and isdatadescriptor() (test.test_inspect.test_inspect.TestIsDataDescriptor), making them more comprehensive and consistent.

• Issue: Need for a few corrections/updates regarding the inspect.ismethoddescriptor()'s and inspect.isdatadescriptor()'s docs and unit tests #122102

[picnixz]

I don't think we need a News entry but I may be wrong. Let's leave it like this for now.

[zuo]

Should I remove this file?

[picnixz]

(I don't know whether the thumb up by Serhiy was to tell me that the first sentence was correct or if it was for the second sentence...).

@serhiy-storchaka Should we keep it or remove it? (I'm in favor of removing it because I don't think people are very interested in that and it's only about coverage).

[zuo]

OK, I remove it then.

[zuo]

@serhiy-storchaka Then probably the skip news label will be needed, won't it?

[picnixz]

I'd prefer havig those descriptions at the test level rather at the docstring level.

[zuo]

Hm. I find attaching them directly to the classes they describe most direct and readable.

However, if you insist they should be listed in the for-loop's source collection, I'll move them.

Do you? :)

[picnixz]

Well, it's generally easier when you read the code so I'll leave that decision to the one merging your PR (I don't have those rights). For now, you can keep them as is I think.

[serhiy-storchaka]

If they are not used in other tests, and there is no other reason for them to be global, I think that it is easy to read if all related classes are defined in close proximity to the testing code.

[zuo]

So, should those classes and/or their short descriptions (currently being their docstrings) stay as-is, or be moved/reorganized somehow?

[picnixz]

I personally don't like the descriptions at the docstring level because you need to go back and forth with the class whenever you see the actual test. Alternatively, you can just rename the class to include whatever is being done, namely:

• MethodDescriptor -> MethodDescriptorWith_Get_NoSet_NoDelete.
• MethodDescriptorWithGetNone -> MethodDescriptor_GetIsNone_NoSet_NoDelete.

Or, as I said before, do something like:

class MethodDescriptor:
	def __get__(self, *_): pass

for cls, desc in [
	(MethodDescriptor, "__get__ and no __set__/__delete__"),
    ...
]:
	with self.subTest(f'ismethoddescriptor: {desc}', cls=cls):
		...

I see that you also use some classes twice but the tests have a different check (one is on cls() and the other is on cls. Since you don't include the docstring in the second test, you don't need to repeat "__get__ and no __set__/__delete__" the second time as well.

I think it's clearer that way, because when you read the test, you likely forget about the docstrings after you've read them, whereas when you read the list of classes being tested, it's easy to see which description correponds to which class (in addition, with a meaningful name, it's easier to remember when you use them twice).

One thing that I would say: try to keep the diff small (so if you can reduce the number of changes, it's better) and try to be consistent with what has been done (the tests use explicit assertion messages, and not docstrings, so I think there was a reason to).

[zuo]

I have not hard feelings about this matter... @serhiy-storchaka What do you think?

[picnixz]

Except the docstrings on the classes, I think it's fine.

@serhiy-storchaka Do you want to take a look?

[picnixz]

I still think those should be closer to the test they are used in.

[zuo]

That is the trade-off: to make, within this test method, all definitions/assignments of the descriptors used in the test be grouped in one place (and then only use them in consecutive assertions), or to place each of those definitions as close to the assertions they are used in as possible.

I prefer the former variant, but if you insist on the latter one, I'll move the definitions. One problem I see is that it is hard to do that consistently without splitting some of the definitions (in particular, moving a_property from Owner to a separate class...).; and if I do split them and do all necessary movements, then things may become unnecessarily "fragmented" (and, therefore, harder to read/comprehend).

[picnixz]

I don't understand. attribute_slot is a variable, so you can move that variable where it's being used (line 1976), same for frame_locals. Don't move existing code, only move newest code.

[zuo]

The problem with consistency, as perceived by me, is that if I move attribute_slot and frame_locals, I should also move e.g. a_lambda, and probably also the local def/class statements... But I find it a minor one, so I have no problem with adjusting the code according to your demand.

[zuo]

Hm... I propose, however, to get rid of the entire a_lambda variable – it's redundant, as the code without it does not seem less readable (rather quite contrary, actually...).

[picnixz]

You are improving the tests. While consistency is important, readability is usually more for the tests. You declare a variable that is only used once and quite far from it. It's better to 1) not declare the variable 2) or declare it and use it just after. In general, we don't touch old code if there is no need to (e.g., there is no issue with it) but we can improve new code. I'm not sure I'll have time to review your changes today and won't be there for the next 10 days.

[zuo]

@picnixz For the record, according to your suggestion, the assignments to attribute_slot and frame_locals as well as (for consistency) some other stuff – have been moved, so that now they are as close as possible to the places when they are actually used.

[serhiy-storchaka]

If they are not used in other tests, and there is no other reason for them to be global, I think that it is easy to read if all related classes are defined in close proximity to the testing code.

[serhiy-storchaka]

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]

Indeed, they become too verbose.

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

[serhiy-storchaka]

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]

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]

@serhiy-storchaka @picnixz
The latest commit adjusts the stuff to the changes from gh-122087 (namely, from the PR gh-122219 – i.e., the Python-3.14-dedicated part of the gh-122087's changes).