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

[MRG] Use pytest for asserting exceptions in all test methods. #464

Merged
merged 4 commits into from
Dec 19, 2016

Conversation

kdexd
Copy link

@kdexd kdexd commented Dec 16, 2016

Fourth Phase PR on #411 (Succeeding PR #463 )

pytest.raises produces a less noisy error log upon failure. This PR replaces all the assert_raises and assert_raises_regex calls with pytest_assert_raises. Finally the unittest 's assert_raises is removed from joblib.testing and pytest_assert_raises has been renamed as raises.

  • assert_raises and assert_raises_regex calls have been modified as:
with raises(ExceptionName) as excinfo:   # pytest.raises
    code_that_raises_exception()
    # more code
excinfo.match('exception message regex')
  • assert_raises (old) and assert_raises_regex were the last of helpers used from unittest module. By this PR, the testing infrastructure no longer directly depends on unittest.

@kdexd kdexd force-pushed the use-pytest-assert-raises-everywhere branch from b89586e to 478b66b Compare December 16, 2016 15:27
# Python 2.7
assert_raises_regex = _dummy.assertRaisesRegexp

assert_raises = pytest.raises
Copy link
Member

Choose a reason for hiding this comment

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

I would use raises = pytest.raises it is not really asserting anything any more.

Copy link
Author

Choose a reason for hiding this comment

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

Done in next commit. Although I am skeptical about the changing of old assert_raises calls. Either we should avoid this renaming or replce old assert_raises calls with with blocks as well...

@@ -451,7 +451,7 @@ def test_call_and_shelve(tmpdir):
assert result.get() == 5

result.clear()
assert_raises(KeyError, result.get)
raises(KeyError, result.get)
Copy link
Author

Choose a reason for hiding this comment

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

@lesteve for example, statements like this will seem weird. I did not use a with statement where regex is not checked. What do you think about this ?

Copy link
Member

@lesteve lesteve Dec 16, 2016

Choose a reason for hiding this comment

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

Using blocks (i.e. with with statements) is nicer indeed.

@kdexd kdexd force-pushed the use-pytest-assert-raises-everywhere branch 2 times, most recently from fde404c to 23e20ec Compare December 17, 2016 03:47
@kdexd kdexd force-pushed the use-pytest-assert-raises-everywhere branch from 23e20ec to a5d3e60 Compare December 17, 2016 04:17
@lesteve
Copy link
Member

lesteve commented Dec 19, 2016

LGTM, I just pushed a minor cosmetic change. I'll wait for the CIs to be green and I'll merge this one.

@lesteve
Copy link
Member

lesteve commented Dec 19, 2016

All righty, all green, merging, thanks a lot!

@lesteve lesteve merged commit cb62e8f into joblib:master Dec 19, 2016
@kdexd
Copy link
Author

kdexd commented Dec 19, 2016

Great ! 😄 Time for warnings now..

@kdexd kdexd deleted the use-pytest-assert-raises-everywhere branch December 19, 2016 08:36
Sign up for free to 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.

None yet

2 participants