GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-45756: do not execute `@property` descrs while creating mock autospecs by sobolevn · Pull Request #29901 · python/cpython

Conversation

@sobolevn

@sobolevn sobolevn commented Dec 3, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@sobolevn
bpo-45756: do not execute @property descrs while creating mock auto…
1c545e5 
…specs
@sobolevn
sobolevn requested a review from cjw296 as a code owner December 3, 2021 08:22
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Dec 3, 2021
@bedevere-bot bedevere-bot added the awaiting review label Dec 3, 2021
@sobolevn

sobolevn commented Dec 11, 2021

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hongweipeng

This comment has been minimized.

Sign in to view
@sobolevn

sobolevn commented Dec 30, 2021

Copy link
Copy Markdown
Member Author

CC @corona10

Maybe you will be interested to review this? 🙂
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kumaraditya303
kumaraditya303 approved these changes Jan 12, 2022
View reviewed changes

@kumaraditya303 kumaraditya303 left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM, Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/NEWS.d/next/Library/2021-12-03-11-19-44.bpo-45756.nSDJWj.rst Outdated
@@ -0,0 +1,2 @@
We no longer execute ``@property`` descriptors while creating autospecs in

@kumaraditya303 kumaraditya303 Jan 12, 2022

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/NEWS.d/next/Library/2021-12-03-11-19-44.bpo-45756.nSDJWj.rst Outdated
@@ -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.

@kumaraditya303 kumaraditya303 Jan 12, 2022

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/unittest/mock.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Jan 12, 2022
sobolevn and others added 3 commits January 12, 2022 11:55
@sobolevn @kumaraditya303
Update Lib/unittest/mock.py
49a5451 
Co-authored-by: Kumar Aditya <[email protected]>
@sobolevn
Update 2021-12-03-11-19-44.bpo-45756.nSDJWj.rst
345c3f4
@sobolevn
Update 2021-12-03-11-19-44.bpo-45756.nSDJWj.rst
78cbfc6
@sobolevn sobolevn closed this Jan 12, 2022
@sobolevn sobolevn reopened this Jan 12, 2022
@hongweipeng hongweipeng mentioned this pull request Feb 15, 2022
bpo-45756: Fix mock triggers dynamic lookup via the descriptor protocol. #31348
Closed
@rhettinger
rhettinger requested a review from voidspace May 10, 2022 17:44
@rhettinger

rhettinger commented May 10, 2022

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kjamieson kjamieson mannequin mentioned this pull request Apr 10, 2022
mock raises exception when using a spec with an attribute that raises exception on access #89917
Closed
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@sobolevn sobolevn closed this Aug 28, 2022
@sobolevn sobolevn mentioned this pull request Oct 16, 2022
unittest.mock.create_autospec() can crash when accessing properties from objects #98301
Closed
@f0k f0k mentioned this pull request Feb 22, 2023
unittest.mock spec calls class properties #85934
Closed
@carljm

carljm commented May 19, 2023

Copy link
Copy Markdown
Member

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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
@cjw296 cjw296 Awaiting requested review from cjw296 cjw296 is a code owner
@voidspace voidspace Awaiting requested review from voidspace

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@sobolevn @hongweipeng @rhettinger @carljm @kumaraditya303 @the-knights-who-say-ni @ezio-melotti @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.