Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

colorfulappl
Copy link
Contributor

@colorfulapplcolorfulappl commentedAug 22, 2022•
edited by bedevere-bot
Loading

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

@bedevere-bot
Copy link

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

@bedevere-bot
Copy link

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

@colorfulapplcolorfulapplforce-pushed thetest_clinic_functionality branch fromb703757 toe67dc12CompareAugust 22, 2022 15:55
@erlend-aaslanderlend-aasland marked this pull request as draftAugust 22, 2022 18:59
@erlend-aasland
Copy link
Contributor

Thanks for taking this on!

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

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

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

  1. Tests that are set up in C and tested in Python, for exampletest_instancemethod:

classInstanceMethod:
id=_testcapi.instancemethod(id)
testfunction=_testcapi.instancemethod(testfunction)
classCAPITest(unittest.TestCase):
deftest_instancemethod(self):
inst=InstanceMethod()
self.assertEqual(id(inst),inst.id())
self.assertTrue(inst.testfunction()isinst)
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")

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

You'll also need to modifyconfigure.ac,Makefile.pre.in, andModules/Setup.stdlib for the new module; you cangrep -i for_testcapi in those files to see what needs to be done.

isidentical and colorfulappl reacted with thumbs up emoji

@colorfulapplcolorfulapplforce-pushed thetest_clinic_functionality branch frome67dc12 to962434cCompareAugust 23, 2022 12:37
@colorfulappl
Copy link
ContributorAuthor

You'll also need to modifyconfigure.ac,Makefile.pre.in, andModules/Setup.stdlib for the new module; you cangrep -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.

Copy link
Contributor

@erlend-aaslanderlend-aasland left a 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 newLib/test/test_clinic_functionality.py file; we could probably just expandLib/test_clinic.py. Alternatively, create a file-system namespaceLib/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)

colorfulappl reacted with thumbs up emoji
@bedevere-bot
Copy link

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 phraseI 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
Copy link
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.

@colorfulappl
Copy link
ContributorAuthor

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 emojierlend-aasland reacted with rocket emoji

@kumaraditya303kumaraditya303 self-requested a reviewOctober 26, 2022 05:40
@colorfulappl
Copy link
ContributorAuthor

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

Copy link
Contributor

@erlend-aaslanderlend-aasland left a comment

Choose a reason for hiding this comment

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

NowRETURN_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↩

@erlend-aasland
Copy link
Contributor

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

@isidentical, do you want to have a look?

@erlend-aasland
Copy link
Contributor

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

# Conflicts:#Modules/Setup.stdlib.in
@colorfulappl
Copy link
ContributorAuthor

Sure. It's done. :)

erlend-aasland reacted with hooray emoji

@erlend-aasland
Copy link
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

Copy link
Contributor

@kumaraditya303kumaraditya303 left a comment

Choose a reason for hiding this comment

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

LGTM

@erlend-aasland
Copy link
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

@colorfulappl
Copy link
ContributorAuthor

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

# Conflicts:#Modules/Setup.stdlib.in
@erlend-aaslanderlend-aasland merged commitc450c8c intopython:mainNov 21, 2022
@erlend-aasland
Copy link
Contributor

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

colorfulappl reacted with thumbs up emoji

@erlend-aasland
Copy link
Contributor

erlend-aasland commentedNov 30, 2022•
edited
Loading

@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
Copy link
Contributor

I think we should backport this to 3.11 and 3.10.

SGTM

erlend-aasland reacted with thumbs up emoji

colorfulappl added a commit to colorfulappl/cpython that referenced this pull requestDec 14, 2022
(cherry picked from commitc450c8c)Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@bedevere-bot
Copy link

GH-100230 is a backport of this pull request to the3.11 branch.

colorfulappl added a commit to colorfulappl/cpython that referenced this pull requestDec 14, 2022
(cherry picked from commitc450c8c)Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@bedevere-bot
Copy link

GH-100232 is a backport of this pull request to the3.10 branch.

kumaraditya303 added a commit that referenced this pull requestDec 17, 2022
…100230)(cherry picked from commitc450c8c)Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
kumaraditya303 added a commit that referenced this pull requestDec 17, 2022
…100232)(cherry picked from commitc450c8c)Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@erlend-aaslanderlend-aaslanderlend-aasland approved these changes

@kumaraditya303kumaraditya303kumaraditya303 approved these changes

@isidenticalisidenticalAwaiting requested review from isidentical

Assignees
No one assigned
Labels
testsTests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@colorfulappl@bedevere-bot@erlend-aasland@kumaraditya303
[8]ページ先頭
©2009-2025 Movatter.jp