python: collect: ignore exceptions with isinstance#4284
Conversation
| return | ||
| # nothing was collected elsewhere, let's do it here | ||
| if isclass(obj): | ||
| try: |
There was a problem hiding this comment.
if this was factored into a function called _hardened_isclass the test could be a unittest
There was a problem hiding this comment.
I've factored it out and created a unit test, but kept the integration test, since it not only tests if _safe_isclass is really used during collection, but would also show other issues in the future with Django's settings.
Codecov Report
@@ Coverage Diff @@
## master #4284 +/- ##
==========================================
+ Coverage 95.65% 95.78% +0.13%
==========================================
Files 109 109
Lines 24630 24666 +36
Branches 2396 2397 +1
==========================================
+ Hits 23560 23627 +67
+ Misses 759 738 -21
+ Partials 311 301 -10
Continue to review full report at Codecov.
|
| try: | ||
| return isclass(obj) | ||
| except Exception: | ||
| return False |
There was a problem hiding this comment.
should this live in pytest._compat, that way other uses of isclass get the same benefits?
seems to be also used in these places:
$ git grep compat.*isclass
src/_pytest/fixtures.py:from _pytest.compat import isclass
src/_pytest/python.py:from _pytest.compat import isclass
src/_pytest/python_api.py:from _pytest.compat import isclassThere was a problem hiding this comment.
I've thought it could be moved when needed.
Currently there are several places that handle things safely already - i.e. with the test case for django.conf.settings I've seen it coming to the "raise" at least once before.
Therefore I've thought to keep it near to the single user for now.
There was a problem hiding this comment.
I.e. I do not think we should wrap isclass/isinstance unnecessarily.
There was a problem hiding this comment.
🤷♂️ it's triggerable in raises at least:
import pytest
class CrappyClass(Exception):
@property
def __class__(self):
assert False, 'nuuuu'
with pytest.raises(CrappyClass()):
passTraceback (most recent call last):
File "t.py", line 9, in <module>
with pytest.raises(CrappyClass()):
File "/tmp/pytest/venv/lib/python3.6/site-packages/_pytest/python_api.py", line 654, in raises
for exc in filterfalse(isclass, always_iterable(expected_exception, BASE_TYPE)):
File "/tmp/pytest/venv/lib/python3.6/site-packages/more_itertools/more.py", line 1305, in always_iterable
if (base_type is not None) and isinstance(obj, base_type):
File "t.py", line 6, in __class__
assert False, 'nuuuu'
AssertionError: nuuuu
There was a problem hiding this comment.
Hmm, might be good to see this for raises after all?! Not sure.
It's not as bad as with collection at least.
There was a problem hiding this comment.
I'll move it to compat and use it for your test case, too.
There was a problem hiding this comment.
Well, it fails there through always_iterable already, which does:
if (base_type is not None) and isinstance(obj, base_type):
Fixing this to use for exc in filterfalse(safe_isclass, always_iterable(expected_exception, base_type=None)): (base_type=None) then makes it fail with:
TypeError: exceptions must be old-style classes or derived from BaseException, not <class 'test_compat.test_safe_getclass..CrappyClass'>
(beause it returns False)
Let's stick to "Arbitrary exceptions should not be silenced." here at least.
|
this seemed odd enough so I made a bpo issue for it: https://bugs.python.org/issue35137 |
asottile
left a comment
There was a problem hiding this comment.
either way, this seems good -- thanks for this 🎉
b2042be to
c9adfd1
Compare
|
I've reworked it a bit and added tests for raises. |
c9adfd1 to
e30f709
Compare
|
Amended a fixup: diff --git a/testing/python/raises.py b/testing/python/raises.py
index 926a2d06..a72aeef6 100644
--- a/testing/python/raises.py
+++ b/testing/python/raises.py
@@ -181,9 +181,6 @@ class CrappyClass(Exception):
def __class__(self):
assert False, "via __class__"
- def __call__(self):
- return CrappyClass
-
if six.PY2:
with pytest.raises(pytest.fail.Exception) as excinfo:
with pytest.raises(CrappyClass()): |
Fixes #4266.