TST: restructure internal extension arrays tests (split between /arrays and /extension)
State: closed

Pull request to discuss what to do with the tests for internal EAs (and one of the comments I still had in https://github.com/pandas-dev/pandas/pull/21160)

Basically, I would keep the `tests/extension/..` only for subclassing the base extension array test suite, and any array-specific functionality is tested in `tests/arrays/..` (eg closed attribute for IntervalArray, specific arithmetic behaviour for IntegerArray, ...)

This means that when adding a test related to EAs, we need to think about: is this testing something that is applicable to all EAs? (-> add a base test to `tests/extension/base` so this is tested for all internal and external EAs) or is this testing something specific to a particular EA? (-> add a test in `tests/array/EAtype/..`) 
Of course often there can be some ambiguity here.

Main reason that I would split them is that over time, we probably add a lot of EA-type-specific tests, and then keeping the general ones mixed with the specific ones will make it only confusing / hard to see what is going on. Drawback is of course that it is tested in two places. 
In practice what I propose in this PR, is also what we already do for Categorical at the moment: Categorical has its own tests in `tests/arrays/categorical` (and probably also some in `indexes` and `frame`, ..), but we also run the base extension tests for Categorical in `tests/extension/`

Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: ExtensionArray
description: Extending pandas with custom dtypes or arrays.

Comments:
General note: I didn't really think about how to structure the new `arrays/test_integer.py` and `test_interval.py` files, for now simply copied things from the extension tests. In the future we should restructure that (and if there will be added more, probably makes sense to have a subdir for each array type with multiple files)
+1 to your proposed restructure.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22026?src=pr&el=h1) Report
> Merging [#22026](https://codecov.io/gh/pandas-dev/pandas/pull/22026?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/e2e1a1051576a48f210ce17272fc24b90ebcf24a?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/22026/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/22026?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master   #22026   +/-   ##
=======================================
  Coverage   92.04%   92.04%           
=======================================
  Files         169      169           
  Lines       50782    50782           
=======================================
  Hits        46742    46742           
  Misses       4040     4040
```

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


------

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

@jreback @jbrockmendel @jschendel any comments?
I think this is terribly confusing

where do so yes something is in 2 places

I would much be all internal arrays in there entirely to pandas/tests/arrays

and leave the base classes and inherited machinery in pandas/tests/extension


> I would much be all internal arrays in there entirely to pandas/tests/arrays
> and leave the base classes and inherited machinery in pandas/tests/extension

So you mean leaving the actual test base classes in `pandas/test/extension` but inheriting from them and overwriting certain tests in `pandas/tests/array`? How is that not more confusing to what I proposed?
> So you mean leaving the actual test base classes in pandas/test/extension but inheriting from them and overwriting certain tests in pandas/tests/array? How is that not more confusing to what I proposed?

I would simply move test_integer, test_interval, and test_categorical in there entirety. All the tests are then in 1 place. It is immateterial that a test is inherited or not. In fact I didnt' respond to your comments above, but that IS the reason I purposely write the tests in the way I did. When debugging you want to change a single line and not even think / worry about any testing artifices (e.g. the fact that most tests are automatically run on your behalf and thus inherited).
Sorry, Jeff, I don't understand your comment. Can you try to clarify it? 

> It is immateterial that a test is inherited or not.

What does this mean? You mean that you don't find it relevant or confusing that tests in `pandas/tests/array` are inheriting from tests in `pandas/test/extension` ?

> In fact I didnt' respond to your comments above, but that IS the reason I purposely write the tests in the way I did.

You mean the fact that `get_op_from_name` is used in both places (my comment https://github.com/pandas-dev/pandas/pull/22026#discussion_r204437425)? That is currently the *only* duplication (10 lines of code), so I don't think this is any blocker for any decision, we can easily create a common utility function for that if needed.

> When debugging you want to change a single line and not even think / worry about any testing artifices.

Can you give a concrete example or clarify this? 
How is having base tests in one directory and the inherited tests / overwritten methods in another directory helping with this? That would exactly mean that changing a line in one place can have consequences for tests located somewhere else, which makes it IMO worse. 


To be clear: my proposal is to keep only the most basic tests as possible in `pandas/tests/extension`, so eg only test there that the Base extension tests actually make sense using the internal extension arrays, not to have it as exhaustive tests of the internal extension array. 
I *could* move `pandas/tests/extension/integer/test_integer.py` to for example `pandas/tests/array/integer/test_base_extension.py`, but still keeping the 'real' integer array tests separate from the inherited extension base test class tests. In such a case I only don't see the added value to move them over there.
well that’s the issue

i couldn’t find an obvious split which makes it clear of what belongs where 
> i couldn’t find an obvious split which makes it clear of what belongs where

Well, I propose here a possible, IMO relatively clear, split
> Well, I propose here a possible, IMO relatively clear, split

Agreed. When I'm working on the interface, I want to be able to run it against all the test and public EAs, so I'd like to run `pytest pandas/tests/extensions`. There's no need to run EA-specific things like the Categorical constructor tests when working on the EA interface.
this is not clear at all
i don’t think it adds value 
rather it makes the question of where to put things come up
Interface test under extensions.

Other tests under arrays.

________________________________
From: Jeff Reback <notifications@github.com>
Sent: Wednesday, July 25, 2018 6:23:55 AM
To: pandas-dev/pandas
Cc: Tom Augspurger; Comment
Subject: Re: [pandas-dev/pandas] TST: restructure internal extension arrays tests (split between /arrays and /extension) (#22026)


this is not clear at all
i don’t think it adds value
rather it makes the question of where to put things come up

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<https://github.com/pandas-dev/pandas/pull/22026#issuecomment-407721626>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABQHIkGa4t9zZ6i4DTSbajxHWk5PZlF1ks5uKFVLgaJpZM4VbJ4J>.

The current organisation is for sure not clear as well. 

For example, when actually doing the reorganisation in this PR, I had to look at lot which tests were in the base classes that were or were not overridden, which made it very complicated to work with those tests. If, eg in the TestArithmeticOps of the integer array tests, you need to override almost everything from the base class, there is something wrong (and in fact, it was also not needed as I show in this PR). 

Also, for the integer array, we are going to add a lot more tests while we develop it further (proper reduction, groupby, overflow handling, ...) that all will have a lot of integer-array specificities. Further mixing tests for this with Base test classes that are not meant for such specificities, will further complicate this IMO. 

As Tom summarized, the proposed split: in the extension tests we only run the existing interface tests (the base classes) and only do the minimum to get those passing for eg IntegerArray (see this PR), everything else that is specific about integer arrays is tested in `tests/arrays/integer`. 

So if you really think that this is "terribly confusing", then please try to give an alternative proposal.




@jorisvandenbossche its up to you to give a new proposal. I think this confusing. If I am confused, think about the average contributor. 
> its up to you to give a new proposal.

He has, with this PR!

Are you -1 here? Otherwise I don't think it's worth trying to explain again.
yes I am -1 here. I think the existing test structure is fine.
Doing this PR I had a very hard time understanding the tests with the current structure. "Think about the average contributor" then.

I *give* a proposal. 
I claim (and another core dev seems to think the same) that this is at least *less confusing* than the current test structure. If you don't agree, it is up to *you* to provide at least arguments *why* the current structure is better, or *why* you don't find this an improvement. For now I don't see any arguments (at least that I understand) from your side.



Maybe it's confusing since you wrote the original tests, and put them where they made sense to you, even if it doesn't make sense to others?

Anyway, let's get some other opinions to test that: @WillAyd, @jschendel, @toobaz, @jbrockmendel, does the rule "Interface tests go in `pandas/tests/extension`, dtype-specific tests go in `pandas/tests/arrays/<array>`" make sense to you? 
And I am -1 on your proposal.  It is so non-obvious where tests should go, or which ones that I need to run to debug things. Now I run a single directory ``pandas/tests/extensions``.

The burden is on the PR to avoid uneeded complexity. This does not pass this test.
>  It is so non-obvious where tests should go

Can you then explain what you understand as the *current* "rules" ? 

(because for me those are not clear: adding it as a new test in one of the base classes vs overwrite an existing test vs adding new tests to the inherited classes (but then you also get a strange split of tests) vs adding new classes for things that cannot be tested for all EAs in general ....)
As I mentioned above (https://github.com/pandas-dev/pandas/pull/22026#issuecomment-407707694), I am OK with moving the "interface tests" to `tests/arrays/` as well, but keep them in a separated file. 
That makes that you can do `pytest pandas/tests/arrays/integer/` to run *all* integer array tests, but still separates the "generic interface tests" (the base classes) and array-specific tests. 

(however, this has the drawback that if you then change something in the EA interface / base tests, you need to run both `tests/arrays/` as `tests/extension/`. I think that would be mainly annoying for small changes (eg a change in one of the base test classes), as for more fundamental changes in the EA interface you will need to do that anyway)
> (because for me those are not clear: adding it as a new test in one of the base classes vs overwrite an existing test vs adding new tests to the inherited classes (but then you also get a strange split of tests) vs adding new classes for things that cannot be tested for all EAs in general ....)

I agree this is absolutely true.

My whole problem with moving things is that this makes debugging WAY harder. You have no idea which test / tests to run to exercise things. As they are all together this is quite simple now.

I will push back to you, what is the motivation for splitting them? what problem are you actually trying to solve here?

> I will push back to you, what is the motivation for splitting them?

From https://github.com/pandas-dev/pandas/pull/22026#issuecomment-407717834

> When I'm working on the interface, I want to be able to run it against all the test and public EAs, so I'd like to run pytest pandas/tests/extensions. There's no need to run EA-specific things like the Categorical constructor tests when working on the EA interface.

From the original post:

> Main reason that I would split them is that over time, we probably add a lot of EA-type-specific tests, and then keeping the general ones mixed with the specific ones will make it only confusing / hard to see what is going on.
> I will push back to you, what is the motivation for splitting them? what problem are you actually trying to solve here?

Because now working on the tests (adding tests, updating them for changes in the code, ..) is hard. You say in the same comment "I agree this is absolutely true." about the complexities that I describe. Well, that is the motivation to improve this, and it seems you at least agree with that original motivation?
I agree with the motivation to do this in theory. However from a practical POV it is *very* very confusing on what to put where. We are trying to simplify things. This does the reverse.
Joris and I think this PR makes things simpler. Statements like

> However from a practical POV it is *very* very confusing on what to put where.

are subjective. Our preference is also subjective, so let's just put this to a simple majority vote, once the
other maintainers respond?

On Wed, Jul 25, 2018 at 7:49 AM Jeff Reback <notifications@github.com>
wrote:

> I agree with the motivation to do this in theory. However from a practical
> POV it is *very* very confusing on what to put where. We are trying to
> simplify things. This does the reverse.
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/22026#issuecomment-407742871>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIkULTbPkNV1Dqtgb3F4akf6PLh6Wks5uKGlJgaJpZM4VbJ4J>
> .
>

> are subjective. Our preference is also subjective, so let's just put this to a simple majority vote, once the
other maintainers respond?

when have we ever done that? 
It seems like we're stuck here, and it's all subjective anyway. I think
it's worth a shot.

On Wed, Jul 25, 2018 at 7:56 AM Jeff Reback <notifications@github.com>
wrote:

> are subjective. Our preference is also subjective, so let's just put this
> to a simple majority vote, once the
> other maintainers respond?
>
> when have we ever done that?
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/22026#issuecomment-407744723>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIn9tlEZ-bMwDwXakikPTA-av4ll5ks5uKGrmgaJpZM4VbJ4J>
> .
>