- Notifications
You must be signed in to change notification settings - Fork 93
Check for warnings output during unit tests #116
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
Check for warnings output during unit tests #116
Conversation
Hm, the tests fail for |
The problem is that the stored outcomes are |
Thanks for the tip @The-Compiler, let me try tonight |
So I realized what I was doing was inherently wrong. Checking general warnings is extremely fragile at best, since pytest itself emits warnings and there's no way of knowing which warnings came from this plugin vs. pytest. Checking to make sure we emitted at least some amount of warnings 'x' also doesn't tell us anything since those warnings could be coming from somewhere else, so the test would give a false sense of security or just confuse people. That leads me to think there are 2 possible solutions:
Thoughts @icemac and @The-Compiler ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I think we can go with this approach.
test_pytest_rerunfailures.py Outdated
@@ -1,4 +1,5 @@ | |||
from unittest import mock | |||
import pkg_resources |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import is no longer needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the warning text sounds like a good idea. I commented on a minor change, but LGTM otherwise!
test_pytest_rerunfailures.py Outdated
result = testdir.runpytest('--reruns', '3', | ||
'--reruns-delay', str(delay_time)) | ||
| ||
num_warnings = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could probably be a has_warnings
or warning_expected
boolean now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yes, this is wrong. sorry for leaving it, let me remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new version looks better, though now you have two identical if
statements after each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry just changed it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 This can be merged after rebase.
In the tests, check to make sure that:
Before, we weren't checking but I think we should be since we are setting these warnings in this plugin, and they are not coming from pytest.
No change log as this is trivial