Skip to content

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

Merged
lesteve merged 4 commits intojoblib:masterfrom
kdexd:use-pytest-assert-raises-everywhere
Dec 19, 2016
Merged

[MRG] Use pytest for asserting exceptions in all test methods.#464
lesteve merged 4 commits intojoblib:masterfrom
kdexd:use-pytest-assert-raises-everywhere

Conversation

@kdexd
Copy link
Copy Markdown

@kdexd kdexd commented Dec 16, 2016 •
edited
Loading

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
lesteve
lesteve reviewed Dec 16, 2016
View reviewed changes
joblib/testing.py Outdated
# Python 2.7
assert_raises_regex = _dummy.assertRaisesRegexp

assert_raises = pytest.raises
Copy link
Copy Markdown
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.

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

Copy link
Copy Markdown
Author

@kdexd kdexd Dec 16, 2016

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...

kdexd
kdexd commented Dec 16, 2016
View reviewed changes
joblib/test/test_memory.py Outdated

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

@kdexd kdexd Dec 16, 2016

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
Copy Markdown
Member

@lesteve lesteve Dec 16, 2016 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@kdexd kdexd mentioned this pull request Dec 19, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lesteve lesteve lesteve left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kdexd @lesteve