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.

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

gh-94808: Cover PyObject_PyBytes case with custom __bytes__ method #96610

Merged
merged 2 commits into from Oct 6, 2022
Merged

gh-94808: Cover PyObject_PyBytes case with custom __bytes__ method #96610

merged 2 commits into from Oct 6, 2022

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Sep 6, 2022 •
edited by bedevere-bot

There were two options:

  1. Add a new _testcapimodule.c function to test PyObject_PyBytes directly. But, there's already pyobject_bytes_from_null which only covers NULL case. So, it is somewhat covered
  2. Add a test for some call that uses PyObject_Bytes, like int_from_bytes_impl. In this case we not only test C-API itself, but also do something useful for the end implementation (in this case int.from_bytes)

So, I went with 2.
Probably it is still possible to add a new _testcapimodule.c function in the next PR.

@bedevere-bot bedevere-bot added awaiting review tests Tests in the Lib/test dir labels Sep 6, 2022
@kumaraditya303 kumaraditya303 added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Sep 6, 2022
JelleZijlstra
JelleZijlstra reviewed Oct 6, 2022
View reviewed changes
Lib/test/test_long.py Outdated
self.assertEqual(int.from_bytes(ValidBytes()), 1)
self.assertRaises(TypeError, int.from_bytes, InvalidBytes)
self.assertRaises(TypeError, int.from_bytes, MissingBytes)
self.assertRaises(TypeError, int.from_bytes, RaisingBytes)
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't raise TypeError:

>>> class RaisingBytes:
...             def __bytes__(self):
...                 1 / 0
... 
>>> int.from_bytes(RaisingBytes())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 3, in __bytes__
ZeroDivisionError: division by zero

And yet CI is green. Am I missing something? I'm compiling your PR locally now to see what's going on.

Copy link
Member

Choose a reason for hiding this comment

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

Ah it's because you don't instantiate the class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, Good catch! Even I missed this.

JelleZijlstra
JelleZijlstra reviewed Oct 6, 2022
View reviewed changes
Lib/test/test_long.py Outdated Show resolved Hide resolved
@JelleZijlstra JelleZijlstra self-assigned this Oct 6, 2022
@sobolevn @JelleZijlstra
Update Lib/test/test_long.py
27d5e5c 
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@JelleZijlstra JelleZijlstra merged commit e39ae6b into python:main Oct 6, 2022
@miss-islington
Copy link
Contributor

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

@bedevere-bot
Copy link

GH-97960 is a backport of this pull request to the 3.11 branch.

@miss-islington
Copy link
Contributor

Sorry, @sobolevn and @JelleZijlstra, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker e39ae6bef2c357a88e232dcab2e4b4c0f367544b 3.10

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 6, 2022
@sobolevn @miss-islington
pythongh-94808: Cover PyObject_PyBytes case with custom __bytes__…
017f52d 
… method (pythonGH-96610)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
(cherry picked from commit e39ae6b)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
@JelleZijlstra
Copy link
Member

@sobolevn do you want to do a manual 3.10 backport?

@sobolevn
Copy link
Member Author

sobolevn commented Oct 6, 2022

Yes, will do!

@JelleZijlstra
Copy link
Member

Thanks!

miss-islington added a commit that referenced this pull request Oct 6, 2022
@miss-islington @sobolevn
gh-94808: Cover PyObject_PyBytes case with custom __bytes__ method (
4aa93af 
GH-96610)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
(cherry picked from commit e39ae6b)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
carljm added a commit to carljm/cpython that referenced this pull request Oct 6, 2022
@carljm
Merge branch 'main' into dictwatch
7938d6f 
* main:
  pythonGH-88050: fix race in closing subprocess pipe in asyncio  (python#97951)
  pythongh-93738: Disallow pre-v3 syntax in the C domain (python#97962)
  pythongh-95986: Fix the example using match keyword (python#95989)
  pythongh-97897: Prevent os.mkfifo and os.mknod segfaults with macOS 13 SDK (pythonGH-97944)
  pythongh-94808: Cover `PyUnicode_Count` in CAPI (python#96929)
  pythongh-94808: Cover `PyObject_PyBytes` case with custom `__bytes__` method (python#96610)
  pythongh-95691: Doc BufferedWriter and BufferedReader (python#95703)
  pythonGH-88968: Add notes about socket ownership transfers (python#97936)
  pythongh-96865: [Enum] fix Flag to use CONFORM boundary (pythonGH-97528)
carljm added a commit to carljm/cpython that referenced this pull request Oct 8, 2022
@carljm
Merge branch 'main' into typewatch
1af2a98 
* main: (53 commits)
  pythongh-94808: Coverage: Test that maximum indentation level is handled (python#95926)
  pythonGH-88050: fix race in closing subprocess pipe in asyncio  (python#97951)
  pythongh-93738: Disallow pre-v3 syntax in the C domain (python#97962)
  pythongh-95986: Fix the example using match keyword (python#95989)
  pythongh-97897: Prevent os.mkfifo and os.mknod segfaults with macOS 13 SDK (pythonGH-97944)
  pythongh-94808: Cover `PyUnicode_Count` in CAPI (python#96929)
  pythongh-94808: Cover `PyObject_PyBytes` case with custom `__bytes__` method (python#96610)
  pythongh-95691: Doc BufferedWriter and BufferedReader (python#95703)
  pythonGH-88968: Add notes about socket ownership transfers (python#97936)
  pythongh-96865: [Enum] fix Flag to use CONFORM boundary (pythonGH-97528)
  pythongh-65961: Raise `DeprecationWarning` when `__package__` differs from `__spec__.parent` (python#97879)
  docs(typing): add "see PEP 675" to LiteralString (python#97926)
  pythongh-97850: Remove all known instances of module_repr() (python#97876)
  I changed my surname early this year (python#96671)
  pythongh-93738: Documentation C syntax (:c:type:<C type> -> :c:expr:<C type>) (python#97768)
  pythongh-91539: improve performance of get_proxies_environment  (python#91566)
  build(deps): bump actions/stale from 5 to 6 (python#97701)
  pythonGH-95172 Make the same version `versionadded` oneline (python#95172)
  pythongh-88050: Fix asyncio subprocess to kill process cleanly when process is blocked (python#32073)
  pythongh-93738: Documentation C syntax (Function glob patterns -> literal markup) (python#97774)
  ...
@sobolevn sobolevn mentioned this pull request Oct 9, 2022
Merged
@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Oct 9, 2022
@bedevere-bot
Copy link

GH-98121 is a backport of this pull request to the 3.10 branch.

sobolevn added a commit to sobolevn/cpython that referenced this pull request Oct 9, 2022
@sobolevn
[3.10] pythongh-94808: Cover PyObject_PyBytes case with custom `__b…
1568968 
…ytes__` method (pythonGH-96610)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>.
(cherry picked from commit e39ae6b)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
sobolevn added a commit to sobolevn/cpython that referenced this pull request Oct 9, 2022
@sobolevn
[3.10] pythongh-94808: Cover PyObject_PyBytes case with custom `__b…
23419ec 
…ytes__` method (pythonGH-96610)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>.
(cherry picked from commit e39ae6b)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
JelleZijlstra pushed a commit that referenced this pull request Oct 9, 2022
@sobolevn
[3.10] gh-94808: Cover PyObject_PyBytes case with custom `__bytes__…
ccb5680 
…` method (GH-96610) (#98121)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>.
Co-authored-by: Nikita Sobolev <mail@sobolevn.me>

(cherry picked from commit e39ae6b)
mpage pushed a commit to mpage/cpython that referenced this pull request Oct 11, 2022
@sobolevn @JelleZijlstra @mpage
pythongh-94808: Cover PyObject_PyBytes case with custom __bytes__…
a857648 
… method (python#96610)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

Assignees

@JelleZijlstra JelleZijlstra

Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@sobolevn @miss-islington @bedevere-bot @JelleZijlstra @kumaraditya303