Skip to content

gh-91288: Move set.test_c_api to _testcapimodule.test_set #32133

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from

Conversation

arhadthedev
Copy link
Member

@arhadthedev arhadthedev commented Mar 26, 2022

Currently, tests for PySet/PyFrozenSet C API are defined in Objects/setobject.c and available via set.test_c_api().

Moving them to, for example, _testcapi gives the following advantanges:

  • an internal, CPython-specific method stops being available in a public interface of the set class

  • CPython already has tests grouped into dedicated packages

  • the target package undefines NDEBUG thus making asserts available in release builds

  • a user may choose to omit the tests from a build not carrying them around

  • a file of 2.5k lines of codes becomes 152 lines smaller.

Closes gh-91288.

@arhadthedev arhadthedev marked this pull request as ready for review March 27, 2022 06:18
@arhadthedev arhadthedev requested a review from rhettinger as a code owner March 27, 2022 06:18
@arhadthedev
Copy link
Member Author

Tests / Check if generated files are up to date fails with:

./_bootstrap_python ./Programs/_freeze_module.py abc ./Lib/abc.py Python/frozen_modules/abc.h
make[1]: execvp: ./_bootstrap_python: Permission denied
./_bootstrap_python ./Programs/_freeze_module.py codecs ./Lib/codecs.py Python/frozen_modules/codecs.h
make[1]: execvp: ./_bootstrap_python: Permission denied

@arhadthedev arhadthedev reopened this Mar 27, 2022
@rhettinger
Copy link
Contributor

I don't see the point of this. It seem like unnecessary code churn that just moves related pieces of code farther apart. The method is only visible in debug builds which is the entire point of the having the test method.

an internal, CPython-specific method stops being available in a public interface of the set class

If it isn't in a release build and isn't documented as public , then it isn't public.

@arhadthedev
Copy link
Member Author

Thank you for the review! I had a sleep on it and now I agree that the initial rationale gives no benefit being just about aestetics.

However, I believe that the PR is still useful. All core classes except set store their tests separately, in Lib/test/ for the Python API and in Modules/_testcapimodule.c for the C API.

I'm aware that initially it wasn't an option for set because its C test called static set_clear_internal and set_update_internal. However, one year later they got non-static counterparts PySet_Clear and _PySet_Update so the initial restriction no longer applies. Sidenote: the internal ones appeared in 9f1a679 that detached set internals from dict; the public ones appeared in 49fd7fa.

If it isn't in a release build and isn't documented as public, then it isn't public.

Initially I got an impression that everything not starting with an underscore and discoverable via dir() is public and just has no documentation yet.

@arhadthedev
Copy link
Member Author

@rhettinger There is no sense to keep some test cases separately from the rest of the C API tests after their dependency on private functions is eliminated.

@arhadthedev arhadthedev changed the title bpo-47132: Move set.test_c_api to _testcapimodule.test_set gh-91288: Move set.test_c_api to _testcapimodule.test_set Apr 12, 2022
@rhettinger
Copy link
Contributor

Thank for the suggestion but I'm going to decline. AFAICT no real user problem is being solved here. Also, as the maintainer of the code, it is a bit easier for me to have it all in one place.

@arhadthedev
Copy link
Member Author

arhadthedev commented Apr 17, 2022

it is a bit easier for me to have it all in one place.

Actually, I don't object. I agree that unnavigable ~8k lines of _testcapimodule.c is hardly a solution. I've just got an impression that all tests gravitate towards Lib/test so decided to move the piece to its kins without a second thought.

@arhadthedev arhadthedev deleted the _testcapimodule-set branch April 17, 2022 03:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move tests from setobject.c to _testcapimodule
4 participants