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

MAINT: tm.assert_raises_regex --> pytest.raises #23592

Merged
merged 3 commits into from
Nov 10, 2018

Conversation

gfyoung
Copy link
Member

@gfyoung gfyoung commented Nov 9, 2018

pytest.raises has all of the functionality that we need from tm.assert_raises_regex.

Closes #16521 (for good this time! 🙂 ).

If this is something we would like to have for 0.24.0, I would ask that this get reviewed and merged ASAP, given how massive the refactoring is and the high probability of merge conflicts.

cc @pandas-dev/pandas-core

@gfyoung gfyoung added Refactor Internal refactoring of code Testing pandas testing functions or related to the test suite Clean labels Nov 9, 2018
@codecov
Copy link

codecov bot commented Nov 9, 2018

Codecov Report

Merging #23592 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #23592      +/-   ##
==========================================
- Coverage   92.25%   92.24%   -0.02%     
==========================================
  Files         161      161              
  Lines       51277    51278       +1     
==========================================
- Hits        47305    47299       -6     
- Misses       3972     3979       +7
Flag Coverage Δ
#multiple 90.62% <100%> (-0.02%) ⬇️
#single 42.28% <0%> (-0.04%) ⬇️
Impacted Files Coverage Δ
pandas/util/testing.py 86.05% <100%> (-0.66%) ⬇️

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 8ed92ef...2b7fb24. Read the comment docs.

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

Really nice work. My browser had problems to load the whole diff, so I guess this kept you entertained for a while... ;)

Looks really good, there are just few things I don't understand.

pandas/tests/io/test_pytables.py Show resolved Hide resolved
pandas/tests/test_algos.py Show resolved Hide resolved
pandas/tests/test_strings.py Outdated Show resolved Hide resolved
pandas/tests/test_strings.py Outdated Show resolved Hide resolved
pandas/tests/frame/test_alter_axes.py Outdated Show resolved Hide resolved
pandas/tests/io/parser/c_parser_only.py Show resolved Hide resolved
@jreback jreback added this to the 0.24.0 milestone Nov 9, 2018
@jreback
Copy link
Contributor

jreback commented Nov 9, 2018

@gfyoung wasn't expecting all of these in a single PR. ok though.

@jreback
Copy link
Contributor

jreback commented Nov 9, 2018

can you deprecate tm.assert_raises_regex as well. and update the docs.

@jreback
Copy link
Contributor

jreback commented Nov 9, 2018

ping when ready

@jorisvandenbossche
Copy link
Member

can you deprecate tm.assert_raises_regex as well. and update the docs.

Let's do that in a separate PR? This diff is already huge to check

@jreback
Copy link
Contributor

jreback commented Nov 9, 2018

Let's do that in a separate PR? This diff is already huge to check

yes this is ok

@datapythonista
Copy link
Member

assert_raises_regex is removed in this PR, I think it's the last file, you need to expand the diff, as it's collapsed because of the size.

@jorisvandenbossche
Copy link
Member

Then let's add it back here, if we want to deprecate it (which I think is a good idea)

@gfyoung
Copy link
Member Author

gfyoung commented Nov 9, 2018

@jreback @jorisvandenbossche :

Why are we deprecating? It's not even part of the public testing API (in pandas/testing). There are no docs to update in the first place.

assert_raises_regex is essentially our internal implementation for doing what pytest.raises does now.
@datapythonista identified several places for refactorings and improvements, but I would like to save those for a follow-up. From the viewpoint of infrastructure, this is ready to merge.

@jreback
Copy link
Contributor

jreback commented Nov 10, 2018

the reason to deprecate is that it’s in our public ish testing interface and has been around for a while - we can just remove in the next version (short deprecation cycle)
this is a little more friendly

@gfyoung
Copy link
Member Author

gfyoung commented Nov 10, 2018

the reason to deprecate is that it’s in our public ish testing interface and has been around for a while - we can just remove in the next version (short deprecation cycle)

@jreback : Okay, that's fair. I'll deprecate this then and have it removed in the next major release (either 0.25 or 1.0, given that this is not totally public, only "public-ish" 🙂 ).

@gfyoung gfyoung force-pushed the assert-raises-regex-remove branch 2 times, most recently from f0cfe1c to 325adea Compare November 10, 2018 12:07
@gfyoung
Copy link
Member Author

gfyoung commented Nov 10, 2018

@jreback @datapythonista : Addressed all comments, and all is still green. PTAL.

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @gfyoung

@jreback jreback merged commit 383d052 into pandas-dev:master Nov 10, 2018
@jreback
Copy link
Contributor

jreback commented Nov 10, 2018

thanks!

@gfyoung gfyoung deleted the assert-raises-regex-remove branch November 10, 2018 23:22
thoo added a commit to thoo/pandas that referenced this pull request Nov 11, 2018
…fixed

* upstream/master:
  DOC: Fixes to docstring to add validation to CI (pandas-dev#23560)
  DOC: Remove incorrect periods at the end of parameter types (pandas-dev#23600)
  MAINT: tm.assert_raises_regex --> pytest.raises (pandas-dev#23592)
  DOC: Updating Series.resample and DataFrame.resample docstrings (pandas-dev#23197)
  ENH: Support for partition_cols in to_parquet (pandas-dev#23321)
  TST: Use intp as expected dtype in IntervalIndex indexing tests (pandas-dev#23609)
thoo added a commit to thoo/pandas that referenced this pull request Nov 11, 2018
* upstream/master:
  BUG: Casting tz-aware DatetimeIndex to object-dtype ndarray/Index (pandas-dev#23524)
  BUG: Delegate more of Excel parsing to CSV (pandas-dev#23544)
  API: DataFrame.__getitem__ returns Series for sparse column (pandas-dev#23561)
  CLN: use float64_t consistently instead of double, double_t (pandas-dev#23583)
  DOC: Fix Order of parameters in docstrings (pandas-dev#23611)
  TST: Unskip some Categorical Tests (pandas-dev#23613)
  TST: Fix integer ops comparison test (pandas-dev#23619)
  DOC: Fixes to docstring to add validation to CI (pandas-dev#23560)
  DOC: Remove incorrect periods at the end of parameter types (pandas-dev#23600)
  MAINT: tm.assert_raises_regex --> pytest.raises (pandas-dev#23592)
  DOC: Updating Series.resample and DataFrame.resample docstrings (pandas-dev#23197)
@jreback
Copy link
Contributor

jreback commented Nov 11, 2018

didn’t check but do we fail an internal build if assert_raises_regex is used?

sure it would raise a FutureWarning but that doesn’t always fail it i think

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Nov 11, 2018 via email

@jreback
Copy link
Contributor

jreback commented Nov 11, 2018

ok @gfyoung can u add to the code_checks then to look for this (we already do for things like np.testing)

@gfyoung
Copy link
Member Author

gfyoung commented Nov 11, 2018

@jreback @TomAugspurger : I grepped the entire codebase, and the only instances of assert_raises_regex are in testing.py and test_testing.py.

The warnings should cause the numpydev build to fail if it were to happen (which it doesn't).

@datapythonista
Copy link
Member

I think the idea is not on what we have now, but in case someone creates a new PR that uses assert_raises_regex. I can add the check, so we show the CI in red if that's the case. Will send a PR with it shortly.

@datapythonista
Copy link
Member

Opened #23627 with the check.

JustinZhengBC pushed a commit to JustinZhengBC/pandas that referenced this pull request Nov 14, 2018
* MAINT: tm.assert_raises_regex --> pytest.raises

pytest.raises has all of the functionality that
we need from tm.assert_raises_regex.

Closes pandas-devgh-16521.

* Don't remove, just deprecate assert_raises_regex

* CLN: Test cleanups and follow-ups
JustinZhengBC pushed a commit to JustinZhengBC/pandas that referenced this pull request Nov 14, 2018
tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 2018
* MAINT: tm.assert_raises_regex --> pytest.raises

pytest.raises has all of the functionality that
we need from tm.assert_raises_regex.

Closes pandas-devgh-16521.

* Don't remove, just deprecate assert_raises_regex

* CLN: Test cleanups and follow-ups
tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 2018
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
* MAINT: tm.assert_raises_regex --> pytest.raises

pytest.raises has all of the functionality that
we need from tm.assert_raises_regex.

Closes pandas-devgh-16521.

* Don't remove, just deprecate assert_raises_regex

* CLN: Test cleanups and follow-ups
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
* MAINT: tm.assert_raises_regex --> pytest.raises

pytest.raises has all of the functionality that
we need from tm.assert_raises_regex.

Closes pandas-devgh-16521.

* Don't remove, just deprecate assert_raises_regex

* CLN: Test cleanups and follow-ups
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Refactor Internal refactoring of code Testing pandas testing functions or related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants