-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
[CLN] parametrize and cleanup a bunch of tests #22093
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
Conversation
Hello @jbrockmendel! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on July 28, 2018 at 16:31 Hours UTC |
@@ -74,29 +74,29 @@ def test_corr_non_numeric(self): | |||
tm.assert_frame_equal(result, expected) | |||
|
|||
@td.skip_if_no_scipy | |||
def test_corr_nooverlap(self): | |||
@pytest.mark.parametrize('meth', ['pearson', 'kendall', 'spearman']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should make this a fixture
@@ -658,21 +658,21 @@ def test_numeric_only_flag(self, meth): | |||
pytest.raises(TypeError, lambda: getattr(df2, meth)( | |||
axis=1, numeric_only=False)) | |||
|
|||
def test_mixed_ops(self): | |||
@pytest.mark.parametrize('op', ['mean', 'std', 'var', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have these as fixtures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not AFAICT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cython_table_items yields these (well it yields them all, maybe need to make a subset)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well it yields them all, maybe need to make a subset
Consider:
@pytest.mark.parametrize('op', SPECIFIC_EXPLICIT_LIST)
def test_thing(self, op):
def test_thing(self, cython_table_items):
if cython_table_items not in SPECIFIC_EXPLICIT_LIST:
return
There are good use cases for fixtures, this isn't near the top of that list.
for fax in [0, 1]: | ||
self._check_align_fill('right', meth, ax, fax) | ||
@pytest.mark.parametrize('meth', ['pad', 'bfill']) | ||
@pytest.mark.parametrize('ax', [0, 1, None]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have the axis arg as a fixture (though may need to add another one which adds None to it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok for sure need to use the fixture (but again can come back later on this)
def test_align_fill_method_left(self, meth, ax, fax): | ||
self._check_align_fill('left', meth, ax, fax) | ||
|
||
@pytest.mark.parametrize('meth', ['pad', 'bfill']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add as a fixture (if these are named appropriately they immeditaly become obvious as fixtures)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll circle back to see what makes sense here. For now this can be reduced a bunch by parametrizing the first arg going into these four tests.
@@ -855,21 +855,21 @@ def _test_stack_with_multiindex(multiindex): | |||
dtype=df.dtypes[0]) | |||
assert_frame_equal(result, expected) | |||
|
|||
def test_stack_preserve_categorical_dtype(self): | |||
@pytest.mark.parametrize('ordered', [False, True]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ordered is a fixture already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in conftest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pandas/tests//arrays/categorical/conftest.py
maybe can simply move it to top level and use it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I would love to create a helper in utils/testing.py
to generate boolean fixtures like these. Not sure if it's possible, but just throwing it out there for thought.
@@ -1038,9 +1038,10 @@ def test_add_raises(self): | |||
dt1 + dt2 | |||
|
|||
boxes = [lambda x: x, lambda x: pd.Series([x]), lambda x: pd.Index([x])] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe move these 2 lines to the top of the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use them more than once it makes sense, but ATM they are defined right next to where they are used, which is pretty ideal.
@@ -245,7 +245,7 @@ def test_set_axis_inplace(self): | |||
expected = s.copy() | |||
expected.index = list('abcd') | |||
|
|||
for axis in 0, 'index': | |||
for axis in [0, 'index']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
axis_series
@@ -1587,7 +1606,25 @@ def test_operators_reverse_object(self, op): | |||
expected = op(1., arr.astype(float)) | |||
assert_series_equal(result.astype(float), expected) | |||
|
|||
def test_operators_combine(self): | |||
pairings = [] | |||
for op in ['add', 'sub', 'mul', 'pow', 'truediv', 'floordiv']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we already have these no as fixtures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conftest.py doesn't have one specifically for this subset. Figuring out what is worth fixturizing will be much easier after arithmetic tests are centralized (#22033 and follow-ups)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure we do, all_arithmetic_operators (and several slices of this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above re subsetting fixtures.
comments and pls rebase |
Codecov Report
@@ Coverage Diff @@
## master #22093 +/- ##
==========================================
- Coverage 92.06% 92.05% -0.01%
==========================================
Files 170 170
Lines 50705 50705
==========================================
- Hits 46680 46678 -2
- Misses 4025 4027 +2
Continue to review full report at Codecov.
|
for fax in [0, 1]: | ||
self._check_align_fill('right', meth, ax, fax) | ||
@pytest.mark.parametrize('meth', ['pad', 'bfill']) | ||
@pytest.mark.parametrize('ax', [0, 1, None]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok for sure need to use the fixture (but again can come back later on this)
thanks |
No description provided.