Test nested PandasArray
State: closed

May be easiest to view the commits individually.

https://github.com/pandas-dev/pandas/commit/38e7413f3f650230eca75ab50580630838930c34 
has the commit fixing the actual issue in #24986. That one was easy.

https://github.com/pandas-dev/pandas/commit/558cdbec47c9d9e7862a3d3c688fcf51260d367d is just moving the numpy-backed EA tests down to a directory, to facilitate the real changes in 
https://github.com/pandas-dev/pandas/commit/9122bb6d5e7f0eb2296016f35e9d8eb7ef7293e1

Having a PandasArray with nested data breaks lots of tests. Some of these are because of how we construct the expected, and how NumPy handles nested data (not the best). Adding checks to the individual tests like

```
def test_foo(self, data):
    if data.dtype == 'object':
        raise pytest.skip('skipping for object')
    pass
```

wasn't really feasible, as we'd need to duplicate all the parametrized fixtures defined on the tests themselves.

So, I opted to define a `pytest_collection_modifyitems` to add skips to a known list of tests that won't pass for nested data.

Closes #24986 

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

Comments:
```
 ========================================================== 382 passed, 145 skipped, 30 xfailed, 2 warnings in 4.76 seconds ==========================================================
```
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24993?src=pr&el=h1) Report
> Merging [#24993](https://codecov.io/gh/pandas-dev/pandas/pull/24993?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/145ade2a2f011dbfbe531347fd0dc563ee0b5642?src=pr&el=desc) will **increase** coverage by `49.49%`.
> The diff coverage is `100%`.

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

```diff
@@             Coverage Diff             @@
##           master   #24993       +/-   ##
===========================================
+ Coverage   42.88%   92.38%   +49.49%     
===========================================
  Files         166      166               
  Lines       52400    52400               
===========================================
+ Hits        22472    48408    +25936     
+ Misses      29928     3992    -25936
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.8% <100%> (?)` | |
| #single | `42.89% <0%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24993?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/numpy\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/24993/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL251bXB5Xy5weQ==) | `93.51% <100%> (+54.16%)` | :arrow_up: |
| [pandas/core/computation/pytables.py](https://codecov.io/gh/pandas-dev/pandas/pull/24993/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY29tcHV0YXRpb24vcHl0YWJsZXMucHk=) | `92.35% <0%> (+0.3%)` | :arrow_up: |
| [pandas/io/pytables.py](https://codecov.io/gh/pandas-dev/pandas/pull/24993/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3B5dGFibGVzLnB5) | `92.31% <0%> (+0.92%)` | :arrow_up: |
| [pandas/util/\_test\_decorators.py](https://codecov.io/gh/pandas-dev/pandas/pull/24993/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvX3Rlc3RfZGVjb3JhdG9ycy5weQ==) | `90.54% <0%> (+4.05%)` | :arrow_up: |
| [pandas/compat/\_\_init\_\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/24993/diff?src=pr&el=tree#diff-cGFuZGFzL2NvbXBhdC9fX2luaXRfXy5weQ==) | `57.91% <0%> (+8.1%)` | :arrow_up: |
| [pandas/core/config\_init.py](https://codecov.io/gh/pandas-dev/pandas/pull/24993/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY29uZmlnX2luaXQucHk=) | `99.24% <0%> (+9.84%)` | :arrow_up: |
| [pandas/core/api.py](https://codecov.io/gh/pandas-dev/pandas/pull/24993/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXBpLnB5) | `100% <0%> (+13.79%)` | :arrow_up: |
| [pandas/compat/numpy/\_\_init\_\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/24993/diff?src=pr&el=tree#diff-cGFuZGFzL2NvbXBhdC9udW1weS9fX2luaXRfXy5weQ==) | `92.85% <0%> (+14.28%)` | :arrow_up: |
| [pandas/core/computation/common.py](https://codecov.io/gh/pandas-dev/pandas/pull/24993/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY29tcHV0YXRpb24vY29tbW9uLnB5) | `85.71% <0%> (+14.28%)` | :arrow_up: |
| [pandas/core/indexes/api.py](https://codecov.io/gh/pandas-dev/pandas/pull/24993/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9hcGkucHk=) | `99% <0%> (+14.85%)` | :arrow_up: |
| ... and [124 more](https://codecov.io/gh/pandas-dev/pandas/pull/24993/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24993?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/24993?src=pr&el=footer). Last update [145ade2...642b01a](https://codecov.io/gh/pandas-dev/pandas/pull/24993?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/24993?src=pr&el=h1) Report
> Merging [#24993](https://codecov.io/gh/pandas-dev/pandas/pull/24993?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/b9f2e2bd526ad69fe3d737d395742193e29fd0e5?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

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

```diff
@@           Coverage Diff           @@
##           master   #24993   +/-   ##
=======================================
  Coverage   92.38%   92.38%           
=======================================
  Files         166      166           
  Lines       52401    52401           
=======================================
  Hits        48409    48409           
  Misses       3992     3992
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.8% <100%> (ø)` | :arrow_up: |
| #single | `42.88% <0%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24993?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/numpy\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/24993/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL251bXB5Xy5weQ==) | `93.51% <100%> (ø)` | :arrow_up: |

------

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

@TomAugspurger the alternative would be to have a separate `test_numpy_nested_object.py` that inherits all the test classes, runs them specifically for an object dtype PandasArray and skips those tests needed? But that would be a bit duplicative?
> But that would be a bit duplicative?

Initially I was worried about duplicating the `allow_in_pandas` fixture, but I realize now that we can put that in a contest and share it: https://github.com/pandas-dev/pandas/pull/24993/files#diff-82c6f88d57372980337a571084e30d8aR18

Let me play with that for a bit. I realize now that tests using `data_for_sorting`, `data_for_groupby`, etc. aren't really being tested with the nested version.
I reverted the changes to `extension/numpy_/test_numpy.py` and put the object / nested tests in their own file.

https://github.com/pandas-dev/pandas/pull/24993/commits/518315cb0a9a133abb8c22c5019bf0b159760e82
so i've tried just the first on the list: TestCasting.test_astype_str

made the following change to pandas\tests\extension\numpy_\test_numpy.py
```
class TestCasting(BaseNumPyTests, base.BaseCastingTests):
    def test_astype_str(self, data, dtype):
        if dtype.numpy_dtype == 'object':
            raise pytest.skip("Skipping for object dtype.")
        super(TestCasting, self).test_astype_str(data)
```
and it seems to work

@simonjayhawkins right, that one is not bad.

The problem is when the base test itself includes parametrized fixtures. e.g. `BaseMissingTests.test_fillna_series_method`. For those tests, the subclass needs to re-define all the parametrized fixtures, so that they can be passed to the super call.
> @simonjayhawkins right, that one is not bad.
> 
> The problem is when the base test itself includes parametrized fixtures. e.g. `BaseMissingTests.test_fillna_series_method`. For those tests, the subclass needs to re-define all the parametrized fixtures, so that they can be passed to the super call.

OK i'll take a look.. maybe indirect parametrization. just tried on `TestCasting.test_astype_str` and maybe better than skipping.

```
class TestCasting(BaseNumPyTests, base.BaseCastingTests):
    @pytest.mark.parametrize('dtype', ['float'], indirect=True)
    def test_astype_str(self, data):
        super(TestCasting, self).test_astype_str(data)
```
@TomAugspurger 

replace the pytest_collection_modifyitems hook with a pytest_generate_tests hook

```
def pytest_generate_tests(metafunc):
    # called once per each test function
    if 'dtype' in metafunc.fixturenames:
        qualname = metafunc.cls.__name__ + '.' + metafunc.function.__name__
        if qualname in skips:
            metafunc.parametrize('dtype', ['float'], indirect=True)
        else:
            metafunc.parametrize('dtype', ['float', 'object'], indirect=True)
```

you'll also need to remove the paramatrization from the fixture..

```
@pytest.fixture()
def dtype(request):
    return PandasDtype(np.dtype(request.param))
```
@simonjayhawkins thanks, I think I'm going with the more verbose, but simpler alternative of splitting these tests into their own file.

That's an interesting approach. I'll keep it in mind for recommending to downstream projects wishing to optionally skip / modify some tests, without repeating the parametrized fixtures.
OK. but if you want to skip tests, then an autouse fixture would be simplier..

```
@pytest.fixture(autouse=True)
def skip_nested_data(request):
    if 'dtype' in request.fixturenames:
        if request.getfixturevalue('dtype') == 'object':
            qualname = request.cls.__name__ + '.' + request.function.__name__
            if qualname in skips:
                raise pytest.skip("Skipping for object dtype.")
```
Maybe more fixtures would help? </snark>
All green.
Good to merge?

On Wed, Jan 30, 2019 at 10:46 AM Jeff Reback <notifications@github.com>
wrote:

> *@jreback* commented on this pull request.
> ------------------------------
>
> In pandas/tests/extension/numpy_/test_numpy_nested.py
> <https://github.com/pandas-dev/pandas/pull/24993#discussion_r252341233>:
>
> > @@ -0,0 +1,276 @@
> +import pytest
> +
> +import pandas as pd
>
> it makes sense, I suppose enough. Problem is a future reader may not
> understand exactly what you are getting at here.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/24993#discussion_r252341233>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHItI2n6Zqdu5pIp0CIw3w9sXkU7F-ks5vIcxogaJpZM4aXPjT>
> .
>

thanks @TomAugspurger