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

Jump to bottom

bpo-25750: add testcase on bad descriptor __get__() #9084

Merged
merged 1 commit into from Oct 19, 2018
Merged

bpo-25750: add testcase on bad descriptor __get__() #9084

merged 1 commit into from Oct 19, 2018

Conversation

jdemeyer
Copy link
Contributor

@jdemeyer jdemeyer commented Sep 6, 2018 •
edited by bedevere-bot

This adds a testcase for bpo-25750 (#6118). This is a separate pull request since the testcase is possibly controversial.

https://bugs.python.org/issue25750

@jdemeyer jdemeyer mentioned this pull request Sep 6, 2018
Merged
@vstinner
Copy link
Member

vstinner commented Sep 7, 2018

Hi @serhiy-storchaka, in PR #6118 I wrote that I wasn't really happy about adding an unit test on such really tiny corner case. What do you think? Is it ok to add such test? The unit test is indirectly restricted to CPython since it requires _testcapi.

@vstinner
Copy link
Member

vstinner commented Sep 7, 2018

@jdemeyer: since I merged your PR #6118, I get that you have now to rebase your PR on top of master to get your fix (to fix the CI).

@jdemeyer
Copy link
Contributor Author

jdemeyer commented Sep 7, 2018

The unit test is indirectly restricted to CPython since it requires _testcapi.

If that's a problem, I could do

try:
    from _testcapi import bad_get
except ImportError:
    def bad_get(self, obj, cls):
        cls()
        return repr(self)

@encukou
Copy link
Member

encukou commented Sep 13, 2018

NEWS entry is already there (from #6118).

Even if it's an edge case, I think the regression test should be merged.

@vstinner vstinner changed the title bpo-25750: add testcase bpo-25750: add testcase on bad descriptor __get__() Sep 13, 2018
@vstinner vstinner requested a review from serhiy-storchaka Sep 13, 2018
@vstinner
Copy link
Member

vstinner commented Sep 13, 2018

@encukou: Would you just mind to approve the change in that case?

@serhiy-storchaka: What do you think?

I don't have an opinion strong enough against this change to block it. @encukou: if you want to, please go ahead :-)

berkerpeksag
berkerpeksag reviewed Sep 13, 2018
View changes
Lib/test/test_descr.py
@@ -4757,6 +4762,22 @@ def __call__(self, arg):
self.assertRegex(repr(method),
r"<bound method qualname of <object object at .*>>")

@unittest.skipIf(_testcapi is None, 'need the _testcapi module')
Copy link
Member

@berkerpeksag berkerpeksag Sep 13, 2018

Choose a reason for hiding this comment

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

We can wrap the test with @cpython_only.

Copy link
Member

@vstinner vstinner Sep 13, 2018

Choose a reason for hiding this comment

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

I wanted to propose the same change, but IMHO @unittest.skipIf(_testcapi is None, ...) is fine since _testcapi is specific to CPython and it's a common pattern.

Copy link
Member

@serhiy-storchaka serhiy-storchaka Sep 14, 2018

Choose a reason for hiding this comment

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

cpython_only serves also documenting purpose. It is common to decorate all tests that use _testcapi with cpython_only.

Copy link
Member

@vstinner vstinner Sep 20, 2018

Choose a reason for hiding this comment

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

@jdemeyer: Would you mind to add @cpython_only? IMHO it's better to add it first, before "_testcapi is None".

Copy link
Contributor Author

@jdemeyer jdemeyer Oct 12, 2018

Choose a reason for hiding this comment

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

Would you mind to add @cpython_only? IMHO it's better to add it first, before "_testcapi is None".

Do we really need both? Doesn't @cpython_only imply that _testcapi is available?

serhiy-storchaka
serhiy-storchaka reviewed Sep 14, 2018
View changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

I don't have opinion about adding this test. Technically it looks correct.

Lib/test/test_descr.py
@@ -4757,6 +4762,22 @@ def __call__(self, arg):
self.assertRegex(repr(method),
r"<bound method qualname of <object object at .*>>")

@unittest.skipIf(_testcapi is None, 'need the _testcapi module')
Copy link
Member

@serhiy-storchaka serhiy-storchaka Sep 14, 2018

Choose a reason for hiding this comment

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

cpython_only serves also documenting purpose. It is common to decorate all tests that use _testcapi with cpython_only.

@vstinner vstinner merged commit 5a30620 into python:master Oct 19, 2018
@vstinner
Copy link
Member

vstinner commented Oct 19, 2018

I was the one who blocked the addition of the test. It seems like everybody likes the test, there was just a minor discussion on the @cpython_only decorator. I dislike seeing this PR stuck for such non-blocker issue, so I decided to merge it.

Thanks @jdemeyer for this test. Sorry for being so pendantic :-)

@vstinner
Copy link
Member

vstinner commented Oct 19, 2018

I propose to not backport this new test to stable Python version. IMHO this test is borderline, I prefer to see how it goes in Python 3.8 first.

@serhiy-storchaka serhiy-storchaka mentioned this pull request Oct 30, 2018
Merged
yahya-abou-imran pushed a commit to yahya-abou-imran/cpython that referenced this pull request Nov 2, 2018
@jdemeyer @yahya-abou-imran
bpo-25750: Add test on bad descriptor __get__() (pythonGH-9084)
8da6cce
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Nov 20, 2018
@serhiy-storchaka
Fix a compiler warning introduced in pythonGH-9084.
9cc5d44
serhiy-storchaka added a commit that referenced this pull request Nov 20, 2018
@serhiy-storchaka
bpo-25750: Fix a compiler warning introduced in GH-9084. (GH-10234)
b1dede3
@jdemeyer jdemeyer mentioned this pull request May 21, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@berkerpeksag berkerpeksag

@vstinner vstinner

@serhiy-storchaka serhiy-storchaka

@encukou encukou

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jdemeyer @vstinner @encukou @berkerpeksag @serhiy-storchaka @the-knights-who-say-ni @bedevere-bot