started to fixturize pandas/tests/base #31701

SaturnFromTitan · 2020-02-05T14:32:07Z

tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff

I didn't apply the fixtures to the rest of test_ops.py yet since @jreback voiced some concerns in this comment.

SaturnFromTitan · 2020-02-05T14:34:33Z

pandas/tests/base/conftest.py

@@ -0,0 +1,52 @@
+import numpy as np


Here's a link to the original mixin this aims to replace:
https://github.com/pandas-dev/pandas/blob/master/pandas/tests/base/test_ops.py#L43-L83

WillAyd · 2020-02-05T21:11:46Z

pandas/tests/base/conftest.py

+
+
+@pytest.fixture(params=_all_objs.keys())
+def index_or_series_obj(request):


Hmm I think this is a rather complicated way of composing things. Given there is already a fixture for indeces that does something very similar can we just not modify this to use that and create the Series objects as required?

I'm not sure I understand you right. What do you mean by "create the Series objects as required"?

I need a fixture to replace self.all_objs from the Ops mixin. This will be used in a lot more places in pandas/tests/base/test_ops.py, so I can't just construct the Series' in the test cases.

I was hoping that there are other fixtures which I could use instead of _series and _narrow_series, but I think I still know too little about the codebase to find them. I'd be grateful for any hints to point me in the right direction ✌️

To rephrase I'm asking if there is a way to leverage the already existing indices fixture, as this duplicates part of the logic of that

Not really. I checked two possibilities:

Creating more parametrized fixtures for series and narrow_series:

@pytest.fixture def series(indices): data = np.random.randn(len(indices)) return pd.Series(data, index=indices, name=indices.name)

The problem is, that when using these fixtures in a test like

def test1(indices, series): ...

the product of all fixture values is executed instead of the union. So a lot of redundant test cases are run.

Letting index_or_series_obj depend on indices:
The problem is that index_or_series_obj is supposed to have a different length. So this doesn't really work either.

-> Based on this, the current implementation seems to be the best we can do.

Issue 1 and a potential solution (requires a new dependency) is well explained in this SO post.

jreback · 2020-02-09T17:33:54Z

pandas/tests/base/conftest.py

+}
+
+
+def _create_narrow_series(data_dtype):


just call this dtype, use the is_integer and so one for comparisons; we really try to avoid using numpy things generally (as they don't scale to all of our dtypes).

@jreback I just tried that, but is_integer(np.int16) is actually False. Similar for the other dtypes or is_float 😐

jreback · 2020-02-09T17:36:55Z

pandas/tests/base/conftest.py

+import pytest
+
+import pandas as pd
+from pandas.tests.indexes.conftest import indices_dict


I would rather just move the indices_dict to pandas/conftest.py and create these helper series there (as fixtures). ok to do this in 2 PRs

I just did it in this one. Please let me know if it's what you intended.

create these helper series there (as fixtures)

Not sure what you mean with "as fixtures". Like described in my comment above, I don't see a better way to create this index_or_series_obj fixture

…_obj fixture there as well

pandas/conftest.py

SaturnFromTitan · 2020-02-14T21:38:13Z

I think I addressed all the review comments. Friendly ping @jreback @WillAyd @jbrockmendel

jreback

lgtm. some comments for followups; if you can do them (and track), then no-need to keep a public list, if you are busy pls make an issue with these types of TODOs

pandas/conftest.py

jreback · 2020-02-15T01:09:43Z

pandas/conftest.py

+
+@pytest.fixture(params=indices_dict.keys())
+def indices(request):
+    # copy to avoid mutation, e.g. setting .name


next time around can you give these doc-strings

jreback · 2020-02-15T01:10:51Z

pandas/conftest.py

@@ -953,3 +954,69 @@ def __len__(self):
            return self._data.__len__()

    return TestNonDictMapping
+
+


can you do 'sections' here e.g. some text markers so reading the file is easy (@jbrockmendel has added these in several files); this can be a followup PR.

jreback · 2020-02-15T01:11:39Z

pandas/tests/indexes/test_base.py

@@ -34,6 +34,7 @@
    period_range,
 )
 import pandas._testing as tm
+from pandas.conftest import indices_dict


we really want to avoid this import (but understand doing this in steps)

jreback · 2020-02-15T01:11:47Z

pandas/tests/indexes/test_setops.py

@@ -13,7 +13,7 @@
 from pandas import Float64Index, Int64Index, RangeIndex, UInt64Index
 import pandas._testing as tm
 from pandas.api.types import pandas_dtype
-from pandas.tests.indexes.conftest import indices_dict
+from pandas.conftest import indices_dict


SaturnFromTitan · 2020-02-15T01:22:29Z

xref: #31989

started to fixturize pandas/tests/base

3a9802d

SaturnFromTitan requested a review from jreback February 5, 2020 14:32

SaturnFromTitan commented Feb 5, 2020

View reviewed changes

resolved a futurewarning

ac4a5eb

WillAyd requested changes Feb 5, 2020

View reviewed changes

WillAyd added the Testing pandas testing functions or related to the test suite label Feb 5, 2020

jreback requested changes Feb 9, 2020

View reviewed changes

SaturnFromTitan requested a review from jreback February 10, 2020 01:35

moved indices fixture to pandas/conftest and creating index_or_series…

0f88d47

…_obj fixture there as well

SaturnFromTitan force-pushed the refactor-test-base-part-4 branch from c3cc757 to 0f88d47 Compare February 10, 2020 16:14

jbrockmendel reviewed Feb 12, 2020

View reviewed changes

pandas/conftest.py Show resolved Hide resolved

jbrockmendel reviewed Feb 12, 2020

View reviewed changes

pandas/conftest.py Show resolved Hide resolved

jbrockmendel reviewed Feb 12, 2020

View reviewed changes

pandas/conftest.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Feb 12, 2020

View reviewed changes

pandas/conftest.py Show resolved Hide resolved

minor simplifications of _narrow_series creation in pandas/conftest.py

e865e90

jreback approved these changes Feb 15, 2020

View reviewed changes

jreback merged commit 8f49265 into pandas-dev:master Feb 15, 2020

SaturnFromTitan mentioned this pull request Feb 15, 2020

pandas/conftest.py refactoring #31989

Closed

4 tasks

roberthdevries pushed a commit to roberthdevries/pandas that referenced this pull request Mar 2, 2020

started to fixturize pandas/tests/base (pandas-dev#31701)

439e92e

SaturnFromTitan mentioned this pull request Mar 21, 2020

TST: Replace tm.all_index_generator with indices fixture #32886

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

started to fixturize pandas/tests/base #31701

started to fixturize pandas/tests/base #31701

SaturnFromTitan commented Feb 5, 2020

SaturnFromTitan Feb 5, 2020

WillAyd Feb 5, 2020

SaturnFromTitan Feb 5, 2020

WillAyd Feb 6, 2020

SaturnFromTitan Feb 8, 2020 •

edited

Loading

SaturnFromTitan Feb 10, 2020

jreback Feb 9, 2020

SaturnFromTitan Feb 10, 2020

jreback Feb 9, 2020

SaturnFromTitan Feb 10, 2020

SaturnFromTitan commented Feb 14, 2020

jreback left a comment

jreback Feb 15, 2020

jreback Feb 15, 2020

jreback Feb 15, 2020

jreback Feb 15, 2020

SaturnFromTitan commented Feb 15, 2020



		@pytest.fixture(params=_all_objs.keys())
		def index_or_series_obj(request):

		@@ -953,3 +954,69 @@ def __len__(self):
		return self._data.__len__()

		return TestNonDictMapping

started to fixturize pandas/tests/base #31701

started to fixturize pandas/tests/base #31701

Conversation

SaturnFromTitan commented Feb 5, 2020

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

SaturnFromTitan Feb 8, 2020 • edited Loading

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

SaturnFromTitan commented Feb 14, 2020

jreback left a 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

SaturnFromTitan commented Feb 15, 2020

SaturnFromTitan Feb 8, 2020 •

edited

Loading