Elevate warnings to errors in the test suite #6094
Conversation
graingert
force-pushed
the
enable-pytest-strict
branch
from
6367375
to
f9c1515
Compare
| @@ -194,7 +194,7 @@ def close(self, timeout=None): | |||
| return self.sync(self._close, callback_timeout=timeout) | |||
|
|
|||
| def __del__(self): | |||
| if self.status != Status.closed: | |||
| if getattr(self, "status", Status.closed) != Status.closed: | |||
There was a problem hiding this comment.
When would a Cluster instance not have a status attribute?
There was a problem hiding this comment.
attributes aren't guaranteed to exist in def __del__
|
|
||
|
|
||
| @pytest.mark.avoid_ci | ||
| def test_old_ssh_with_local_dir(loop): | ||
| with pytest.warns(Warning): |
There was a problem hiding this comment.
It appears we're just dropping the pytest.warns ctx manager here. Was this test just failing before, but went unnoticed due to the pytest.mark.avoid_ci marker?
There was a problem hiding this comment.
introduced here, it doesn't warn anymore (or at least when I run the test on my machine)!
| @@ -264,12 +265,14 @@ async def test_assert_no_warning_no_overload(c, s, a): | |||
| class Dummy(WorkerPlugin): | |||
| pass | |||
|
|
|||
| with pytest.warns(None): | |||
| with warnings.catch_warnings(record=True) as record: | |||
There was a problem hiding this comment.
Nice, thanks for cleaning this pytest deprecation up
There was a problem hiding this comment.
technically with warnings.catch_warnings(record=True) as record: .... assert not record isn't needed with filterwarnings=error because warnings will throw errors and fail the test. But I left it in in case filterwarnings config is ever removed
| for i in range(10): | ||
| future = c.submit(lambda x, y: x, data, i) | ||
|
|
||
| assert len(record) < 2 |
There was a problem hiding this comment.
IIUC it looks like we're dropping this check
There was a problem hiding this comment.
I've now restored this as assert not record
graingert
force-pushed
the
enable-pytest-strict
branch
from
f9c1515
to
5c20cda
Compare
graingert
force-pushed
the
enable-pytest-strict
branch
from
ffee354
to
c576960
Compare
graingert
force-pushed
the
enable-pytest-strict
branch
4 times, most recently
from
befe6e1
to
d6e7f12
Compare
pytest.warns automatically configures warnings.simplefilter("always")
graingert
force-pushed
the
enable-pytest-strict
branch
from
d6e7f12
to
8f11999
Compare
|
@jrbourbeau do you have time to continue reviewing this? |
jrbourbeau
changed the title
There was a problem hiding this comment.
Thanks @graingert -- the changes here look good to me, thank you for working on this. As I mentioned offline, I'm curious to see how often new warnings pop up (now as test failures). But this certainly has value, so let's give it a try
Also, it's nice to see a fully green build ✅
pre-commit run --all-files