Fail pytest runs on warnings #7809
Conversation
jacobtylerwalls
added
Maintenance
jacobtylerwalls
force-pushed
the
fail-on-warnings
branch
from
862de2a
to
3a9d011
Compare
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
|
I have to look at this more closely this evening. Currently I don't see why this change should work as intended. |
| @@ -89,6 +89,7 @@ test = "pytest" | |||
| testpaths = ["tests"] | |||
| python_files = ["*test_*.py"] | |||
| addopts = "--strict-markers" | |||
| filterwarnings = "error" | |||
There was a problem hiding this comment.
will this make pytest fail at any len(warnings) > 0?
There's one pyreverse test that still makes use of |
If you have time to help, that would be great. You can either push to my branch (GH's own editor makes that easier than it used to) or open a new PR, whatever works 👍🏻 |
There was a problem hiding this comment.
Managed to squeeze the review in. As suspected the change does not help in this case, but I can understand the confusion (the Generator return type together with TESTS sounding like plural it seems like we are iterating over different directories here).
Let me know if you need more help!
tests/pyreverse/test_inspector.py
Outdated
| @@ -30,7 +30,14 @@ def project(get_project: GetProjectCallable) -> Generator[Project, None, None]: | |||
| with _test_cwd(TESTS): | |||
| project = get_project("data", "data") | |||
| linker = inspector.Linker(project) | |||
| linker.visit(project) | |||
| if "clientmodule_test" in str(project): | |||
There was a problem hiding this comment.
This is never true, as the project is always the same here (the data directory underneath /tests). The string representation of project does not contain the substring clientmodule_test.
I would suggest just get rid of the if condition here.
There was a problem hiding this comment.
I tried that, but I received many failures. I'll try again tonight. 🤔
There was a problem hiding this comment.
Hm, but with the current change we are always hitting the else, and nothing has changed effectively compared to before.
I haven't looked at how to actually properly catch the warning, my guess would be that there are some other tests that don't catch this Warning yet (the majority of the legacy tests for pyreverse work with the same test data). I will also take a closer look at it this or tomorrow evening!
There was a problem hiding this comment.
Neither on your branch nor on main could I reproduce the deprecation warnings on the pyreverse tests.
On a recent CI run there is only one DeprecationWarning for some other test case, but not for pyreverse.
Am I missing something here like an option I have to turn on first?
Edit: That's strange - If I only run pytest tests/pyreverse/test_inspector I can see the DeprecationWarning in the warnings summary, but if I run pytest tests/pyreverse/ instead it does not appear. Maybe a bug in pytest?
Edit 2: I have the feeling that if a specific DeprecationWarning is caught and suppressed anywhere in the test suite using pytest.warns(DeprecationWarning), it will be suppressed for the whole remaining test suite as well.
While reading the docs I also found that it is possible to filter and ignore warnings for an entire test module via pytestmark = pytest.mark.filterwarnings("ignore:pyreverse:DeprecationWarning") at the top of the file, which is more convenient than wrapping all .visit() calls into with pytest.warns() blocks.
There was a problem hiding this comment.
Sorry @DudeNr33 you're quite right, the else branch always executes. I've just pushed a change to just ignore warnings in this fixture.
Pull Request Test Coverage Report for Build 3528574134
💛 - Coveralls |
Type of Changes
Description
I've opened a few PRs over time to clean up warnings in the test suite when I see them. Figured we might as well fail CI so that we don't merge with known issues.
Failure expected due to #7795, mentioned here: #7795 (comment). Edit: resolved.