Skip to content

bpo-30524: Write unit tests for FASTCALL #2022

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 9, 2017
Merged

bpo-30524: Write unit tests for FASTCALL #2022

merged 1 commit into from
Jun 9, 2017

Conversation

@vstinner
Copy link
Member

@vstinner vstinner commented Jun 9, 2017

Test C functions:

  • _PyObject_FastCall()
  • _PyObject_FastCallDict()
  • _PyObject_FastCallKeywords()

@vstinner vstinner requested a review from serhiy-storchaka June 9, 2017 11:23
@vstinner vstinner added the tests Tests in the Lib/test dir label Jun 9, 2017
@vstinner vstinner mentioned this pull request Jun 9, 2017
Merged
serhiy-storchaka
serhiy-storchaka reviewed Jun 9, 2017
View reviewed changes
Modules/_testcapimodule.c Outdated
Copy link
Member

@serhiy-storchaka serhiy-storchaka Jun 9, 2017

Choose a reason for hiding this comment

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

I would remove the test_ prefix (and may be even pyobject_). Functions with the test_ prefix usually do all testing internally, they don't return a value for checking in Python test.

Copy link
Member Author

@vstinner vstinner Jun 9, 2017

Choose a reason for hiding this comment

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

"test_" is a prefix to avoid name conflict with the called function: "pyobject_fastcall" vs "_PyObject_FastCall". It avoids confusion when you have to debug _testcapi.

Modules/_testcapimodule.c Outdated
Copy link
Member

@serhiy-storchaka serhiy-storchaka Jun 9, 2017

Choose a reason for hiding this comment

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

Unfortunately this can't be used for testing the case nargs == 0, stack != NULL. Maybe set stack to NULL only when pass None as args?

Copy link
Member Author

@vstinner vstinner Jun 9, 2017

Choose a reason for hiding this comment

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

As the author of _PyObject_FastCall(), I prefer to test passing NULL since I expect more bugs for stack == NULL, like the _PyStack_UnpackDict() bug.

I don't think that it's worth it to test calling _PyObject_FastCall() with nargs==0 and stack != NULL.

Copy link
Member

@serhiy-storchaka serhiy-storchaka Jun 9, 2017

Choose a reason for hiding this comment

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

Bugs hide in untested corners.

Lib/test/test_call.py Outdated
Copy link
Member

@serhiy-storchaka serhiy-storchaka Jun 9, 2017

Choose a reason for hiding this comment

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

I think expected=expected is redundant. func and args are enough for identifying the test case, and expected is reported if check_result() fails.

Copy link
Member Author

@vstinner vstinner Jun 9, 2017

Choose a reason for hiding this comment

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

Ok, removed.

serhiy-storchaka
serhiy-storchaka reviewed Jun 9, 2017
View reviewed changes
Lib/test/test_call.py Outdated
Copy link
Member

@serhiy-storchaka serhiy-storchaka Jun 9, 2017

Choose a reason for hiding this comment

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

Maybe test also _testcapi.pyobject_fastcalldict(func, args, {})?

Copy link
Member Author

@vstinner vstinner Jun 9, 2017

Choose a reason for hiding this comment

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

Done

Lib/test/test_call.py Outdated
Copy link
Member

@serhiy-storchaka serhiy-storchaka Jun 9, 2017

Choose a reason for hiding this comment

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

This case (and the case for pyfunc_noarg) can be removed if add testing _testcapi.pyobject_fastcalldict(func, args, {}) and _testcapi.pyobject_fastcallkeywords(func, args, {}) for CALLS_POSARGS.

Copy link
Member Author

@vstinner vstinner Jun 9, 2017

Choose a reason for hiding this comment

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

Fixed

Modules/_testcapimodule.c Outdated
Copy link
Member

@serhiy-storchaka serhiy-storchaka Jun 9, 2017

Choose a reason for hiding this comment

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

Bugs hide in untested corners.

@vstinner
bpo-30524: Write unit tests for FASTCALL
ba51ca0 
Test C functions:

* _PyObject_FastCall()
* _PyObject_FastCallDict()
* _PyObject_FastCallKeywords()
@vstinner
Copy link
Member Author

vstinner commented Jun 9, 2017

I rewrote my change to test much more cases. Most (or all?) possible ways to call FastCall functions should now be tested.

serhiy-storchaka
serhiy-storchaka approved these changes Jun 9, 2017
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Would be nice to backport tests at least to 3.6. This could expose 3.6 bugs fixed in 3.7.

@vstinner vstinner merged commit 3b5cf85 into python:master Jun 9, 2017
@vstinner vstinner deleted the test_fastcall branch June 9, 2017 14:48
@vstinner
Copy link
Member Author

vstinner commented Jun 9, 2017

Would be nice to backport tests at least to 3.6.

Sure, I wrote this change for 3.6, to test the datetime.datetime.now() bug :-) But obvious, firstI wrote it for master.

vstinner added a commit that referenced this pull request Jun 9, 2017
@vstinner
bpo-30524: Write unit tests for FASTCALL (#2022) (#2030)
b757745 
Test C functions:

* _PyObject_FastCall()
* _PyObject_FastCallDict()
* _PyObject_FastCallKeywords()
(cherry picked from commit 3b5cf85)
ma8ma added a commit to ma8ma/cpython that referenced this pull request Jun 13, 2017
@ma8ma
Merge commit 'master' into pep539-tss-api
e49207b 
Resolve conflicts:
3b5cf85 bpo-30524: Write unit tests for FASTCALL (python#2022)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

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.

3 participants

@vstinner @serhiy-storchaka @the-knights-who-say-ni