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.

Sign up for GitHub

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

Jump to bottom

TST Replace unittest.TestCase.fail calls, use pytest.raises instead of legacy constructs #22681

Merged
merged 1 commit into from
Sep 13, 2018
Merged

TST Replace unittest.TestCase.fail calls, use pytest.raises instead of legacy constructs #22681

merged 1 commit into from
Sep 13, 2018

Conversation

rth
Copy link
Contributor

@rth rth commented Sep 12, 2018 •
edited
Loading

Similarly to numpy/numpy#11933,

Following pytest migration there are unit tests that used to call unittest.TestCase.fail and now will produce a misleading error message on failure because the fail method no longer exists (test classes just inherit from object).

This fixes this issue, by raising an exception on failure explicitly.

In addition this uses pytest.raises instead of more complicated legacy constructions of the form,

try:
   something
except ExceptedException:
   pass
else:
   raise AssertionError(error_message)

A few additional minor issues are discussed in comments below.

@pep8speaks
Copy link

Hello @rth! Thanks for submitting the PR.

rth
rth commented Sep 12, 2018
View reviewed changes
pandas/tests/plotting/test_frame.py Outdated
@@ -588,17 +588,13 @@ def test_subplots_layout(self):
@pytest.mark.slow
def test_subplots_warnings(self):
# GH 9464
warnings.simplefilter('error')
try:
with pytest.warns(Warning):
Copy link
Contributor Author

@rth rth Sep 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Asserting that a warning is raised is too general. Currently this passes, but no warning related to matplotlib are raised (at least locally for me), instead I get,

The list of emitted warnings is: 
[DeprecationWarning("Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated, and in 3.8 it will stop working"),
 DeprecationWarning("Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated, and in 3.8 it will stop working")].

Just FYI, it's an unrelated issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

use tm.assert_produces_warning

Copy link
Contributor Author

@rth rth Sep 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the review!

Sorry, I misread the code, it meant to check that no warnings are raised, please ignore my comment above. Fixed it now.

@rth rth force-pushed the use-assert-raises branch from 32b7b0f to 56a0394 Compare September 12, 2018 21:24
@codecov
Copy link

codecov bot commented Sep 12, 2018

Codecov Report

Merging #22681 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #22681   +/-   ##
=======================================
  Coverage   92.17%   92.17%           
=======================================
  Files         169      169           
  Lines       50715    50715           
=======================================
  Hits        46747    46747           
  Misses       3968     3968
Flag Coverage Δ
#multiple 90.58% <ø> (ø) ⬆️
#single 42.35% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c040353...56a0394. Read the comment docs.

@gfyoung gfyoung added Testing pandas testing functions or related to the test suite Refactor Internal refactoring of code labels Sep 13, 2018
@rth rth force-pushed the use-assert-raises branch 2 times, most recently from 8135ebc to 89c2a1e Compare September 13, 2018 08:43
@rth rth mentioned this pull request Sep 13, 2018
Closed
@jorisvandenbossche jorisvandenbossche removed the Refactor Internal refactoring of code label Sep 13, 2018
@jorisvandenbossche jorisvandenbossche added this to the 0.24.0 milestone Sep 13, 2018
@jreback
Copy link
Contributor

jreback commented Sep 13, 2018

can you rebase on master, ping on green.

@rth rth force-pushed the use-assert-raises branch from 89c2a1e to d0f2bed Compare September 13, 2018 18:41
@rth
Copy link
Contributor Author

rth commented Sep 13, 2018

@jreback I rebased -- Travis CI passed but there is still one failure in pandas.tests.io.parser.test_parsers.TestPythonParser in CircleCI py35_ascii. I don't think this PR changed that file, though this doesn't seem to be happening on master...

@jreback jreback merged commit 2b08e06 into pandas-dev:master Sep 13, 2018
@jreback
Copy link
Contributor

jreback commented Sep 13, 2018

thanks @rth

@rth rth deleted the use-assert-raises branch September 13, 2018 22:11
aeltanawy pushed a commit to aeltanawy/pandas that referenced this pull request Sep 20, 2018
@rth @aeltanawy
TST Use pytest.raises instead of legacy constructs (pandas-dev#22681)
3445e19
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018
@rth
TST Use pytest.raises instead of legacy constructs (pandas-dev#22681)
9d069ba
@jreback jreback mentioned this pull request Oct 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback left review comments

@gfyoung gfyoung gfyoung left review comments

@jorisvandenbossche jorisvandenbossche jorisvandenbossche approved these changes

Assignees
No one assigned
Labels
Clean Testing pandas testing functions or related to the test suite
Projects
None yet
Milestone
0.24.0
Development

Successfully merging this pull request may close these issues.
5 participants
@rth @pep8speaks @jreback @jorisvandenbossche @gfyoung