Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't suppress pytest warning summaries #1892

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

EliahKagan
Copy link
Contributor

@EliahKagan EliahKagan commented Mar 31, 2024

This removes the --disable-warnings option from those specified in pyproject.toml for pytest to use automatically.

I originally tried this in the hope that it would show a warning for misspelled keyword arguments passed when constructing pytest marks (which should not be confused with misspelling the mark itself, which is something pytest can warn for). That bug (#1893) was discovered in #1888. This turns out not to be useful for that at all, so I'm opening this PR separate from anything related to that.

Nonetheless, there doesn't seem to be a significant disadvantage to showing this output, which is not easy to mistake for errors or test failures, and which might potentially help catch other kind of cases where a misspelling that intuitively feels like it would lead to an error instead produces a major change in semantics for the tests, in addition to other problems.

There are also some resource-related warnings that may help point the way to debugging things, especially in future examinations of how GitPython interacts with garbage collection on Windows.

Therefore, I think it makes sense to allow these warnings summaries to be shown.

I believe this does not worsen #1891, because these do not look like errors. I suspect it might very slightly mitigate it in some cases, even, since the warnings section near the xfailures section makes it easier to notice that these sections are not just for reporting errors and actual test failures. However, I definitely do not advocate this on the basis of that possibility; in my opinion, allowing the warning summaries to be shown is justified in spite of how it increases the overall length and number of sections of the test output, not because of that.

Deprecation warnings are separately suppressed and this does not change that, though perhaps it ought to, since eventually all areas in the tests that allow a deprecation warning to emanate from them should probably be marked for that specifically. This is also separate from how the new tests in test/deprecation (added in #1886) that check for the presence or absence of deprecation warnings work: those tests will produce hard failures if their expectations are not met, regardless of the changes here or to whether deprecation warnings elsewhere are allowed into the warnings summaries.

@EliahKagan EliahKagan marked this pull request as ready for review March 31, 2024 23:52
@Byron
Copy link
Member

Byron commented Apr 2, 2024

Thanks a lot! Anything on the way to improving on #1891 is appreciated.

@Byron Byron merged commit f8cad58 into gitpython-developers:main Apr 2, 2024
26 checks passed
@EliahKagan EliahKagan deleted the pytest-warnings branch April 2, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants