REF: .dot tests

[jbrockmendel]

Fully parametrized over Series/DataFrame

[simonjayhawkins]

from the docs "Going forward, we are moving to a more functional style using the pytest framework, which offers a richer testing framework that will facilitate testing and developing. Thus, instead of writing test classes, we will write test functions like this:"

can you refactor to eliminate the base class.

[jreback]

from the docs "Going forward, we are moving to a more functional style using the pytest framework, which offers a richer testing framework that will facilitate testing and developing. Thus, instead of writing test classes, we will write test functions like this:"

can you refactor to eliminate the base class.

we don't want to make changes like this generally when moving, and to be honest the docs are misleading / wrong here. We do need classes to namespace things.

[jreback]

can you make these module level fixtures, having in a class is an anti-patter

[jreback]

even better is to use the globally defined fixtures (but if hard nbd)

[jreback]

@jbrockmendel do we need these class level fixtures like this?

[jbrockmendel]

for some value of "need". Simon and i do not have a consensus on this

[jreback]

ok, the reason i question this is that we do not do this anywhere else (except for a PR you just did), and we DO this also in the resample tests. Since we aren't intentionally doing this globally I want to be sure this pattern is a valid / reasonable usecase that simply doesn't fit into our other patterns.

[jbrockmendel]

We use it a bunch in tests.indexes

[simonjayhawkins]

We do need classes to namespace things.

I'm OK with classes for that purpose, but in other modules we have been rmoving base classes, not adding them.

[jreback]

We do need classes to namespace things.

I'm OK with classes for that purpose, but in other modules we have been rmoving base classes, not adding them.

right, I wouldn't change the structure generally (meaning removing just for the sake of removing); i think we might have been doing that in the past.

[simonjayhawkins]

i think we might have been doing that in the past.

yeah, in some cases, having the classes is an advantage since it allows decorating the class. (if I recall, you weren't keen on the syntax required to do the same for a module with pytest, i.e. setting module globals)

[jbrockmendel]

@simonjayhawkins can you give an example of what your preferred pattern would look like? the only thing that comes to mind is something like:

@pytest.fixture(params=[Series, DataFrame])
def obj(request):
    if request.param is Series:
          return what_the_series_class_fixture_has_now
    else:
          return what_the_frame_class_fixture_has_now

which i dont find to be an improvement over class-based implementation

[simonjayhawkins]

which i dont find to be an improvement over class-based implementation

that's subjective, and OK.

but we've been moving away from the class based tests (with the exception of extension arrays which are used downstream)

so the concern here is consistency. and fixtures and parameterisation of tests has been the way we have been moving. (rightly or wrongly)

maybe for this PR, you could just move for now to collect the tests and refactor in a follow up.

[jbrockmendel]

maybe for this PR, you could just move for now to collect the tests and refactor in a follow up.

if the end result is going to be "this needs another refactor" either way, aren't we better off with the version where the non-trivial work of sharing the tests is already done?

[simonjayhawkins]

@jreback wrote "we don't want to make changes like this generally when moving" #33214 (comment)

so that's the reason i suggested that a straight move be done first.

aren't we better off with the version where the non-trivial work of sharing the tests is already done?

inherited tests is a valid approach but not something we've been doing (except extension array tests). if we do this we should have a discussion about this and update the guidance first.

[jreback]

comments

[jreback]

can you rebase. also we should clarify when / how to use class based fixtures.