CLN: Refactor pandas/tests/base - part3 #30147

SaturnFromTitan · 2019-12-08T21:55:16Z

Part of #23877

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

Starting to dissolve the Ops Mixin to drive fixturization of pandas/tests/base/

pandas/tests/base/utils.py

pandas/conftest.py

pandas/tests/base/test_ops.py

SaturnFromTitan · 2019-12-09T08:16:47Z

Thanks for your reviews!

SaturnFromTitan · 2019-12-09T20:58:31Z

I addressed all your comments @WillAyd @jschendel. Please have another look

pandas/conftest.py

pandas/tests/base/utils.py

SaturnFromTitan · 2019-12-10T13:13:55Z

@jreback Like mentioned in the issue description and in this comment: The utils and fixtures in this PR will be used in more places than just tests/base. That's why I put the fixtures directly into the base conftest. Applying them right away would have yielded a huge PR, so I wanted to split it into parts.

I see now that not using the utils and fixtures anywhere makes it impossible to understand their purpose. I will update the PR and remove the usage of Ops in tests/indexes which needs much fewer changes than tests/base.

jreback · 2019-12-10T13:15:52Z

@jreback Like mentioned in the issue description and in this comment: The utils and fixtures in this PR will be used in more places than just tests/base. That's why I put the fixtures directly into the base conftest. Applying them right away would have yielded a huge PR, so I wanted to split it into parts.

I see now that not using the utils and fixtures anywhere makes it impossible to understand their purpose. I will update the PR and remove the usage of Ops in tests/indexes which needs much fewer changes than tests/base.

this is likely true, however, I would rather move them locally pandas/tests/base, THEN in a follow on conform names & patterns between multiple places in the codebase.

SaturnFromTitan · 2019-12-10T13:39:18Z

Makes sense. It makes the PRs more atomic

…nywhere else for now

SaturnFromTitan · 2019-12-10T15:53:20Z

@jreback @WillAyd @jschendel
Thanks for your feedback. I added the following changes to the PR:

I extended the scope of this PR and replaced the usage of the Ops mixin in tests/indexes/ to provide some more context of the added fixtures and utils.
Like suggested, I moved the new fixtures into tests/indexes/conftest.py as that's the only place the fixtures are used for now
I'm using the existing indeces_dict from tests/indexes/conftest.py now instead of creating my own indexes fixtures now.

Please have another look at the PR

WillAyd

Looks good one comment

pandas/tests/indexes/datetimes/test_ops.py

SaturnFromTitan · 2020-01-24T10:14:31Z

pandas/tests/indexes/datetimes/test_ops.py

-        f = lambda x: isinstance(x, DatetimeIndex)
-        self.check_ops_properties(DatetimeIndex._field_ops, f)
-        self.check_ops_properties(DatetimeIndex._object_ops, f)
-        self.check_ops_properties(DatetimeIndex._bool_ops, f)


Already tested in pandas/tests/series/test_datetime_values.py::test_dt_namespace_accessor

Good catch. test_dt_namespace_accessor could use a good refactor if youre up to it (separate PR)

SaturnFromTitan · 2020-01-24T10:18:33Z

pandas/tests/base/test_ops.py

@@ -83,74 +75,29 @@ def setup_method(self, method):

        self.objs = self.indexes + self.series + self.narrow_series

-    def check_ops_properties(self, props, filter=None, ignore_failures=False):


This method was only used in pandas/tests/indexes. I could remove all tests where it was used though as they were redundant (already tested in pandas/tests/series/test_datetime_values.py::test_dt_namespace_accessor)

SaturnFromTitan · 2020-01-24T10:47:57Z

pandas/tests/indexes/period/test_ops.py

-        self.is_valid_objs = [o for o in self.objs if mask(o)]
-        self.not_valid_objs = [o for o in self.objs if not mask(o)]
-
-    def test_ops_properties(self):


Already tested in pandas/tests/series/test_datetime_values.py::test_dt_namespace_accessor

SaturnFromTitan · 2020-01-24T10:48:01Z

pandas/tests/indexes/timedeltas/test_ops.py

-        self.is_valid_objs = [o for o in self.objs if mask(o)]
-        self.not_valid_objs = []
-
-    def test_ops_properties(self):


Already tested in pandas/tests/series/test_datetime_values.py::test_dt_namespace_accessor

…for index tests

SaturnFromTitan · 2020-01-24T11:43:21Z

@jreback Thanks to your comment about redundant tests I could reduce the scope of this PR quite a bit further. It's ready to review now.

pandas/conftest.py

jreback

ok looks pretty good. @jbrockmendel if you'd have a look

pandas/tests/base/test_ops.py

pandas/tests/indexes/common.py

jbrockmendel · 2020-01-26T17:49:51Z

pandas/core/ops/__init__.py

@@ -265,7 +265,7 @@ def _get_opstr(op):
        rtruediv: "/",
        operator.floordiv: "//",
        rfloordiv: "//",
-        operator.mod: None,  # TODO: Why None for mod but '%' for rmod?
+        operator.mod: "%",


do we have tests where this makes a difference? maybe this lets us use numexpr and performance is affected?

Yeah, I wasn't sure about this one. Thought I give it a try and see if any of the CI hooks is failing. The 2 failing ones in pandas-dev.pandas (Linux py37_locale) look unrelated though. I'll rebase and try again.

@jbrockmendel CI is green now, so all tests seem to be fine with this change.
@jreback do you have any concerns about it?

looks ok, original issue is that this looks like a format string to python

pandas/tests/base/test_ops.py

…fferently in pandas/tests/base/test_ops.py

… case differently in pandas/tests/base/test_ops.py" This reverts commit 8bf1142.

SaturnFromTitan · 2020-01-31T13:02:27Z

Friendly reminder @jreback

pandas/tests/base/test_ops.py

jreback · 2020-02-01T15:11:23Z

pandas/core/ops/__init__.py

@@ -265,7 +265,7 @@ def _get_opstr(op):
        rtruediv: "/",
        operator.floordiv: "//",
        rfloordiv: "//",
-        operator.mod: None,  # TODO: Why None for mod but '%' for rmod?
+        operator.mod: "%",


looks ok, original issue is that this looks like a format string to python

jreback · 2020-02-01T15:13:36Z

thanks @SaturnFromTitan yeah love these test refactor PRs; we have built up some monotliths...though many are being taken down / split now, so thanks.

SaturnFromTitan changed the title ~~added new fixtures that can replace the Ops mixin~~ CLN: Refactor pandas/tests/base - part3 Dec 8, 2019

SaturnFromTitan force-pushed the refactor-test-base-part3 branch from 0d9a817 to 1685be9 Compare December 8, 2019 21:57

added new fixtures that can replace the Ops mixin

4d1750c

SaturnFromTitan force-pushed the refactor-test-base-part3 branch from 1685be9 to 4d1750c Compare December 8, 2019 22:03

SaturnFromTitan commented Dec 8, 2019

View reviewed changes

pandas/tests/base/utils.py Outdated Show resolved Hide resolved

SaturnFromTitan commented Dec 8, 2019

View reviewed changes

pandas/conftest.py Outdated Show resolved Hide resolved

fixed type hinting

3d04ff2

WillAyd reviewed Dec 9, 2019

View reviewed changes

pandas/conftest.py Outdated Show resolved Hide resolved

WillAyd added the Testing pandas testing functions or related to the test suite label Dec 9, 2019

jschendel reviewed Dec 9, 2019

View reviewed changes

jschendel added the Clean label Dec 9, 2019

jschendel added this to the 1.0 milestone Dec 9, 2019

refactoring according to review comments

89d84b4

WillAyd reviewed Dec 9, 2019

View reviewed changes

pandas/conftest.py Outdated Show resolved Hide resolved

removing unused variable arr and fixing the _narrow_series data

199896f

jreback requested changes Dec 10, 2019

View reviewed changes

pandas/conftest.py Outdated Show resolved Hide resolved

pandas/conftest.py Outdated Show resolved Hide resolved

pandas/tests/base/utils.py Outdated Show resolved Hide resolved

SaturnFromTitan added 5 commits December 10, 2019 14:43

removing usage of Ops mixin in tests/indexes

e269b09

moved new fixtures to tests/indexes/conftest.py as they're not used a…

baab827

…nywhere else for now

using new utils and fixtures in tests/indexes

28291f1

fixturizing tests/base/test_ops.py::test_binary_ops_docs

589ae3b

refactoring of tests/indexes/conftest.py

2bbc3fd

WillAyd reviewed Dec 10, 2019

View reviewed changes

pandas/tests/indexes/datetimes/test_ops.py Outdated Show resolved Hide resolved

SaturnFromTitan added 2 commits December 10, 2019 22:57

using pytest.skip instead of early return

b3d0252

skipping broken tests

53db63f

took care of review comments

69a0a0d

SaturnFromTitan force-pushed the refactor-test-base-part3 branch from 340b47f to 69a0a0d Compare January 23, 2020 23:13

SaturnFromTitan added 5 commits January 24, 2020 11:16

removed redundant tests

0fce4c5

removed changes that aren't needed for this PR anymore

b7892fa

moved helper to more appropriate location

471f217

Merge branch 'master' into refactor-test-base-part3

baa4965

removed some outdated changes

7562479

SaturnFromTitan commented Jan 24, 2020

View reviewed changes

moved datetime_series fixture to the root conftest so it's available …

85b16cb

…for index tests

SaturnFromTitan commented Jan 24, 2020

View reviewed changes

pandas/conftest.py Show resolved Hide resolved

jreback requested changes Jan 26, 2020

View reviewed changes

pandas/tests/base/test_ops.py Show resolved Hide resolved

pandas/tests/indexes/common.py Outdated Show resolved Hide resolved

SaturnFromTitan added 2 commits January 26, 2020 15:14

using all_arithmetic_functions in test_binary_ops_docs now

87e0a5b

undid some unrelated changes

3979b3d

jbrockmendel reviewed Jan 26, 2020

View reviewed changes

pandas/tests/base/test_ops.py Show resolved Hide resolved

SaturnFromTitan added 4 commits January 26, 2020 19:35

Merge branch 'master' into refactor-test-base-part3

452335a

undid my change in pandas/core/ops/__init__.py and handle the case di…

8bf1142

…fferently in pandas/tests/base/test_ops.py

Revert "undid my change in pandas/core/ops/__init__.py and handle the…

c1e9f28

… case differently in pandas/tests/base/test_ops.py" This reverts commit 8bf1142.

Merge branch 'master' into refactor-test-base-part3

d9bea94

jbrockmendel reviewed Jan 31, 2020

View reviewed changes

pandas/tests/base/test_ops.py Outdated Show resolved Hide resolved

using hard-coded values to parametrize test_binary_ops

87247a5

jreback added this to the 1.1 milestone Feb 1, 2020

jreback approved these changes Feb 1, 2020

View reviewed changes

jreback merged commit 01623f8 into pandas-dev:master Feb 1, 2020

SaturnFromTitan mentioned this pull request Feb 5, 2020

started to fixturize pandas/tests/base #31701

Merged

3 tasks

This was referenced Sep 22, 2020

BUG: modulo of pd.Int64Index returns negative values under some conditions #36526

Closed

REGR: Series.__mod__ behaves different with numexpr #36552

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLN: Refactor pandas/tests/base - part3 #30147

CLN: Refactor pandas/tests/base - part3 #30147

SaturnFromTitan commented Dec 8, 2019 •

edited

Loading

SaturnFromTitan commented Dec 9, 2019

SaturnFromTitan commented Dec 9, 2019 •

edited

Loading

SaturnFromTitan commented Dec 10, 2019

jreback commented Dec 10, 2019

SaturnFromTitan commented Dec 10, 2019

SaturnFromTitan commented Dec 10, 2019

WillAyd left a comment

SaturnFromTitan Jan 24, 2020

jbrockmendel Jan 31, 2020

SaturnFromTitan Jan 24, 2020

SaturnFromTitan Jan 24, 2020

SaturnFromTitan Jan 24, 2020

SaturnFromTitan commented Jan 24, 2020

jreback left a comment

jbrockmendel Jan 26, 2020

SaturnFromTitan Jan 26, 2020 •

edited

Loading

SaturnFromTitan Jan 27, 2020

jreback Feb 1, 2020

SaturnFromTitan commented Jan 31, 2020

jreback Feb 1, 2020

jreback commented Feb 1, 2020

		@@ -83,74 +75,29 @@ def setup_method(self, method):

		self.objs = self.indexes + self.series + self.narrow_series

		def check_ops_properties(self, props, filter=None, ignore_failures=False):

CLN: Refactor pandas/tests/base - part3 #30147

CLN: Refactor pandas/tests/base - part3 #30147

Conversation

SaturnFromTitan commented Dec 8, 2019 • edited Loading

SaturnFromTitan commented Dec 9, 2019

SaturnFromTitan commented Dec 9, 2019 • edited Loading

SaturnFromTitan commented Dec 10, 2019

jreback commented Dec 10, 2019

SaturnFromTitan commented Dec 10, 2019

SaturnFromTitan commented Dec 10, 2019

WillAyd 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

SaturnFromTitan commented Jan 24, 2020

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SaturnFromTitan Jan 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SaturnFromTitan commented Jan 31, 2020

Choose a reason for hiding this comment

jreback commented Feb 1, 2020

SaturnFromTitan commented Dec 8, 2019 •

edited

Loading

SaturnFromTitan commented Dec 9, 2019 •

edited

Loading

SaturnFromTitan Jan 26, 2020 •

edited

Loading