TST/CLN: break up & parametrize tests for df.set_index
State: closed

While working on #22225 I had the strong urge to clean up the tests for `df.set_index` (to be able to work off of them for `series.set_index`)

Since the diff is pretty busted, here's a description. In `tests/frame/test_alter_axes.py`, I did:
- externalised an often-used df as a fixture
- broke up `test_set_index2` into several pieces
- renamed `test_set_index_bug` to `test_set_index_after_mutation` (following corresponding GH issue)
- renamed `test_set_index_nonuniq` to `test_set_index_verify_integrity` (also added a MI-case)
- strongly parametrized `test_set_index_pass_arrays` and `test_set_index_duplicate_names`, including several combinations and cases that were not tested before
- replaced everything `pd.` with direct imports (best practice according to review in #21899):
  > don’t import pd, directly import instead
- replaced `assert_series_equal` etc. with `tm.assert_series_equal` (best practice according to review in #21899):
  > use tm; don’t import assert_series_equal
- cleaned up the other tests a bit too, including several open TODOs and getting rid of the `check_names=False` in several cases.

I also added better warnings in case there are duplicate column labels in `keys`, and some corresponding tests.

Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22236?src=pr&el=h1) Report
> Merging [#22236](https://codecov.io/gh/pandas-dev/pandas/pull/22236?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/0976e1261781153603064eb67d66630a099edcd7?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/22236/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/22236?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master   #22236   +/-   ##
=======================================
  Coverage   92.17%   92.17%           
=======================================
  Files         169      169           
  Lines       50708    50708           
=======================================
  Hits        46740    46740           
  Misses       3968     3968
```

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


------

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

Hello @h-vetinari! Thanks for updating the PR.





Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on August 31, 2018 at 20:35 Hours UTC
@jreback Don't why my comments aren't showing up here - copying them to the thread.

> no that is a very bad pattern. we don't want to do that. pls use fixtures instead

I don't think it's fair to blame me for following an existing pattern - just look at

```
    @cache_readonly
    def simple(self):
        arr = np.array([[1., 2., 3.],
                        [4., 5., 6.],
                        [7., 8., 9.]])

        return pd.DataFrame(arr, columns=['one', 'two', 'three'],
                            index=['a', 'b', 'c'])
```
directly above (https://github.com/pandas-dev/pandas/blob/v0.23.4/pandas/tests/frame/common.py#L97-104)

Do you want me to change all of test_alter_axes from classes to functions? That's the only other (clean) way I can think of.

> this is obtuse, I can't tell what you are doing. if you need something create it where you need it.

Refactored. Hope you like it better like this - the advantage is that it stays local and that the generated test names are understandable
@jreback 

I translated all the "fixture"-attributes from `TestData` to `conftest.py`, but I only replaced them in `test_alter_axes`, because they occur just too frequently to do it in one go. ;-)
@jreback All green, and should be good to go.
@jreback Another ping :)
i will look soon
we have gotten a ton of PRs lately

@jreback I know you're busy, but you already said "ok looks closer a few more comments", and the changes since then are easy: just the fixtures you asked for. Would like to get this through as it's blocking two other PRs.
@jreback 
> huh? pls use the ABC version, we do this everywhere else. To be very honest I would split this PR up into 2 parts as I think you are actually changing something here but its very very hard to tell.

Fair enough, this was actually a good intuition. I split out all the new things into a new issue/PR (#22484 #22486), and reverted all the new warnings here.
@jreback 

> > huh? pls use the ABC version, we do this everywhere else. To be very honest I would split this PR up into 2 parts as I think you are actually changing something here but its very very hard to tell.

> Fair enough, this was actually a good intuition. I split out all the new things into a new issue/PR (#22484 #22486), and **reverted all the new warnings here**.

All green now.
@jreback 
Hope there's less open questions around this PR ;-)
Also blocking a couple follow-ups...
@jreback 
All feedback is incorporated (split off new warnings into separate PR). In fact, I also preempted the fixture-docstrings you required in #22526: 
> the point is new features need a doc-string

I'd suggest *not* to rename them right away, as then #22471 would be much more tedious to carry out, but I included a note about that plus a name suggestion per fixture. Happy to hear feedback on that.

I've been responding quickly to any and all reviews, and happy to make it smaller/bigger/split it, whatever - just wanna get this done ASAP. The diffs since your last review are tiny btw:
* ```https://github.com/pandas-dev/pandas/pull/22236/commits/9121d4912cdcd7f8a7e3336fc385bf00ccab93ad``` - 5 lines
* ```https://github.com/pandas-dev/pandas/pull/22236/commits/15fc11fff5f52811df6b20b5b47fdc115b5b080b``` - 34 lines
* ```https://github.com/pandas-dev/pandas/pull/22236/commits/fb65735bb0b11f0e2266c1153bdd8addc74454dd``` 5 lines
* last commit for docstrings slightly larger, but just easy text
you can rebase this off of #22550 if you want or rebase off of master after merge.
@jreback
> you can rebase this off of #22550 if you want or rebase off of master after merge.

The number you referenced is an issue, I guess you mean #22526? In any case, I can easily rebase afterwards, but there won't be a conflict because these `conftest` files live in different folders (`tests/series` vs. `tests/frame`). This PR deals with the DF-side, #22526 deals with the series side.
@jreback 

#22526 is merged, and there's no conflict here.
@jreback 

The only thing left should be if you like the fixture names I chose (the old and new name for each fixture are also recorded in #22471).
@jreback 

Had to rebase due to unrelated test failures (`ResourceWarning` again...), but there's only one new commit, and not much in it ```https://github.com/pandas-dev/pandas/pull/22236/commits/81ff9c240ae86483ff57a72c9d3a6beecefa7b44```.

Double-checking those fixture name choices should be a triviality - and then we can finally get this over the finish line. :)
@jreback 

Green.

> Double-checking those fixture name choices should be a triviality - and then we can finally get this over the finish line. :)
@jreback 
Can we get this over the line?
Things are getting tight for v0.24 with this -> #22486 -> #22225 (-> #21645)
@h-vetinari i will look at some point
@jreback 
I truly understand that review time is scarce (and that you're probably getting annoyed at my pings here). Still, we've been at
> ok looks closer a few more comments

since over a month, stuck in the following pattern: push trivial fix, wait ~2 weeks for a comment, push trivial fix, wait ~2 weeks for a comment, push trivial fix, etc., and I can't understand why following through on this (within that timeframe) is such a hurdle. Renaming some (still unused!) fixtures is about as simple as it gets.

still applicable:
> I've been responding quickly to any and all reviews, and happy to make it smaller/bigger/split it, whatever - just wanna get this done ASAP.
lgtm. can you rebase to make sure, ping on green for merge.
I've had a `ResourceWarning` ~~twice~~ thrice, and then restarted a third time because AppVeyor isn't triggering for some reason. It didn't start a second time, so I've left it like that ATM.
@jreback Green (after 5 retriggers...)
thanks. in the future, not necessary to push multiple times, just needed a single one, if the failure is unrelated.