bpo-25750: add testcase on bad descriptor get() by jdemeyer · Pull Request #9084 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation13 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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.
@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).
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)
NEWS entry is already there (from #6118).
Even if it's an edge case, I think the regression test should be merged.
bpo-25750: add testcase bpo-25750: add testcase on bad descriptor __get__()
@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 :-)
| @@ -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') |
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.
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.
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.
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".
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?
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.
| @@ -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') |
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.
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 :-)
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 added a commit to serhiy-storchaka/cpython that referenced this pull request
