Test nested PandasArray

[TomAugspurger]

May be easiest to view the commits individually.

38e7413
has the commit fixing the actual issue in #24986. That one was easy.

558cdbe is just moving the numpy-backed EA tests down to a directory, to facilitate the real changes in
9122bb6

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

[TomAugspurger]

 ========================================================== 382 passed, 145 skipped, 30 xfailed, 2 warnings in 4.76 seconds ==========================================================

[TomAugspurger]

I'd be eager to hear of a better way to do this. Right now, we rely on the name being like TestMethods::test_where_series[object-True] do determine if we should skip. Not the cleanest. Ideally we would be able to resolve that to the actual value, but pytest may not have that yet.

[jreback]

@simonjayhawkins any thoughts here

[jreback]

@TomAugspurger why don't you just add a marker to specific tests? (and skip on that)

[TomAugspurger]

I'm not aware of a way for a test marker to get access to the value of another fixture.

[simonjayhawkins]

Ideally we would be able to resolve that to the actual value, but pytest may not have that yet.

IIUC, from a test or fixture you get the value supplied to the fixture by including that (used by) fixture in the function signature. seems to work...

https://github.com/simonjayhawkins/pandas/blob/6ebf2c52d8925663a9242d3c58a069000b2c3f06/pandas/tests/io/formats/conftest.py#L141-L146

that would mean changing all the tests individually though. so this may not be what your after.

[codecov]

Codecov Report

Merging #24993 into master will increase coverage by 49.49%.
The diff coverage is 100%.

@@             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%> (ø)	⬆️

Impacted Files	Coverage Δ
pandas/core/arrays/numpy_.py	93.51% <100%> (+54.16%)	⬆️
pandas/core/computation/pytables.py	92.35% <0%> (+0.3%)	⬆️
pandas/io/pytables.py	92.31% <0%> (+0.92%)	⬆️
pandas/util/_test_decorators.py	90.54% <0%> (+4.05%)	⬆️
pandas/compat/__init__.py	57.91% <0%> (+8.1%)	⬆️
pandas/core/config_init.py	99.24% <0%> (+9.84%)	⬆️
pandas/core/api.py	100% <0%> (+13.79%)	⬆️
pandas/compat/numpy/__init__.py	92.85% <0%> (+14.28%)	⬆️
pandas/core/computation/common.py	85.71% <0%> (+14.28%)	⬆️
pandas/core/indexes/api.py	99% <0%> (+14.85%)	⬆️
... and 124 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 145ade2...642b01a. Read the comment docs.

[codecov]

Codecov Report

Merging #24993 into master will not change coverage.
The diff coverage is 100%.

@@           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%> (ø)	⬆️
#single	42.88% <0%> (ø)	⬆️

Impacted Files	Coverage Δ
pandas/core/arrays/numpy_.py	93.51% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9f2e2b...358df86. Read the comment docs.

[jorisvandenbossche]

@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?

[TomAugspurger]

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.

[TomAugspurger]

I reverted the changes to extension/numpy_/test_numpy.py and put the object / nested tests in their own file.

518315c

[simonjayhawkins]

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

[TomAugspurger]

@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]

@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)

[simonjayhawkins]

@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))

[TomAugspurger]

@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.

[simonjayhawkins]

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.")

[jbrockmendel]

Maybe more fixtures would help?

[jreback]

can you add a comment here on what this particular file is testing. (as a casual glance makes it look very similar to test_numpy.py)

[jreback]

@TomAugspurger but still this does not answer the question of why you duplicated things

[TomAugspurger]

does https://github.com/pandas-dev/pandas/pull/24993/files#diff-c77963c2757e522c9ed516402633c932R6 make sense?

[jreback]

it makes sense, I suppose enough. Problem is a future reader may not understand exactly what you are getting at here.

[simonjayhawkins]

@TomAugspurger why don't you just add a marker to specific tests? (and skip on that)

I'm not aware of a way for a test marker to get access to the value of another fixture.

@TomAugspurger I fear that I may have led you in the wrong direction with the first example on just including the fixture that you want the value of in the function signature. that was just to show that fixtures can be added to the function signature and that additional unwanted permutations would not occur.

from the pytest docs on request.getfixturevalue.. "Declaring fixtures via function argument is recommended where possible. But if you can only decide whether to use another fixture at test setup time, you may use this function to retrieve it inside a fixture or test function body."

hence why i mentioned the function argument approach first.

@jreback is right about adding markers, and the pytest.mark.usefixtures is probably the appropriate marker to use.

if this marker was used only on tests that depended on the dtype fixture, then the autouse fixture I suggested could be used without the request.getfixturevalue('dtype') and dtype included in the fixture signature along with the request fixture which gives access to the class, instance and function to determine if the test should be skipped.

Good to merge?

My comments should not be taken as a reason not to merge. A follow-on PR could look into this approach.

[TomAugspurger]

I'm relatively happy with current approach. It's lower-tech which is fine for me in tests.

[TomAugspurger]

All green.

[TomAugspurger]

Good to merge?

…

On Wed, Jan 30, 2019 at 10:46 AM Jeff Reback ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pandas/tests/extension/numpy_/test_numpy_nested.py <#24993 (comment)>: > @@ -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 <#24993 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHItI2n6Zqdu5pIp0CIw3w9sXkU7F-ks5vIcxogaJpZM4aXPjT> .

[jreback]

thanks @TomAugspurger