bpo-25750: add testcase on bad descriptor __get__() by jdemeyer · Pull Request #9084 · python/cpython · GitHub
Skip to content

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

Merged
vstinner merged 1 commit into
python:masterfrom
jdemeyer:bpo25750test
Oct 19, 2018
Merged

bpo-25750: add testcase on bad descriptor __get__()#9084
vstinner merged 1 commit into
python:masterfrom
jdemeyer:bpo25750test

Conversation

@jdemeyer

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

Copy link
Copy Markdown
Contributor

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

vstinner commented Sep 7, 2018

Copy link
Copy Markdown
Member

@vstinner

vstinner commented Sep 7, 2018

Copy link
Copy Markdown
Member

@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

jdemeyer commented Sep 7, 2018

Copy link
Copy Markdown
Contributor Author

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

encukou commented Sep 13, 2018

Copy link
Copy Markdown
Member

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 commented Sep 13, 2018

Copy link
Copy Markdown
Member

@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 reviewed changes
Comment thread Lib/test/test_descr.py
self.assertRegex(repr(method),
r"<bound method qualname of <object object at .*>>")

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

@berkerpeksag berkerpeksag Sep 13, 2018

Copy link
Copy Markdown
Member

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.

@vstinner vstinner Sep 13, 2018

Copy link
Copy Markdown
Member

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.

@serhiy-storchaka serhiy-storchaka Sep 14, 2018

Copy link
Copy Markdown
Member

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 Sep 20, 2018

Copy link
Copy Markdown
Member

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".

@jdemeyer jdemeyer Oct 12, 2018

Copy link
Copy Markdown
Contributor Author

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 reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment thread Lib/test/test_descr.py
self.assertRegex(repr(method),
r"<bound method qualname of <object object at .*>>")

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

@serhiy-storchaka serhiy-storchaka Sep 14, 2018

Copy link
Copy Markdown
Member

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

vstinner commented Oct 19, 2018

Copy link
Copy Markdown
Member

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

vstinner commented Oct 19, 2018

Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka mentioned this pull request Oct 30, 2018
Merged
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 berkerpeksag left review comments
@vstinner vstinner vstinner left review comments
@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments
@encukou encukou encukou approved these changes

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@jdemeyer @vstinner @encukou @berkerpeksag @serhiy-storchaka @the-knights-who-say-ni @bedevere-bot