TST: Use fixtures instead of setup_method for index tests #28865

jschendel · 2019-10-09T04:50:28Z

The common index tests use setup_method to create a dict of indexes to test against:

pandas/pandas/tests/indexes/test_range.py

Lines 25 to 30 in df2e081

    
           def setup_method(self, method): 
        
               self.indices = dict( 
        
                   index=RangeIndex(0, 20, 2, name="foo"), 
        
                   index_dec=RangeIndex(18, -1, -2, name="bar"), 
        
               ) 
        
               self.setup_indices()

This dict of indexes is then iterated over within the tests:

pandas/pandas/tests/indexes/common.py

Lines 359 to 363 in df2e081

    
           def test_numpy_argsort(self): 
        
               for k, ind in self.indices.items(): 
        
                   result = np.argsort(ind) 
        
                   expected = ind.argsort() 
        
                   tm.assert_numpy_array_equal(result, expected)

The bulk of this PR involves converting self.indices into a parametrized fixture of indexes, and adjusting the tests to support this (largely just unindenting). I had to do this conversion for all indexes at once since common test code for all index classes utilizes this pattern, so the diff is fairly large, but it should be relatively simple changes.

I also had to make some changes to references to specific indexes as well (e.g. self.index, self.strIndex, etc.) since the setup_method code also directly set each index in the dict as a class attribute.

jschendel · 2019-10-09T04:51:30Z

pandas/tests/indexes/datetimelike.py

@@ -58,13 +58,14 @@ def test_view(self):
        tm.assert_index_equal(result, i_view)

    def test_map_callable(self):
-        expected = self.index + self.index.freq
-        result = self.index.map(lambda x: x + x.freq)
+        index = self.create_index()


Planning to convert self.create_index() into a fixture in a follow up PR; in this PR I've used it to replace references to indexes set via setup_method.

i'm probably not be following the logic correctly, but self.create_index() is a different index to self.index for the datetimelike tests?

i.e. tdi

def setup_method(self, method): self.indices = dict(index=tm.makeTimedeltaIndex(10)) self.setup_indices()

so self.index is created with self.setup_indices() and not the same as

def create_index(self): return pd.to_timedelta(range(5), unit="d") + pd.offsets.Hour(1)

jschendel · 2019-10-09T04:54:15Z

pandas/tests/indexes/test_base.py

 import pandas.util.testing as tm
 from pandas.util.testing import assert_almost_equal


 class TestIndex(Base):
    _holder = Index

-    def setup_method(self, method):
-        self.indices = dict(


I didn't create an indices fixture for this class since there was already an identical indices fixture in conftest.py, so sharing that one here. I updated the conftest.py fixture to use the same naming conventions converted to snake_case, e.g. unicodeIndex --> unicode_index.

jschendel · 2019-10-09T05:01:48Z

pandas/tests/indexes/test_base.py

-        )
-        self.setup_indices()
+    @pytest.fixture
+    def index(self, request):


I think this is pretty much the only non-standard thing I had to do. A lot of the tests in this file only tested a subset of the indexes in indices, so I needed a way to mimic that with fixtures. The idea here is to use indirect parametrization to allow this behavior, e.g.

@pytest.mark.parametrize("index", ["int", "uint", "float"], indirect=True)

Would result in this fixture returning the three indexes keyed by "int", "uint", "float" in indices_dict.

I think this is pretty much the only non-standard thing I had to do

this is OK. used in io.formats.test_to_html for the biggie_df_fixture.

the alternative is to create a fixture yielding just the ids which is then requested by the indices fixture. (see filepath_or_buffer_id and filepath_or_buffer fixtures in io.formats.test_format.) and then override the id fixture directly at function, class or module level.

both patterns (direct and indirect parameterisation) are equivalent.

looking at this in detail. I don't think you need to create a separate fixture in order to do this. you could apply indirect parametrisation to the indices fixture.

simonjayhawkins

@jschendel Very Nice!

a few comments, but mostly can probably be done as a follow-on.

not finished reviewing all the changes yet. hopefully can get back to it in a few hours.

simonjayhawkins · 2019-10-09T14:35:00Z

pandas/tests/indexes/common.py

        # gh-12309: Check that the "name" argument
        # passed at initialization is honored.
+        if isinstance(indices, MultiIndex):
+            return


maybe this should be a pytest.skip instead of a return?

simonjayhawkins · 2019-10-09T14:37:55Z

pandas/tests/indexes/conftest.py

+}
+
+
+@pytest.fixture(params=indices_dict.keys())
 def indices(request):


maybe a smaller diff if the fixture was named index?

Not just this PR, but in general maybe we shouldn't use the plural when creating fixtures since when the fixture is used it creates a single value at a time and so the plural name in the tests can sometimes be confusing.

simonjayhawkins · 2019-10-09T14:39:18Z

pandas/tests/indexes/common.py

-                with pytest.raises(InvalidIndexError, match=e):
-                    index.get_indexer(index[0:2])
+        if isinstance(indices, IntervalIndex):
+            return


pytest.skip?

simonjayhawkins · 2019-10-09T14:41:02Z

pandas/tests/indexes/common.py

+        elif isinstance(indices, (RangeIndex, MultiIndex, CategoricalIndex)):
+            # RangeIndex cannot be initialized from data
+            # MultiIndex and CategoricalIndex are tested separately
+            return


pytest.skip?

simonjayhawkins · 2019-10-09T14:42:20Z

pandas/tests/indexes/common.py

+            )
+        elif isinstance(indices, IntervalIndex):
+            # checked in test_interval.py
+            pass


pytest.skip?

simonjayhawkins · 2019-10-09T15:01:04Z

pandas/tests/indexes/common.py

+        values = np.asarray(idx.values)
+
+        if len(indices) == 0:
+            return


simonjayhawkins · 2019-10-09T15:01:11Z

pandas/tests/indexes/common.py

+        elif isinstance(indices, DatetimeIndexOpsMixin):
+            values[1] = iNaT
+        elif isinstance(indices, (Int64Index, UInt64Index)):
+            return


simonjayhawkins · 2019-10-09T15:02:11Z

pandas/tests/indexes/common.py

+            if isinstance(indices, DatetimeIndexOpsMixin):
+                values[1] = iNaT
+            elif isinstance(indices, (Int64Index, UInt64Index)):
+                return


simonjayhawkins · 2019-10-09T15:02:29Z

pandas/tests/indexes/common.py

-                with pytest.raises(NotImplementedError, match=msg):
-                    idx.fillna(idx[0])
+        if len(indices) == 0:
+            pass


simonjayhawkins · 2019-10-09T15:15:54Z

pandas/tests/indexes/datetimelike.py

@@ -58,13 +58,14 @@ def test_view(self):
        tm.assert_index_equal(result, i_view)

    def test_map_callable(self):
-        expected = self.index + self.index.freq
-        result = self.index.map(lambda x: x + x.freq)
+        index = self.create_index()


i'm probably not be following the logic correctly, but self.create_index() is a different index to self.index for the datetimelike tests?

i.e. tdi

def setup_method(self, method): self.indices = dict(index=tm.makeTimedeltaIndex(10)) self.setup_indices()

so self.index is created with self.setup_indices() and not the same as

def create_index(self): return pd.to_timedelta(range(5), unit="d") + pd.offsets.Hour(1)

simonjayhawkins

some more comments, again many could be a follow-on.

still have more files to review. see what others think but maybe this PR needs to be done in smaller chunks.

simonjayhawkins · 2019-10-09T22:59:36Z

pandas/tests/indexes/test_base.py

@@ -41,80 +41,65 @@
 from pandas.core.indexes.api import Index, MultiIndex
 from pandas.core.sorting import safe_sort
 from pandas.tests.indexes.common import Base
+from pandas.tests.indexes.conftest import indices_dict


we shouldn't really be importing from a conftest file. probably best to move indices_dict to common and then import from there.

simonjayhawkins · 2019-10-09T23:03:20Z

pandas/tests/indexes/test_base.py

-        )
-        self.setup_indices()
+    @pytest.fixture
+    def index(self, request):


looking at this in detail. I don't think you need to create a separate fixture in order to do this. you could apply indirect parametrisation to the indices fixture.

simonjayhawkins · 2019-10-09T23:09:45Z

pandas/tests/indexes/test_base.py

-        new_index = self.dateIndex[None, :]
+    @pytest.mark.parametrize("index", ["datetime"], indirect=True)
+    def test_new_axis(self, index):
+        new_index = index[None, :]


here and elsewhere I think result may be more appropriate than new_index

simonjayhawkins · 2019-10-09T23:12:48Z

pandas/tests/indexes/test_base.py

@@ -570,37 +555,50 @@ def test_constructor_cast(self):
        with pytest.raises(ValueError, match=msg):
            Index(["a", "b", "c"], dtype=float)

-    def test_view_with_args(self):
-        restricted = ["unicodeIndex", "strIndex", "catIndex", "boolIndex", "empty"]


it might be better to keep this and just do a pytest.skip if in restricted.

simonjayhawkins · 2019-10-09T23:15:27Z

pandas/tests/indexes/test_base.py

-        d = self.dateIndex[0]
-        assert self.dateIndex.asof(d) == d
-        assert isna(self.dateIndex.asof(d - timedelta(1)))
+    @pytest.mark.parametrize("index", ["datetime"], indirect=True)


tests of a single index type probably don't belong in base.

simonjayhawkins · 2019-10-09T23:23:35Z

pandas/tests/indexes/test_base.py

-            index = self.indices[name]
-            expected = Index(np.arange(len(index), 0, -1))
-
+    @pytest.mark.parametrize(


rather than duplicating the parameterisation, is it worth making mapper a fixture?

simonjayhawkins · 2019-10-09T23:30:57Z

pandas/tests/indexes/test_base.py

+    def _check_method_works(self, method, index):
+        method(index)


only used twice, probably best to inline.

simonjayhawkins · 2019-10-09T23:32:52Z

pandas/tests/indexes/test_base.py

@@ -1402,7 +1408,7 @@ def test_format(self):
            expected = [str(index[0])]
            assert formatted == expected

-        self.strIndex[:0].format()
+        Index([]).format()


why is this changing?

simonjayhawkins · 2019-10-09T23:36:37Z

pandas/tests/indexes/test_base.py

        tm.assert_index_equal(dropped, expected)

+    @pytest.mark.parametrize("index", ["string", "int", "float"], indirect=True)


if this combination is common could create an alias to the mark.

simonjayhawkins · 2019-10-09T23:42:24Z

pandas/tests/indexes/test_base.py

-                index.contains(1)
+    def test_deprecated_contains(self, indices):
+        # deprecated for all types except IntervalIndex
+        warning = FutureWarning if not isinstance(indices, pd.IntervalIndex) else None


may be more appropriate to skip on IntervalIndex

jreback · 2019-10-11T14:13:50Z

lgtm. @simonjayhawkins ok here?

simonjayhawkins · 2019-10-11T14:25:53Z

lgtm. @simonjayhawkins ok here?

@jreback I think good to merge and tidy up as a folow-up.

jreback · 2019-10-11T15:14:26Z

thanks @simonjayhawkins agreed

nice @jschendel

…#28865)

TST: Use fixtures instead of setup_method for index tests

ac686e4

jschendel added Refactor Internal refactoring of code Testing pandas testing functions or related to the test suite Index Related to the Index class or subclasses labels Oct 9, 2019

jschendel added this to the 1.0 milestone Oct 9, 2019

jschendel commented Oct 9, 2019

View reviewed changes

jschendel added 2 commits October 8, 2019 23:22

explicitly specify indexes

4ac0187

fix 32bit

286a81c

simonjayhawkins reviewed Oct 9, 2019

View reviewed changes

jreback merged commit 981299c into pandas-dev:master Oct 11, 2019

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

TST: Use fixtures instead of setup_method for index tests (pandas-dev…

0a473b6

…#28865)

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

TST: Use fixtures instead of setup_method for index tests (pandas-dev…

49123ff

…#28865)

jschendel deleted the idx-common-param branch December 20, 2019 01:03

bongolegend pushed a commit to bongolegend/pandas that referenced this pull request Jan 1, 2020

TST: Use fixtures instead of setup_method for index tests (pandas-dev…

c0784d4

…#28865)

simonjayhawkins mentioned this pull request Mar 24, 2020

TST: Use indices fixture in tests/indexes/test_base.py #32963

Merged

3 tasks

SaturnFromTitan mentioned this pull request Mar 24, 2020

TST: Rename indices fixture #32974

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TST: Use fixtures instead of setup_method for index tests #28865

TST: Use fixtures instead of setup_method for index tests #28865

jschendel commented Oct 9, 2019

jschendel Oct 9, 2019

simonjayhawkins Oct 9, 2019

jschendel Oct 9, 2019

jschendel Oct 9, 2019 •

edited

Loading

simonjayhawkins Oct 9, 2019

simonjayhawkins Oct 9, 2019

simonjayhawkins left a comment

simonjayhawkins Oct 9, 2019

simonjayhawkins Oct 9, 2019

simonjayhawkins Oct 9, 2019

simonjayhawkins Oct 9, 2019

simonjayhawkins Oct 9, 2019

simonjayhawkins Oct 9, 2019

simonjayhawkins Oct 9, 2019

simonjayhawkins Oct 9, 2019

simonjayhawkins Oct 9, 2019

simonjayhawkins Oct 9, 2019

simonjayhawkins left a comment

simonjayhawkins Oct 9, 2019

simonjayhawkins Oct 9, 2019

simonjayhawkins Oct 9, 2019

simonjayhawkins Oct 9, 2019

simonjayhawkins Oct 9, 2019

simonjayhawkins Oct 9, 2019

simonjayhawkins Oct 9, 2019

simonjayhawkins Oct 9, 2019

simonjayhawkins Oct 9, 2019

simonjayhawkins Oct 9, 2019

jreback commented Oct 11, 2019

simonjayhawkins commented Oct 11, 2019

jreback commented Oct 11, 2019

	def setup_method(self, method):
	self.indices = dict(
	index=RangeIndex(0, 20, 2, name="foo"),
	index_dec=RangeIndex(18, -1, -2, name="bar"),
	)
	self.setup_indices()

	def test_numpy_argsort(self):
	for k, ind in self.indices.items():
	result = np.argsort(ind)
	expected = ind.argsort()
	tm.assert_numpy_array_equal(result, expected)

		tm.assert_index_equal(dropped, expected)

		@pytest.mark.parametrize("index", ["string", "int", "float"], indirect=True)

TST: Use fixtures instead of setup_method for index tests #28865

TST: Use fixtures instead of setup_method for index tests #28865

Conversation

jschendel commented Oct 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jschendel Oct 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonjayhawkins 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

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

simonjayhawkins 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

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

jreback commented Oct 11, 2019

simonjayhawkins commented Oct 11, 2019

jreback commented Oct 11, 2019

jschendel Oct 9, 2019 •

edited

Loading