REF/TST: Stop using singleton fixtures
State: closed

Found when trying to collect reduction tests that we are still using a lot of singleton fixtures (agreed undesirable in #23701).  This gets rid of singleton fixtures in tests.frame

Following this it will be easier to more usefully parametrize some of those tests.

Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: Clean

Comments:
> we are still using a lot of singleton fixtures (agreed undesirable

@jbrockmendel : Indeed, too bad we can't lint these very easily AFAICT...
The test failures look very weird to me.  @gfyoung any ideas?
> The test failures look very weird to me.

@jbrockmendel : I see only one test failure regarding your refactoring of `test_rename_inplace`.  Not sure exactly why it's failing now on those platforms, but the failure doesn't seem completely weird to me.  It's seem to be suggesting that the `.copy` somehow didn't actually create a copy of the object.
> really? this is the entire point of fixtures.

@jreback we discussed this back in #23701 and you agreed "if it’s truly a singleton the. no a fixture is not warranted".  Pls see the discussion there.

> @jreback we discussed this back in #23701 and you agreed "if it’s truly a singleton the. no a fixture is not warranted". Pls see the discussion there.

key poiint. IF its truly a singleton. These are used all over the place, or at least should be. There are many many other files that actually just use TestData that should simply use these fixtures. They just have not been converted. 
Are you two using different definitions of singleton? @jbrockmendel's is (roughly, ignoring things like setup / teardown that don't apply to these), "a non-parametrized fixture". In this case, using a helper method in `tm.` to generate the data seems equivalent to accepting the data as a fixture.

@jreback's definition might be "a fixture that's only used in a single test"?
@TomAugspurger thanks, I think you've got it right on the terminology mismatch.

Even if a fixture is used in many places, for one-liners or otherwise non-parametrized fixtures, we're better off inlining or putting them in pd.util.testing like this PR does.  We get improved copy/paste-ability, reduce pytest lockin, and improve clarity.  As a reader, which of these is clearer:

```
    def test_td64ser_div_numeric_scalar(self, tdser):
        expected = Series(['29.5D', '29.5D', 'NaT'], dtype='timedelta64[ns]')
        result = tdser / 2
        tm.assert_equal(result, expected)

    def test_td64ser_div_numeric_scalar(self):
        tdser = pd.Series(['59 Days', '59 Days', 'NaT'], dtype='timedelta64[ns]')
        expected = Series(['29.5D', '29.5D', 'NaT'], dtype='timedelta64[ns]')
        result = tdser / 2
        tm.assert_equal(result, expected)
```

I claim the latter is clearer by a wide margin.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24769?src=pr&el=h1) Report
> Merging [#24769](https://codecov.io/gh/pandas-dev/pandas/pull/24769?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/6d3565a5181b3d0fb8342e69dd1d21a98ac3d380?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/24769/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/24769?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #24769      +/-   ##
==========================================
+ Coverage   92.38%   92.39%   +<.01%     
==========================================
  Files         166      166              
  Lines       52363    52400      +37     
==========================================
+ Hits        48377    48413      +36     
- Misses       3986     3987       +1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.81% <100%> (ø)` | :arrow_up: |
| #single | `42.9% <21.62%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24769?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/24769/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `88.4% <100%> (+0.3%)` | :arrow_up: |
| [pandas/core/dtypes/common.py](https://codecov.io/gh/pandas-dev/pandas/pull/24769/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2NvbW1vbi5weQ==) | `96.78% <0%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24769?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24769?src=pr&el=footer). Last update [6d3565a...032a74e](https://codecov.io/gh/pandas-dev/pandas/pull/24769?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24769?src=pr&el=h1) Report
> Merging [#24769](https://codecov.io/gh/pandas-dev/pandas/pull/24769?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/fa12b9ecbea36a504249e74579ee2124f98bcc3f?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/24769/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/24769?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #24769      +/-   ##
==========================================
+ Coverage   92.38%   92.39%   +<.01%     
==========================================
  Files         166      166              
  Lines       52402    52439      +37     
==========================================
+ Hits        48414    48450      +36     
- Misses       3988     3989       +1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.81% <100%> (ø)` | :arrow_up: |
| #single | `42.88% <21.62%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24769?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/24769/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `88.43% <100%> (+0.3%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24769?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24769?src=pr&el=footer). Last update [fa12b9e...3b87f34](https://codecov.io/gh/pandas-dev/pandas/pull/24769?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

@jbrockmendel 

> The test failures look very weird to me. @gfyoung any ideas?

if the only change to this test is replacing the fixture then it could be a change to N or K in tm.
in cases like this `pytest --showlocals` would be helpful. you can use print statements to debug and the output should be displayed for failing tests.. that's the default locally.. haven't tried it with the ci  
I can't reproduce this locally.  Can anyone else?
If somehow the test behavior depends on whether a fixture is used, that would be troubling
I can reproduce the failure in 032a74eab1d7894d9c7306859cd5046d6b8726aa locally. And on 2253bdfd5b32debc8dc8f6ca89520bb93b7d625a

Interestingly, it seems to be a flaky test

```
 $ pytest pandas/tests/frame/test_alter_axes.py -k test_rename_inplace --count=5
================================================================================ test session starts ================================================================================
platform darwin -- Python 3.7.2, pytest-4.1.0, py-1.6.0, pluggy-0.7.1
rootdir: /Users/taugspurger/sandbox/pandas, inifile: setup.cfg
plugins: xdist-1.23.2, repeat-0.7.0, forked-0.2, faulthandler-1.5.0, cov-2.6.0, hypothesis-3.59.1
collected 3735 items / 3730 deselected

pandas/tests/frame/test_alter_axes.py .....                                                                                                                                   [100%]

===================================================================== 5 passed, 3730 deselected in 0.60 seconds =====================================================================

 $ pytest pandas/tests/frame/test_alter_axes.py -k test_rename_inplace --count=5
================================================================================ test session starts ================================================================================
platform darwin -- Python 3.7.2, pytest-4.1.0, py-1.6.0, pluggy-0.7.1
rootdir: /Users/taugspurger/sandbox/pandas, inifile: setup.cfg
plugins: xdist-1.23.2, repeat-0.7.0, forked-0.2, faulthandler-1.5.0, cov-2.6.0, hypothesis-3.59.1
collected 3735 items / 3730 deselected

pandas/tests/frame/test_alter_axes.py F....                                                                                                                                   [100%]

===================================================================================== FAILURES ======================================================================================
__________________________________________________________________ TestDataFrameAlterAxes.test_rename_inplace[1/5] __________________________________________________________________

self = <pandas.tests.frame.test_alter_axes.TestDataFrameAlterAxes object at 0x10ea03eb8>

    def test_rename_inplace(self):
        float_frame = DataFrame(tm.getSeriesData())

        float_frame.rename(columns={'C': 'foo'})
        assert 'C' in float_frame
        assert 'foo' not in float_frame

        c_id = id(float_frame['C'])
        float_frame = float_frame.copy()
        float_frame.rename(columns={'C': 'foo'}, inplace=True)

        assert 'C' not in float_frame
        assert 'foo' in float_frame
>       assert id(float_frame['foo']) != c_id
E       assert 4519856224 != 4519856224
E        +  where 4519856224 = id(3iGk6Mrg5P   -0.684722\nafMjhpeaJk   -1.726612\nlHGFsvRhY6    0.868385\n44rJSOpKPB    0.413032\ndBoNJu9vYI    0.017283\nHT4...4\ngwBsqTWequ    0.262972\nmBUNfZKGWs   -0.194150\nxNhGyHIH99    1.499986\nXnHXz6qUoM   -0.264342\nName: foo, dtype: float64)

pandas/tests/frame/test_alter_axes.py:693: AssertionError
================================================================ 1 failed, 4 passed, 3730 deselected in 0.85 seconds ================================================================
```

Just had 6/100 fail.
On the PR itself, I personally don't care about copy/paste-ability, and am not too concerned with pytest lock-in.

I do care about how long the test suite takes, so reducing that without sacrificing readability / maintainability is a win. And to me, a simple fixture that just provides a value (no setup / teardown) is equivalent to a method call inside the test.
i am -1 on this change - this goes against the entire maxim of using fixtures 

this just puts more distance between what is being tested and construction

this is just reinventing the pytest and is a giant leap backwards 
> this just puts more distance between what is being tested and construction

How are you measuring distance? It's just a function call either way. And, depending on your editor, this may be less "distance" since goto definition typically works for function calls, but not necessarily for fixtures.
> this just puts more distance between what is being tested and construction

Um... the opposite of that.  In many cases the way the PR does it doesn't even have to call into `tm`, just inlines the construction.

What it _does_ do is decrease the ambiguity about where something is defined.

> this is just reinventing the pytest and is a giant leap backwards

Since when did pytest invent inlining code or function calls?

By this logic, _every_ object we use in more than one test should be made into a fixture.  Where would you draw the line and why?

> I do care about how long the test suite takes, so reducing that without sacrificing readability / maintainability is a win. And to me, a simple fixture that just provides a value (no setup / teardown) is equivalent to a method call inside the test.

For the places where the fixtures are inlined instead of moved into `tm`, there is a strict decrease in the function call overhead (though I doubt it is big enough to be measurable).  For others it should balance out exactly.

>  so reducing that without sacrificing readability / maintainability is a win

I find the readability strictly improved by this.  It also makes it easier to figure out what refactoring opportunities are available, which has other maintainability/perf upside.

---
In the status quo there are a handful of fixtures in `tests.frame.conftest` that aren't used at all.  In other cases tests have a fixture in the arguments but never actually use it; the linting doesn't detect that.
> For the places where the fixtures are inlined instead of moved into tm, there is a strict decrease in the function call overhead (though I doubt it is big enough to be measurable). For others it should balance out exactly.

Just to be clear, I was talking about reduced setup time from avoiding fixtures (btw, do you have rough at how much this shaves off? I suppose a `pytest --collect-only` would show it?)
> Just to be clear, I was talking about reduced setup time from avoiding fixtures (btw, do you have rough at how much this shaves off? I suppose a pytest --collect-only would show it?)

N=5 they are indistinguishable.

Incidentally, for me the collection takes about 12% of test runtime (with parallelization).  I think some of the way to cut down on that is to push back on over-parametrization.
just had this ci failure..

```
    def test_setindex(self, string_series):
        # wrong type
        msg = (r"Index\(\.\.\.\) must be called with a collection of some"
               r" kind, None was passed")
        with pytest.raises(TypeError, match=msg):
            string_series.index = None
    
        # wrong length
        msg = (r"Length mismatch: Expected axis has 30 elements, new values"
               r" have 29 elements")
        with pytest.raises(ValueError, match=msg):
>           string_series.index = np.arange(len(string_series) - 1)
E           AssertionError: Pattern 'Length mismatch: Expected axis has 30 elements, new values have 29 elements' not found in 'Length mismatch: Expected axis has 100 elements, new values have 99 elements'
pandas/tests/series/test_alter_axes.py:28: AssertionError
___________ TestSeriesConstructors.test_constructor_dtype_datetime64 ___________

```
so I think that tm.N and tm.K take on different values depending on the order that the tests are run.
Seems like we have tests that set those, which seems iffy

```
$ ag 'tm.N ='
doc/source/reshaping.rst
18:   tm.N = 3
43:   tm.N = 3

pandas/tests/test_multilevel.py
50:        tm.N = 100

pandas/tests/plotting/test_datetimelike.py
317:        tm.N = 300
319:        tm.N = n

pandas/tests/indexing/multiindex/conftest.py
24:    tm.N = 100
```
@TomAugspurger agreed that altering tm.K and tm.N is not great.  But I don't see how that would change the affected test's behavior, do you?

> Interestingly, it seems to be a flaky test

float-frame defined in function..

```
$ pytest pandas/tests/frame/test_alter_axes.py -k test_rename_inplace --count=100 --tb=no
============================= test session starts =============================
platform win32 -- Python 3.7.2, pytest-4.1.1, py-1.7.0, pluggy-0.8.1
hypothesis profile 'ci' -> deadline=500, timeout=unlimited, suppress_health_check=[HealthCheck.too_slow], database=DirectoryBasedExampleDatabase('C:\\Users\\simon\\OneDrive\\code\\pandas-simonjayhawkins\\.hypothesis\\examples')
rootdir: C:\Users\simon\OneDrive\code\pandas-simonjayhawkins, inifile: setup.cfg
plugins: xdist-1.26.0, repeat-0.7.0, forked-0.2, cov-2.6.1, hypothesis-4.0.1
collected 49500 items / 49400 deselected

pandas\tests\frame\test_alter_axes.py .F.....FF.....FFF......F.......FFF [ 34%]
......F.......FFF......F.......FF......F.....F.....F....F...F.....       [100%]

=========== 22 failed, 78 passed, 49400 deselected in 13.29 seconds ===========
```

float-frame fixture..

```
$ pytest pandas/tests/frame/test_alter_axes.py -k test_rename_inplace --count=100 --tb=no
============================= test session starts =============================
platform win32 -- Python 3.7.2, pytest-4.1.1, py-1.7.0, pluggy-0.8.1
hypothesis profile 'ci' -> deadline=500, timeout=unlimited, suppress_health_check=[HealthCheck.too_slow], database=DirectoryBasedExampleDatabase('C:\\Users\\simon\\OneDrive\\code\\pandas-simonjayhawkins\\.hypothesis\\examples')
rootdir: C:\Users\simon\OneDrive\code\pandas-simonjayhawkins, inifile: setup.cfg
plugins: xdist-1.26.0, repeat-0.7.0, forked-0.2, cov-2.6.1, hypothesis-4.0.1
collected 49500 items / 49400 deselected

pandas\tests\frame\test_alter_axes.py .................................. [ 34%]
..................................................................       [100%]

================ 100 passed, 49400 deselected in 13.20 seconds ================
```

change code to ...
```
    def test_rename_inplace(self):
        float_frame = DataFrame(tm.getSeriesData())

        float_frame.rename(columns={'C': 'foo'})
        assert 'C' in float_frame
        assert 'foo' not in float_frame

        c_id = id(float_frame['C'])
        float_frame2 = float_frame.copy()
        float_frame2.rename(columns={'C': 'foo'}, inplace=True)

        assert 'C' not in float_frame2
        assert 'foo' in float_frame2
        assert id(float_frame2['foo']) != c_id
```
```
$ pytest pandas/tests/frame/test_alter_axes.py -k test_rename_inplace --count=100 --tb=no
============================= test session starts =============================
platform win32 -- Python 3.7.2, pytest-4.1.1, py-1.7.0, pluggy-0.8.1
hypothesis profile 'ci' -> deadline=500, timeout=unlimited, suppress_health_check=[HealthCheck.too_slow], database=DirectoryBasedExampleDatabase('C:\\Users\\simon\\OneDrive\\code\\pandas-simonjayhawkins\\.hypothesis\\examples')
rootdir: C:\Users\simon\OneDrive\code\pandas-simonjayhawkins, inifile: setup.cfg
plugins: xdist-1.26.0, repeat-0.7.0, forked-0.2, cov-2.6.1, hypothesis-4.0.1
collected 49500 items / 49400 deselected

pandas\tests\frame\test_alter_axes.py .................................. [ 34%]
..................................................................       [100%]

================ 100 passed, 49400 deselected in 11.97 seconds ================
(pandas-dev)
```

so it appears to be due to re-use of memory space after dereferencing 

@jreback @jbrockmendel @simonjayhawkins 
There reason most of the fixtures coming out of `TestData` are singletons at all is that there is an ongoing (*cough*) transition from the old `TestData`-attributes-as-fixtures towards proper fixtures, see #22236 (where @jreback asked me to open the following) #22471 #22550.

I haven't touched these issues that much anymore, since it was supposed to be a "good first issue" (and had some response along those lines for a while), and I was busy with other PRs.
100/100 pass rate with
```
    @pytest.mark.parametrize('float_frame', [DataFrame(tm.getSeriesData())])
    def test_rename_inplace(self, float_frame):
        float_frame.rename(columns={'C': 'foo'})
```

also with holding a reference to original object
```
    def test_rename_inplace(self):
        float_frame = DataFrame(tm.getSeriesData())
        float_frame_orig = float_frame

        float_frame.rename(columns={'C': 'foo'})
        assert 'C' in float_frame
        assert 'foo' not in float_frame

        c_id = id(float_frame['C'])
        float_frame = float_frame.copy()
        float_frame.rename(columns={'C': 'foo'}, inplace=True)

        assert 'C' not in float_frame
        assert 'foo' in float_frame
        assert id(float_frame['foo']) != c_id
        assert float_frame is not float_frame_orig
```
> 100/100 pass rate with

Is this on a platform/version that was previously failing?  we've still got three failing builds
> > 100/100 pass rate with
> 
> Is this on a platform/version that was previously failing? we've still got three failing builds

yes, i'm running locally on windows and getting failures with a copy of your branch. 

```
$ git log
commit fde7cd88b23a4711398e4708fac29a95984fe16f (HEAD -> jbrockmendel-singletons)
Author: Brock Mendel <jbrockmendel@gmail.com>
Date:   Sun Jan 20 10:28:14 2019 -0800

    See if removing duplicate fixture is OK now...
```

four methods of preventing dereferencing always pass. fixture, @pytest.mark.parametrize, assigning original object to another variable and using different variable for the copy.

before the fixture was created, the float-frame object would have been created outside the function. so it makes sense that before the fixture was created that a reference would have been kept to the float-frame object and the test passed.
Thanks for digging into this.

>  so it makes sense that before the fixture was created that a reference would have been kept to the float-frame object and the test passed.

I think I follow.  For the time being I'm going to use the `float_frame_orig` edit.