CLN: res/exp and GH references in frame tests
State: closed

EDIT2: reverted import-related clean-up due to https://github.com/pandas-dev/pandas/pull/22730#issuecomment-423844217

EDIT: after review by @WillAyd, the fixturization was split out to #22735, #22736 and #22738. Now contains only the following clean-ups, also for #22733:
* Replace instances of pd. with direct imports (from review https://github.com/pandas-dev/pandas/pull/21899#discussion_r202524353):
  > don’t import pd, directly import instead
* Replace direct use of `assert_..._equal` with the `tm.assert_..._equal`
    (from review https://github.com/pandas-dev/pandas/pull/21899#discussion_r202524374):
  > use tm; don’t import assert_series_equal
* Clean up usage of `exp/res` (from review https://github.com/pandas-dev/pandas/pull/22733#discussion_r218306730):
  > Here you can use `result` and `expected` in spite of the existing code using `exp`
* Unify references to github issues

Original post below. [END EDIT]

- [x] 3 modules of #22471
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`

I split the commits per module, and (where possible) into fixturizing and cleaning up the imports. Other notes:
* For `test_apply`, I added an another often-used fixture
* In translating the quasi-fixtures from `TestData` to `conftest` in #22236, I sorted the dtypes for the columns of `mixed_float_frame` and `mixed_int_frame`, which turns out to have been a mistake. This is reverted here to be a true translation of the attribute of `TestData`. Otherwise, tests in the newly fixturized `test_arithmetic.py` would fail.

En passant, I also unified GH issue references.


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/5d16b52b9a89f639cb89795e2c672886cee632ab/pandas/tests/frame/conftest.py) !

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

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

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


Can't figure out ATM why pytest cannot find some of the fixtures. Everything works fine locally...
I reverted the changes in `test_api` for now, because I couldn't get `SharedWithSparse` to work for `tests/sparse/frame/test_frame.py`, even with a new `conftest.py` there. It's nevertheless interesting that some builds (https://travis-ci.org/pandas-dev/pandas/jobs/429371967, https://circleci.com/gh/pandas-dev/pandas/20582) worked out. Not sure if those jobs are skipping sparse tests...?
Even if things were done in separate commits it's still makes things harder than it needs to be to have to step through them, especially as changes / edits get made over time. Again if you could do away with anything not directly relevant to "fixturization" would be very helpful and can take another look after that
@WillAyd It's something I'll take into account for future PRs. I make best-effort attempts considering the information I've gotten so far; your review contradicts that somewhat. Nevertheless, I've (grudgingly) split off #22735 and #22736.
as a general rule when changing tests

change only tests (no code)
if moving things - separate PR, don’t mix moving and changing

smaller is better - test changes have to be carefully scrutinized to make sure things are not breaking

@jreback 

Now as small and focused as possible after split into #22735 #22736 #22738. Will keep this PR around for doing the import clean-ups after merge.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22730?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@66c2e5f`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `n/a`.

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

```diff
@@            Coverage Diff            @@
##             master   #22730   +/-   ##
=========================================
  Coverage          ?   92.18%           
=========================================
  Files             ?      169           
  Lines             ?    50830           
  Branches          ?        0           
=========================================
  Hits              ?    46860           
  Misses            ?     3970           
  Partials          ?        0
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.6% <ø> (?)` | |
| #single | `42.37% <ø> (?)` | |


------

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

This is now based on #22733, #22735, #22736 and #22738. The commits are nice and modular so reviewing should be easy (once the remaining two PRs are merged).
Before we do any more of this

> Replace instances of pd. with direct imports (from review #21899 (comment)):
don’t import pd, directly import instead

> Replace direct use of assert_..._equal with the tm.assert_..._equal
(from review #21899 (comment)):
use tm; don’t import assert_series_equal

I would like alint rule to see what is what (and basically list the offenders as exclusions for now).

 We have been changing things like this for a long time and I think we somtimes change them back (e.g. to use the imports of ``assert_*``, rather than ``tm.``, and to use the ``pd.*`` rather than the direct imports.

Please remove these for now (or split out to a separate PR).


@jreback 
> I would like alint rule to see what is what (and basically list the offenders as exclusions for now).

There just needs to be a clear directive - also to write a lint rule. Furthermore, as far as I can tell from the test modules I've seen, almost all would fail both versions of that lint rule.
> There just needs to be a clear directive - also to write a lint rule. Furthermore, as far as I can tell from the test modules I've seen, almost all would fail both versions of that lint rule.

of course, but that is exactly the point. let's see which iway is more common, then have a lint rule to enforce it, rather than ad-hoc conversions (which has been the policy up till now).
> of course, but that is exactly the point. let's see which iway is more common, then have a lint rule to enforce it, rather than ad-hoc conversions (which has been the policy up till now).

Fair enough, but that's above my pay grade. Reverting the import-related commits.
can you limit the scope of this PR as it seems to overlap greating with your other one on test_analytics. I prefer PR's that change 1 thing generally; these will be reviewed an merged much faster that longer complicated ones.
@jreback 
> can you limit the scope of this PR as it seems to overlap greating with your other one on test_analytics. 

See OP and https://github.com/pandas-dev/pandas/pull/22730#discussion_r219712070:
> [...] fixturization was split out to #22735, #22736 and #22738. Now contains only the following clean-ups, also for #22733:

> Better to review the parts for test_analytics and test_arithmetic after #22733 and #22736.
@WillAyd @jreback @gfyoung 

Ready again to review. Also, did someone maybe change (reduce?) my GH privileges. I can't merge fixed conflicts anymore, and can't comment on a PR without simultaneously closing it.
> Ready again to review. Also, did someone maybe change (reduce?) my GH privileges. I can't merge fixed conflicts anymore, and can't comment on a PR without simultaneously closing it.

hah, not possible. 

> I can't merge fixed conflicts anymore,

what does this mean?
> > I can't merge fixed conflicts anymore,
> what does this mean?

I fixed the merge conflict in the github UI, but there was no button to actually merge them. I've already restarted my browser and everything, but my "comment" button has gone (leaving only "close and comment"). Really annoying.
> > I can't merge fixed conflicts anymore,

> what does this mean?

I fixed the merge conflict in the github UI, but there was no button to actually merge them. I've already restarted my browser and everything, but my "comment" button has gone (leaving only "close and comment"). Really annoying.

EDIT: and also, I can't edit posts anymore.
@h-vetinari I am surprised you use the github UI to do *anything*. I have no idea what controls are on that, generally much better to use the command line.
lgtm. @WillAyd merge when satisfied.
> @h-vetinari I am surprised you use the github UI to do anything. I have no idea what controls are on that, generally much better to use the command line.

For tiny conflicts (handful of lines) it's quicker for me. Rest I do manually or in IDE. However, you might wanna click on a SHA hash in one of these threads sometimes, especially if there's many commits - it allows you to easily browse through the commits one by one (which is how I split up #22733).
thanks @h-vetinari !