gh-96002: Add functional test for Argument Clinic

[colorfulappl]

A draft implementation of Argument Clinic functional test.

The test do:

• Add a module with AC-declared functions, each function corresponds to one type of function signatures
• Setup a venv, build the module and install it
• Call the functions in the module to check if all the arguments are processed correctly (gathered to a tuple and returned)

Currently it's a draft and only two PoC of #32092 is added, so the test crashes.

cc. @erlend-aasland @kumaraditya303

• Issue: Functional tests for argument clinic #96002

[bedevere-bot]

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

[bedevere-bot]

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

[erlend-aasland]

Thanks for taking this on!

I suggest you take a look at how the test_capi module is laid out; the C code lives in the Modules/ dir, and the test code lives in Lib/test/test_capi.py. The tests there can be split in two patterns:

1. Tests that are fully implemented in C, for example test_from_spec_metatype_inheritance:

cpython/Modules/_testcapi/heaptype.c

Lines 54 to 106 in 129998b

	test_from_spec_metatype_inheritance(PyObject *self, PyObject *Py_UNUSED(ignored))
	{
	PyObject *metaclass = NULL;
	PyObject *class = NULL;
	PyObject *new = NULL;
	PyObject *subclasses = NULL;
	PyObject *result = NULL;
	int r;
	metaclass = PyType_FromSpecWithBases(&MinimalMetaclass_spec, (PyObject*)&PyType_Type);
	if (metaclass == NULL) {
	goto finally;
	}
	class = PyObject_CallFunction(metaclass, "s(){}", "TestClass");
	if (class == NULL) {
	goto finally;
	}
	MinimalType_spec.basicsize = (int)(((PyTypeObject*)class)->tp_basicsize);
	new = PyType_FromSpecWithBases(&MinimalType_spec, class);
	if (new == NULL) {
	goto finally;
	}
	if (Py_TYPE(new) != (PyTypeObject*)metaclass) {
	PyErr_SetString(PyExc_AssertionError,
	"Metaclass not set properly!");
	goto finally;
	}
	/* Assert that __subclasses__ is updated */
	subclasses = PyObject_CallMethod(class, "__subclasses__", "");
	if (!subclasses) {
	goto finally;
	}
	r = PySequence_Contains(subclasses, new);
	if (r < 0) {
	goto finally;
	}
	if (r == 0) {
	PyErr_SetString(PyExc_AssertionError,
	"subclasses not set properly!");
	goto finally;
	}
	result = Py_NewRef(Py_None);
	finally:
	Py_XDECREF(metaclass);
	Py_XDECREF(class);
	Py_XDECREF(new);
	Py_XDECREF(subclasses);
	return result;
	}

2. Tests that are set up in C and tested in Python, for example test_instancemethod:

cpython/Lib/test/test_capi.py

Lines 50 to 65 in 129998b

	class InstanceMethod:
	id = _testcapi.instancemethod(id)
	testfunction = _testcapi.instancemethod(testfunction)
	class CAPITest(unittest.TestCase):
	def test_instancemethod(self):
	inst = InstanceMethod()
	self.assertEqual(id(inst), inst.id())
	self.assertTrue(inst.testfunction() is inst)
	self.assertEqual(inst.testfunction.__doc__, testfunction.__doc__)
	self.assertEqual(InstanceMethod.testfunction.__doc__, testfunction.__doc__)
	InstanceMethod.testfunction.attribute = "test"
	self.assertEqual(testfunction.attribute, "test")
	self.assertRaises(AttributeError, setattr, inst.testfunction, "attribute", "test")

cpython/Modules/_testcapimodule.c

Lines 6518 to 6519 in 129998b

	Py_INCREF(&PyInstanceMethod_Type);
	PyModule_AddObject(m, "instancemethod", (PyObject *)&PyInstanceMethod_Type);

You'll also need to modify configure.ac, Makefile.pre.in, and Modules/Setup.stdlib for the new module; you can grep -i for _testcapi in those files to see what needs to be done.

[colorfulappl]

You'll also need to modify configure.ac, Makefile.pre.in, and Modules/Setup.stdlib for the new module; you can grep -i for _testcapi in those files to see what needs to be done.

Thanks for your advise.
I have rewritten this commit and now it is able to run testcases written both in C or Python.
Could you take a look again?

Once the framework is ready, I will add more case into it.

[erlend-aasland]

This is nice. I don't think there's need for a new Lib/test/test_clinic_functionality.py file; we could probably just expand Lib/test_clinic.py. Alternatively, create a file-system namespace Lib/test_clinic/, but I don't know if that's worth it (and we could always do such a refactor afterwards).

(I'm not sure I like the _testclinicfunctionality.c name, but I have no good alternative either; perhaps just _testclinic.c)

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[erlend-aasland]

@colorfulappl, are you planning to pursue this PR? If not, would you mind me taking over and driving it forward. I'd really like to have this merged; it is good work.

[colorfulappl]

Sorry, I have been busy with other stuff these days so this PR is delayed. :(

I am planning to finish this in this week. Here I made some changes at your suggestions and am going to add more test cases.

[colorfulappl]

Now RETURN_PACKED_ARGS macro accepts a wrapper function and invoke it inside a for loop.
This indeed handles the arguments one-by-one, though not in an elegant manner.

[erlend-aasland]

Now RETURN_PACKED_ARGS macro accepts a wrapper function and invoke it inside a for loop.
This indeed handles the arguments one-by-one, though not in an elegant manner.

IMO, it matters more that it is readable and that it is correct, rather than how elegant it is.

I added a few more comments.

(Just a heads-up regarding the macro: I'll reformat it before merging¹, but let's not spend time doing that while finishing the review trying to land this; it'll be too much distraction.)

Footnotes

1. fix indentation and align line breakers neatly ↩

[erlend-aasland]

Also, I'll wait for @kumaraditya303's thumbs up before landing this.

@isidentical, do you want to have a look?

[erlend-aasland]

BTW, @colorfulappl, please resolve the conflicts in Modules/Setup.stdlib.in (it's because of my recent PRs for the _testcapi module).

[colorfulappl]

Sure. It's done. :)

[erlend-aasland]

Note to self: Like with _testcapi, we should consider adding a short README (or C comment) regarding how to add new tests. We can do this in a follow-up PR.

[kumaraditya303]

LGTM

[erlend-aasland]

I'm going to give Batuhan a chance to chime in before merging. If I haven't heard anything until Friday, I'll land this. Thanks for doing this, and thanks for your patience with our nitpicking, @colorfulappl :)

[colorfulappl]

@erlend-aasland @kumaraditya303 Thank you very much for your patient guidance! 🎉

[erlend-aasland]

@colorfulappl: please go ahead with the various Argument Clinic bugfixes and their accompanying tests :)

[erlend-aasland]

@kumaraditya303 I think we should backport this to 3.11 and 3.10. CC. @pablogsal as RM.

There are multiple clinic bugfixes coming up; IMO we should backport those bugfixes. Backporting this will benefit such bugfix backports.

[kumaraditya303]

I think we should backport this to 3.11 and 3.10.

SGTM

[bedevere-bot]

GH-100230 is a backport of this pull request to the 3.11 branch.

[bedevere-bot]

GH-100232 is a backport of this pull request to the 3.10 branch.