Fixturize tests/frame/test_axis_select_reindex.py #25627

h-vetinari · 2019-03-10T14:33:05Z

Picking up #22471 again. Needed to readd some fixtures that were removed by #24885.

codecov · 2019-03-10T15:08:33Z

Codecov Report

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

@@           Coverage Diff           @@
##           master   #25627   +/-   ##
=======================================
  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 e28ae70...aa312da. Read the comment docs.

codecov · 2019-03-10T15:08:34Z

Codecov Report

❗ No coverage uploaded for pull request base (master@87d7cdf). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #25627   +/-   ##
=========================================
  Coverage          ?   91.83%           
=========================================
  Files             ?      174           
  Lines             ?    50643           
  Branches          ?        0           
=========================================
  Hits              ?    46510           
  Misses            ?     4133           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`90.37% <ø> (?)`
#single	`41.69% <ø> (?)`

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 87d7cdf...8afc817. Read the comment docs.

jreback · 2019-03-10T15:39:24Z

pandas/tests/frame/conftest.py

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


+@pytest.fixture
+def datetime_series():
+    """


this fixture itself is not very useful, however, possibly a fixture that is a float_frame but uses this an index might be

Yea I would agree with this comment. It also seems strange to put a fixture in this conftest which returns a series, as that could either create duplication or friction with the conftest for the series directory.

Any way to do what @jreback suggests above here?

@h-vetinari: @jreback Can't do 3 things at the same time reasonably.
You're already requesting to do fixturization and renaming at the same time, let's keep the reorg for later. Also, case in point about the necessity of (some of) the fixtures re-added here: #25635

@jreback: +1 on reorg of fixtures later

@WillAyd, of course both of you are right. But the quasi-fixtures from tests.frame.common.TestData that are supposed to be translated already have this (as self.ts1).

I guess I should have pushed back harder against #24885, because datetime_series was already in tests.frame.conftest (since #22236) precisely for this translation. There's a limit to how many balls any one PR should have up in the air, and fixturization plus renaming is more than enough IMO.

jreback · 2019-03-10T15:40:44Z

pandas/tests/frame/test_axis_select_reindex.py

-    def test_align(self):
-        af, bf = self.frame.align(self.frame)
-        assert af._data is not self.frame._data
+    def test_align(self, float_frame, int_frame, float_string_frame,


if you are adding many fixtures to a test, would rather use a parametrization of those fixtures, or create separate tests

jreback · 2019-03-10T15:41:38Z

i would like a more coherent strategy that just adding singleton fixtures, so better to show that a fixture is useful across multiple files first.

h-vetinari · 2019-03-10T17:03:33Z

@jreback
I get that #22236 and #22471 are a while back - your hard requirement was fixturization of all frame tests. All the existing fixtures already exists as "quasi-fixtures" in tests.frame.common.TestData, and #22471 is purely for translating.

The fact that they are attributes of TestData is enough of a reason to translate them to fixtures (which #22236 did, until @jbrockmendel deleted some that weren't used yet due to the "good first issue" losing its drive after a while).

jreback · 2019-03-10T17:11:44Z

@h-vetinari I think the approach here needs to change. Let's put fixtures inside the modules that use them. Once they are used by at least 2 but preferable more test modules, then we can pull them out.

The problem is we have a hard time discovering cross modules what fixtures are useful an which are not. Since the code currently is not updated to use fixtures at all.

h-vetinari · 2019-03-10T17:13:06Z

@jreback
Can't do 3 things at the same time reasonably.

You're already requesting to do fixturization and renaming at the same time, let's keep the reorg for later. Also, case in point about the necessity of (some of) the fixtures re-added here: #25635

jreback · 2019-03-10T17:15:35Z

+1 on reorg of fixtures later

h-vetinari · 2019-03-11T07:26:00Z

@jreback @WillAyd
Thanks for all the reviews. This one seems to have slipped through the last round. ;-)

WillAyd · 2019-03-19T04:25:55Z

pandas/tests/frame/conftest.py

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


+@pytest.fixture
+def datetime_series():
+    """


Yea I would agree with this comment. It also seems strange to put a fixture in this conftest which returns a series, as that could either create duplication or friction with the conftest for the series directory.

Any way to do what @jreback suggests above here?

h-vetinari

PTAL

h-vetinari · 2019-03-19T06:58:34Z

pandas/tests/frame/conftest.py

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


+@pytest.fixture
+def datetime_series():
+    """


@h-vetinari: @jreback Can't do 3 things at the same time reasonably.
You're already requesting to do fixturization and renaming at the same time, let's keep the reorg for later. Also, case in point about the necessity of (some of) the fixtures re-added here: #25635

@jreback: +1 on reorg of fixtures later

@WillAyd, of course both of you are right. But the quasi-fixtures from tests.frame.common.TestData that are supposed to be translated already have this (as self.ts1).

I guess I should have pushed back harder against #24885, because datetime_series was already in tests.frame.conftest (since #22236) precisely for this translation. There's a limit to how many balls any one PR should have up in the air, and fixturization plus renaming is more than enough IMO.

jreback · 2019-04-20T17:35:17Z

if you remove the datetime_series fixture and rebase would consider

h-vetinari · 2019-05-30T15:20:58Z

@jreback
This is updated and green. About the datetime_series fixture, please see my comment (that you had already agreed to), that these PRs should be translation only, and not rewrite tests:

jreback · 2019-06-27T03:38:05Z

well, this would need a rebase and remove the added fixture; I am not sure I said to keep that fixtures, its pretty useless.

h-vetinari · 2019-06-28T06:59:31Z

Thanks for taking care of this!

jreback · 2019-06-28T12:13:26Z

thanks @h-vetinari

Fixturize tests/frame/test_axis_select_reindex.py

aa312da

jreback requested changes Mar 10, 2019

View reviewed changes

jreback added the Testing pandas testing functions or related to the test suite label Mar 10, 2019

h-vetinari mentioned this pull request Mar 10, 2019

TST/CLN: remove TestData from frame-tests; replace with fixtures #22471

Closed

34 tasks

h-vetinari added 2 commits March 10, 2019 19:47

Review (jreback)

949e518

Lint

a4bb468

h-vetinari mentioned this pull request Mar 11, 2019

Fixturize tests/frame/test_constructors.py #25635

Merged

WillAyd requested changes Mar 19, 2019

View reviewed changes

h-vetinari commented Mar 19, 2019

View reviewed changes

h-vetinari added 2 commits May 30, 2019 15:42

Merge remote-tracking branch 'upstream/master' into fixturize_frame_axis

5f605ff

lint

0d4333b

jreback added 2 commits June 27, 2019 18:31

Merge branch 'master' into PR_TOOL_MERGE_PR_25627

62f7a78

clean and split

8afc817

jreback added this to the 0.25.0 milestone Jun 27, 2019

jreback approved these changes Jun 27, 2019

View reviewed changes

jreback merged commit bd72942 into pandas-dev:master Jun 28, 2019

h-vetinari deleted the fixturize_frame_axis branch June 28, 2019 12:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixturize tests/frame/test_axis_select_reindex.py #25627

Fixturize tests/frame/test_axis_select_reindex.py #25627

h-vetinari commented Mar 10, 2019

codecov bot commented Mar 10, 2019

codecov bot commented Mar 10, 2019 •

edited

Loading

jreback Mar 10, 2019

WillAyd Mar 19, 2019

h-vetinari Mar 19, 2019 •

edited

Loading

jreback Mar 10, 2019

jreback commented Mar 10, 2019

h-vetinari commented Mar 10, 2019

jreback commented Mar 10, 2019

h-vetinari commented Mar 10, 2019

jreback commented Mar 10, 2019

h-vetinari commented Mar 11, 2019

WillAyd Mar 19, 2019

h-vetinari left a comment

h-vetinari Mar 19, 2019 •

edited

Loading

jreback commented Apr 20, 2019

h-vetinari commented May 30, 2019

jreback commented Jun 27, 2019

h-vetinari commented Jun 28, 2019

jreback commented Jun 28, 2019

Fixturize tests/frame/test_axis_select_reindex.py #25627

Fixturize tests/frame/test_axis_select_reindex.py #25627

Conversation

h-vetinari commented Mar 10, 2019

codecov bot commented Mar 10, 2019

Codecov Report

codecov bot commented Mar 10, 2019 • edited Loading

Codecov Report

jreback Mar 10, 2019

Choose a reason for hiding this comment

WillAyd Mar 19, 2019

Choose a reason for hiding this comment

h-vetinari Mar 19, 2019 • edited Loading

Choose a reason for hiding this comment

jreback Mar 10, 2019

Choose a reason for hiding this comment

jreback commented Mar 10, 2019

h-vetinari commented Mar 10, 2019

jreback commented Mar 10, 2019

h-vetinari commented Mar 10, 2019

jreback commented Mar 10, 2019

h-vetinari commented Mar 11, 2019

WillAyd Mar 19, 2019

Choose a reason for hiding this comment

h-vetinari left a comment

Choose a reason for hiding this comment

h-vetinari Mar 19, 2019 • edited Loading

Choose a reason for hiding this comment

jreback commented Apr 20, 2019

h-vetinari commented May 30, 2019

jreback commented Jun 27, 2019

h-vetinari commented Jun 28, 2019

jreback commented Jun 28, 2019

codecov bot commented Mar 10, 2019 •

edited

Loading

h-vetinari Mar 19, 2019 •

edited

Loading

h-vetinari Mar 19, 2019 •

edited

Loading