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 coverage for bytesarray_setitem #95802

Merged
merged 1 commit into from
Oct 10, 2022

Conversation

mdboom
Copy link
Contributor

@mdboom mdboom commented Aug 8, 2022

When both are provided, tp_ass_subscript takes precedence over tp_ass_item. Since bytesarray provides both, the existing test_setitem tests for bytesarray were not testing bytesarray_setitem, but bytesarray_ass_subscript. This is mostly fine, since Python code has to jump through some hoops to even call it, but a third-party library using PySequence_SetItem could potentially run into this uncovered case.

@mdboom mdboom added the tests Tests in the Lib/test dir label Aug 11, 2022
@mdboom mdboom closed this Aug 12, 2022
@mdboom mdboom reopened this Aug 12, 2022
@mdboom mdboom force-pushed the coverage-bytesarray-setitem branch from 5f24775 to e717c5c Compare August 12, 2022 18:11
@mdboom mdboom force-pushed the coverage-bytesarray-setitem branch from e717c5c to 1c2a344 Compare October 3, 2022 21:20
Copy link
Member

@JelleZijlstra JelleZijlstra 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. Any reason you didn't mark this for backporting to 3.10?

@mdboom
Copy link
Contributor Author

mdboom commented Oct 10, 2022

Looks good. Any reason you didn't mark this for backporting to 3.10?

No real reason not to -- for all these other coverage improvements linked to #94808, we've just been backporting to 3.11.

@JelleZijlstra JelleZijlstra added the needs backport to 3.10 only security fixes label Oct 10, 2022
@JelleZijlstra JelleZijlstra merged commit dfcdee4 into python:main Oct 10, 2022
@miss-islington
Copy link
Contributor

Thanks @mdboom for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @mdboom and @JelleZijlstra, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker dfcdee4a18ece6f7c4fe1aa5830ee861f8b15b24 3.11

@miss-islington
Copy link
Contributor

Sorry @mdboom and @JelleZijlstra, I had trouble checking out the 3.10 backport branch.
Please backport using cherry_picker on command line.
cherry_picker dfcdee4a18ece6f7c4fe1aa5830ee861f8b15b24 3.10

@JelleZijlstra
Copy link
Member

Thanks! I think it's good to backport new tests to the bugfix branches so we can be more confident in any future bugfixes. But testing on 3.11 is definitely more important.

Would you mind doing the manual backports? Feel free to skip 3.10 if you don't think it's worth it.

mpage pushed a commit to mpage/cpython that referenced this pull request Oct 11, 2022
@mdboom mdboom deleted the coverage-bytesarray-setitem branch December 22, 2022 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants