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-103272: add regression test for getattr that raises #103336

Merged
merged 6 commits into from
Apr 7, 2023

Conversation

sunmy2019
Copy link
Member

Add test case taken from gh-103272 for #103332.

@bedevere-bot bedevere-bot added the tests Tests in the Lib/test dir label Apr 7, 2023
@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

Copy link
Contributor

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Awesome work, thanks for narrowing this down!

I think we can make the test case slightly simpler to make it clearer what's getting tested:

class A:
    def __getattr__(self, name):
        raise ValueError
    @property
    def foo(self):
        return self.__getattr__("asdf")

A().foo

And maybe rename the test to test_getattr_raises or something

@hauntsaninja hauntsaninja changed the title add test case for gh-103272 gh-103272: add regression test Apr 7, 2023
@hauntsaninja hauntsaninja changed the title gh-103272: add regression test gh-103272: add regression test for getattr that raises Apr 7, 2023
@sobolevn
Copy link
Member

sobolevn commented Apr 7, 2023

I've got the revert merged, now this test must pass :)

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thank you!

Lib/test/test_descr.py Outdated Show resolved Hide resolved
Lib/test/test_descr.py Outdated Show resolved Hide resolved
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Congrats on your first CPython PR 🎉

Copy link
Contributor

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks again, this is great!

@hauntsaninja hauntsaninja added the needs backport to 3.11 only security fixes label Apr 7, 2023
@hauntsaninja hauntsaninja merged commit 5d7d86f into python:main Apr 7, 2023
@miss-islington
Copy link
Contributor

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 7, 2023
@bedevere-bot
Copy link

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

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants