GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-96002: Add functional test for Argument Clinic by colorfulappl · Pull Request #96178 · python/cpython

Conversation

@colorfulappl

@colorfulappl colorfulappl commented Aug 22, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot

bedevere-bot commented Aug 22, 2022

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting review label Aug 22, 2022
@bedevere-bot

bedevere-bot commented Aug 22, 2022

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colorfulappl
colorfulappl force-pushed the test_clinic_functionality branch from b703757 to e67dc12 Compare August 22, 2022 15:55
@erlend-aasland
erlend-aasland marked this pull request as draft August 22, 2022 18:59
@erlend-aasland

erlend-aasland commented Aug 22, 2022

Copy link
Copy Markdown
Contributor

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

  1. 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.
isidentical and colorfulappl reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland
erlend-aasland self-requested a review August 22, 2022 19:15
@erlend-aasland erlend-aasland mentioned this pull request Aug 22, 2022
gh-64490: Fix bugs in argument clinic varargs processing #32092
Merged
@isidentical
isidentical self-requested a review August 22, 2022 20:26
colorfulappl and others added 5 commits August 23, 2022 09:32
@colorfulappl
Support functional test for Argument Clinic
f00bb03
@blurb-it @colorfulappl
📜🤖 Added by blurb_it.
a59e304
@colorfulappl
Revert "Support functional test for Argument Clinic"
2bd331e 
This reverts commit f00bb03.
@colorfulappl
Add functional test for Argument Clinic
fac5b77
@colorfulappl
Add testcases written in C
962434c
@colorfulappl
colorfulappl force-pushed the test_clinic_functionality branch from e67dc12 to 962434c Compare August 23, 2022 12:37
@colorfulappl

colorfulappl commented Aug 23, 2022

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland
erlend-aasland requested changes Sep 4, 2022
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colorfulappl reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Python/stdlib_module_names.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_testclinicfunctionality.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_testclinicfunctionality.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_testclinicfunctionality.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_clinic_functionality.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting review label Sep 4, 2022
@bedevere-bot

bedevere-bot commented Sep 4, 2022

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label Sep 4, 2022
@erlend-aasland

erlend-aasland commented Oct 25, 2022

Copy link
Copy Markdown
Contributor

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colorfulappl added 7 commits October 25, 2022 22:56
@colorfulappl
Rename _testclinicfunctionality.c to _testclinic.c
b93b2ae
@colorfulappl
Add _testclinic to stdlib IGNORE list
35c5f13
@colorfulappl
Merge TestClinicFunctionalityC class into TestClinicFunctionality
86703de
@colorfulappl
Format code in _testclinic.c
e66d60b
@colorfulappl
Merge test_clinic_functionality.py into test_clinic.py
f1fb377
@colorfulappl
Simplify testcases
449c8fe
@colorfulappl
Merge branch 'main' into test_clinic_functionality
1553574
@colorfulappl

colorfulappl commented Oct 26, 2022

Copy link
Copy Markdown
Contributor Author

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.
erlend-aasland reacted with heart emoji erlend-aasland reacted with rocket emoji
All reactions
  • ❤️ 1 reaction
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kumaraditya303
kumaraditya303 self-requested a review October 26, 2022 05:40
@colorfulappl
Fix object leaking
d568683
@colorfulappl

colorfulappl commented Nov 14, 2022

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland
erlend-aasland reviewed Nov 14, 2022
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.)

Footnotes

  1. fix indentation and align line breakers neatly ↩

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_testclinic.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_testclinic.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Nov 14, 2022

Copy link
Copy Markdown
Contributor

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

@isidentical, do you want to have a look?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colorfulappl added 2 commits November 15, 2022 10:31
@colorfulappl
Remove unnecessary logic
ce337e8
@colorfulappl
Merge branch 'main' into test_clinic_functionality
3314465 
# Conflicts:
#	Modules/Setup.stdlib.in
@erlend-aasland

erlend-aasland commented Nov 15, 2022

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colorfulappl
Merge branch 'main' into test_clinic_functionality
68277c5 
# Conflicts:
#	Modules/Setup.stdlib.in
@colorfulappl

colorfulappl commented Nov 15, 2022

Copy link
Copy Markdown
Contributor Author

Sure. It's done. :)
erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland
erlend-aasland approved these changes Nov 15, 2022
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting changes labels Nov 15, 2022
@erlend-aasland

erlend-aasland commented Nov 15, 2022

Copy link
Copy Markdown
Contributor

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.
colorfulappl and kumaraditya303 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kumaraditya303
kumaraditya303 approved these changes Nov 15, 2022
View reviewed changes

@kumaraditya303 kumaraditya303 left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@erlend-aasland

erlend-aasland commented Nov 15, 2022

Copy link
Copy Markdown
Contributor

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 reacted with rocket emoji
All reactions
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colorfulappl

colorfulappl commented Nov 15, 2022

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colorfulappl
Merge branch 'main' into test_clinic_functionality
da0789f 
# Conflicts:
#	Modules/Setup.stdlib.in
@erlend-aasland
erlend-aasland merged commit c450c8c into python:main Nov 21, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Nov 21, 2022
@erlend-aasland

erlend-aasland commented Nov 21, 2022

Copy link
Copy Markdown
Contributor

@colorfulappl: please go ahead with the various Argument Clinic bugfixes and their accompanying tests :)
colorfulappl reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Nov 30, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kumaraditya303

kumaraditya303 commented Nov 30, 2022

Copy link
Copy Markdown
Contributor

I think we should backport this to 3.11 and 3.10.

SGTM
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colorfulappl added a commit to colorfulappl/cpython that referenced this pull request Dec 14, 2022
@colorfulappl @kumaraditya303 @erlend-aasland
pythongh-96002: Add functional test for Argument Clinic (python#96178)
f1f15f0 
(cherry picked from commit c450c8c)

Co-authored-by: Kumar Aditya <[email protected]>
Co-authored-by: Erlend E. Aasland <[email protected]>
@bedevere-bot

bedevere-bot commented Dec 14, 2022

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colorfulappl added a commit to colorfulappl/cpython that referenced this pull request Dec 14, 2022
@colorfulappl @kumaraditya303 @erlend-aasland
pythongh-96002: Add functional test for Argument Clinic (python#96178)
6424170 
(cherry picked from commit c450c8c)

Co-authored-by: Kumar Aditya <[email protected]>
Co-authored-by: Erlend E. Aasland <[email protected]>
@bedevere-bot

bedevere-bot commented Dec 14, 2022

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kumaraditya303 added a commit that referenced this pull request Dec 17, 2022
@colorfulappl @kumaraditya303 @erlend-aasland
[3.11] gh-96002: Add functional test for Argument Clinic (GH-96178) (#…
dd323af 
…100230)

(cherry picked from commit c450c8c)

Co-authored-by: Kumar Aditya <[email protected]>
Co-authored-by: Erlend E. Aasland <[email protected]>
kumaraditya303 added a commit that referenced this pull request Dec 17, 2022
@colorfulappl @kumaraditya303 @erlend-aasland
[3.10] gh-96002: Add functional test for Argument Clinic (GH-96178) (#…
3144aca 
…100232)

(cherry picked from commit c450c8c)

Co-authored-by: Kumar Aditya <[email protected]>
Co-authored-by: Erlend E. Aasland <[email protected]>
@erlend-aasland erlend-aasland mentioned this pull request May 1, 2023
Add test for Argument Clinic converters #79759
Closed
@erlend-aasland erlend-aasland mentioned this pull request May 18, 2023
Clinic test are skipped on Windows #104629
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erlend-aasland erlend-aasland erlend-aasland approved these changes
@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes
@isidentical isidentical Awaiting requested review from isidentical

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@colorfulappl @bedevere-bot @erlend-aasland @kumaraditya303
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.