Skip to content
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

gh-94808: add tests covering PySequence_[InPlace_]Repeat #99196

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Nov 7, 2022

Things I've skipped:

  1. NULL checks for PyLong allocation and NULL check for self. Cannot be reproduced from python
  2. Different values and corner cases for sq_repeat and sq_inplace_repeat, I expect them to be covered in * and *= operators

Coverage view before:
Снимок экрана 2022-11-07 в 14 56 38

Снимок экрана 2022-11-07 в 14 57 07

@sobolevn
Copy link
Member Author

Merge conflicts are resolved, @serhiy-storchaka would you mind taking a look? :)

Copy link
Member

@skirpichev skirpichev left a comment

Choose a reason for hiding this comment

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

Looks good, except for few pep7 nitpicks.

On another hand, I'm not sure if this is a right approach to test abstract interfaces. Another example is PyNumber_* API (#111996). Maybe we should use special, dedicated just for tests types, that have required magic methods, instead of using core types?

sequence_inplace_repeat(PyObject* self, PyObject *args)
{
PyObject *sequence;
Py_ssize_t count;
Copy link
Member

Choose a reason for hiding this comment

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

PEP 7: blank line after local variable declarations;)

sequence_repeat(PyObject* self, PyObject *args)
{
PyObject *sequence;
Py_ssize_t count;
Copy link
Member

Choose a reason for hiding this comment

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

And here.

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.

3 participants