pandas (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation34 Commits10 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

One more step towards #22471. Happy to move the fixture to the test_indexing module if that's now the preferred form.

Choose a reason for hiding this comment

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

see comments. I don't really like to have multiple fixtures injected that are used in parts of tests. its just plain confusing (I know we let some thru before, but let's not do that anymore)

jreback added the Testing

pandas testing functions or related to the test suite

label

Mar 10, 2019

Choose a reason for hiding this comment

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

Codecov Report

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

@@ Coverage Diff @@ ## master #25633 +/- ##

Coverage 91.26% 91.26%

Files 173 173
Lines 52968 52968

Hits 48339 48339
Misses 4629 4629

Flag	Coverage Δ
#multiple	89.83% <ø> (ø)	⬆️
#single	41.71% <ø> (ø)	⬆️

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 df771cc...ddc1d44. Read the comment docs.

@jreback
Split up the test and it's green.

@@ -127,6 +127,17 @@ def timezone_frame():
return df


@pytest.fixture
def uint64_frame():
"""

Choose a reason for hiding this comment

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

instead of this can you just use the mixed_int_frame (maybe even add a column to it if needed)

Choose a reason for hiding this comment

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

Nope, not changing the semantics of tests here. Just translating.

piece = self.frame.loc[self.frame.index[:2], ['A', 'B']]
self.frame.loc[self.frame.index[-2]:, ['A', 'B']] = piece.values
result = self.frame.loc[self.frame.index[-2:], ['A', 'B']].values
def test_setitem_frame(self, float_frame, float_string_frame):

Choose a reason for hiding this comment

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

parametrize or split test

Choose a reason for hiding this comment

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

ok with splitting this test

assert result == expected

def test_lookup(self):
def test_lookup(self, float_frame, float_string_frame):

Choose a reason for hiding this comment

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

same

Choose a reason for hiding this comment

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

same, see if you can split the test

Choose a reason for hiding this comment

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

@jreback
You always insist on keeping PRs focused on one thing. Please don't try to force changing tests in a PR that is strictly for translation.

If you want to split tests, I'll do it as simply as possible (barring someone showing me some fixture magic that I'm not aware of).

@@ -127,6 +127,17 @@ def timezone_frame():
return df


@pytest.fixture
def uint64_frame():
"""

Choose a reason for hiding this comment

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

Nope, not changing the semantics of tests here. Just translating.

@jreback
Any suggestions how you would implement that fixturization you want?

Choose a reason for hiding this comment

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

frame/test_indexing.py is actually pretty huge. so pls change according to my comments, and open an issue to split this to separate smaller files (after)

piece = self.frame.loc[self.frame.index[:2], ['A', 'B']]
self.frame.loc[self.frame.index[-2]:, ['A', 'B']] = piece.values
result = self.frame.loc[self.frame.index[-2:], ['A', 'B']].values
def test_setitem_frame(self, float_frame, float_string_frame):

Choose a reason for hiding this comment

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

ok with splitting this test

assert result == expected

def test_lookup(self):
def test_lookup(self, float_frame, float_string_frame):

Choose a reason for hiding this comment

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

same, see if you can split the test

this one might be ok, merge master and ping on green.

@jreback

Sorry, I overlooked the request to merge master (busy times last month). Please reopen and I'll happily update.

@h-vetinari can you merge master

Conflicting files
pandas/tests/frame/test_indexing.py

Choose a reason for hiding this comment

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

anyplace you are adding multiple fixtures should have the tests splits; there are only a couple and shouldn't be a big deal to do

@@ -1329,15 +1331,15 @@ def test_getitem_fancy_1d(self):
# slice of mixed-frame

Choose a reason for hiding this comment

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

can you split this test

def test_xs(self):
idx = self.frame.index[5]
xs = self.frame.xs(idx)
def test_xs(self, float_frame, datetime_frame):

Choose a reason for hiding this comment

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

can you split

@@ -2582,7 +2593,8 @@ def test_boolean_indexing_mixed(self):
with pytest.raises(TypeError, match=msg):
df[df > 0.3] = 1

def test_where(self):
def test_where(self, float_string_frame, mixed_float_frame,

Choose a reason for hiding this comment

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

can you split

pls merge master and will look again

Thanks for taking care of this!

Labels

Testing

pandas testing functions or related to the test suite

Fixturize tests/frame/test_indexing.py by h-vetinari · Pull Request #25633 · pandas-dev/pandas (original) (raw)

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Codecov Report

@@ Coverage Diff @@ ## master #25633 +/- ##

Coverage 91.26% 91.26%

Files 173 173 Lines 52968 52968

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Files 173 173
Lines 52968 52968