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

bpo-45756: do not execute @property descrs while creating mock autospecs #29901

Closed
wants to merge 4 commits into from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Dec 3, 2021

In this PR I am trying to solve this corner case with inspect.getattr_static, so atrtibutes won't be actually resolved.

I guess @property is special enough to skip it there.
Any other similar cases, that we would need to fix (probably in the upcoming PRs)?

https://bugs.python.org/issue45756

Related: https://bugs.python.org/issue41768

@sobolevn
Copy link
Member Author

I am going to friendly ping @serhiy-storchaka to review this 🙂

@hongweipeng

This comment has been minimized.

@sobolevn
Copy link
Member Author

CC @corona10

Maybe you will be interested to review this? 🙂

Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@@ -0,0 +1,2 @@
We no longer execute ``@property`` descriptors while creating autospecs in
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
We no longer execute ``@property`` descriptors while creating autospecs in
Fixed :class:`property` decorators executing descriptors while creating autospecs in

@@ -0,0 +1,2 @@
We no longer execute ``@property`` descriptors while creating autospecs in
``mock.py``. This was not safe and could affect user's code in unknown way.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
``mock.py``. This was not safe and could affect user's code in unknown way.
:mod:`unittest.mock`. This was not safe and could affect user's code in unknown ways.

Lib/unittest/mock.py Outdated Show resolved Hide resolved
@rhettinger
Copy link
Contributor

Is this potentially a breaking change (can some mocks be relying on running a property)?

In general, it is a good idea to avoid accidentally running code; however, mocks are advanced case where users are already doing something invasive, and properties are also a special case where programmers are intentionally running behavior upon access.

Long ago, In a slightly related issue, it was decided that hasattr() would be left as-is rather than making perhap futile efforts to avoid running a descriptor.

@carljm
Copy link
Member

carljm commented May 19, 2023

@rhettinger

Is this potentially a breaking change (can some mocks be relying on running a property)?

It is possible. However, the execution of properties by specced mocks was introduced relatively recently (3.8) along with AsyncMock, not intentionally but as an accidental side effect of looking for async methods. This was an unintentional breaking change at the time, and it was reported reasonably promptly in #85934. Although we've failed to fix it promptly, it seems to me that it should still be fixed and restored to the long-term established behavior.

The current PR to fix it is #22209.

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.

8 participants