[MRG] Refactor test_testing.py as per pytest design. #458
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I am in two minds about this I have to say ... I like |
|
Well we can keep a general thumb rule as it was - we must match the description if we defined it ourselves. Default ones by python need not be tested. (Hence I left out Or we can console ourselves by naming it |
|
Hmm actually looking into this a bit more, it looks like from joblib.testing import assert_raises_regex
def raise_func():
1/0
def test_unittest_wrong_message():
assert_raises_regex(ZeroDivisionError, 'this will not match',
raise_func)Output: All in all it may be worth googling a bit to see whether this problem has already been reported. If this gives nothing we should move everywhere to |
|
OK I was actually using pytest 3.0.3 which has this problem. Turns out I was very unlucky 3.0.4 and 3.0.5 don't have this problem and neither has 2.9.2 ... |
|
Here is a snippet to compare the error message when using assert_raises_regex compared to pytest.raises. from joblib.testing import assert_raises_regex
import pytest
def raise_func(exception_class, message):
# Some code
some_code = 1
raise_func_inner(exception_class, message)
# Some more code
some_more_code = 2
return 42
def raise_func_inner(exception_class, message):
raise exception_class(message)
def test_pytest_wrong_exception():
with pytest.raises(ValueError):
raise_func(TypeError, 'bla')
def test_pytest_wrong_message():
with pytest.raises(ValueError) as excinfo:
raise_func(ValueError, 'bla')
excinfo.match('boo')
def test_unittest_wrong_exception():
assert_raises_regex(ValueError, 'bla',
raise_func, TypeError, 'bla')
def test_unittest_wrong_message():
assert_raises_regex(ValueError, 'bla',
raise_func, ValueError, 'boo')Output: As expected the pytest error message are a bit less noisy. I would be in favour of replacing all the |
@lesteve Actually, lucky enough to catch this, as we have just specified pytest >= 3 and some other user might have complained in the future 😆
Sure. I will handle the already done modules at the end and take care about this for upcoming modules to process, including this one. |
|
@lesteve Thanks for the informative code snippet and execution log, I should really learn very fine demonstration of code snippets from your comments 😄 |
|
@lesteve: |
kdexd
force-pushed
the
pytestify-test-testing
branch
from
82b9a63
to
371ad7c
Compare
|
LGTM, merging. Keep up the good work! |
For the record, you don't have to duplicate your work. With git there are multiple ways to keep your work from a branch and have it available in another branch. One is |
|
I used to make one branch one another and rebase it later on master... But this |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Third Phase PR on #411 (Succeeding PR #454)
This PR deals with refactor of tests in test_testing.py to adopt the pytest flavor.
Regex checking of exception messages is carried out by
pytest_assert_raises.