TST/CLN: Fixturize frame/test_analytics
State: closed

- [x] 1 step closer towards #22471
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`

This module is **much** harder to fixturize than e.g. #22236 or #22730, mainly due to the class methods `_check_stat_op` and `_check_bool_op`, which, despite having an argument for the frame they're testing, are *also* testing on other quasi-fixtures of `TestData`. Since I can't import directly from `frame/conftest` without getting `RemovedInPytest4Warnings`, I made theses fixtures explicit arguments of the respective methods.

Furthermore, I extracted two fixtures that basically correspond to those methods being called without a `frame` argument, and added them to `conftest`.

The larger question is how to avoid all these redundant calls being made (e.g. in `test_max`), and how `_check_stat_op` / `_check_stat_op` should be properly split up into several tests/parametrizations. So I don't view this PR as ready, but needing discussion regarding how to best proceed.


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

Comments:
Hello @h-vetinari! Thanks for submitting the PR.

 - There are no PEP8 issues in the file [`pandas/tests/frame/conftest.py`](https://github.com/h-vetinari/pandas/blob/f846265c14ad21316d2555911707b77bc4cee8a3/pandas/tests/frame/conftest.py) !

 - There are no PEP8 issues in the file [`pandas/tests/frame/test_analytics.py`](https://github.com/h-vetinari/pandas/blob/f846265c14ad21316d2555911707b77bc4cee8a3/pandas/tests/frame/test_analytics.py) !


@WillAyd Reverted all the offending orthogonal changes
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22733?src=pr&el=h1) Report
> Merging [#22733](https://codecov.io/gh/pandas-dev/pandas/pull/22733?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/d523d9fe056d88c0a80a016123eca0f3ae734ae1?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

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

```diff
@@           Coverage Diff           @@
##           master   #22733   +/-   ##
=======================================
  Coverage   92.19%   92.19%           
=======================================
  Files         169      169           
  Lines       50835    50835           
=======================================
  Hits        46868    46868           
  Misses       3967     3967
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.61% <ø> (ø)` | :arrow_up: |
| #single | `42.35% <ø> (ø)` | :arrow_up: |


------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22733?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/22733?src=pr&el=footer). Last update [d523d9f...c227fa2](https://codecov.io/gh/pandas-dev/pandas/pull/22733?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

@gfyoung @jreback @WillAyd @TomAugspurger
Weird error on Appveyor, could some one please restart?

```
Build started
git clone -q https://github.com/pandas-dev/pandas.git C:\projects\pandas
git fetch -q origin +refs/pull/22733/merge:
git checkout -qf FETCH_HEAD
Running Install scripts
if ($env:APPVEYOR_PULL_REQUEST_NUMBER -and $env:APPVEYOR_BUILD_NUMBER -ne ((Invoke-RestMethod ` https://ci.appveyor.com/api/projects/$env:APPVEYOR_ACCOUNT_NAME/$env:APPVEYOR_PROJECT_SLUG/history?recordsNumber=50).builds | ` Where-Object pullRequestId -eq $env:APPVEYOR_PULL_REQUEST_NUMBER)[0].buildNumber) { ` throw "There are newer queued builds for this pull request, failing early." }
Cannot index into a null array.
At line:1 char:5
+ if ($env:APPVEYOR_PULL_REQUEST_NUMBER -and $env:APPVEYOR_BUILD_NUMBER ...
+     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidOperation: (:) [], RuntimeException
    + FullyQualifiedErrorId : NullArray
 
Command executed with exception: Cannot index into a null array.
```
@WillAyd Green :)
@WillAyd @jreback @gfyoung 

Reverted the underscores for the function parameters again after cross-review in #22730:
@jreback: 
> actually don't think you need to do this as pytest doesn't care

@h-vetinari: 
> This is from #22733, where this was an review request from @WillAyd.
>> Is there any risk here in using arguments identical to the fixture names? Assuming since this method itself is not a test that pytest won't try to inject the fixture here but not sure if that will always be the case

@gfyoung:
> Fixture injection should only happen when we have a function called `test_*`. pytest wouldn't even collect that function at all. Thus, I agree with @jreback to drop underscores.
If we're wrong, IMO that's a design flaw.

@jreback 
> I pushed some commits that just go until moving _check_stat_op and _check_bool_op and making them run. I'll push the rest of the commits once the CI passes.

Kindly asking you to peruse the copy/paste and unindent commits here, if you can follow as required. The rest of the changes (coming after CI passes; or on your OK) will be trivial.
@jreback 
So the CI was successful except a `CondaHTTPError: HTTP 504 GATEWAY TIME-OUT` for `py36_locale_slow`. Pushing the rest of the commits.
@jreback 
PTAL

I know you wanted separation (just moving the functions to module-level), but that's not possible without the fixturization that's carried out in this PR. However, I've really rolled out the red carpet here, the commits are super modular and easy to digest...
@jreback

If you start here (```https://github.com/pandas-dev/pandas/pull/22733/commits/7ac476ec50d3a9b44112c078a61f9455efe93c07```), the commits are very easy to follow, I promise. :)

@h-vetinari as I said above multiple times. I really prefer to do the diff using the tools which git provides. Its not easy to diff commits like this, pls separate this in to multiple PR's. Moving things first, then change. It will make everyone's life easier and speed time to acceptance of PRs.

Ultimately things get squashed into a single commit anyhow, so putting in multiple commits doesn't really help anyone. Multiple PR's on the other hand, are merged separately.
@jreback 
>  I really prefer to do the diff using the tools which git provides

If you don't like the github-tools that allow comparing separate commits (which I linked; and where you can step through one by one), there's always `git diff <hash1> <hash2>`.

Your request of moving the function was orthogonal to this PR - I still accomodated it. If anything could be split off, it's that (i.e. revert the last 4 commits). Similarly, the whole fixturization effort was your request in #22236 (orthogonal there as well) - it's not something I'm gonna fight for. Feel free to reopen if you wanna have a look again.
Alright, now that I have a bit more time again, I'm less reluctant to do some hoop-jumping.

@jreback, I've reverted back to the original purpose of this PR - fixturization. I'll leave making `_check_stat_op` a module-level function for a follow-up.
@h-vetinari ok this looks ok thanks.
@jreback 
Thanks.

> note for later to clean up the names of these fixtures to be meaningful (e.g. float_frame is, simple_frame not so much, prob need to wait to see how much more things ike this are used)

OK, this was part of the process in #22236. Originally it was just called `simple` - `simple_frame` was the closest in spirit. I'd argue it should be renamed ASAP (and update the "translation guide" in #22471), before more modules get translated. What name would you like to have?
@jreback 
What name would you like to have for `simple_frame`? (see comment above)