Skip to content

Make sure all warnings in pytest get turned into errors - #2838

Merged
YannickJadoul merged 4 commits into
pybind:masterfrom
YannickJadoul:pytest-warnings
Feb 1, 2021
Merged

Make sure all warnings in pytest get turned into errors#2838
YannickJadoul merged 4 commits into
pybind:masterfrom
YannickJadoul:pytest-warnings

Conversation

@YannickJadoul

@YannickJadoulYannickJadoul commented Jan 31, 2021

Copy link
Copy Markdown
Collaborator

Description

Follow-up on #2837.

There's a weird thing going on where filterwarnings=error starts ignoring the DeprecationWarning from https://github.com/python/cpython/blob/3c8d6934436e20163be802f5239c5b4e4925eeec/Objects/longobject.c#L226.

I have no clue why. Apart from that, the -Wa flag seemed to take precedence, so no warning was actually an error.

Suggested changelog entry:

None, just some tests.

@bstaleticbstaletic left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reasons for CI being red:

  • conftest.py:19:RuntimeWarning: a __del__ method added to an existing type will not be called on pypy3
  • test_int_convert did not warn on 3.10-dev

Comment threadtests/pytest.ini
# make warnings into errors but ignore certain third-party extension issues
error
# somehow, some DeprecationWarnings do not get turned into errors
always::DeprecationWarning

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why, oh why, does python try so hard to hide deprecation warnings?!

@YannickJadoulYannickJadoulJan 31, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

I have absolutely no clue. It gets funnier, btw: I added warnings.warn("abc", DeprecationWarning) in a test and that did get turned into an error!

But I didn't feel like wasting even more time, and digging further than https://github.com/python/cpython/blob/3c8d6934436e20163be802f5239c5b4e4925eeec/Objects/longobject.c#L226

@henryiiihenryiiiJan 31, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

FYI, FutureWarning is (as clarified by PEP 565) the "user facing" deprecation warning.

@YannickJadoul

Copy link
Copy Markdown
CollaboratorAuthor
  • test_int_convert did not warn on 3.10-dev

Because

https://github.com/python/cpython/blob/3c8d6934436e20163be802f5239c5b4e4925eeec/Objects/longobject.c#L514

got changed into

https://github.com/python/cpython/blob/a1e9a1e120a11c563e166c15721169184c802f8b/Objects/longobject.c#L390

So the deprecation period ended, but ... our tests don't fail because we still fall back to PyNumber_Long anyway? Reason the more we need to not loose track of #2802, it seems.

Comment threadtests/test_class.cpp
static SamePointer samePointer;
py::class_<SamePointer, std::unique_ptr<SamePointer, py::nodelete>>(m, "SamePointer")
.def(py::init([]() { return &samePointer; }))
.def("__del__", [](SamePointer&) { py::print("__del__ called"); });

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

If this looks like a "sweep it under the carpet" kind of thing, you wouldn't be wrong. But we were never really using the fact that __del__ prints something anyway (possibly because it already didn't work on PyPy?).

@YannickJadoul

Copy link
Copy Markdown
CollaboratorAuthor

Just our own tests. This should be safe enough to merge :-) Thanks, @henryiii & @bstaletic!

@YannickJadoul
YannickJadoul merged commit fe84587 into pybind:masterFeb 1, 2021
@github-actionsgithub-actionsBot added the needs changelog Possibly needs a changelog entry label Feb 1, 2021
@YannickJadoul
YannickJadoul deleted the pytest-warnings branch February 1, 2021 14:16
@henryiiihenryiii removed the needs changelog Possibly needs a changelog entry label Jul 12, 2021
@rwgkrwgk mentioned this pull request Feb 10, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@YannickJadoul@henryiii@bstaletic