gh-96002: Add functional test for Argument Clinic#96178

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:

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; 
        
           }

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 merging1, but let's not spend time doing that while finishing the review trying to land this; it'll be too much distraction.)

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.

bedevere-bot added the awaiting review label Aug 22, 2022

colorfulappl force-pushed the test_clinic_functionality branch from b703757 to e67dc12 Compare August 22, 2022 15:55

erlend-aasland marked this pull request as draft August 22, 2022 18:59

erlend-aasland self-requested a review August 22, 2022 19:15

erlend-aasland mentioned this pull request Aug 22, 2022

gh-64490: Fix bugs in argument clinic varargs processing #32092

Merged

isidentical self-requested a review August 22, 2022 20:26

colorfulappl and others added 5 commits August 23, 2022 09:32

Support functional test for Argument Clinic

f00bb03

📜🤖 Added by blurb_it.

a59e304

Revert "Support functional test for Argument Clinic" …

2bd331e

This reverts commit f00bb03.

Add functional test for Argument Clinic

fac5b77

Add testcases written in C

962434c

colorfulappl force-pushed the test_clinic_functionality branch from e67dc12 to 962434c Compare August 23, 2022 12:37

erlend-aasland requested changes Sep 4, 2022

View reviewed changes

bedevere-bot removed the awaiting review label Sep 4, 2022

bedevere-bot added the awaiting changes label Sep 4, 2022

colorfulappl added 7 commits October 25, 2022 22:56

Rename _testclinicfunctionality.c to _testclinic.c

b93b2ae

Add _testclinic to stdlib IGNORE list

35c5f13

Merge TestClinicFunctionalityC class into TestClinicFunctionality

86703de

Format code in _testclinic.c

e66d60b

Merge test_clinic_functionality.py into test_clinic.py

f1fb377

Simplify testcases

449c8fe

Merge branch 'main' into test_clinic_functionality

1553574

kumaraditya303 self-requested a review October 26, 2022 05:40

Fix object leaking

d568683

erlend-aasland reviewed Nov 14, 2022

View reviewed changes

Comment thread Modules/_testclinic.c Outdated Show resolved Hide resolved

Comment thread Modules/_testclinic.c Outdated Show resolved Hide resolved

colorfulappl added 2 commits November 15, 2022 10:31

Remove unnecessary logic

ce337e8

Merge branch 'main' into test_clinic_functionality …

3314465

# Conflicts: # Modules/Setup.stdlib.in

Merge branch 'main' into test_clinic_functionality …

68277c5

# Conflicts: # Modules/Setup.stdlib.in

erlend-aasland approved these changes Nov 15, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Nov 15, 2022

kumaraditya303 approved these changes Nov 15, 2022

View reviewed changes

Merge branch 'main' into test_clinic_functionality …

da0789f

# Conflicts: # Modules/Setup.stdlib.in

erlend-aasland merged commit c450c8c into python:main Nov 21, 2022

bedevere-bot removed the awaiting merge label Nov 21, 2022

erlend-aasland mentioned this pull request May 1, 2023

Add test for Argument Clinic converters #79759

Closed

erlend-aasland mentioned this pull request May 18, 2023

Clinic test are skipped on Windows #104629

Closed

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

gh-96002: Add functional test for Argument Clinic#96178